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

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

On 7/13/23 12:05, Hao Xu wrote:
> 
> 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.

After re-think of it, I think there is no race in fixed file case as
well, because the f_count is always >1


> 
> 
>>
>> -----
>>
>> 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
> 
>