Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

sys/vfs: use atomic_utils rather C11 atomics #20513

Merged
merged 1 commit into from
Apr 16, 2024
Merged

sys/vfs: use atomic_utils rather C11 atomics #20513

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 1 addition & 9 deletions sys/include/vfs.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,9 @@
* The overall design goals are:
* - Provide implementations for all newlib "file" syscalls
* - Keep it simple, do not add every possible file operation from Linux VFS.
* - Easy to map existing file system implementations for resource constrained systems onto the VFS layer API

Check warning on line 22 in sys/include/vfs.h

View workflow job for this annotation

GitHub Actions / static-tests 

line is longer than 100 characters
* - Avoid keeping a central `enum` of all file system drivers that has to be kept up to date with external packages etc.

Check warning on line 23 in sys/include/vfs.h

View workflow job for this annotation

GitHub Actions / static-tests 

line is longer than 100 characters
* - Use POSIX `<errno.h>` numbers as negative return codes for errors, avoid the global `errno` variable.

Check warning on line 24 in sys/include/vfs.h

View workflow job for this annotation

GitHub Actions / static-tests 

line is longer than 100 characters
* - Only absolute paths to files (no per-process working directory)
* - No dynamic memory allocation
*
Expand Down Expand Up @@ -54,14 +54,6 @@
#define VFS_H

#include <stdint.h>
/* The stdatomic.h in GCC gives compilation errors with C++
* see: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=60932
*/
#ifdef __cplusplus
#include "c11_atomics_compat.hpp"
#else
#include <stdatomic.h> /* for atomic_int */
#endif
#include <sys/stat.h> /* for struct stat */
#include <sys/types.h> /* for off_t etc. */
#include <sys/statvfs.h> /* for struct statvfs */
Expand Down Expand Up @@ -94,7 +86,7 @@
/**
* @brief MAX5 Function to get the largest of 5 values
*/
#define MAX5(a, b, c, d, e) _MAX(_MAX(_MAX((a), (b)), _MAX((c),(d))), (e))

Check warning on line 89 in sys/include/vfs.h

View workflow job for this annotation

GitHub Actions / static-tests 

comma should be followed by whitespace
#endif
/** @} */

Expand Down Expand Up @@ -383,7 +375,7 @@
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 and directories */
uint16_t open_files; /**< Number of currently open files and directories */
void *private_data; /**< File system driver private data, implementation defined */
};

Expand All @@ -401,7 +393,7 @@
union {
void *ptr; /**< pointer to private data */
int value; /**< alternatively, you can use private_data as an int */
uint8_t buffer[VFS_FILE_BUFFER_SIZE]; /**< Buffer space, in case a single pointer is not enough */

Check warning on line 396 in sys/include/vfs.h

View workflow job for this annotation

GitHub Actions / static-tests 

line is longer than 100 characters
} private_data; /**< File system driver private data, implementation defined */
} vfs_file_t;

Expand All @@ -419,7 +411,7 @@
union {
void *ptr; /**< pointer to private data */
int value; /**< alternatively, you can use private_data as an int */
uint8_t buffer[VFS_DIR_BUFFER_SIZE]; /**< Buffer space, in case a single pointer is not enough */

Check warning on line 414 in sys/include/vfs.h

View workflow job for this annotation

GitHub Actions / static-tests 

line is longer than 100 characters
} private_data; /**< File system driver private data, implementation defined */
} vfs_DIR;

Expand Down Expand Up @@ -523,7 +515,7 @@
* same const char array and the strings may overlap
*
* @param[in] filp pointer to open file
* @param[in] name null-terminated name of the file to open, relative to the file system root, including a leading slash

Check warning on line 518 in sys/include/vfs.h

View workflow job for this annotation

GitHub Actions / static-tests 

line is longer than 100 characters
* @param[in] flags flags for opening, see man 2 open, man 3p open
* @param[in] mode mode for creating a new file, see man 2 open, man 3p open
*
Expand Down Expand Up @@ -576,7 +568,7 @@
* @brief Open a directory for reading with readdir
*
* @param[in] dirp pointer to open directory
* @param[in] name null-terminated name of the dir to open, relative to the file system root, including a leading slash

Check warning on line 571 in sys/include/vfs.h

View workflow job for this annotation

GitHub Actions / static-tests 

