vfs: add mount names - #66119
vfs: add mount names#66119pipobscure wants to merge 2 commits into
Conversation
|
Review requested:
|
|
Thanks! Some thoughts:
|
|
@bakkot something you said last night and your comment above ruminated with me and I think you’re right. You said something like “it’s not like you can just fs.readdir /dev/null/vfs” I think that might actually be the natural solution here.
Thoughts? @mcollina this is probably something you should be aware of to vociferously object if you disagree. |
Workers get the same —vfs-mount flags in the same order. Since they are run before startup in he same order and layer numbers are deterministic by order it will have the same initial mounts as the main process. So a worker can be pointed at the same import.meta.dirname relative to the main calling it. This already works and has tests. |
This comment was marked as outdated.
This comment was marked as outdated.
|
Also notice to @jasnell as you were the one to originally critique the version of names I had proposed. I think this actually meets the issues you had as well, but worth double checking. And thanks again to @bakkot ! Pushback makes things better. It did for the original |
vfs.mounted()
mcollina
left a comment
There was a problem hiding this comment.
I like both changes, but I'm unsure if the added machinery is worth it.
How fast is this? It might be simpler to split this into two PRs (one adding the root).
| MapPrototypeForEach(activeNames, (target, name) => { | ||
| if (target === layerId) { | ||
| activeNames.delete(name); | ||
| purgeLoaderCachesForPrefix(getVfsRoot() + sep + name); |
| * }|null} | ||
| */ | ||
| function findVFSOrRoot(inputPath) { | ||
| function resolveVFS(inputPath, followLast = true) { |
There was a problem hiding this comment.
This is a very hot function if I recall correctly. We should do some benchmarks to verify there are no slowdowns.
Also, I don't understand what followLast is used for.
| vfs: activeVFSLayers.get(layerId), | ||
| path, | ||
| normalized: path, | ||
| mountPoint: getVfsRoot() + sep + segment, |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #66119 +/- ##
==========================================
+ Coverage 90.27% 90.29% +0.01%
==========================================
Files 790 791 +1
Lines 271651 272313 +662
Branches 51842 52012 +170
==========================================
+ Hits 245228 245873 +645
- Misses 16928 16947 +19
+ Partials 9495 9493 -2
🚀 New features to boost your workflow:
|
|
Benchmark results for the fs hot path ( I added
This PR vs. its merge base (dd5dfb5), 30 interleaved runs each, n=100000, Welch t-test:
Neither difference is significant. The Within this PR, resolving through a mount name costs about 4% more than resolving by id (three runs each):
For a path outside the VFS root, the only added work compared with main is one string comparison ahead of the existing prefix check. Setup: Windows 11 x64, Release build with clang-cl 19.1.5, 4 cores. |
692498a to
1b1f4e5
Compare
|
What I have left unaddressed so far is splitting into 2 PRs. I’ll do that as soon as I get some focus time. (Possibly tonight or tomorrow) Though I’m a bit worried about conflicts since adding the names/name resolution touches the same things as the root info gathering. But I’ll have a look whether there’s a clean split. |
|
This works too, but it still leaves unanswered the fundamental question of why someone would want to name programmatic mounts. The CLI-provided ones need names, and a programmatic way of looking them up by name, and (Also the Also I still think it's weird to support the unnamed form |
|
@bakkot imagine a suite checking whether /dev/null/vfs/plugins is mounted and if not mounting (at startup) a set of default plugins shipped with the app. The app itself the just always goes to /dev/null/vfs/plugins to get data without having to lookup if they were specified via CLI or it should use the default set supplied. As for the unnamed mounts, they are now reachable and enumerable via |
In the previous design, the way you'd do this would be Though actually now that I'm saying this I don't understand why you'd use |
|
You have a point. What I’m thinking about is packaging an app as a zip archive using something like my bundler where this is just something that crates a level of separation from the app having to handle it. I still think named VFS mounts add value in that they allow you to have a shared place for information without having to inject the mountpoint and pass it all throughout the code. |
|
Well, the app still has to at minimum coordinate to do reading through Re having to pass it around, it just seems like not a very high cost? There's all sorts of config and coordination that a typical app already needs. I don't think that a mounted VFS is any more special than e.g. the app's own CLI arguments, such that it warrants bespoke handling from the node binary itself. |
|
I accept what you’re saying, but that’s really a conversation about removing --vfs-mount entirely. To me it was an easy to do addition. If that’s convincing (it isn’t to me, but it doesn’t have to be) which still leaves the question (separate) of having named mounts at all. I have seen both sides of the argument and both have merit to a degree. However it raises the question of “what is the bar we want to place on new features?” Because we can argue forever about whether named mounts are worth it or not and never reach consensus. me personally, I was happy without named mounts as my use-case is fulfilled by just having --vfs-load and I added this based on feedback, not because I particularly want named mounts. So I guess I want guidance. For now I’ll split it into 2 PRs. One is the capability to |
1b1f4e5 to
5c69f1f
Compare
|
Note: whether we remove/keep --vfs-mount as a thing for other uses, the main reason it's there is that the workers need it. The ordering of --vfs-load and --vfs-mount are kept for workers except that the --vfs-load is mutated into a --vfs-mount. Now the question remains not whether --vfs-mount should exist, but rather whether we want named mounts. And that's a discussion we should have. I'm not particularly botherd either way, but I do think the capability to mount things at a predictable/managable location is a nice to have feature. |
|
That's good context, thanks. (To be clear, --vfs-load definitely makes sense to me!) When you say "transform --vfs-load into a --vfs-mount", do you mean literally the CLI flags, or that the internal thing which is used for --vfs-load gets mapped to the internal thing which is used for --vfs-mount? Because if it's the latter, I agree that capability needs to exist for the reason you say but I don't yet see a reason to expose it to users through a CLI flag. Like, concretely, what's an example of a program you can write using |
|
Right now it's litterally transforming/rewriting the --vfs-load CLI flag into a --vfs-mount CLI flag as I didn't want to hook worker startup into options parsing. By making it a separate flag, I could keep worker startup cleaner and at the same time enable users to also mount additional volumes, so it seemed like a win/win to me. (P.S.: I'll keep saying it, push-back is useful, so thank you for enaging!) |
5c69f1f to
432118d
Compare
|
OK, "this flag exists to simplify starting up workers" is very clarifying. Though, looking at the code, that doesn't seem to actually match what's happening? AFAICT it's still just a Also, relying on the CLI flags for the worker seems fragile; e.g. it breaks entirely if the user specifies their own |
432118d to
9d82f1f
Compare
The reserved root `${os.devNull}/vfs`, which holds the mount points of
all virtual file systems, could not be read: fs calls on it fell through
to the real file system, so nothing could list what was mounted.
While any file system is mounted, serve the root as a read-only
directory. It lists every mount point by its layer id, a recursive
listing descends into each mounted file system, and paths under it that
no mount serves report ENOENT. Creating, removing or changing entries in
it fails with EROFS. When nothing is mounted it does not exist, as
before.
A mount point cannot be removed or renamed, nor replaced by a rename:
rmdir() and rename() fail with EBUSY, and a recursive rm() empties the
file system and then fails the same way. Before, rmdir() of an empty
mount point reported success without doing anything.
Reserve layer 0 for the file system --vfs-load mounts, and number the
others from 1. That source is then at the same reserved mount point in
every thread, whatever else a thread mounts and wherever --vfs-load is
written among the other mounts, so a path into it stays valid in a
worker - including a worker created with its own execArgv, which
inherits none of the parent's options and has to be given --vfs-load
again. A worker still does not run that entry point, but it now has to
recognize which source it belongs to in order to mount it at that layer.
The callback and promise forms of readdir() with `withFileTypes` now
report each Dirent's parentPath as a host path, as readdirSync() did,
instead of the provider-relative one, and split recursive names such as
`dir/file.txt` into their directory and base name. A recursive listing
joins subdirectories with the host separator rather than `/`, which
mixed separators on Windows. realpath() of a mount point no longer
returns it with a trailing separator.
Add vfs.vfsBase(), which returns that directory, so a program can list
what is mounted without spelling out `path.join(os.devNull, 'vfs')`. The
note under vfs.mount() said the path scheme must not be relied on, which
read as a contradiction of the root being listable; it now says where a
mount point comes from, and that only the name within the root is
assigned at runtime.
Signed-off-by: Philipp Dunkel <pip@pipobscure.com>
Nothing identified a mounted virtual file system beyond its mount point,
which is an opaque implementation detail. A mount made with --vfs-mount
in particular could not be found by the program it was mounted for.
Take an optional name as an argument to vfs.mount(). A named mount is
also reachable as `${os.devNull}/vfs/<name>`, a symbolic link in the
reserved root to the mount point: readdir() of the root lists it,
readlink() returns the layer id, and realpath() resolves it to the
mount point. A name at the start of a path is followed to its layer,
except by the operations that act on a link itself (lstat, readlink,
unlink, rm, rename, symlink and the l* variants), which see the link in
the read-only root. require(), import and the fs functions therefore
work through a name, and the loader identifies modules by their real
mount point paths. Loader caches under a name are purged when the name
moves to another mount or goes away.
A later mount under a name takes it over, and unmounting removes the
names linking to that mount. A name must be a single path segment other
than `.` and `..`, and must not be spelled the way a layer id is, as a
non-negative integer in canonical form: `17` is reserved, while `07`
and `-1` are valid names.
--vfs-mount and --vfs-load accept `name=source` and pass the name to
vfs.mount(). Text before the first `=` is only a name if it has no path
separator, so a path containing `=` can still be mounted by writing it
as `./a=b`. --vfs-load takes the prefix too because a worker re-mounts
every source without knowing which one --vfs-load contributed, and must
split each value the same way.
Add a benchmark for the cost of the fs hooks on a real path and on a
layer path reached by id or by name. On Linux x64 (Release, statSync,
n=100000, three runs), resolving through a name averaged about 2% below
resolving by id, within the variation between runs.
Signed-off-by: Philipp Dunkel <pip@pipobscure.com>
9d82f1f to
2d46d34
Compare
Nothing identified a mounted virtual file system beyond its mount point,
which is an opaque implementation detail. A mount made with --vfs-mount
in particular could not be found by the program it was mounted for.
Take an optional name as an argument to vfs.mount(). A named mount is
also reachable as
${os.devNull}/vfs/<name>, a symbolic link in thereserved root to the mount point: readdir() of the root lists it,
readlink() returns the layer id, and realpath() resolves it to the
mount point. A name at the start of a path is followed to its layer,
except by the operations that act on a link itself (lstat, readlink,
unlink, rm, rename, symlink and the l* variants), which see the link in
the read-only root. require(), import and the fs functions therefore
work through a name, and the loader identifies modules by their real
mount point paths. Loader caches under a name are purged when the name
moves to another mount or goes away.
A later mount under a name takes it over, and unmounting removes the
names linking to that mount. A name must be a single path segment other
than
.and.., and must not be spelled the way a layer id is, as anon-negative integer in canonical form:
17is reserved, while07and
-1are valid names.--vfs-mount and --vfs-load accept
name=sourceand pass the name tovfs.mount(). Text before the first
=is only a name if it has no pathseparator, so a path containing
=can still be mounted by writing itas
./a=b. --vfs-load takes the prefix too because a worker re-mountsevery source without knowing which one --vfs-load contributed, and must
split each value the same way.
Add a benchmark for the cost of the fs hooks on a real path and on a
layer path reached by id or by name. On Linux x64 (Release, statSync,
n=100000, three runs), resolving through a name averaged about 2% below
resolving by id, within the variation between runs.
On Windows 11 x64 (Release build with clang-cl 19.1.5, 4 cores,
statSync, n=100000, three runs), resolving through a name averaged about
2% below resolving by id, within the variation between runs. Against
#66140, which this builds on, 30 interleaved runs each showed no
significant difference for a real path (+2.0%, p=0.44) or for a path by
id (+0.6%, p=0.48).