Re: [PATCH 0/2] uring_cmd SQE corruptions

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

On 2/12/25 23:55, Jens Axboe wrote:
> On 2/12/25 4:46 PM, Caleb Sander Mateos wrote:
>> On Wed, Feb 12, 2025 at 3:21?PM Keith Busch <kbusch@kernel.org> wrote:
>>>
>>> On Wed, Feb 12, 2025 at 03:07:30PM -0800, Caleb Sander Mateos wrote:
>>>>
>>>> Yes, we completely agree. We are working on incorporating Keith's
>>>> patchset now. It looks like there is still an open question about
>>>> whether userspace will need to enforce ordering between the requests
>>>> (either using linked operations or waiting for completions before
>>>> submitting the subsequent operations).
>>>
>>> In its current form, my series depends on you *not* using linked
>>> requests. I didn't think it would be a problem as it follows an existing
>>> pattern from the IORING_OP_FILES_UPDATE operation. That has to complete
>>> in its entirety before prepping any subsequent commands that reference
>>> the index, and using links would get the wrong results.
>>
>> As implementers of a ublk server, we would also prefer the current
>> interface in your patch series! Having to explicitly order the
>> requests would definitely make the interface more cumbersome and
>> probably less performant. I was just saying that Ming and Pavel had
>> raised some concerns about guaranteeing the order in which io_uring
>> issues SQEs. IORING_OP_FILES_UPDATE is a good analogy. Do we have any
>> examples of how applications use it? Are they waiting for a
>> completion, linking it, or relying on io_uring to issue it
>> synchronously?
> 
> Yes it's a good similar example - and I don't think it matters much
> how it's used. If you rely on its completion before making progress AND
> you set flags like IOSQE_ASYNC or LINK/DRAIN that will make it go async
> on purposes, then yes you'd need to similarly link dependents on it. If
> you don't set anything that forces it to go async, then it WILL complete
> inline - there's nothing in its implementation that would cause it
> needing to retry. Any failure would be fatal.
> 
> This is very much the same thing with the buf update / insertion, it'll
> behave in exactly the same way. You could argue "but you need to handle
> failures" and that is true. But if the failure case is that your
> consumer of the buffer fails with an import failure, then you can just
> as well handle that as you can the request getting failed with
> -ECANCELED because your dependent link failed.

That should be fine as long as the kernel reserves the ability
to change the order, i.e. in case of an error the implementation
will need to fall back to proper ordering like linking. It might
need a flag from io_uring whether you should ever attempt the
unordered version not to get a perf regression down the road
from submitting it twice.

For the unregistration though it'd likely need proper ordering
with the IO request in case it fails.

-- 
Pavel Begunkov