Re: [PATCH 3/3] io_uring: add support for getdents

[Dominique Martinet <asmadeus@codewreck.org>]

Hao Xu wrote on Wed, Jul 12, 2023 at 03:53:24PM +0800:
> > > +		if (file_count(file) > 1)
> > 
> > I was curious about this so I found it's basically what __fdget_pos does
> > before deciding it should take the f_pos_lock, and as such this is
> > probably correct... But if someone can chime in here: what guarantees
> > someone else won't __fdget_pos (or equivalent through this) the file
> > again between this and the vfs_getdents call?
> > That second get would make file_count > 1 and it would lock, but lock
> > hadn't been taken here so the other call could get the lock without
> > waiting and both would process getdents or seek or whatever in
> > parallel.
> > 
> 
> This file_count(file) is atomic_read, so I believe no race condition here.

I don't see how that helps in the presence of another thread getting the
lock after we possibly issued a getdents without the lock, e.g.

t1 call io_uring getdents here
t1 sees file_count(file) == 1 and skips getting lock
t1 starts issuing vfs_getdents [... processing]
t2 calls either io_uring getdents or getdents64 syscall
t2 gets the lock, since it wasn't taken by t1 it can be obtained
t2 issues another vfs_getdents

Christian raised the same issue so I'll leave this to his part of the
thread for reply, but I hope that clarified my concern.


-----

BTW I forgot to point out: this dropped the REWIND bit from my patch; I
believe some form of "seek" is necessary for real applications to make
use of this (for example, a web server could keep the fd open in a LRU
and keep issuing readdir over and over again everytime it gets an
indexing request); not having rewind means it'd need to close and
re-open the fd everytime which doesn't seem optimal.

A previous iteration discussed that real seek is difficult and not
necessarily needed to I settled for rewind, but was there a reason you
decided to stop handling that?

My very egoistical personal use case won't require it, so I can just say
I don't care here, but it would be nice to have a reason explained at
some point

-- 
Dominique Martinet | Asmadeus