Merge pull request #17660 from chrysn-pull-requests/vfs-drop-per-fs-fstatvfs

vfs: Introduce reliable disk enumeration

sys/include/vfs.h

@@ -304,7 +304,7 @@ struct vfs_mount_struct {
     const vfs_file_system_t *fs; /**< The file system driver for the mount point */
     const char *mount_point;     /**< Mount point, e.g. "/mnt/cdrom" */
     size_t mount_point_len;      /**< Length of mount_point string (set by vfs_mount) */
-    atomic_int open_files;       /**< Number of currently open files */
+    atomic_int open_files;       /**< Number of currently open files and directories */
     void *private_data;          /**< File system driver private data, implementation defined */
 };
 
@@ -660,23 +660,6 @@ struct vfs_file_system_ops {
      * @return <0 on error
      */
     int (*statvfs) (vfs_mount_t *mountp, const char *restrict path, struct statvfs *restrict buf);
-
-    /**
-     * @brief Get file system status of an open file
-     *
-     * @p path is only passed for consistency against the POSIX statvfs function.
-     * @c vfs_statvfs calls this function only when it has determined that
-     * @p path belongs to this file system. @p path is a file system relative
-     * path and does not necessarily name an existing file.
-     *
-     * @param[in]  mountp  file system mount to operate on
-     * @param[in]  filp    pointer to an open file on the file system being queried
-     * @param[out] buf     pointer to statvfs struct to fill
-     *
-     * @return 0 on success
-     * @return <0 on error
-     */
-    int (*fstatvfs) (vfs_mount_t *mountp, vfs_file_t *filp, struct statvfs *buf);
 };
 
 /**
@@ -732,6 +715,17 @@ int vfs_fstat(int fd, struct stat *buf);
  */
 int vfs_fstatvfs(int fd, struct statvfs *buf);
 
+/**
+ * @brief Get file system status of the file system containing an open directory
+ *
+ * @param[in]  dirp     pointer to open directory
+ * @param[out] buf      pointer to statvfs struct to fill
+ *
+ * @return 0 on success
+ * @return <0 on error
+ */
+int vfs_dstatvfs(vfs_DIR *dirp, struct statvfs *buf);
+
 /**
  * @brief Seek to position in file
  *
@@ -904,7 +898,7 @@ int vfs_rename(const char *from_path, const char *to_path);
 /**
  * @brief Unmount a mounted file system
  *
- * This will fail if there are any open files on the mounted file system
+ * This will fail if there are any open files or directories on the mounted file system
  *
  * @param[in]  mountp    pointer to the mount structure of the file system to unmount
  *
@@ -1019,7 +1013,8 @@ int vfs_normalize_path(char *buf, const char *path, size_t buflen);
  *
  * Set @p cur to @c NULL to start from the beginning
  *
- * @see @c sc_vfs.c (@c df command) for a usage example
+ * @deprecated This will become an internal-only function after the 2022.04
+ *   release, use @ref vfs_iterate_mount_dirs instead.
  *
  * @param[in]  cur  current iterator value
  *
@@ -1028,6 +1023,37 @@ int vfs_normalize_path(char *buf, const char *path, size_t buflen);
  */
 const vfs_mount_t *vfs_iterate_mounts(const vfs_mount_t *cur);
 
