Re: [PATCH blktests] nvme/067: test io_uring pass through for NVMe admin queue

[Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com>]

Hello Swarna, thank yo for this patch. I ran the test case and it looks
working good. Please find my reivew comments in line.

May I know the background to add this test case? Is it relevant to any kernel
side fix? (If so, I would like to have a link to the fix)  Or does it intend to
extend kernel code coverage?

On Jan 28, 2026 / 18:37, sw.prabhu6@gmail.com wrote:
> From: Swarna Prabhu <s.prabhu@samsung.com>
> 
> Simple test to excersie the nvme admin commands usage with

s/excersie/exercise/

> io uring passthrough interfaces.
> 
> Signed-off-by: Swarna Prabhu <s.prabhu@samsung.com>
> ---
>  src/Makefile                 |  3 +-
>  src/nvme-passthrough-admin.c | 94 ++++++++++++++++++++++++++++++++++++
>  tests/nvme/067               | 27 +++++++++++
>  tests/nvme/067.out           |  2 +
>  4 files changed, 125 insertions(+), 1 deletion(-)
>  mode change 100644 => 100755 src/Makefile
>  create mode 100755 src/nvme-passthrough-admin.c
>  create mode 100755 tests/nvme/067
>  create mode 100755 tests/nvme/067.out
> 
> diff --git a/src/Makefile b/src/Makefile
> old mode 100644
> new mode 100755

No need to change the file mode of src/Makefile.

> index dac07c7..0cb8d4e
> --- a/src/Makefile
> +++ b/src/Makefile
> @@ -23,7 +23,8 @@ C_TARGETS := \
>  	zbdioctl
>  
>  C_URING_TARGETS := miniublk \
> -		metadata
> +		metadata \
> +		nvme-passthrough-admin

Nit: the name of the program does not show that io_uring is used. To show it,
I suggest the program name "nvme-passthru-admin-uring.c".

>  
>  HAVE_LIBURING := $(call HAVE_C_MACRO,liburing.h,IORING_OP_URING_CMD)
>  HAVE_UBLK_HEADER := $(call HAVE_C_HEADER,linux/ublk_cmd.h,1)
> diff --git a/src/nvme-passthrough-admin.c b/src/nvme-passthrough-admin.c
> new file mode 100755

I think file mode 644 is apporopriate for .c files.

> index 0000000..ef5c241
> --- /dev/null
> +++ b/src/nvme-passthrough-admin.c
> @@ -0,0 +1,94 @@

SPDX-License-Identifier and your copyright are missing.

> +/*
> + * Simple test exercising the admin queue accesses via  io_uring passthrough
> + * commands.
> + */
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <liburing.h>
> +#include <linux/nvme_ioctl.h>
> +
> +#define NVME_IDENTIFY_ADMIN_CMD 0x06     /*Identify command using admin queue */
> +#define NVME_IDENTIFY_CNS_CTRL 0x01    /*Identify controller command to a NVME device*/

Nit: some comments in this file do not have a space after the comment start
"/*".

