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

[Tingmao Wang]

On 8/8/25 11:27, Dominique Martinet wrote:
> Sorry for the delay...

No worries, thanks for picking this up!

> 
> Tingmao Wang wrote on Sun, Apr 06, 2025 at 09:43:01PM +0100:
>> Unrelated to the above problem, it also seems like even with the revert in
>> [2], because in cached mode inode are still reused based on qid (and type,
>> version (aka mtime), etc), the setup mentioned in [2] still causes
>> problems in th latest kernel with cache=loose:
> 
> cache=loose is "you're on your own", I think it's fine to keep as is,
> especially given qemu can handle it with multidevs=remap if required

On 8/8/25 11:52, Christian Schoenebeck wrote:
>> [...]
> 
> As of QEMU 10.0, multidevs=remap (i.e. remapping inodes from host to guest) is
> now the default behaviour, since inode collisions were constantly causing
> issues and confusion among 9p users.
> 
> And yeah, cache=loose means 9p client is blind for whatever changes on 9p
> server side.
> 
> /Christian
> 

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?

Basically, assuming no unexpected server side changes, I would think that
the user would not be totally "on their own" (even if there are colliding
qids).  But given the issue with breaking how hardlinks currently works in
cached mode, as Dominique mentioned below, and the fact that inode
collisions should be rare given the new QEMU default, maybe we do need to
handle this differently (i.e. keep the existing behaviour and working
hardlinks for cached mode)?  I'm happy either way (Landlock already works
currently with cached mode)

On 8/8/25 11:27, Dominique Martinet wrote:
> Tingmao Wang wrote on Sun, Apr 06, 2025 at 09:43:01PM +0100:
> [...]
>> With the above in mind, I have a proposal for 9pfs to:
>> 1. Reuse inodes even in uncached mode
>> 2. However, reuse them based on qid.path AND the actual pathname, by doing
>>    the appropriate testing in v9fs_test(_new)?_inode(_dotl)?
> 
> 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)?

>> 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?)

>> , but doesn't tie the lifetime of the dentry with
>> the inode (I thought about holding a reference to the dentry in the
>> v9fs_inode, but it seemed like a wrong approach and would cause dentries
>> to not be evicted/released).
> 
> That's pretty hard to get right and I wish we had more robust testing
> there... But I guess that's appropriate enough.
> 
> I know Atos has done an implementation that keeps the full path
> somewhere to re-open fids in case of server reconnections, but that code
> has never been submitted upstream that I can see so I can't check how
> they used to store the path :/ Ohwell.
> 
>> Storing one pathname per inode also means we don't reuse the same inode
>> for hardlinks -- maybe this can be fixed as well in a future version, if
>> this approach sounds good?
> 
> Ah, you pointed it out yourself. I don't see how we could fix that, as
> we have no way other than the qid to identify hard links; so this really
> ought to depend on cache level if you want to support landlock/*notify
> in cache=none.

In that case, and as discussed above, I'm happy to change this patch
series to only affect uncached.

> 
> 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.


Note that aside from Mickaël's comments which I will apply in the next
version, I also realized that I haven't done the proper handling for
renames (it should probably update the v9fs_ino_path in the renamed
inode).  I will do that in the next version (in addition to changing the
cached behaviour).  Do let me know if you have any other comment on the
patch series.

Best,
Tingmao