On 7/13/23 00:10, Dominique Martinet wrote:
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.
Hi Dominique,
Ah, I misunderstood your question, sorry. The thing is f_count is
init-ed to be 1,
and normal uring requests do fdget first, so I think it's ok for normal
requests.
What Christian points out is issue with fixed file, that is indeed a
problem I think.
-----
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
Yes, like Al pointed out, getdents with an offset is not the right way
to do it,
So a way to do seek is a must. But like what I said in the cover-letter,
I do think the right thing is to
import lseek/llseek to io_uring, not increment the complex of getdents.
Thanks,
Hao
