Re: [GIT PULL] io_uring updates for 5.16-rc1

[Jens Axboe <axboe@kernel.dk>]

On 11/1/21 10:49 AM, Linus Torvalds wrote:
> On Sun, Oct 31, 2021 at 12:41 PM Jens Axboe <axboe@kernel.dk> wrote:
>>
>> This will throw two merge conflicts, see below for how I resolved it.
>> There are two spots, one is trivial, and the other needs
>> io_queue_linked_timeout() moved into io_queue_sqe_arm_apoll().
> 
> So I ended up resolving it the same way you did, because that was the
> mindless direct thing.
> 
> But I don't much like it.
> 
> Basically, io_queue_sqe_arm_apoll() now ends up doing
> 
>         case IO_APOLL_READY:
>                 if (linked_timeout) {
>                         io_queue_linked_timeout(linked_timeout);
>                         linked_timeout = NULL;
>                 }
>                 io_req_task_queue(req);
>                 break;
>     ...
>         if (linked_timeout)
>                 io_queue_linked_timeout(linked_timeout);
> 
> and that really seems *completely* pointless. Notice how it does that
> 
>         if (linked_timeout)
>                 io_queue_linked_timeout()
> 
> basically twice, and sets linked_timeout to NULL just to avoid the second one...
> 
> Why isn't it just
> 
>         case IO_APOLL_READY:
>                 io_req_task_queue(req);
>                 break;
>   ...
>         if (linked_timeout)
>                 io_queue_linked_timeout(linked_timeout);
> 
> where the only difference would seem to be the order of operations
> between io_req_task_queue() and io_queue_linked_timeout()?
> 
> Does the order of operations really matter here? As far as I can tell,
> io_req_task_queue() really just queues up work for later, so it's not
> really very ordered wrt that io_queue_linked_timeout(), and in the
> _other_ case statement it's apparently fine to do that
> io_queue_async_work() before the io_queue_linked_timeout()..
> 
> Again - I ended up resolving this the same way you had done, because I
> don't know the exact rules here well enough to do anything else. But
> it _looks_ a bit messy.

Yes I agree, and it's mostly just to keep the resolution simpler as I
don't think the current construct makes too much sense when both of them
end up being queueing the linked timeout. I think the cleanup done here
made more sense in the context before, not now.

We'll get a cleanup done for this shortly.

-- 
Jens Axboe