+/**
+ * @brief Iterate through all mounted file systems by their root directories
+ *
+ * Unlike @ref vfs_iterate_mounts, this is thread safe, and allows thread safe
+ * access to the mount point's stats through @ref vfs_dstatvfs. If mounts or
+ * unmounts happen while iterating, this is guaranteed to report all file
+ * systems that stayed mounted, and may report any that are transiently
+ * mounted for up to as often as they are (re)mounted. Note that the volume
+ * being reported can not be unmounted as @p dir is an open directory.
+ *
+ * Zero-initialize @p dir to start. As long as @c true is returned, @p dir is a
+ * valid directory on which the user can call @ref vfs_readdir or @ref
+ * vfs_dstatvfs (or even peek at its `.mp` if they dare ignore the warning in
+ * @ref vfs_DIR).
+ *
+ * Users MUST NOT call @ref vfs_closedir if they intend to keep iterating, but
+ * MUST call it when aborting iteration.
+ *
+ * Note that this requires all enumerated file systems to support the `opendir`
+ * @ref vfs_dir_ops; any file system that does not support that will
+ * prematurely terminate the mount point enumeration.
+ *
+ * @see @c sc_vfs.c (@c df command) for a usage example
+ *
+ * @param[inout]  dir     The root directory of the discovered mount point
+ *
+ * @return     @c true if another file system is mounted; @p dir then contains an open directory.
+ * @return     @c false if the file system list is exhausted; @p dir is uninitialized then.
+ */
+bool vfs_iterate_mount_dirs(vfs_DIR *dir);
+
 /**
  * @brief   Get information about the file for internal purposes
  *

sys/shell/commands/sc_vfs.c

@@ -115,11 +115,11 @@ static int _errno_string(int err, char *buf, size_t buflen)
 }
 #undef _case_snprintf_errno_name
 
-static void _print_df(const char *path)
+static void _print_df(vfs_DIR *dir)
 {
     struct statvfs buf;
-    int res = vfs_statvfs(path, &buf);
-    printf("%-16s ", path);
+    int res = vfs_dstatvfs(dir, &buf);
+    printf("%-16s ", dir->mp->mount_point);
     if (res < 0) {
         char err[16];
         _errno_string(res, err, sizeof(err));
@@ -136,13 +136,24 @@ static int _df_handler(int argc, char **argv)
     puts("Mountpoint              Total         Used    Available     Capacity");
     if (argc > 1) {
         const char *path = argv[1];
-        _print_df(path);
+        /* Opening a directory just to statfs is somewhat odd, but it is the
+         * easiest to support with a single _print_df function */
+        vfs_DIR dir;
+        int res = vfs_opendir(&dir, path);
+        if (res == 0) {
+            _print_df(&dir);
+            vfs_closedir(&dir);
+        } else {
+            char err[16];
+            _errno_string(res, err, sizeof(err));
+            printf("Failed to open `%s`: %s\n", path, err);
+        }
     }
     else {
         /* Iterate through all mount points */
-        const vfs_mount_t *it = NULL;
-        while ((it = vfs_iterate_mounts(it)) != NULL) {
-            _print_df(it->mount_point);
+        vfs_DIR it = { 0 };
+        while (vfs_iterate_mount_dirs(&it)) {
+            _print_df(&it);
         }
     }
     return 0;

sys/vfs/vfs.c

@@ -216,15 +216,25 @@ int vfs_fstatvfs(int fd, struct statvfs *buf)
     }
     vfs_file_t *filp = &_vfs_open_files[fd];
     memset(buf, 0, sizeof(*buf));
-    if (filp->mp->fs->fs_op->fstatvfs == NULL) {
-        /* file system driver does not implement fstatvfs() */
-        if (filp->mp->fs->fs_op->statvfs != NULL) {
-            /* Fall back to statvfs */
-            return filp->mp->fs->fs_op->statvfs(filp->mp, "/", buf);
-        }
+    if (filp->mp->fs->fs_op->statvfs == NULL) {
+        /* file system driver does not implement statvfs() */
         return -EINVAL;
     }
-    return filp->mp->fs->fs_op->fstatvfs(filp->mp, filp, buf);
+    return filp->mp->fs->fs_op->statvfs(filp->mp, "/", buf);
+}
+
+int vfs_dstatvfs(vfs_DIR *dirp, struct statvfs *buf)
+{
+    DEBUG("vfs_dstatvfs: %p, %p\n", (void*)dirp, (void *)buf);
+    if (buf == NULL) {
+        return -EFAULT;
+    }
+    memset(buf, 0, sizeof(*buf));
+    if (dirp->mp->fs->fs_op->statvfs == NULL) {
+        /* file system driver does not implement statvfs() */
+        return -EINVAL;
+    }
+    return dirp->mp->fs->fs_op->statvfs(dirp->mp, "/", buf);
 }
 
 off_t vfs_lseek(int fd, off_t off, int whence)
@@ -512,7 +522,7 @@ int vfs_mount(vfs_mount_t *mountp)
             }
         }
     }
