Re: [PATCH 2/3] io_uring: fix failed linkchain code logic

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

On 8/18/21 1:22 PM, Hao Xu wrote:
> 在 2021/8/18 下午6:20, Pavel Begunkov 写道:
>> On 8/18/21 8:43 AM, Hao Xu wrote:
>>> Given a linkchain like this:
>>> req0(link_flag)-->req1(link_flag)-->...-->reqn(no link_flag)
>>>
[...]
>>>       struct io_submit_link *link = &ctx->submit_state.link;
>>> +    bool is_link = sqe->flags & (IOSQE_IO_LINK | IOSQE_IO_HARDLINK);
>>> +    struct io_kiocb *head;
>>>       int ret;
>>>   +    /*
>>> +     * we don't update link->last until we've done io_req_prep()
>>> +     * since linked timeout uses old link->last
>>> +     */
>>> +    if (link->head)
>>> +        link->last->link = req;
>>> +    else if (is_link)
>>> +        link->head = req;
>>> +    head = link->head;
>>
>> It's a horrorsome amount of overhead. How about to set the fail flag
>> if failed early and actually fail on io_queue_sqe(), as below. It's
>> not tested and a couple more bits added, but hopefully gives the idea.
> I get the idea, it's truely with less change. But why do you think the
> above code bring in more overhead, since anyway we have to link the req
> to the linkchain. I tested it with fio-direct-4k-read-with/without-sqpoll, didn't see performance regression.

Well, it's an exaggeration :) However, we were cutting the overhead,
and there is no atomics or other heavy operations left in the hot path,
just pure number of instructions a request should go through. That's
just to clear the reason why I don't want extras on the path.

For the non-linked path, first it adds 2 ifs in front and removes one
at the end. Then there is is_link, which is most likely to be saved
on stack. And same with @head which I'd expect to be saved on stack.

If we have a way to avoid it, that's great, and it looks we can.

[...]
>>   -    ret = io_req_prep(req, sqe);
>> -    if (unlikely(ret))
>> -        goto fail_req;
>>         /* don't need @sqe from now on */
>>       trace_io_uring_submit_sqe(ctx, req, req->opcode, req->user_data,
>> @@ -6670,8 +6670,10 @@ static int io_submit_sqe(struct io_ring_ctx *ctx, struct io_kiocb *req,
>>           struct io_kiocb *head = link->head;
>>   
> maybe better to add an if(head & FAIL) here, since we don't need to
> prep_async if we know it will be cancelled.

Such an early fail is marginal enough to not care about performance,
but I agree that the check is needed as io_req_prep_async() won't be
able to handle it. E.g. if it failed to grab a file.

-- 
Pavel Begunkov