Re: [PATCHv5 1/4] liburing: provide uring_cmd prep function

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

Looks good to me, just a few minor comments.

On Mon, Oct 13, 2025 at 11:00 AM Keith Busch <kbusch@meta.com> wrote:
>
> From: Keith Busch <kbusch@kernel.org>
>
> The rw prep doesn't clear __pad1, which is a reserved field for

io_uring_prep_rw() does assign to sqe->off, which is unioned with
cmd_op and __pad1. Though obviously __pad1 being set to 0 is dependent
on a offset being passed as 0 to io_uring_prep_rw(). But I certainly
agree a dedicated helper for IORING_OP_URING_CMD is a great
improvement.

> uring_cmd. If a prior submission in that entry did use that field, the
> uring_cmd will fail the kernel's checks.
>
> Also, the nvme uring_cmd tests had a couple places setting the sqe addr
> and length, which are unused fields for the nvme uring_cmds, so they
> shouldn't have been doing that, though had been checking these, so it

"had" -> "hadn't"?

> didn't cause any errors.
>
> Provide a helper function specific to the uring_cmd preparation.
>
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
>  src/include/liburing.h      | 19 +++++++++++++++----
>  test/io_uring_passthrough.c | 14 ++++----------
>  2 files changed, 19 insertions(+), 14 deletions(-)
>
> diff --git a/src/include/liburing.h b/src/include/liburing.h
> index c80bffd3..f7af20aa 100644
> --- a/src/include/liburing.h
> +++ b/src/include/liburing.h
> @@ -1517,6 +1517,19 @@ IOURINGINLINE void io_uring_prep_socket_direct_alloc(struct io_uring_sqe *sqe,
>         __io_uring_set_target_fixed_file(sqe, IORING_FILE_INDEX_ALLOC - 1);
>  }
>
> +IOURINGINLINE void io_uring_prep_uring_cmd(struct io_uring_sqe *sqe,
> +                                          int cmd_op,

I see this is copied from io_uring_prep_cmd_sock(), but u32 is
probably more accurate.


> +                                          int fd)
> +       LIBURING_NOEXCEPT
> +{
> +       sqe->opcode = (__u8) IORING_OP_URING_CMD;

Casting the constant seems unnecessary. Do compilers really warn about this?

> +       sqe->fd = fd;
> +       sqe->cmd_op = cmd_op;
> +       sqe->__pad1 = 0;
> +       sqe->addr = 0ul;
> +       sqe->len = 0;
> +}
> +
>  /*
>   * Prepare commands for sockets
>   */
> @@ -1529,11 +1542,10 @@ IOURINGINLINE void io_uring_prep_cmd_sock(struct io_uring_sqe *sqe,
>                                           int optlen)
>         LIBURING_NOEXCEPT
>  {
> -       io_uring_prep_rw(IORING_OP_URING_CMD, sqe, fd, NULL, 0, 0);
> +       io_uring_prep_uring_cmd(sqe, cmd_op, fd);
>         sqe->optval = (unsigned long) (uintptr_t) optval;
>         sqe->optname = optname;
>         sqe->optlen = optlen;
> -       sqe->cmd_op = cmd_op;
>         sqe->level = level;
>  }
>
> @@ -1607,8 +1619,7 @@ IOURINGINLINE void io_uring_prep_cmd_discard(struct io_uring_sqe *sqe,
>                                              uint64_t offset, uint64_t nbytes)
>         LIBURING_NOEXCEPT
>  {
> -       io_uring_prep_rw(IORING_OP_URING_CMD, sqe, fd, 0, 0, 0);
> -       sqe->cmd_op = BLOCK_URING_CMD_DISCARD;
> +       io_uring_prep_uring_cmd(sqe, BLOCK_URING_CMD_DISCARD, fd);
>         sqe->addr = offset;
>         sqe->addr3 = nbytes;
>  }
> diff --git a/test/io_uring_passthrough.c b/test/io_uring_passthrough.c
> index beaa81ad..26051710 100644
> --- a/test/io_uring_passthrough.c
> +++ b/test/io_uring_passthrough.c
> @@ -148,11 +148,9 @@ static int __test_io(const char *file, struct io_uring *ring, int tc, int read,
>                 if (async)
>                         sqe->flags |= IOSQE_ASYNC;
>                 if (nonvec)
> -                       sqe->cmd_op = NVME_URING_CMD_IO;
> +                       io_uring_prep_uring_cmd(sqe, NVME_URING_CMD_IO, use_fd);

I guess this works because io_uring_prep_uring_cmd() doesn't touch
sqe->buf_index or sqe->flags, but it seems like it would be less
brittle to call io_uring_prep_uring_cmd() before setting any of the
other sqe fields.

>                 else
> -                       sqe->cmd_op = NVME_URING_CMD_IO_VEC;
> -               sqe->fd = use_fd;
> -               sqe->opcode = IORING_OP_URING_CMD;
> +                       io_uring_prep_uring_cmd(sqe, NVME_URING_CMD_IO_VEC, use_fd);
>                 if (do_fixed)
>                         sqe->uring_cmd_flags |= IORING_URING_CMD_FIXED;
>                 sqe->user_data = ((uint64_t)offset << 32) | i;
> @@ -328,9 +326,7 @@ static int test_invalid_passthru_submit(const char *file)
>         }
>
>         sqe = io_uring_get_sqe(&ring);
> -       io_uring_prep_read(sqe, fd, vecs[0].iov_base, vecs[0].iov_len, 0);
> -       sqe->cmd_op = NVME_URING_CMD_IO;
> -       sqe->opcode = IORING_OP_URING_CMD;
> +       io_uring_prep_uring_cmd(sqe, NVME_URING_CMD_IO, fd);
>         sqe->user_data = 1;
>         cmd = (struct nvme_uring_cmd *)sqe->cmd;
>         memset(cmd, 0, sizeof(struct nvme_uring_cmd));
> @@ -401,10 +397,8 @@ static int test_io_uring_submit_enters(const char *file)
>                 __u32 nlb;
>
>                 sqe = io_uring_get_sqe(&ring);
> -               io_uring_prep_readv(sqe, fd, &vecs[i], 1, offset);
> +               io_uring_prep_uring_cmd(sqe, NVME_URING_CMD_IO, fd);
>                 sqe->user_data = i;
> -               sqe->opcode = IORING_OP_URING_CMD;
> -               sqe->cmd_op = NVME_URING_CMD_IO;
>                 cmd = (struct nvme_uring_cmd *)sqe->cmd;
>                 memset(cmd, 0, sizeof(struct nvme_uring_cmd));
>
> --
> 2.47.3
>