Re: [PATCH for-next] io_uring: ensure IOSQE_ASYNC file table grabbing works, with SQPOLL

[Jens Axboe <axboe@kernel.dk>]

On 9/9/20 1:09 AM, Pavel Begunkov wrote:
> On 09/09/2020 01:54, Jens Axboe wrote:
>> On 9/8/20 3:22 PM, Jens Axboe wrote:
>>> On 9/8/20 2:58 PM, Pavel Begunkov wrote:
>>>> On 08/09/2020 20:48, Jens Axboe wrote:
>>>>> Fd instantiating commands like IORING_OP_ACCEPT now work with SQPOLL, but
>>>>> we have an error in grabbing that if IOSQE_ASYNC is set. Ensure we assign
>>>>> the ring fd/file appropriately so we can defer grab them.
>>>>
>>>> IIRC, for fcheck() in io_grab_files() to work it should be under fdget(),
>>>> that isn't the case with SQPOLL threads. Am I mistaken?
>>>>
>>>> And it looks strange that the following snippet will effectively disable
>>>> such requests.
>>>>
>>>> fd = dup(ring_fd)
>>>> close(ring_fd)
>>>> ring_fd = fd
>>>
>>> Not disagreeing with that, I think my initial posting made it clear
>>> it was a hack. Just piled it in there for easier testing in terms
>>> of functionality.
>>>
>>> But the next question is how to do this right...> 
>> Looking at this a bit more, and I don't necessarily think there's a
>> better option. If you dup+close, then it just won't work. We have no
>> way of knowing if the 'fd' changed, but we can detect if it was closed
>> and then we'll end up just EBADF'ing the requests.
>>
>> So right now the answer is that we can support this just fine with
>> SQPOLL, but you better not dup and close the original fd. Which is not
>> ideal, but better than NOT being able to support it.
>>
>> Only other option I see is to to provide an io_uring_register()
>> command to update the fd/file associated with it. Which may be useful,
>> it allows a process to indeed to this, if it absolutely has to.
> 
> Let's put aside such dirty hacks, at least until someone actually
> needs it. Ideally, for many reasons I'd prefer to get rid of

BUt it is actually needed, otherwise we're even more in a limbo state of
"SQPOLL works for most things now, just not all". And this isn't that
hard to make right - on the flush() side, we just need to park/stall the
thread and clear the ring_fd/ring_file, then mark the ring as needing a
queue enter. On the queue enter, we re-set the fd/file if they're NULL,
unpark/unstall the thread. That's it. I'll write this up and test it.

> fcheck(ctx->ring_fd) in favour of synchronisation in io_grab_files(),
> but I wish I knew how.

That'd be nice, and apply equally to all cases as the SQPOLL case isn't
special at all anymore.

-- 
Jens Axboe