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

[Hao Xu <hao.xu@linux.dev>]

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