Re: [PATCH blktests 2/2] nvme/047: add test for uring-passthrough

[Shin'ichiro Kawasaki <shinichiro@fastmail.com>]

Thanks for the patch. I think it's good to have this test case to cover the
uring-passthrough codes in the nvme driver code. Please find my comments in
line.

Also, I ran the new test case on my Fedora system using QEMU NVME device and
found the test case fails with errors like,

    fio: io_u error on file /dev/ng0n1: Permission denied: read offset=266240, buflen=4096

I took a look in this and learned that SELinux on my system does not allow
IORING_OP_URING_CMD by default. I needed to do "setenforce 0" or add a local
policy to allow IORING_OP_URING_CMD so that the test case passes.

I think this test case should check this security requirement. I'm not sure what
is the best way to do it. One idea is to just run fio with io_uring_cmd engine
and check its error message. I created a patch below, and it looks working on my
system. I suggest to add it, unless anyone knows other better way.

diff --git a/tests/nvme/047 b/tests/nvme/047
index a0cc8b2..30961ff 100755
--- a/tests/nvme/047
+++ b/tests/nvme/047
@@ -14,6 +14,22 @@ requires() {
 	_have_fio_ver 3 33
 }
 
+device_requires() {
+	local ngdev=${TEST_DEV/nvme/ng}
+	local fio_output
+
+	if fio_output=$(fio --name=check --size=4k --filename="$ngdev" \
+			    --rw=read --ioengine=io_uring_cmd 2>&1); then
+		return 0
+	fi
+	if grep -qe "Permission denied" <<< "$fio_output"; then
+		SKIP_REASONS+=("IORING_OP_URING_CMD is not allowed for $ngdev")
+	else
+		SKIP_REASONS+=("IORING_OP_URING_CMD check for $ngdev failed")
+	fi
+	return 1
+}
+
 test_device() {
 	echo "Running ${TEST_NAME}"
 	local ngdev=${TEST_DEV/nvme/ng}


On Mar 31, 2023 / 09:14, Kanchan Joshi wrote:
> User can communicate to NVMe char device (/dev/ngXnY) using the
> uring-passthrough interface. This test exercises some of these
> communication pathways, using the 'io_uring_cmd' ioengine of fio.
> 
> Signed-off-by: Kanchan Joshi <joshi.k@samsung.com>
> ---
>  tests/nvme/047     | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/nvme/047.out |  2 ++
>  2 files changed, 48 insertions(+)
>  create mode 100755 tests/nvme/047
>  create mode 100644 tests/nvme/047.out
> 
> diff --git a/tests/nvme/047 b/tests/nvme/047
> new file mode 100755
> index 0000000..a0cc8b2
> --- /dev/null
> +++ b/tests/nvme/047
> @@ -0,0 +1,46 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-3.0+
> +# Copyright (C) 2023 Kanchan Joshi, Samsung Electronics
> +# Test exercising uring passthrough IO on nvme char device
> +
> +. tests/nvme/rc
> +
> +DESCRIPTION="basic test for uring-passthrough io on /dev/ngX"
> +QUICK=1
> +
> +requires() {
> +	_nvme_requires
> +	_have_kver 6 1

In general, it's the better not to depend on version number to check dependency.
Is kernel version the only way to check the kernel dependency?

Also, I think this test case assumes that the kernel is built with
CONFIG_IO_URING. I suggest to add "_have_kernel_option IO_URING" to ensure it.

> +	_have_fio_ver 3 33

Is io_uring_cmd engine the reason to check this fio version? If so, I suggest to
check "fio --enghelp" output. We can add a new helper function with name like
_have_fio_io_uring_cmd_engine. _have_fio_zbd_zonemode in common/fio can be a
reference.

> +}
> +
> +test_device() {
> +	echo "Running ${TEST_NAME}"
> +	local ngdev=${TEST_DEV/nvme/ng}
> +	local common_args=(
> +		--size=1M
> +		--filename="$ngdev"
> +		--bs=4k
> +		--rw=randread
> +		--numjobs=2
> +		--iodepth=8
> +		--name=randread
> +		--ioengine=io_uring_cmd
> +		--cmd_type=nvme
> +		--time_based
> +		--runtime=2
> +	)
> +	#plain read test
> +	_run_fio "${common_args[@]}"
> +
> +	#read with iopoll
> +	_run_fio "${common_args[@]}" --hipri
> +
> +	#read with fixedbufs
> +	_run_fio "${common_args[@]}" --fixedbufs
> +
> +	#if ! _run_fio "${common_args[@]}" >> "${FULL}" 2>&1; then
> +	#	echo "Error: uring-passthru read failed"
> +	#fi

I think you are aware of the comment lines :)

> +	echo "Test complete"
> +}
> diff --git a/tests/nvme/047.out b/tests/nvme/047.out
> new file mode 100644
> index 0000000..915d0a2
> --- /dev/null
> +++ b/tests/nvme/047.out
> @@ -0,0 +1,2 @@
> +Running nvme/047
> +Test complete
> -- 
> 2.25.1
>