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

[Caleb Sander Mateos <csander@purestorage.com>]

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.

> +{
> +       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?

> +               if (WARN_ON_ONCE(!(issue_flags & IO_URING_F_INLINE)))
> +                       return -EFAULT;
> +               if (WARN_ON_ONCE(!sqe))
> +                       return -EFAULT;
> +               memcpy(ac->sqes, sqe, uring_sqe_size(req->ctx));
> +               ioucmd->sqe = ac->sqes;
> +       }
>
> -       /*
> -        * 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.
> -        */
> -       memcpy(ac->sqes, sqe, uring_sqe_size(req->ctx));
> -       ioucmd->sqe = ac->sqes;
>         return 0;
>  }
>
> @@ -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?

> +               return -EAGAIN;
> +       } else if (ret == -EIOCBQUEUED) {

nit: else could be omitted since the if case diverges

Best,
Caleb

> +               return -EIOCBQUEUED;
> +       }
>         if (ret < 0)
>                 req_set_fail(req);
>         io_req_uring_cleanup(req, issue_flags);
> diff --git a/io_uring/uring_cmd.h b/io_uring/uring_cmd.h
> index e6a5142c890e..f956b0e7c351 100644
> --- a/io_uring/uring_cmd.h
> +++ b/io_uring/uring_cmd.h
> @@ -11,6 +11,8 @@ struct io_async_cmd {
>
>  int io_uring_cmd(struct io_kiocb *req, unsigned int issue_flags);
>  int io_uring_cmd_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe);
> +int io_uring_cmd_sqe_copy(struct io_kiocb *req, const struct io_uring_sqe *sqe,
> +                         unsigned int issue_flags);
>  void io_uring_cmd_cleanup(struct io_kiocb *req);
>
>  bool io_uring_try_cancel_uring_cmd(struct io_ring_ctx *ctx,
> --
> 2.49.0
>