Re: recap of 9p problems in 6.9

[Dominique Martinet <asmadeus@codewreck.org>]

Replying to both mails at once

Eric Van Hensbergen wrote on Mon, Apr 08, 2024 at 11:14:07PM +0000:
> Thanks for looking into these Dominique - I've got the reports queued
> up and am just looking to ring fence some time to dig into them before
> I commit to a revert.  The change in handling of inode matching and
> keeping inodes around in the cache longer are likely the root of all
> of these, but the old code had so much unexplained corner cases I
> really want to understand the problems before revert.

My opinion here is that there's been quite a few people reporting to be
impacted, so while I agree we need to understand the old cruft
eventually I think it's time to rollback and try harder to reproduce
the various errors we've been reported and investigate in another branch
(not even -next, as that is also used by people for non-reg -- breaking
other parts of the kernel's non-reg will just make people stop using 9p)

And when we do finally manage to reproduce this that'll make another
workload we can test regularly for future regressions, I think we need
more of these (like my parallel unlink code); everything I'm running is
either too nice (e.g. proper build without parallel write accesses
within a file) or too synthetic (fsstress etc); more "real-world" tests
can't hurt.

> The create/remove a file multiple times in parallel could be linked to
> an underlying issue with only using qid.path for match -- the old
> extended match logic also matched i_generation number -- but I
> convinced myself it wasn't necessary and it required a fresh getattr
> very time which seemed excessive.  The alternative is to work this
> into a server-side fix, but if that breaks all legacy than that's
> likely unacceptable.

There might be a new problem but I definitely also hit some refcount bug
in the old code with that, so I'm thinking there's a place where the ref
isn't obtained in the right order causing a race or maybe it's missing a
ref for the dentry list or something (we properly unref when freeing the
dentry, but I'm not sure I trust all paths to properly take that ref
when adding to the dentry)

That's a hole I digged myself, I've just fixed perf (again[1]) to allow
hooking on the fid refcount tracepoint and I'll dig into this.
If there's a reproducer that should be easy to figure out even if
there's a bit of noise, will work on it over the next few weekends.

[1] https://lore.kernel.org/linux-perf-users/ZhPn0EwXf_bPBj-p@codewreck.org/T/#m8768e2b920e7111364c8f1309f09edb607d5aec5

Eric Van Hensbergen wrote on Tue, Apr 09, 2024 at 12:11:05AM +0000:
> April 7, 2024 at 11:17 PM, "Dominique Martinet" <asmadeus@codewreck.org> wrote:
> > * open / unlink / fstat|ftruncate etc fail
> > 
> > https://lkml.kernel.org/r/E7D462A2-EE93-4A57-9F15-8565EE1567F3@linux.dev
> > 
> > I didn't confirm yet but I think it's a new bug? maybe the 'fix dups 
> > even in uncached mode' patch dropping v9fs_drop_inode(); that's easy
> > enough to test just a new bug so didn't look yet
> 
> I was thinking about this one a bit more -- this smells like the old
> anonymous file bug -- but I started thinking a bit about it.  fstat
> ultimately leads to a getattr -- in 9p classic those happen against
> unopen files only so it'll go looking for a transient fid which won't
> be there if we've unlinked.  Its possible that in this scenario we
> should return the "cached" information for the file (which we have in
> the inode) if we can't find the fid via a walk (which we won't if its
> unlinked).

The file is still open so we must have a fid around; in cached mode it's
probably ok to return cached information (and a quick look makes me
think we do- we're not failing fstat here) but in cache=none we just
need to send a TGETATTR with that fid... and I just checked and we're
doing this correctly; it's qemu using fstatat with the parent directory
and file name and failing with ENOENT instead of using fstat on the
opened file, I was sure this worked before but this apparently hasn't
changed with 6.9 ; sorry for the wrong conclusion.

> Not sure if ftruncate is that same problem or different - right now
> the truncate code seems to be in setattr.  Need to do a bit of tracing
> to see if that's the path we are in.  There was a truncate bug spotted
> in legacy 9p, but I think that's something different.

Yeah, I think we're doing the right thing here, we're sending setattr on
the opened fid and qemu fails as well.

I checked qemu code and this doesn't seem to have changed recently, so
I'll reply to the other thread saying that probably never worked and
needs addressing on the server.

That leaves the other bus error problem that might be the same as
Kent's ultimately, but I can't figure how to hit that yet.


> It would be really nice if we could just get to the file structure,
> but in both of these cases that seems to be "lost along the way" in
> the VFS call-chain.

Yeah there are many places where we could really use the file structure
to get the fid that operation was meant to be on and we can't, but at
least the lookup from dentry here seems to work!

Sorry for the noise on this, we can take that bug more leisurely.
-- 
Dominique Martinet | Asmadeus