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

[Pavel Begunkov <asml.silence@gmail.com>]

On 09/09/2020 19:07, Jens Axboe wrote:
> On 9/9/20 9:48 AM, Pavel Begunkov wrote:
>> On 09/09/2020 16:10, Jens Axboe wrote:
>>> 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
>>
>> I understand that it isn't hard, but I just don't want to expose it to
>> the userspace, a) because it's a userspace API, so couldn't probably be
>> killed in the future, b) works around kernel's problems, and so
>> shouldn't really be exposed to the userspace in normal circumstances.
>>
>> And it's not generic enough because of a possible "many fds -> single
>> file" mapping, and there will be a lot of questions and problems.
>>
>> e.g. if a process shares a io_uring with another process, then
>> dup()+close() would require not only this hook but also additional
>> inter-process synchronisation. And so on.
> 
> I think you're blowing this out of proportion. Just to restate the

I just think that if there is a potentially cleaner solution without
involving userspace, we should try to look for it first, even if it
would take more time. That was the point.

> goal, but it's to have SQPOLL be as useful as the other modes. One of
> those things is making non-registered files work. For some use cases,
> registered files is fine, for others it's basically a non-starter.> With that out of the way, the included patch handles the "close ring
> fd case". You're talking about the dup or receive case, or anything
> that doesn't close an existing ring. And yes, that won't work as-is,
> because we know have multiple fds for that particular ring. That boils
> the case down to "we're now using this fd for the ring", and the only
> requirement here would be to ensure you do a io_uring_enter() if you
> decide to swap fds or use a new fd. Only caveat here is that we can't
> make it automatic like we can for the "old fd gets closed" case, so
> the app would absolutely have to ensure it enters the kernel if it
> uses a new fd.
> 
> Not really a huge deal imho in terms of API, especially since this
> is into the realm of "nobody probably ever does this, or if they do,
> then this requirement isn't really a problem".
> 
>>> 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.
>>
>> I miss the whole story, have you asked fs guys about the problem?
>> Or is it known that nothing would work?
> 
> I haven't looked into it.

Any chance you have someone in mind who can take a look? I don't
think I have a chance to get to anyone in fs.

-- 
Pavel Begunkov