Re: [PATCH v4 10/11] 9pfs: T_readdir latency optimization

[Christian Schoenebeck <qemu_oss@crudebyte.com>]

On Dienstag, 17. März 2020 15:14:23 CET Greg Kurz wrote:
> > > > I think the cause of disagreements we have are solely our use cases of
> > > > 9pfs: your personal use case does not seem to require any performance
> > > > considerations or multi-user aspects, whereas my use case requires at
> > > > least some minimum performance grade for utilizing 9pfs for server
> > > > applications.
> > > 
> > > Your point about the personal use case is right indeed but our
> > > disagreements, if any, aren't uniquely related to that. It's more about
> > > maintainership and available time really. I'm 100% benevolent "Odd
> > > fixer"
> > > now and I just try to avoid being forced into fixing things after my PR
> > > is
> > > merged. If you want to go faster, then you're welcome to upgrade to
> > > maintainer and send PRs. This would make sense since you care for 9p,
> > > you
> > > showed a good understanding of the code and you provided beneficial
> > > contributions so far :)
> > 
> > That maintainership upgrade is planned. The question is just when. What
> > was
> > your idea, co-maintainership?
> 
> Basically yes.

Ok, I'll send out a MAINTAINERS patch tomorrow or so to make that
co-maintainer upgrade official. But I obviously still need a while to learn 
the bureaucracy involved for PRs, signing, ML tools, Wiki, etc.

> > > > > Ok, fair enough to leave 9p2000.U behind for now but I had to ask :)
> > > > > /me is even wondering if we should start deprecating 9p2000.U since
> > > > > most clients can use 9p2000.L now...
> > > > 
> > > > I probably wouldn't. On macOS for instance there's only 9p2000.u. In
> > > > the
> > > > end
> > > 
> > > Hmm... the only thing I've heard about is:
> > > 
> > > https://github.com/benavento/mac9p
> > > 
> > > and unless I'm missing something, this seems to only have a socket based
> > > transport... the server in QEMU is for virtio-9p and Xen 9p devices
> > > only.
> > > Unless mac9p seriously plans to implement a driver for either of those,
> > > I'm still not convinced it is worth keeping .U around... and BTW, our
> > > deprecation policy imposes a 2 QEMU versions cooling period before
> > > actually removing the code. This would leave ample time for someone
> > > to speak up.
> > 
> > Well, I leave that up to you. I could consider adding a transport for
> > macOS
> > guests, but that's definitely not going to happen in any near future.
> 
> The whole idea behind dropping support for .U is really just about
> reducing the potential attack surface. But I'm also fine to keep
> things as is until the next CVE since I'm confident someone will
> help ;-)

Sure, sounds like a plan!

> > > > > > the loop. The normal case is not requiring a lock at all, like the
> > > > > > comment
> > > > > > describes. Doing multiple locks inside the loop unnecessaririly
> > > > > > reduces
> > > > > > performance for no reason.
> > > > > > 
> > > > > > About *_trylock() instead of v9fs_readdir_lock(): might be a
> > > > > > candidate
> > > > > > to
> > > > > 
> > > > > Hmm... are you suggesting that do_readdir_lowlat() should return if
> > > > > it
> > > > > can't take the lock ?
> > > > 
> > > > ... yep, that's what I had in mind for it later on. I would not mind
> > > > running trylock in a loop like you suggested; if it fails, log a
> > > > warning
> > > > and return. Because if the lock fails, then client is buggy. User can
> > > > read the warning in the logs and report the bug for client to be
> > > > fixed.
> > > > Not worth to handle that case in any fancy way by server.
> > > 
> > > The locking is just a safety net because readdir(3) isn't necessarily
> > > thread safe. It was never my intention to add an error path for that.
> > > And thinking again, sending concurrent Treaddir requests on the same
> > > fid is certainly a weird thing to do but is it really a bug ?
> > 
> > Well, I was unresolved on that issue as well, hence my decision to only
> > leave a TODO comment for now. My expectation would be separate fid for
> > concurrent readdir, but you are right of course, Treaddir (unlike its
> > 9p2000.u counterpart) is reentrant by design, since it has an offset
> > parameter, so from protocol perspective concurrent Treaddir is not per se
> > impossible.
> > 
> > The lock would not be noticable either with this patch, since unlike on
> > master, the lock is just retained on driver side now (with this patch),
> > which returns very fast even on large directories, as you can see from
> > the benchmark output.
> > 
> > So yes, returning from function with an error is probably not the best
> > choice. My tendency is still though logging a message at least. I don't
> > care about that too much though to deal with trylock() in a loop or
> > something right now. There are more important things to take care of ATM.
> 
> Yeah, that can wait.

Okay.

The only thing I actually still need to figure out for this patch series to be 
complete (at least from my side), is how to fix the test environment bug 
discussed. Probably I can reset the test environment's buffer after every test 
somehow ...

Best regards,
Christian Schoenebeck