vfs: Introduce reliable disk enumeration #17660
Conversation
The addition is helpful because directories are visibly different from open files (and generally are not treated like files).
No current file system implements it, there is no defined semantic difference between running fstatfs and the fallback currently implemented, and there is practically no optimization gained from not just running it through a single statvfs.
882ea6d to
bd22de4
Compare
|
Tests are now in, were run on native and microbit-v2 (but there isn't anything about this that should be platform dependent). (And typo fixed, but I don't want to push it and lose my place in the queue :-) ) |
|
Disabling CI because it worked except for the now fixed Makefile.ci; please reenable when a new result is useful. |
| /** | ||
| * @brief Iterate through all mounted file systems by their root directories | ||
| * | ||
| * Unlike @ref vfs_iterate_mounts, this is thread safe, and allows thread safe |
There was a problem hiding this comment.
Would probably be a good call to make vfs_iterate_mounts a private function then
There was a problem hiding this comment.
For release cycle reasons I could only deprecate its public use -- but then it'd probably be prudent to change sc_vfs too, and ...
yeah I've probably just been too lazy to do that. (Nice side effect of that will be that it'll differentiate the previosu error "statvfs failed" into cases of "diropen failed" (thing does not exist) and "statvfs failed" (the FS just doesn't implement statting).)
There was a problem hiding this comment.
Done now (both the deprecation and the switch in sc_vfs), with a deprecation for public use, becoming internal after 2022.04.
| * @ref vfs_DIR). | ||
| * | ||
| * Users MUST NOT call @ref vfs_closedir if they intend to keep iterating, but | ||
| * MUST call it when aborting iteration. |
There was a problem hiding this comment.
Would it make sense to require the user to always call ´vfs_closedir()` when they are done, no matter if they abort early or iterated over all mounts?
There was a problem hiding this comment.
Hm, it's a trade-off between complexity of the simple thing (just iterating over all, as df does) and loops with breaks (of which I wouldn't know an example, because if you want only one, you'd diropen or statvfs that right away). So I think that this way is the more practical one.
There was a problem hiding this comment.
IMHO 'you only need to use vfs_closedir() sometimes is inconsistent and will lead to it being forgotten even if needed.
vfs_iterate_mount_dirs() calls vfs_closedir() internally anyway, so why not remove it from there and always have it called by the user, so the behavior is always consistent. "When done iterating, call vfs_closedir()". Not "When done iterating AND no more mount points are left, call vfs_closedir()"
There was a problem hiding this comment.
I'm just pushing back here because I have a hard time seeing why the loop would be aborted in the first place; can you imagine any situation why it would? Even when paginating mount points one'd need to go through all to get some consistency metric ... although OK, I can envision some patterns of pagination that do work with incomplete lists -- fine, will change it.
(After all it will make the error of someone forgetting to clean up more visible, as they can never unmount after the first iteration).
There was a problem hiding this comment.
There is one weird corner case that makes me go back on this (noticed when starting work on it): If no file system is mounted at all, that would now require a special case. (You can't just zero out the initial value, then start iterating, function returns false immediately, and then closing the dir goes boom).
Applications would handle that even worse than the current exit condition (and it'd be even rarer in their applications), so I'd stick with the current one.
There was a problem hiding this comment.
I've been briefly pondering whether it makes sense to, instead of a boolean, return the dir argument. Then it'd behave more like any allocation function, for which "if it returns something, you must free it" (or pass it on, eg. to a realloc) is a more common pattern. Do you think that'd enhance usability? Binary-wise it'd be pretty much the same, provided the conditional branch tests for zeroness (which is the same for ==NULL and ==false).
Use local variable and pass pointer to helper function See-Also: RIOT-OS#17660 (comment)
|
Please squash! |
This is a cosmetic change when considering vfs_iterate_mounts alone (which now reports FSs in precise mount order rather than reporting the most recently mounted, followed by the rest in mount order), but is helpful for uses of it that need some guarantees in order to produce reliable results even when iteration and (un)mounting interact.
... as thread-safe replacement for vfs_iterate_mounts
... adding precision to the documentation where a corner case was discovered during testing and is permitted. The test is too large for one small board, just like the other existing VFS test.
This solves highly theoretical race conditions of file systems being unmounted in an application while a shell `df` runs, fixes the previous weird behavior that `/mountpoint/non-existant-path` could be df'd and would even report that non-existant path as a file name, but more practically ensures that an example of vfs_iter_mount_dirs is around.
d0da80e to
d60f7ae
Compare
Depends-On: RIOT-OS/RIOT#17660 When that is not present, due to an old RIOT version or a riot-sys version that doesn't yet provide the marker, the mount list is silently shown as empty.
use `vfs_iterate_mount_dirs()` instead deprecated in RIOT-OS#17660
use `vfs_iterate_mount_dirs()` instead deprecated in RIOT-OS#17660
use `vfs_iterate_mount_dirs()` instead deprecated in RIOT-OS#17660
Contribution description
Listing mount points was previously documented as racy, and generic modules using it could barely ensure that nothing else in the system would mount/unmount at impractical times (#17658 was an attempt to fix that by making the mutex public, but that had other downsides).
This introduces an iteration mechanism that is thread-safe because it keeps a directory handle open for the currently viewed FS, and that (just like an open file) keeps the FS from being unmounted.
In bycatch and related changes, it
fstatvfscalls (for lack of reason to have that as a separate driver method, when all users go through the genericstatvfsmethod that has the same properties anyways)vfs_iterate_mountsto produce the actual mount sequence (and not some 2-instructions-shorter-but-confusing reordering that is an artifact of clists), thus making it practical to get a view on the mount points that at least captures those mount points that are not altered during iterationdstatvfs(in analogy tofstatvfs, and it's really the slimmest of all the*statvfscalls because the directory readily contains a valid pointer to the FS)Testing procedure
A new test covers this:
(I tested this with the riot-wrappers, but without actually interspersing mounting with iteration -- and anyhow that's not a practical thing to describe in a PR).
Issues/PRs references
#17658 was an earlier attempt.
The documentation fix was originally in #17659.
[edit: Added reference to #17659; tests described]