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

[Jens Axboe <axboe@kernel.dk>]

On 2/25/20 2:41 PM, Jens Axboe wrote:
> On 2/25/20 2:25 PM, 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.
> 
> Because I'm a clown, the patch applied with offset. I meant to modify
> the __io_queue_sqe() path, as mentioned in the commit message. Here's
> the right one, dropped the check
> 
> The other one would not be correct without the nxt check, as the work
> queue handler could pass it back. For the __io_queue_sqe() path, we
> should do a 5.7 cleanup of the 'nxt passing, though. I don't want to
> do that for 5.6.

So this found something funky, we really should only be picking up
the next request if we're dropping the final reference to the
request. And io_put_req_find_next() also says that in the comment,
but it always looks it up. That doesn't seem safe at all, I think
this is what it should be:


commit eff5fe974f332c1b86c9bb274627e88b4ecbbc85
Author: Jens Axboe <axboe@kernel.dk>
Date:   Tue Feb 25 13:25:41 2020 -0700

    io_uring: pick up link work on submit reference drop
    
    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.
    
    This also fixes an issue with io_put_req_find_next(), which always looks
    up the next work item. That should only be done if we're dropping the
    last reference to the request, to prevent multiple lookups of the same
    work item.
    
    Signed-off-by: Jens Axboe <axboe@kernel.dk>

diff --git a/fs/io_uring.c b/fs/io_uring.c
index ffd9bfa84d86..f79ca494bb56 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -1483,10 +1483,10 @@ static void io_free_req(struct io_kiocb *req)
 __attribute__((nonnull))
 static void io_put_req_find_next(struct io_kiocb *req, struct io_kiocb **nxtptr)
 {
-	io_req_find_next(req, nxtptr);
-
-	if (refcount_dec_and_test(&req->refs))
+	if (refcount_dec_and_test(&req->refs)) {
+		io_req_find_next(req, nxtptr);
 		__io_free_req(req);
+	}
 }
 
 static void io_put_req(struct io_kiocb *req)
@@ -4749,7 +4749,7 @@ static void __io_queue_sqe(struct io_kiocb *req, const struct io_uring_sqe *sqe)
 
 err:
 	/* drop submission reference */
-	io_put_req(req);
+	io_put_req_find_next(req, &nxt);
 
 	if (linked_timeout) {
 		if (!ret)

-- 
Jens Axboe