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

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

[Jens Axboe]

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)
+		io_put_req(req);
+	else
+		io_put_req_find_next(req, &nxt);
 
 	if (ret) {
 		req_set_fail_links(req);

-- 
Jens Axboe

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

[Pavel Begunkov]

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

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.


> +		io_put_req(req);
> +	else
> +		io_put_req_find_next(req, &nxt);
>  
>  	if (ret) {
>  		req_set_fail_links(req);
> 

-- 
Pavel Begunkov

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

[Jens Axboe]

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.

-- 
Jens Axboe

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

[Jens Axboe]

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.

commit 7df2fa5c9f6e92b2f80f4699425b463973d5242b
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.
    
    Signed-off-by: Jens Axboe <axboe@kernel.dk>

diff --git a/fs/io_uring.c b/fs/io_uring.c
index ffd9bfa84d86..c4ed8601e225 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -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

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

[Pavel Begunkov]

[-- Attachment #1.1: Type: text/plain, Size: 2183 bytes --]

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?

-- 
Pavel Begunkov


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Pavel Begunkov]

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

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

[Jens Axboe]

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

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

[Jens Axboe]

On 2/25/20 2:52 PM, Pavel Begunkov wrote:
> 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...

It very much can complete the req after io_read() returns, that's what
happens for any async disk request! By the time io_read() returns, the
request could be completed, or it could just be in-flight. This is
different from lots of the other opcodes, where the actual call returns
completion sync (either success, or EAGAIN).

-- 
Jens Axboe

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

[Pavel Begunkov]

On 26/02/2020 01:24, Jens Axboe wrote:
> It very much can complete the req after io_read() returns, that's what
> happens for any async disk request! By the time io_read() returns, the
> request could be completed, or it could just be in-flight. This is
> different from lots of the other opcodes, where the actual call returns
> completion sync (either success, or EAGAIN).

For some reason, I've got the idea that it do the same things as
__vfs_read/write. At least I don't see the difference between
io_read_prep()+io_read() and new_sync_read().
Thanks for the explanation, I should drop these futile attempts.

-- 
Pavel Begunkov

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

[Pavel Begunkov]

[-- Attachment #1.1: Type: text/plain, Size: 2209 bytes --]

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.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Pavel Begunkov]

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

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

[Jens Axboe]

On 2/26/20 1: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().

It is.

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

Indeed! That means we can get rid of that. We should do that for 5.7, though.

-- 
Jens Axboe