> +struct nvme_id_ctrl {
> +           __le16 vid;
> +           __le16 ssvid;
> +           char sn[20];
> +           char mn[40];
> +           char fr[8];
> +           __u8 rab;
> +           __u8 ieee[3];
> +	   char pad[4020];

Spaces are used for ident in the above lines. I suggest to use tabs instead.

> +};
> +
> +int main(int argc, char **argv)
> +{
> +	int fd, ret;
> +	struct nvme_passthru_cmd *cmd;
> +	struct nvme_id_ctrl *nctrl;
> +	struct io_uring nvring;
> +	int queue_depth = 80;
> +	struct io_uring_sqe *sqe = NULL;
> +	struct io_uring_cqe *cqe = NULL;
> +
> +	if (argc < 2) {
> +		fprintf(stderr, "usage: %s /dev/nvmeXnY", argv[0]);

Nit: I suggest to add "\n" to the end of the usage message.

> +		return -EINVAL;
> +	}
> +
> +	fd = open(argv[1], O_RDONLY);
> +	if (fd < 0) {
> +		perror("open");
> +		return -errno;
> +	}
> +
> +	nctrl = (struct nvme_id_ctrl *)calloc(1, sizeof(struct nvme_id_ctrl));
> +	if (!nctrl) {
> +		fprintf(stderr, "Memory allocation failure\n");
> +		return -ENOMEM;

Nit: Its a userspace program and we do not care much about resource clean up,
     but it would be a bit better to "ret = -ENOMEM" and "goto" to the line of
     close(fd).

> +	}
> +
> +	ret = io_uring_queue_init(queue_depth, &nvring, IORING_SETUP_SQE128 | IORING_SETUP_CQE32);
> +	if (ret < 0) {
> +		fprintf(stderr, "Initialize io uring fail %d \n", ret);

Nit: same, it's a better go goto to the line of free(nctrl).

> +		return ret;
> +	}
> +	/* Prepare the SQE to use the IORING_OP_URING_CMD opcode */
> +	sqe = io_uring_get_sqe(&nvring);
> +	sqe->fd = fd;
> +	sqe->opcode = IORING_OP_URING_CMD;
> +	sqe->cmd_op = NVME_URING_CMD_ADMIN;
> +
> +	cmd = (struct nvme_passthru_cmd *)&sqe->cmd;
> +	memset(cmd, 0, sizeof(*cmd));
> +
> +	/* populate the cmd struct for the opcode */
> +	cmd->opcode = NVME_IDENTIFY_ADMIN_CMD;
> +	cmd->addr = (__u64)(uintptr_t)nctrl;
> +	cmd->data_len = sizeof(struct nvme_id_ctrl);
> +	cmd->cdw10 = NVME_IDENTIFY_CNS_CTRL;
> +
> +	/*submit the SQE */
> +	io_uring_submit(&nvring);
> +
> +	ret = io_uring_wait_cqe(&nvring, &cqe);
> +
> +	if (ret < 0) {
> +		fprintf(stderr, "wait_cqe: %s\n", strerror(-ret));
> +	} else if (cqe && cqe->res < 0) {
> +		fprintf(stderr, "Command failed (cqe->res): %d\n", cqe->res);
> +		ret = cqe->res;
> +	} else {
> +		ret = 0;
> +	}
> +
> +	if (cqe)
> +		io_uring_cqe_seen(&nvring, cqe);
> +	io_uring_queue_exit(&nvring);
> +	close(fd);
> +	free(nctrl);

Nit: To allow goto from the error paths, the order of the two lines above
     can be reversed:

free_nctrl:
	free(nctrl);
free_fd:
	close(fd);

> +
> +	return ret;
> +}
> \ No newline at end of file
> diff --git a/tests/nvme/067 b/tests/nvme/067
> new file mode 100755
> index 0000000..8c273fa
> --- /dev/null
> +++ b/tests/nvme/067
> @@ -0,0 +1,27 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-3.0+
> +# Test out admin commands through the io uring passthrough interfaces.
> +
> +. tests/nvme/rc
> +
> +requires() {
> +	_nvme_requires
> +	_have_kernel_option IO_URING
> +}
> +
> +DESCRIPTION="exercise the nvme admin commands usage with io uring passthrough interfaces"
> +QUICK=1
> +
> +test_device() {
> +	echo "Running ${TEST_NAME}"
> +
> +	# extract the ctrl dev from the test dev
> +	devname=${TEST_DEV#/dev/}
> +	ctrl_dev="/dev/${devname%n*}"

I suggest to add _io_uring_enable here,

> +
> +	if ! sudo src/nvme-passthrough-admin "${ctrl_dev}"; then
> +		echo "src/nvme-passthrough-admin failed"
> +	fi

and _io_uring_restore here, just in case the system has
/proc/sys/kernel/io_uring_disabled value "2".

> +
> +	echo "Test Complete"
> +}
> diff --git a/tests/nvme/067.out b/tests/nvme/067.out
> new file mode 100755

I suggest to run "make check" or "make check-parallel" before blktests patch
post. It reports that 067.out should have the file mode 644 instead of 755.

  $ make check-parallel
  Running shellcheck with 4 parallel jobs...
  ./tests/nvme/067.out is executable

> index 0000000..c56866e
> --- /dev/null
> +++ b/tests/nvme/067.out
> @@ -0,0 +1,2 @@
> +Running nvme/067
> +Test Complete
> -- 
> 2.39.5
>