diff mbox series

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

Message ID 20230331034414.42024-3-joshi.k@samsung.com (mailing list archive)
State New, archived
Headers show
Series nvme uring-passthrough test | expand

Commit Message

Kanchan Joshi March 31, 2023, 3:44 a.m. UTC 
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

Comments

Shin'ichiro Kawasaki April 7, 2023, 8:07 a.m. UTC | #1 
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
>
Shin'ichiro Kawasaki April 10, 2023, 12:43 a.m. UTC | #2 
On Apr 07, 2023 / 17:07, Shin'ichiro Kawasaki wrote:
> 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")

I revisited the code I suggested and noticed this part is not good. The failures
other than "Permission denied" are the failures that this test case intended to
catch. It is not good to skip them.

> +	fi
> +	return 1
> +}
> +
>  test_device() {
>  	echo "Running ${TEST_NAME}"
>  	local ngdev=${TEST_DEV/nvme/ng}

As far as I read the kernel code related to security_uring_cmd(), calling uring
command is the only way to check its security permission. It means that we can
not separate the check for the security permission and the uring command test
itself. So it's the better to check the security permission not in
device_requires() but in test_device(), as follows:

diff --git a/tests/nvme/047 b/tests/nvme/047
index a0cc8b2..741ccd9 100755
--- a/tests/nvme/047
+++ b/tests/nvme/047
@@ -30,6 +30,16 @@ test_device() {
 		--time_based
 		--runtime=2
 	)
+	local fio_output
+
+	# check security permission
+	if ! fio_output=$(fio --name=check --size=4k --filename="$ngdev" \
+			    --rw=read --ioengine=io_uring_cmd 2>&1) &&
+			grep -qe "Permission denied" <<< "$fio_output"; then
+		SKIP_REASONS+=("IORING_OP_URING_CMD is not allowed for $ngdev")
+		return
+	fi
+
 	#plain read test
 	_run_fio "${common_args[@]}"
Kanchan Joshi April 10, 2023, 12:43 p.m. UTC | #3 
On Fri, Apr 07, 2023 at 05:07:46PM +0900, Shin'ichiro Kawasaki wrote:
>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.

I will use the latest one you posted. Thanks for taking care of it.

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

The tests checks for iopoll and fixed-buffer paths which are present
from 6.1 onwards, therefore this check. Hope that is ok?

>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.

Sure, will add.

>> +	_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.

fixed-buffer support[1] went into this fio relese, therefore check for
the specific version.

[1]https://lore.kernel.org/fio/20221003033152.314763-1-anuj20.g@samsung.com/
Shin'ichiro Kawasaki April 11, 2023, 1:35 a.m. UTC | #4 
On Apr 10, 2023 / 18:13, Kanchan Joshi wrote:
> On Fri, Apr 07, 2023 at 05:07:46PM +0900, Shin'ichiro Kawasaki wrote:
[...]
> > > +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?
> 
> The tests checks for iopoll and fixed-buffer paths which are present
> from 6.1 onwards, therefore this check. Hope that is ok?

Okay, thanks for the clarification. The changes are not visible from userland,
then we need the kernel version check.

> 
> > 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.
> 
> Sure, will add.
> 
> > > +	_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.
> 
> fixed-buffer support[1] went into this fio relese, therefore check for
> the specific version.
> 
> [1]https://lore.kernel.org/fio/20221003033152.314763-1-anuj20.g@samsung.com/

Thanks again. This can not be checked with fio commands. Let's do with
_have_fio_ver as you suggested.

On top of this, could you renumber the test case to nvme/049? Other new test
cases will consume nvme/047 and nvme/048 soon.
diff mbox series

Patch

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
+	_have_fio_ver 3 33
+}
+
+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
+	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