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

[Kanchan Joshi <joshi.k@samsung.com>]

[-- Attachment #1: Type: text/plain, Size: 3714 bytes --]

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/

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]