Re: [RFC PATCH 0/6] fs/9p: Reuse inode based on path (in addition to qid)

[Dominique Martinet <asmadeus@codewreck.org>]

Tingmao Wang wrote on Wed, Aug 13, 2025 at 12:53:37AM +0100:
> My understanding of cache=loose was that it only assumes that the server
> side can't change the fs under the client, but otherwise things like
> inodes should work identically, even though currently it works differently
> due to (only) cached mode re-using inodes - was this deliberate?

Right, generally speaking I agree things should work as long as the
mount is exclusive (can't change server side/no other client); but
I think it's fine to extend this to server being "sane" (as in,
protocol-wise we're supposed to be guaranteed that two files are
identical if and only if qid is identical)

> > I think that's fine for cache=none, but it breaks hardlinks on
> > cache=loose so I think this ought to only be done without cache
> > (I haven't really played with the cache flag bits, not check pathname if
> > any of loose, writeback or metadata are set?)
> 
> I think currently 9pfs reuse inodes if cache is either loose or metadata
> (but not writeback), given:
> 
> 	else if (v9ses->cache & (CACHE_META|CACHE_LOOSE))
> 		inode = v9fs_get_inode_from_fid(v9ses, fid, dir->i_sb);
> 	else
> 		inode = v9fs_get_new_inode_from_fid(v9ses, fid, dir->i_sb);
> 
> in v9fs_vfs_lookup, so maybe we keep this pattern and not check pathname
> if (loose|metadata) is set (but not writeback)?

This makes sense, let's stick to loose/metadata for this as well.

> >> The main problem here is how to store the pathname in a sensible way and
> >> tie it to the inode.  For now I opted with an array of names acquired with
> >> take_dentry_name_snapshot, which reuses the same memory as the dcache to
> >> store the actual strings
> 
> (Self correction: technically, the space is only shared if they are long
> enough to not be inlined, which is DNAME_INLINE_LEN = 40 for 64bit or 20
> for 32bit, so in most cases the names would probably be copied.  Maybe it
> would be more compact in terms of memory usage to just store the path as a
> string, with '/' separating components?  But then the code would be more
> complex and we can't easily use d_same_name anymore, so maybe it's not
> worth doing, unless this will prove useful for other purposes, like the
> re-opening of fid mentioned below?)

I think it's better to keep things simple first and check the actual
impact with a couple of simple benchmarks (re-opening a file in a loop
with cache=none should hit this path?)

If we want to improve this, rather than keeping the full string it might
make sense to carry a partial hash in each dentry so we can get away
with checking only the parent's dentry + current dentry, or something
like that?
But, simple and working is better than fast and broken, so let's have a
simple v1 first.

> > Frankly the *notify use-case is difficult to support properly, as files
> > can change from under us (e.g. modifying the file directly on the host
> > in qemu case, or just multiple mounts of the same directory), so it
> > can't be relied on in the general case anyway -- 9p doesn't have
> > anything like NFSv4 leases to get notified when other clients write a
> > file we "own", so whatever we do will always be limited...
> > But I guess it can make sense for limited monitoring e.g. rebuilding
> > something on change and things like that?
> 
> One of the first use case I can think of here is IDE/code editors
> reloading state (e.g. the file tree) on change, which I think didn't work
> for 9pfs folders opened with vscode if I remembered correctly (but I
> haven't tested this recently).  Even though we can't monitor for remote
> changes, having this work for local changes would still be nice.

Yeah, I also do this manually with entr[1], and git's fsmonitor (with
watchman) does that too -- so I guess people living in a 9p mount would
see it..
[1] https://github.com/eradman/entr

But I think 9p is just slow in general so such setups can probably be
improved more drastically by using something else :P

Anyway, thank you for your time/work, I'll try to be more timely in
later reviews.
-- 
Dominique Martinet | Asmadeus