On Tue, 30 Jun 2020 17:16:40 +0200 Christian Schoenebeck <qemu_...@crudebyte.com> wrote:
> On Montag, 29. Juni 2020 18:39:02 CEST Greg Kurz wrote: > > On Wed, 03 Jun 2020 19:16:08 +0200 > > > > Christian Schoenebeck <qemu_...@crudebyte.com> wrote: > > > On Sonntag, 19. April 2020 17:06:17 CEST Christian Schoenebeck wrote: > > > > Make top half really top half and bottom half really bottom half: > > > > > > > > Each T_readdir request handling is hopping between threads (main > > > > I/O thread and background I/O driver threads) several times for > > > > every individual directory entry, which sums up to huge latencies > > > > for handling just a single T_readdir request. > > > > > > > > Instead of doing that, collect now all required directory entries > > > > (including all potentially required stat buffers for each entry) in > > > > one rush on a background I/O thread from fs driver by calling the > > > > previously added function v9fs_co_readdir_many() instead of > > > > v9fs_co_readdir(), then assemble the entire resulting network > > > > response message for the readdir request on main I/O thread. The > > > > fs driver is still aborting the directory entry retrieval loop > > > > (on the background I/O thread inside of v9fs_co_readdir_many()) > > > > as soon as it would exceed the client's requested maximum R_readdir > > > > response size. So this will not introduce a performance penalty on > > > > another end. > > > > > > > > Signed-off-by: Christian Schoenebeck <qemu_...@crudebyte.com> > > > > --- > > > > > > > > hw/9pfs/9p.c | 122 +++++++++++++++++++++++---------------------------- > > > > 1 file changed, 55 insertions(+), 67 deletions(-) > > > > > > Ping. Anybody? > > > > > > I would like to roll out this patch set soon and this is the only patch in > > > this series missing a review yet. > > > > Hi Christian, > > Hi Greg, > > > Sorry for getting back to this only now :-\ > > > > So I still have some concerns about the locking of the directory stream > > pointer a fid. It was initially introduced to avoid concurrent accesses > > by multiple threads to the corresponding internal glibc object, as > > recommended in the readdir(3) manual page. Now, this patch considerably > > extends the critical section to also contain calls to telldir() and all > > the _many_ readdir()... so I'm not sure exactly what's the purpose of > > that mutex right now. Please provide more details. > > The intention of this lock is to prevent concurrent r/w (i.e. telldir()/ > readdir()) of the dir stream position by two or more active readdir requests > handled in parallel, provided that they would use the same FID. Due to the > latter requirement for this to become relevant, we already agreed that this > is > not something that would usually happen with a Linux 9p client, but there is > nothing from protocol point of view that would prohibit this to be done by a > client, so the resulting undefined behaviour should be prevented, which this > lock does. > Makes sense. The dir stream position is no different from glibc's internal dirent in that respect: it shouldn't be used by concurrent threads. > What's your precise concern? > My overall concern is still the same. The patches are big and I've been away for too long, so I need some more discussion to reassemble my thoughts and the puzzle :) But now that I'm starting to understand why it's a good thing to extend the critical section, I realize it should be extended even more: we also have calls to seekdir() and rewindir() in v9fs_readdir() and friends that could _theoretically_ cause the very same kind of undefined behavior you're mentioning. I think the change is important enough that it deserves its own patch. I expect that moving the locking to the top-level handler might also simplify the existing code, and thus your series as well. Please let me come up with a patch first. > Best regards, > Christian Schoenebeck > > Cheers, -- Greg
