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