Re: [PATCH] io_uring/uring_cmd: be smarter about SQE copying

[Jens Axboe <axboe@kernel.dk>]

On 6/3/25 9:05 AM, Caleb Sander Mateos wrote:
> On Sat, May 31, 2025 at 1:52?PM Jens Axboe <axboe@kernel.dk> wrote:
>>
>> uring_cmd currently copies the SQE unconditionally, which was introduced
>> as a work-around in commit:
>>
>> d6211ebbdaa5 ("io_uring/uring_cmd: unconditionally copy SQEs at prep time")
>>
>> because the checking for whether or not this command may have ->issue()
>> called from io-wq wasn't complete. Rectify that, ensuring that if the
>> request is marked explicitly async via REQ_F_FORCE_ASYNC or if it's
>> part of a link chain, then the SQE is copied upfront.
>>
>> Always copying can be costly, particularly when dealing with SQE128
>> rings. But even a normal 64b SQE copy is noticeable at high enough
>> rates.
>>
>> Reported-by: Caleb Sander Mateos <csander@purestorage.com>
>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>>
>> ---
>>
>> diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c
>> index 929cad6ee326..cb4b867a2656 100644
>> --- a/io_uring/uring_cmd.c
>> +++ b/io_uring/uring_cmd.c
>> @@ -181,29 +181,42 @@ void io_uring_cmd_done(struct io_uring_cmd *ioucmd, ssize_t ret, u64 res2,
>>  }
>>  EXPORT_SYMBOL_GPL(io_uring_cmd_done);
>>
>> +static void io_uring_sqe_copy(struct io_kiocb *req, struct io_uring_cmd *ioucmd)
>> +{
>> +       struct io_async_cmd *ac = req->async_data;
>> +
>> +       if (ioucmd->sqe != ac->sqes) {
>> +               memcpy(ac->sqes, ioucmd->sqe, uring_sqe_size(req->ctx));
>> +               ioucmd->sqe = ac->sqes;
>> +       }
>> +}
>> +
>>  static int io_uring_cmd_prep_setup(struct io_kiocb *req,
>>                                    const struct io_uring_sqe *sqe)
>>  {
>>         struct io_uring_cmd *ioucmd = io_kiocb_to_cmd(req, struct io_uring_cmd);
>> +       struct io_ring_ctx *ctx = req->ctx;
>>         struct io_async_cmd *ac;
>>
>>         /* see io_uring_cmd_get_async_data() */
>>         BUILD_BUG_ON(offsetof(struct io_async_cmd, data) != 0);
>>
>> -       ac = io_uring_alloc_async_data(&req->ctx->cmd_cache, req);
>> +       ac = io_uring_alloc_async_data(&ctx->cmd_cache, req);
>>         if (!ac)
>>                 return -ENOMEM;
>>         ac->data.op_data = NULL;
>>
>>         /*
>> -        * Unconditionally cache the SQE for now - this is only needed for
>> -        * requests that go async, but prep handlers must ensure that any
>> -        * sqe data is stable beyond prep. Since uring_cmd is special in
>> -        * that it doesn't read in per-op data, play it safe and ensure that
>> -        * any SQE data is stable beyond prep. This can later get relaxed.
>> +        * Copy SQE now, if we know we're going async. Drain will set
>> +        * FORCE_ASYNC, and assume links may cause it to go async. If not,
>> +        * copy is deferred until issue time, if the request doesn't issue
>> +        * or queue inline.
>>          */
>> -       memcpy(ac->sqes, sqe, uring_sqe_size(req->ctx));
>> -       ioucmd->sqe = ac->sqes;
>> +       ioucmd->sqe = sqe;
>> +       if (req->flags & (REQ_F_FORCE_ASYNC| REQ_F_LINK | REQ_F_HARDLINK) ||
>> +           ctx->submit_state.link.head)
> 
> To check my understanding, io_init_req() will set REQ_F_FORCE_ASYNC on
> any request with IOSQE_IO_DRAIN as well as all subsequent requests
> until the IOSQE_IO_DRAIN request completes? Looks like this condition

Correct

> should work then. I think you can drop REQ_F_LINK | REQ_F_HARDLINK,
> though; the initial request of a linked chain will be issued
> synchronously, and ctx->submit_state.link.head will be set for the
> subsequent requests.
> 
> I do share Pavel's concern that whether or not a request will be
> initially issued asynchronously is up to the core io_uring code, so it
> seems a bit fragile to make these assumptions in the uring_cmd layer.
> I think I would prefer either passing a bool issue_async to the
> ->prep() handler, or adding an optional ->prep_async() hook called if
> the initial issue may happen asynchronously.

Yes I do too, which is why I suggested we add a specific cold handler
for this kind of case. That leaves the core of io_uring calling it
appropriately, and eliminates the need for storing an sqe pointer. Which
would've been fine before resizing, but not a great idea to do now. I'll
do a v2 of this with that in mind, just haven't gotten around to it yet.

>> +               io_uring_sqe_copy(req, ioucmd);
>> +
>>         return 0;
>>  }
>>
>> @@ -259,6 +272,12 @@ int io_uring_cmd(struct io_kiocb *req, unsigned int issue_flags)
>>         }
>>
>>         ret = file->f_op->uring_cmd(ioucmd, issue_flags);
>> +       if (ret == -EAGAIN) {
>> +               io_uring_sqe_copy(req, ioucmd);
>> +               return ret;
>> +       } else if (ret == -EIOCBQUEUED) {
>> +               return ret;
>> +       }
>>         if (ret == -EAGAIN || ret == -EIOCBQUEUED)
>>                 return ret;
> 
> This if condition is always false now, remove it?

Heh yes, just forgot to remove those lines when adding the above change.

-- 
Jens Axboe