line is longer than 100 characters
*
* @return 0 on success
* @return <0 on error
Expand Down
91 changes: 63 additions & 28 deletions sys/vfs/vfs.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,16 @@
#include <fcntl.h> /* for O_ACCMODE, ..., fcntl */
#include <unistd.h> /* for STDIN_FILENO, STDOUT_FILENO, STDERR_FILENO */

#include "atomic_utils.h"
#include "clist.h"
#include "compiler_hints.h"
#include "container.h"
#include "modules.h"
#include "vfs.h"
#include "mutex.h"
#include "thread.h"
#include "sched.h"
#include "clist.h"
#include "test_utils/expect.h"
#include "thread.h"
#include "vfs.h"

#define ENABLE_DEBUG 0
#include "debug.h"
Expand Down Expand Up @@ -295,7 +298,8 @@
if (fd < 0) {
DEBUG("vfs_open: _init_fd: ERR %d!\n", fd);
/* remember to decrement the open_files count */
atomic_fetch_sub(&mountp->open_files, 1);
uint16_t before = atomic_fetch_sub_u16(&mountp->open_files, 1);
assume(before > 0);
return fd;
}
vfs_file_t *filp = &_vfs_open_files[fd];
Expand Down Expand Up @@ -425,7 +429,8 @@
int res = dirp->d_op->opendir(dirp, rel_path);
if (res < 0) {
/* remember to decrement the open_files count */
atomic_fetch_sub(&mountp->open_files, 1);
uint16_t before = atomic_fetch_sub_u16(&mountp->open_files, 1);
assume(before > 0);
return res;
}
}
Expand Down Expand Up @@ -463,7 +468,8 @@
}
}
memset(dirp, 0, sizeof(*dirp));
atomic_fetch_sub(&mountp->open_files, 1);
uint16_t before = atomic_fetch_sub_u16(&mountp->open_files, 1);
assume(before > 0);
return res;
}

Expand Down Expand Up @@ -563,8 +569,9 @@
DEBUG("vfs_umount: invalid fs\n");
return -EINVAL;
}
DEBUG("vfs_umount: -> \"%s\" open=%d\n", mountp->mount_point, atomic_load(&mountp->open_files));
if (atomic_load(&mountp->open_files) > 0 && !force) {
DEBUG("vfs_umount: -> \"%s\" open=%u\n", mountp->mount_point,
(unsigned)atomic_load_u16(&mountp->open_files));
if (atomic_load_u16(&mountp->open_files) > 0 && !force) {
mutex_unlock(&_mount_mutex);
return -EBUSY;
}
Expand Down Expand Up @@ -610,7 +617,8 @@
/* rename not supported */
DEBUG("vfs_rename: rename not supported by fs!\n");
/* remember to decrement the open_files count */
atomic_fetch_sub(&mountp->open_files, 1);
uint16_t before = atomic_fetch_sub_u16(&mountp->open_files, 1);
assume(before > 0);
return -EROFS;
}
const char *rel_to;
Expand All @@ -621,15 +629,18 @@
/* No mount point maps to the requested file name */
DEBUG("vfs_rename: to: no matching mount\n");
/* remember to decrement the open_files count */
atomic_fetch_sub(&mountp->open_files, 1);
uint16_t before = atomic_fetch_sub_u16(&mountp->open_files, 1);
assume(before > 0);
return res;
}
if (mountp_to != mountp) {
/* The paths are on different file systems */
DEBUG("vfs_rename: from_path and to_path are on different mounts\n");
/* remember to decrement the open_files count */
atomic_fetch_sub(&mountp->open_files, 1);
atomic_fetch_sub(&mountp_to->open_files, 1);
uint16_t before = atomic_fetch_sub_u16(&mountp->open_files, 1);
assume(before > 0);
before = atomic_fetch_sub_u16(&mountp_to->open_files, 1);
assume(before > 0);
return -EXDEV;
}
res = mountp->fs->fs_op->rename(mountp, rel_from, rel_to);
Expand All @@ -642,8 +653,10 @@
DEBUG("\n");
}
/* remember to decrement the open_files count */
atomic_fetch_sub(&mountp->open_files, 1);
atomic_fetch_sub(&mountp_to->open_files, 1);
uint16_t before = atomic_fetch_sub_u16(&mountp->open_files, 1);
assume(before > 0);
before = atomic_fetch_sub_u16(&mountp_to->open_files, 1);
assume(before > 0);
return res;
}