-    /* insert last in list */
+    /* Insert last in list. This property is relied on by vfs_iterate_mount_dirs. */
     clist_rpush(&_vfs_mounts_list, &mountp->list_entry);
     mutex_unlock(&_mount_mutex);
     DEBUG("vfs_mount: mount done\n");
@@ -886,16 +896,62 @@ const vfs_mount_t *vfs_iterate_mounts(const vfs_mount_t *cur)
             /* empty list */
             return NULL;
         }
+        node = node->next;
     }
     else {
         node = cur->list_entry.next;
-        if (node == _vfs_mounts_list.next) {
+        if (node == _vfs_mounts_list.next->next) {
             return NULL;
         }
     }
     return container_of(node, vfs_mount_t, list_entry);
 }
 
+/* General implementation note: This heavily relies on the produced opened dir
+ * to lock keep the underlying mount point from closing. */
+bool vfs_iterate_mount_dirs(vfs_DIR *dir)
+{
+    /* This is NULL after the prescribed initialization, or a valid (and
+     * locked) mount point otherwise */
+    vfs_mount_t *last_mp = dir->mp;
+
+    /* This is technically violating vfs_iterate_mounts' API, as that says no
+     * mounts or unmounts on the chain while iterating. However, as we know
+     * that the current dir's mount point is still on, the equivalent procedure
+     * of starting a new round of `vfs_iterate_mounts` from NULL and calling it
+     * until it produces `last_mp` (all while holding _mount_mutex) would leave
+     * us with the very same situation as if we started iteration with last_mp.
+     *
+     * On the cast discarding const: vfs_iterate_mounts's type is more for
+     * public use */
+    vfs_mount_t *next = (vfs_mount_t *)vfs_iterate_mounts(last_mp);
+    if (next == NULL) {
+        /* Ignoring errors, can't help with them */
+        vfs_closedir(dir);
+        return false;
+    }
+
+    /* Even if we held the mutex up to here (see above comment on the fiction
+     * of acquiring it, iterating to where we are, and releasing it again),
+     * we'd need to let go of it now to actually open the directory. This
+     * temporary count ensures that the file system will stick around for the
+     * directory open step that follows immediately */
+    atomic_fetch_add(&next->open_files, 1);
+
+    /* Ignoring errors, can't help with them */
+    vfs_closedir(dir);
+
+    int err = vfs_opendir(dir, next->mount_point);
+    /* No matter the success, the open_files lock has done its duty */
+    atomic_fetch_sub(&next->open_files, 1);
+
+    if (err != 0) {
+        DEBUG("vfs_iterate_mount opendir erred: vfs_opendir(\"%s\") = %d\n", next->mount_point, err);
+        return false;
+    }
+    return true;
+}
+
 const vfs_file_t *vfs_file_get(int fd)
 {
     if (_fd_is_valid(fd) == 0) {

tests/vfs_iterate_mount/Makefile

@@ -0,0 +1,6 @@
+include ../Makefile.tests_common
+
+USEMODULE += vfs
+USEMODULE += constfs
+
+include $(RIOTBASE)/Makefile.include

tests/vfs_iterate_mount/Makefile.ci

@@ -0,0 +1,3 @@
+BOARD_INSUFFICIENT_MEMORY := \
+    nucleo-l011k4 \
+    #

tests/vfs_iterate_mount/main.c

@@ -0,0 +1,124 @@
+/*
+ * Copyright (C) Christian Amsüss <chrysn@fsfe.org>
+ *
+ * This file is subject to the terms and conditions of the GNU Lesser
+ * General Public License v2.1. See the file LICENSE in the top level
+ * directory for more details.
+ */
+
+/**
+ * Mount and unmount a few file systems, demonstrating that
+ * vfs_iterate_mount_dirs performs as advertised.
+ *
+ * @author      Christian Amsüss <chrysn@fsfe.org>
+ */
+
+#include <stdio.h>
+#include <assert.h>
+#include <string.h>
+
+#include <vfs.h>
+#include <fs/constfs.h>
+
+static constfs_file_t constfs_files[1] = {
+    /* Not completely empty -- that'd be a hassle around empty arrays and
+     * their size */
+    {
+        .path = "some-file",
+        .size = 0,
+        .data = (void*)"",
+    },
+};
+
+static constfs_t constfs_desc = {
+    .nfiles = ARRAY_SIZE(constfs_files),
+    .files = constfs_files,
+};
+
+static vfs_mount_t mount1 = {
+    .fs = &constfs_file_system,
+    .mount_point = "/const1",
+    .private_data = &constfs_desc,
+};
+
+static vfs_mount_t mount2 = {
+    .fs = &constfs_file_system,
+    .mount_point = "/const2",
+    .private_data = &constfs_desc,
+};
+
+static vfs_mount_t mount3 = {
+    .fs = &constfs_file_system,
+    .mount_point = "/const3",
+    .private_data = &constfs_desc,
+};
+
+static vfs_mount_t mount4 = {
+    .fs = &constfs_file_system,
+    .mount_point = "/const4",
+    .private_data = &constfs_desc,
+};
+
+/* Crank the iterator, reporting "N%s" for the next entry, or "O\n" for the end
+ * of the iterator (avoiding the letter "E" which may be misread for an error
+ * in a casual look at the error output) */
+static void iter_and_report(vfs_DIR *iter) {
+    bool result = vfs_iterate_mount_dirs(iter);
+    if (result) {
+        printf("N(%s)", iter->mp->mount_point);
+    } else {
+        printf("O\n");
+        /* Zero out so we're ready for next round immediately */
+        memset(iter, 0, sizeof(*iter));
+    }
+}
+
+int main(void) {
+    int res = 0;
+
+    vfs_DIR iter;
+    memset(&iter, 0, sizeof(iter));
+
+    res |= vfs_mount(&mount1);
+    res |= vfs_mount(&mount2);
+    res |= vfs_mount(&mount3);
+    res |= vfs_mount(&mount4);
+    assert(res == 0);
+    printf("Mounted 1234\n");
+
+    /* N1N2N3N4E */
+    iter_and_report(&iter);
+    iter_and_report(&iter);
+    iter_and_report(&iter);
+    iter_and_report(&iter);
+    iter_and_report(&iter);
+
+    /* N1N2, unmount 3, N4E */
+    iter_and_report(&iter);
+    iter_and_report(&iter);
+    res |= vfs_umount(&mount3);
+    iter_and_report(&iter);
+    iter_and_report(&iter);
+
+    /* N1, unmount 2, (3 is already unmounted), N4, mount 3 N3, unmount 1 and remount it at the end N1, O */
+    /* It is OK that 1 is reported twice, because its first occurrence is its
+     * old mounting, and later it reappears */
+    iter_and_report(&iter);
+    res |= vfs_umount(&mount2);
+    iter_and_report(&iter);
+    res |= vfs_mount(&mount3);
+    iter_and_report(&iter);
+    res |= vfs_umount(&mount1);
+    res |= vfs_mount(&mount1);
+    iter_and_report(&iter);
+    iter_and_report(&iter);
+
+    /* This ensures we're not leaking locks */
+    res |= vfs_umount(&mount1);
+    res |= vfs_umount(&mount3);
+    res |= vfs_umount(&mount4);
+    printf("All unmounted\n");
+
+    /* Only O */
+    iter_and_report(&iter);
+}

tests/vfs_iterate_mount/tests/01-run.py

@@ -0,0 +1,23 @@
+#!/usr/bin/env python3
+
+# Copyright (C) 2021 Christian Amsüss <chrysn@fsfe.org>
+#
+# This file is subject to the terms and conditions of the GNU Lesser
+# General Public License v2.1. See the file LICENSE in the top level
+# directory for more details.
+
+import sys
+from testrunner import run
+
+
+def testfunc(child):
+    child.expect_exact("Mounted 1234")
+    child.expect_exact("N(/const1)N(/const2)N(/const3)N(/const4)O")
+    child.expect_exact("N(/const1)N(/const2)N(/const4)O")
+    child.expect_exact("N(/const1)N(/const4)N(/const3)N(/const1)O")
+    child.expect_exact("All unmounted")
+    child.expect_exact("O")
+
+
+if __name__ == "__main__":
+    sys.exit(run(testfunc))