Re: [PATCH 4/4] io_uring/uring_cmd: implement ->sqe_copy() to avoid unnecessary copies

[Jens Axboe <axboe@kernel.dk>]

On 6/6/25 11:39 AM, Caleb Sander Mateos wrote:
> On Thu, Jun 5, 2025 at 12:47?PM Jens Axboe <axboe@kernel.dk> wrote:
>>
>> uring_cmd currently copies the full SQE at prep time, just in case it
>> needs it to be stable. Opt in to using ->sqe_copy() to let the core of
>> io_uring decide when to copy SQEs.
>>
>> This provides two checks to see if ioucmd->sqe is still valid:
>>
>> 1) If ioucmd->sqe is not the uring copied version AND IO_URING_F_INLINE
>>    isn't set, then the core of io_uring has a bug. Warn and return
>>    -EFAULT.
>>
>> 2) If sqe is NULL AND IO_URING_F_INLINE isn't set, then the core of
>>    io_uring has a bug. Warn and return -EFAULT.
>>
>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>> ---
>>  io_uring/opdef.c     |  1 +
>>  io_uring/uring_cmd.c | 35 ++++++++++++++++++++++++-----------
>>  io_uring/uring_cmd.h |  2 ++
>>  3 files changed, 27 insertions(+), 11 deletions(-)
>>
>> diff --git a/io_uring/opdef.c b/io_uring/opdef.c
>> index 6e0882b051f9..287f9a23b816 100644
>> --- a/io_uring/opdef.c
>> +++ b/io_uring/opdef.c
>> @@ -759,6 +759,7 @@ const struct io_cold_def io_cold_defs[] = {
>>         },
>>         [IORING_OP_URING_CMD] = {
>>                 .name                   = "URING_CMD",
>> +               .sqe_copy               = io_uring_cmd_sqe_copy,
>>                 .cleanup                = io_uring_cmd_cleanup,
>>         },
>>         [IORING_OP_SEND_ZC] = {
>> diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c
>> index e204f4941d72..f682b9d442e1 100644
>> --- a/io_uring/uring_cmd.c
>> +++ b/io_uring/uring_cmd.c
>> @@ -205,16 +205,25 @@ int io_uring_cmd_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
>>         if (!ac)
>>                 return -ENOMEM;
>>         ac->data.op_data = NULL;
>> +       ioucmd->sqe = sqe;
>> +       return 0;
>> +}
>> +
>> +int io_uring_cmd_sqe_copy(struct io_kiocb *req, const struct io_uring_sqe *sqe,
>> +                         unsigned int issue_flags)
> 
> Is it necessary to pass the sqe? Wouldn't it always be ioucmd->sqe?
> Presumably any other opcode that implements ->sqe_copy() would also
> have the sqe pointer stashed somewhere. Seems like it would simplify
> the core io_uring code a bit not to have to thread the sqe through
> several function calls.

It's not necessary, but I would rather get rid of needing to store an
SQE since that is a bit iffy than get rid of passing the SQE. When it
comes from the core, you _know_ it's going to be valid. I feel like you
need a fairly intimate understanding of io_uring issue flow to make any
judgement on this, if you were adding an opcode and defining this type
of handler.


>> +{
>> +       struct io_uring_cmd *ioucmd = io_kiocb_to_cmd(req, struct io_uring_cmd);
>> +       struct io_async_cmd *ac = req->async_data;
>> +
>> +       if (sqe != ac->sqes) {
> 
> Maybe return early if sqe == ac->sqes to reduce indentation?

Heh, the current -git version has it like that, since yesterday. So
yeah, I agree.

>> @@ -251,8 +260,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 || ret == -EIOCBQUEUED)
>> -               return ret;
>> +       if (ret == -EAGAIN) {
>> +               io_uring_cmd_sqe_copy(req, ioucmd->sqe, issue_flags);
> 
> Is it necessary to call io_uring_cmd_sqe_copy() here? Won't the call
> in io_queue_async() already handle this case?
> 

Good point, yes it should not be necessary at all. I'll kill it.

>> +               return -EAGAIN;
>> +       } else if (ret == -EIOCBQUEUED) {
> 
> nit: else could be omitted since the if case diverges

With the above removal of cmd_sqe_copu(), then this entire hunk goes
away.

-- 
Jens Axboe