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

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

On 2/26/2020 11:33 AM, Pavel Begunkov wrote:
> On 26/02/2020 01:18, Jens Axboe wrote:
>> 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:
> 
> It was weird indeed, it looks good. And now it's safe to do the same in
> io_wq_submit_work().
> 
> Interestingly, this means that passing @nxt into the handlers is useless, as
> they won't ever return !=NULL, isn't it? I'll prepare the cleanup.

... and it will return @nxt==NULL as well, as there is a ref hold by
io_{put/get}_work().

Let's have this patch as is, and I'll cook up something on top
Reviewed-by: Pavel Begunkov <asml.silence@gmail.com>

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

-- 
Pavel Begunkov