Re: [PATCH 1/1] io_uring: replace inflight_wait with tctx->wait

[Jens Axboe <axboe@kernel.dk>]

On 11/16/20 10:16 AM, Pavel Begunkov wrote:
> On 16/11/2020 16:57, Jens Axboe wrote:
>> On 11/16/20 9:48 AM, Pavel Begunkov wrote:
>>> On 16/11/2020 16:33, Jens Axboe wrote:
>>>> On 11/15/20 5:56 AM, Pavel Begunkov wrote:
>>>>> As tasks now cancel only theirs requests, and inflight_wait is awaited
>>>>> only in io_uring_cancel_files(), which should be called with ->in_idle
>>>>> set, instead of keeping a separate inflight_wait use tctx->wait.
>>>>>
>>>>> That will add some spurious wakeups but actually is safer from point of
>>>>> not hanging the task.
>>>>>
>>>>> e.g.
>>>>> task1                   | IRQ
>>>>>                         | *start* io_complete_rw_common(link)
>>>>>                         |        link: req1 -> req2 -> req3(with files)
>>>>> *cancel_files()         |
>>>>> io_wq_cancel(), etc.    |
>>>>>                         | put_req(link), adds to io-wq req2
>>>>> schedule()              |
>>>>>
>>>>> So, task1 will never try to cancel req2 or req3. If req2 is
>>>>> long-standing (e.g. read(empty_pipe)), this may hang.
>>>>
>>>> This looks like it's against 5.11, but also looks like we should add
>>>> it for 5.10?
>>>
>>> Yeah, 5.10 completely slipped my mind, I'll resend
>>
>> I applied it to 5.10, and fixed up the 5.11 side of things. So all good,
>> just wanted to confirm.
> 
> Hmm, this won't work with 5.10, at least without
> 
> b7e7fb9960b03c ("io_uring: cancel only requests of current task")
> https://git.kernel.dk/cgit/linux-block/commit/?h=for-5.11/io_uring&id=b7e7fb9960b03ca07866b5c016ac3ce5373ef207
> 
> That's because tctx->wait is kicked only by requests of current task,
> but 5.10 cancels everything with specified ->files, including owned
> by other tasks.

Ah good point, let's leave it 5.11 for now.

-- 
Jens Axboe