Expand All @@ -669,7 +682,8 @@
/* unlink not supported */
DEBUG("vfs_unlink: unlink not supported by fs!\n");
/* remember to decrement the open_files count */
atomic_fetch_sub(&mountp->open_files, 1);
uint16_t before = atomic_fetch_sub_u16(&mountp->open_files, 1);
assume(before > 0);
return -EROFS;
}
res = mountp->fs->fs_op->unlink(mountp, rel_path);
Expand All @@ -682,7 +696,8 @@
DEBUG("\n");
}
/* remember to decrement the open_files count */
atomic_fetch_sub(&mountp->open_files, 1);
uint16_t before = atomic_fetch_sub_u16(&mountp->open_files, 1);
assume(before > 0);
return res;
}

Expand All @@ -706,7 +721,8 @@
/* mkdir not supported */
DEBUG("vfs_mkdir: mkdir not supported by fs!\n");
/* remember to decrement the open_files count */
atomic_fetch_sub(&mountp->open_files, 1);
uint16_t before = atomic_fetch_sub_u16(&mountp->open_files, 1);
assume(before > 0);
return -EROFS;
}
res = mountp->fs->fs_op->mkdir(mountp, rel_path, mode);
Expand All @@ -719,7 +735,8 @@
DEBUG("\n");
}
/* remember to decrement the open_files count */
atomic_fetch_sub(&mountp->open_files, 1);
uint16_t before = atomic_fetch_sub_u16(&mountp->open_files, 1);
assume(before > 0);
return res;
}

Expand All @@ -743,7 +760,8 @@
/* rmdir not supported */
DEBUG("vfs_rmdir: rmdir not supported by fs!\n");
/* remember to decrement the open_files count */
atomic_fetch_sub(&mountp->open_files, 1);
uint16_t before = atomic_fetch_sub_u16(&mountp->open_files, 1);
assume(before > 0);
return -EROFS;
}
res = mountp->fs->fs_op->rmdir(mountp, rel_path);
Expand All @@ -756,7 +774,8 @@
DEBUG("\n");
}
/* remember to decrement the open_files count */
atomic_fetch_sub(&mountp->open_files, 1);
uint16_t before = atomic_fetch_sub_u16(&mountp->open_files, 1);
assume(before > 0);
return res;
}

Expand All @@ -780,13 +799,15 @@
/* stat not supported */
DEBUG("vfs_stat: stat not supported by fs!\n");
/* remember to decrement the open_files count */
atomic_fetch_sub(&mountp->open_files, 1);
uint16_t before = atomic_fetch_sub_u16(&mountp->open_files, 1);
assume(before > 0);
return -EPERM;
}
memset(buf, 0, sizeof(*buf));
res = mountp->fs->fs_op->stat(mountp, rel_path, buf);
/* remember to decrement the open_files count */
atomic_fetch_sub(&mountp->open_files, 1);
uint16_t before = atomic_fetch_sub_u16(&mountp->open_files, 1);
assume(before > 0);
return res;
}

Expand All @@ -810,13 +831,15 @@
/* statvfs not supported */
DEBUG("vfs_statvfs: statvfs not supported by fs!\n");
/* remember to decrement the open_files count */
atomic_fetch_sub(&mountp->open_files, 1);
uint16_t before = atomic_fetch_sub_u16(&mountp->open_files, 1);
assume(before > 0);
return -EPERM;
}
memset(buf, 0, sizeof(*buf));
res = mountp->fs->fs_op->statvfs(mountp, rel_path, buf);
/* remember to decrement the open_files count */
atomic_fetch_sub(&mountp->open_files, 1);
uint16_t before = atomic_fetch_sub_u16(&mountp->open_files, 1);
assume(before > 0);
return res;
}

Expand Down Expand Up @@ -954,14 +977,20 @@
* 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);
uint16_t before = atomic_fetch_add_u16(&next->open_files, 1);
/* We cannot use assume() here, an overflow could occur in absence of
* any bugs and should also be checked for in production code. We use
* expect() here, which was actually written for unit tests but works
* here as well */
Comment on lines +980 to +984
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
uint16_t before = atomic_fetch_add_u16(&next->open_files, 1);
/* We cannot use assume() here, an overflow could occur in absence of
* any bugs and should also be checked for in production code. We use
* expect() here, which was actually written for unit tests but works
* here as well */
uint16_t before = atomic_fetch_add_u16(&next->open_files, 1);
/* We cannot use assume() here, an overflow could occur when the only
* bug present is a memory leak, and should also be checked for in
* production code. We use expect() here, which was actually written for
* unit tests but works here as well */

