Re: [PATCH 5.13 v2] io_uring: maintain drain requests' logic

Hao Xu
2021-04-12T15:29:04Z

在 2021/4/12 下午11:07, Hao Xu 写道:
> 在 2021/4/9 下午3:50, Pavel Begunkov 写道:
>> On 09/04/2021 08:05, Hao Xu wrote:
>>> 在 2021/4/9 下午2:15, Hao Xu 写道:
>>>> 在 2021/4/9 上午12:18, Jens Axboe 写道:
>>>>> On 4/8/21 6:22 AM, Pavel Begunkov wrote:
>>>>>> On 08/04/2021 12:43, Hao Xu wrote:
>>>>>>> 在 2021/4/8 下午6:16, Hao Xu 写道:
>>>>>>>> 在 2021/4/7 下午11:49, Jens Axboe 写道:
>>>>>>>>> On 4/7/21 5:23 AM, Hao Xu wrote:
>>>>>>>>>> more tests comming, send this out first for comments.
>>>>>>>>>>
>>>>>>>>>> Hao Xu (3):
>>>>>>>>>>      io_uring: add IOSQE_MULTI_CQES/REQ_F_MULTI_CQES for
>>>>>>>>>> multishot requests
>>>>>>>>>>      io_uring: maintain drain logic for multishot requests
>>>>>>>>>>      io_uring: use REQ_F_MULTI_CQES for multipoll IORING_OP_ADD
>>>>>>>>>>
>>>>>>>>>>     fs/io_uring.c                 | 34
>>>>>>>>>> +++++++++++++++++++++++++++++-----
>>>>>>>>>>     include/uapi/linux/io_uring.h |  8 +++-----
>>>>>>>>>>     2 files changed, 32 insertions(+), 10 deletions(-)
>>>>>>>>>
>>>>>>>>> Let's do the simple cq_extra first. I don't see a huge need to
>>>>>>>>> add an
>>>>>>>>> IOSQE flag for this, probably best to just keep this on a per
>>>>>>>>> opcode
>>>>>>>>> basis for now, which also then limits the code path to just
>>>>>>>>> touching
>>>>>>>>> poll for now, as nothing else supports multishot CQEs at this
>>>>>>>>> point.
>>>>>>>>>
>>>>>>>> gotcha.
>>>>>>>> a small issue here:
>>>>>>>>     sqe-->sqe(link)-->sqe(link)-->sqe(link, multishot)-->sqe(drain)
>>>>>>>>
>>>>>>>> in the above case, assume the first 3 single-shot reqs have
>>>>>>>> completed.
>>>>>>>> then I think the drian request won't be issued now unless the
>>>>>>>> multishot request in the linkchain has been issued. The trick
>>>>>>>> is: a multishot req
>>>>>>>> in a linkchain consumes cached_sq_head when io_get_sqe(), which
>>>>>>>> means it
>>>>>>>> is counted in seq, but we will deduct the sqe when it is issued
>>>>>>>> if we
>>>>>>>> want to do the job per opcode not in the main code path.
>>>>>>>> before the multishot req issued:
>>>>>>>>         all_sqes(4) - multishot_sqes(0) == all_cqes(3) -
>>>>>>>> multishot_cqes(0)
>>>>>>>> after the multishot req issued:
>>>>>>>>         all_sqes(4) - multishot_sqes(1) == all_cqes(3) -
>>>>>>>> multishot_cqes(0)
>>>>>>>
>>>>>>> Sorry, my statement is wrong. It's not "won't be issued now
>>>>>>> unless the
>>>>>>> multishot request in the linkchain has been issued". Actually I now
>>>>>>> think the drain req won't be issued unless the multishot request
>>>>>>> in the
>>>>>>> linkchain has completed. Because we may first check req_need_defer()
>>>>>>> then issue(req->link), so:
>>>>>>>      sqe0-->sqe1(link)-->sqe2(link)-->sqe3(link,
>>>>>>> multishot)-->sqe4(drain)
>>>>>>>
>>>>>>>     sqe2 is completed:
>>>>>>>       call req_need_defer:
>>>>>>>       all_sqes(4) - multishot_sqes(0) == all_cqes(3) -
>>>>>>> multishot_cqes(0)
>>>>>>>     sqe3 is issued:
>>>>>>>       all_sqes(4) - multishot_sqes(1) == all_cqes(3) -
>>>>>>> multishot_cqes(0)
>>>>>>>     sqe3 is completed:
>>>>>>>       call req_need_defer:
>>>>>>>       all_sqes(4) - multishot_sqes(1) == all_cqes(3) -
>>>>>>> multishot_cqes(0)
>>>>>>>
>>>>>>> sqe4 shouldn't wait sqe3.
>>>>>>
>>>>>> Do you mean it wouldn't if the patch is applied? Because any drain
>>>>>> request must wait for all requests submitted before to complete. And
>>>>>> so before issuing sqe4 it must wait for sqe3 __request__ to die, and
>>>>>> so for all sqe3's CQEs.
>>>>>>
>>>>>> previously
>>>>>
>>>>> I think we need to agree on what multishot means for dependencies.
>>>>> Does
>>>>> it mean it just needs to trigger once? Or does it mean that it
>>>>> needs to
>>>>> be totally finished. The latter may obviously never happen,
>>>>> depending on
>>>>> the use case. Or it may be an expected condition because the caller
>>>>> will
>>>>> cancel it at some point.
>>>>>
>>>>> The most logical view imho is that multishot changes nothing wrt
>>>>> drain.
>>>>> If you ask for drain before something executes and you are using
>>>>> multishot, then you need to understand that the multishot request
>>>>> needs
>>>>> to fully complete before that condition is true and your dependency
>>>>> can
>>>>> execute.
>>>> This makes sense, and the implementation would be quite simpler. but we
>>>> really need to document it somewhere so that users easily get to know
>>>> that they cannot put a drain req after some multishot reqs if they
>>>> don't
>>>> want it to wait for them. Otherwise I worry about wrong use of it since
>>>> the meaning of 'put a drain req after some multishot reqs' isn't so
>>>> obvious:
>>>>      - does it waits for those multishot reqs to complete once
>>>>      - or does it waits for those ones to fully complete
>>>>      - or does it ignore those ones at all
>>>>
>>> I realised that if a drain req has to wait for multishot reqs' fully
>>>   completion, then users have to explicitly cancel all the previous
>>> multishot reqs, otherwise it won't execute forever:
>>>      sqe0(multishot)-->sqe1(drain)-->sqe2(cancel multishot)    stuck
>>
>> And it's not a new behaviour, e.g. read(pipe); drain(); where nobody
>> writes to the pipe will stuck as well.
>>
>> I like that it currently provides a full barrier between requests, are
>> there other patterns used by someone?
>>
> As I'm writing a test for it, I found there is something different.
> we can break the stuck case above(read(pipe); drain();) easily since
> writing something to the pipe is independant to the sqring itself.
> But for a multishot req, there are many restrictions for the cancel req.
>  1. we cannot mark a cancel as LINK or DRAIN:
>        (1)sqe(multishot)->sqe(link, cancel)->sqe(link, drain)
>        (2)sqe(multishot)->sqe(cancel, drain)
>        (3)the linkchain fails at some member, which leads to
>           cancellation of the cancel req. and users have to retry.
>
>  2. we have to be careful when marking a multishot req with LINK or
>     DRAIN
>      (1)sqe0(nop, link)->sqe1(multishot, link)->sqe2(nop)->sqe3(cancel)
>        *  sqe3 may execute before sqe1, and cancels nothing
>        in other words, we have to carefully arrange them to make sure
>        the cancel req works.
>      (2) sqe(multshot, drain)
>
> There may be other cases. I feel it not easy for users to jump over
> these traps.
>
sorry, correct something above:
1. we cannot mark a cancel as LINK or DRAIN:
(1)the linkchain fails at some member, which leads to
cancellation of the cancel req. and users have to retry.
(2)sqe(multishot)->sqe(cancel, drain)

2. we have to be careful when marking a multishot req with LINK
(1)sqe0(nop, link)->sqe1(multishot, link)->sqe2(nop)->sqe3(cancel)
* sqe3 may execute before sqe1, and cancels nothing
in other words, we have to carefully arrange them to make sure
the cancel req works.
(2)sqe0(link, drain)->sqe1(link, multishot)->sqe2(nop)->sqe3(cancel)
though sqe2 is a normal sqe, but it will be marked as drain since
there is a drain req in linkchain.
* here the word 'link' means being logically in the linkchain, I
know that the first req without IOSQE_IO_LINK after the
linkchain is still in the linkchain

>