Re: [PATCH] io_uring: pick up link work on submit reference drop

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

On 26/02/2020 00:45, Pavel Begunkov wrote:
> On 26/02/2020 00:25, Jens Axboe wrote:
>> On 2/25/20 2:22 PM, Pavel Begunkov wrote:
>>> On 25/02/2020 23:27, Jens Axboe wrote:
>>>> If work completes inline, then we should pick up a dependent link item
>>>> in __io_queue_sqe() as well. If we don't do so, we're forced to go async
>>>> with that item, which is suboptimal.
>>>>
>>>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>>>>
>>>> ---
>>>>
>>>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>>>> index ffd9bfa84d86..160cf1b0f478 100644
>>>> --- a/fs/io_uring.c
>>>> +++ b/fs/io_uring.c
>>>> @@ -4531,8 +4531,15 @@ static void io_wq_submit_work(struct io_wq_work **workptr)
>>>>  		} while (1);
>>>>  	}
>>>>  
>>>> -	/* drop submission reference */
>>>> -	io_put_req(req);
>>>> +	/*
>>>> +	 * Drop submission reference. In case the handler already dropped the
>>>> +	 * completion reference, then it didn't pick up any potential link
>>>> +	 * work. If 'nxt' isn't set, try and do that here.
>>>> +	 */
>>>> +	if (nxt)
>>>
>>> It can't even get here, because of the submission ref, isn't it? would the
>>> following do?
>>>
>>> -	io_put_req(req);
>>> +	io_put_req_find_next(req, &nxt);
>>
>> I don't think it can, let me make that change. And test.
>>
>>> BTW, as I mentioned before, it appears to me, we don't even need completion ref
>>> as it always pinned by the submission ref. I'll resurrect the patches doing
>>> that, but after your poll work will land.
>>
>> We absolutely do need two references, unfortunately. Otherwise we could complete
>> the io_kiocb deep down the stack through the callback.
> 
> And I need your knowledge here to not make mistakes :)
> I remember the conversation about the necessity of submission ref, that's to
> make sure it won't be killed in the middle of block layer, etc. But what about
> removing the completion ref then?
> 
> E.g. io_read(), as I see all its work is bound by lifetime of io_read() call,
> so it's basically synchronous from the caller perspective. In other words, it
> can't complete req after it returned from io_read(). And that would mean it's
> save to have only submission ref after dealing with poll and other edge cases.
> 
> Do I miss something?

Hmm, just started to question myself, whether handlers can be not as synchronous
as described...

-- 
Pavel Begunkov