Changes after the first lines are just impact of (guessed) reflowing.

Yes, technically there can be an application where open files exceed 2^16 and still not leak memory, but the file handles alone would be 256KiB at the very least, so it's safe to assume that there is a leak.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should be careful about ruling out use cases that appear crazy ;)

Technically, even some 8 bit CPUs can use multiple MiB of RAM. (The ATXmega board's RAM can be extended with PSRAM and does have a way to address 2^24 Bytes or 16 MiB of memory.) And ESPs with some MiBs of PSRAM are quite common, e.g. those with a high megapixel camera module need lots of RAM to be able to take pictures.

So, while the use case of having ten thousands of files open appears pretty crazy, it is technically possible. And everything that is technically possible will eventually some professional lemming.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough. I'd still be confused reading this as to how this can happen without a bug (such as a memory leak), but no biggie.

expect(before < UINT16_MAX);

/* 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);
before = atomic_fetch_sub_u16(&next->open_files, 1);
assume(before > 0);

if (err != 0) {
DEBUG("vfs_iterate_mount opendir erred: vfs_opendir(\"%s\") = %d\n", next->mount_point, err);
Expand Down Expand Up @@ -1018,12 +1047,13 @@
{
DEBUG("_free_fd: %d, pid=%d\n", fd, _vfs_open_files[fd].pid);
if (_vfs_open_files[fd].mp != NULL) {
atomic_fetch_sub(&_vfs_open_files[fd].mp->open_files, 1);
uint16_t before = atomic_fetch_sub_u16(&_vfs_open_files[fd].mp->open_files, 1);
assume(before > 0);
}
_vfs_open_files[fd].pid = KERNEL_PID_UNDEF;
}

static inline int _init_fd(int fd, const vfs_file_ops_t *f_op, vfs_mount_t *mountp, int flags, void *private_data)

Check warning on line 1056 in sys/vfs/vfs.c

View workflow job for this annotation

GitHub Actions / static-tests 

line is longer than 100 characters
{
fd = _allocate_fd(fd);
if (fd < 0) {
Expand Down Expand Up @@ -1082,7 +1112,12 @@
return -ENOENT;
}
/* Increment open files counter for this mount */
atomic_fetch_add(&mountp->open_files, 1);
uint16_t before = atomic_fetch_add_u16(&mountp->open_files, 1);
/* We cannot use assume() here, an overflow could occur in absence of
* any bugs and should also be checked for in production code. We use
* expect() here, which was actually written for unit tests but works
* here as well */
expect(before < UINT16_MAX);
mutex_unlock(&_mount_mutex);
*mountpp = mountp;

Expand Down Expand Up @@ -1130,7 +1165,7 @@
return true;
}

int vfs_sysop_stat_from_fstat(vfs_mount_t *mountp, const char *restrict path, struct stat *restrict buf)

Check warning on line 1168 in sys/vfs/vfs.c

View workflow job for this annotation

GitHub Actions / static-tests 

line is longer than 100 characters
{
const vfs_file_ops_t * f_op = mountp->fs->f_op;

Expand Down
7 changes: 4 additions & 3 deletions tests/unittests/tests-vfs/tests-vfs-file-system-ops.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,12 @@
*/
#include <errno.h>
#include <stddef.h>
#include <stdint.h>
#include <string.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <unistd.h>

#include "atomic_utils.h"
#include "embUnit/embUnit.h"

#include "vfs.h"
Expand Down Expand Up @@ -72,7 +72,7 @@ static void setup(void)
static void teardown(void)
{
vfs_umount(&_test_vfs_mount_null, false);
atomic_store(&_test_vfs_mount_null.open_files, 0);
atomic_store_u16(&_test_vfs_mount_null.open_files, 0);
}

static void test_vfs_null_fs_ops_mount(void)
Expand All @@ -96,7 +96,8 @@ static void test_vfs_null_fs_ops_umount(void)
static void test_vfs_null_fs_ops_umount__EBUSY(void)
{
TEST_ASSERT_EQUAL_INT(0, _test_vfs_fs_op_mount_res);
atomic_fetch_add(&_test_vfs_mount_null.open_files, 1);
uint16_t before = atomic_fetch_add_u16(&_test_vfs_mount_null.open_files, 1);
TEST_ASSERT(before < UINT16_MAX);
int res = vfs_umount(&_test_vfs_mount_null, false);
TEST_ASSERT_EQUAL_INT(-EBUSY, res);
/* force unmount */
Expand Down
Loading