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
