Skip to content

vfs: Expose vfs_mount_mutex to make vfs_iterate_mounts usable reliably #17658

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.

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

Closed

Conversation

chrysn
Copy link
Member

@chrysn chrysn commented Feb 15, 2022

Contribution description

The conditions on using vfs_iterate_mounts are hard to guarantee in any
system where mounts happen outside auto_init, and impossible to
guarantee for a generic component.

This makes the mount point mutex public, along with documentation on what goes bad if held for too long.

Maturity level

This is more a documentation of something attempted than a full PR -- the big downsides are in the documentation.

[edit: Also, there is an ugly #define in there, but that's just to keep the delta low; if this PR were followed up on, a fixup commit would contain the text replacement of _mount_mutex to vfs_mount_mutex and remove that define]

More illustratively, all use of this I can imagine will fall into a single pattern:

  1. Initialize last-seen-FS pointer as NULL
  2. Lock mutex
  3. Iterate over vfs_iterate_mounts (starting from NULL) until after the last-seen-FS pointer. (We can't start from last-seen-FS because that may be invalid by now; comparing is somewhat OK because if someone sneakily remounts an FS we see either the first or the second version of it, or see a truncated list of mounts which is kind of OK during a remount).
  4. If the iteration is done, break; otherwise, store as last-seen-FS.
  5. Copy path out into a buffer
  6. Unlock mutex
  7. Do something with the path in the buffer (typically vfs_opendir or vfs_statvfs)
  8. Loop back to 2.

The only applications that does not fall into this pattern is plainly listing paths of mount points (without doing anything more with them, that's my application in the CoAP file system server), or those that have sufficient room to first list the mount points and then stat / listdir them from the stored path buffers.

Next steps

As that seems inconvenient, I'd try something else: Maybe a similar function to vfs_iterate_mounts could produce open vfs_DIR. These could then be either be readdir'd, or fstatvfs'd (TBD: does that even work, or do we need a dstatvfs for that?), or fd2path'd (TBD).

All not trivial :-/

Pinging @jnohlgard for any insights from back when the VFS was crafted, and @benpicco who briefly engaged in yesterday's IRC discussion on the mutex.

The conditions on using vfs_iterate_mounts are hard to guarantee in any
system where mounts happen outside auto_init, and impossible to
guarantee for a generic component.
@github-actions github-actions bot added the Area: sys Area: System label Feb 15, 2022
@chrysn chrysn added Area: fs Area: File systems State: demonstrator State: This PR (loosely) demonstrates a concept and is not necessarily meant to be merged. labels Feb 15, 2022
@chrysn
Copy link
Member Author

chrysn commented Feb 15, 2022

I now think #17660 is the better approach; will close this as soon as that receives any positive review.

@chrysn
Copy link
Member Author

chrysn commented Feb 15, 2022

As that seems inconvenient, I'd try something else: Maybe a similar function to vfs_iterate_mounts could produce open vfs_DIR. These could then be either be readdir'd, or fstatvfs'd (TBD: does that even work, or do we need a dstatvfs for that?), or fd2path'd (TBD).

These turned out to be simple: dstatvfs was easy to introduce, and fd2path is not needed on the mount point if one is willing to dip one's toes into the forbidden dir's internal ->mp member and look there -- that already has the path conveniently available (as the root dir's path is the mountpoint's path) and sufficiently longlived (as the dir can't be umounted while alive).

@chrysn chrysn closed this Feb 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: fs Area: File systems Area: sys Area: System State: demonstrator State: This PR (loosely) demonstrates a concept and is not necessarily meant to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant