[PATCH 0/6] blktests: char device tests with iouring-cmd fio

[PATCH 0/6] blktests: char device tests with iouring-cmd fio

[Luis Chamberlain]

As io-uring cmd grows there's a desire to do a bit more funky things
with it. Add basic support with fio and add a few simple tests to
tests the NVMe conventional drives character device as well as the
ZNS character device.

These tests are perhaps a bit *too* basic to merge, not sure, let
me know. But I figured that this would provide example to let us
grow this with more complex things later as folks add support for
more features.

Luis Chamberlain (6):
  common/fio: add helpers using io-uring cmd engine
  tests/nvme: add new test for rand-read on the nvme character device
  tests/nvme: add new test for rand-write on the nvme character device
  tests/nvme: add new test for optimal write on the nvme character
    device
  tests/zbd: add new basic test for reading zone character device
  tests/zbd: add new basic test for reading zone character device

 common/fio         | 47 +++++++++++++++++++++++++++++++++++++++++
 tests/nvme/046     | 42 +++++++++++++++++++++++++++++++++++++
 tests/nvme/046.out |  2 ++
 tests/nvme/047     | 41 ++++++++++++++++++++++++++++++++++++
 tests/nvme/047.out |  2 ++
 tests/nvme/048     | 41 ++++++++++++++++++++++++++++++++++++
 tests/nvme/048.out |  2 ++
 tests/zbd/011      | 52 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/zbd/011.out  |  2 ++
 tests/zbd/012      | 50 ++++++++++++++++++++++++++++++++++++++++++++
 tests/zbd/012.out  |  2 ++
 11 files changed, 283 insertions(+)
 create mode 100755 tests/nvme/046
 create mode 100644 tests/nvme/046.out
 create mode 100755 tests/nvme/047
 create mode 100644 tests/nvme/047.out
 create mode 100755 tests/nvme/048
 create mode 100644 tests/nvme/048.out
 create mode 100755 tests/zbd/011
 create mode 100644 tests/zbd/011.out
 create mode 100755 tests/zbd/012
 create mode 100644 tests/zbd/012.out

-- 
2.35.1

[PATCH 1/6] common/fio: add helpers using io-uring cmd engine

[Luis Chamberlain]

This will allow us to add tests which use the io-uring cmd engine,
part of fio. They are inspired by the work by Anuj Gupta and
Ankit Kumar which added sample fio files onto fio for this exact
purpose.

We can build on those to expand test coverage with elaborate tests.

We don't specify the cmd to allow other types of io-uring cmd
users to use this other than nvme.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 common/fio | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/common/fio b/common/fio
index bed76d555b2a..4da90804ed21 100644
--- a/common/fio
+++ b/common/fio
@@ -184,6 +184,53 @@ _run_fio_verify_io() {
 	rm -f local*verify*state
 }
 
+_run_fio_rand_iouring_cmd() {
+	_run_fio --bs=4k --rw=randread --numjobs="$(nproc)" \
+		--ioengine=io_uring_cmd --iodepth=32 \
+		--thread=1 --stonewall=1 \
+		--name=reads "$@"
+}
+
+_run_fio_verify_iouring_cmd_randwrite() {
+	_run_fio --bs=4k --rw=randwrite --numjobs="$(nproc)" \
+		--ioengine=io_uring_cmd --iodepth=32 \
+		--thread=1 --stonewall=1 \
+		--sqthread_poll=1 --sqthread_poll_cpu=0 \
+		--nonvectored=1 --registerfiles=1 \
+		--verify=crc32c \
+		--name=verify "$@"
+	rm -f local*verify*state
+}
+
+_run_fio_verify_iouring_cmd_write_opts() {
+	_run_fio --bs=4k --rw=write --numjobs="$(nproc)" \
+		--ioengine=io_uring_cmd --iodepth=32 \
+		--thread=1 --stonewall=1 \
+		--sqthread_poll=1 --sqthread_poll_cpu=0 \
+		--nonvectored=1 --registerfiles=1 \
+		--verify=crc32c \
+		--name=verify "$@"
+	rm -f local*verify*state
+}
+
+_run_fio_iouring_cmd_zone() {
+	_run_fio --rw=randread --numjobs="$(nproc)" \
+		--ioengine=io_uring_cmd --iodepth=1 \
+		--stonewall=1 \
+		--zonemode=zbd \
+		--name=reads "$@"
+}
+
+_run_fio_verify_iouring_cmd_write_opts_zone() {
+	_run_fio --rw=randread --numjobs="$(nproc)" \
+		--ioengine=io_uring_cmd --iodepth=1 \
+		--stonewall=1 \
+		--zonemode=zbd \
+		--sqthread_poll=1 --registerfiles=1 --sqthread_poll_cpu=0 \
+		--verify=crc32c \
+		--name=verify "$@"
+}
+
 _fio_perf_report() {
 	# If there is more than one group, we don't know what to report.
 	if [[ $(wc -l < "$TMPDIR/fio_perf") -gt 1 ]]; then
-- 
2.35.1

[PATCH 2/6] tests/nvme: add new test for rand-read on the nvme character device

[Luis Chamberlain]

This does basic rand-read testing of the character device of a
conventional NVMe drive.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 tests/nvme/046     | 42 ++++++++++++++++++++++++++++++++++++++++++
 tests/nvme/046.out |  2 ++
 2 files changed, 44 insertions(+)
 create mode 100755 tests/nvme/046
 create mode 100644 tests/nvme/046.out

diff --git a/tests/nvme/046 b/tests/nvme/046
new file mode 100755
index 000000000000..3526ab9eedab
--- /dev/null
+++ b/tests/nvme/046
@@ -0,0 +1,42 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-3.0+
+# Copyright (C) 2022 Luis Chamberlain <mcgrof@kernel.org>
+#
+# This does basic sanity test for the nvme character device. This is a basic
+# test and if it fails it is probably very likely other nvme character device
+# tests would fail.
+#
+. tests/nvme/rc
+
+DESCRIPTION="basic rand-read io_uring_cmd engine for nvme-ns character device"
+QUICK=1
+
+requires() {
+	_nvme_requires
+	_have_fio
+}
+
+device_requires() {
+	_require_test_dev_is_nvme
+}
+
+test_device() {
+	echo "Running ${TEST_NAME}"
+	local ngdev=${TEST_DEV/nvme/ng}
+	local fio_args=(
+		--size=1M
+		--cmd_type=nvme
+		--filename="$ngdev"
+		--time_based
+		--runtime=10
+	) &&
+	_run_fio_rand_iouring_cmd "${fio_args[@]}" >>"${FULL}" 2>&1 ||
+	fail=true
+
+	if [ -z "$fail" ]; then
+		echo "Test complete"
+	else
+		echo "Test failed"
+		return 1
+	fi
+}
diff --git a/tests/nvme/046.out b/tests/nvme/046.out
new file mode 100644
index 000000000000..2b5fa6af63b1
--- /dev/null
+++ b/tests/nvme/046.out
@@ -0,0 +1,2 @@
+Running nvme/046
+Test complete
-- 
2.35.1

[PATCH 3/6] tests/nvme: add new test for rand-write on the nvme character device

[Luis Chamberlain]

This does basic rand-write testing of the character device of a
conventional NVMe drive.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 tests/nvme/047     | 41 +++++++++++++++++++++++++++++++++++++++++
 tests/nvme/047.out |  2 ++
 2 files changed, 43 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 000000000000..8ba55b250bc5
--- /dev/null
+++ b/tests/nvme/047
@@ -0,0 +1,41 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-3.0+
+# Copyright (C) 2022 Luis Chamberlain <mcgrof@kernel.org>
+#
+# This does basic random write test for the the convential nvme character
+# device.
+
+. tests/nvme/rc
+
+DESCRIPTION="basic rand-write io_uring_cmd engine for nvme-ns character device"
+QUICK=1
+
+requires() {
+	_nvme_requires
+	_have_fio
+}
+
+device_requires() {
+	_require_test_dev_is_nvme
+}
+
+test_device() {
+	echo "Running ${TEST_NAME}"
+	local ngdev=${TEST_DEV/nvme/ng}
+	local fio_args=(
+		--size=1M
+		--cmd_type=nvme
+		--filename="$ngdev"
+		--time_based
+		--runtime=10
+	) &&
+	_run_fio_verify_iouring_cmd_randwrite "${fio_args[@]}" >>"${FULL}" 2>&1 ||
+	fail=true
+
+	if [ -z "$fail" ]; then
+		echo "Test complete"
+	else
+		echo "Test failed"
+		return 1
+	fi
+}
diff --git a/tests/nvme/047.out b/tests/nvme/047.out
new file mode 100644
index 000000000000..915d0a2389ca
--- /dev/null
+++ b/tests/nvme/047.out
@@ -0,0 +1,2 @@
+Running nvme/047
+Test complete
-- 
2.35.1

[PATCH 4/6] tests/nvme: add new test for optimal write on the nvme character device

[Luis Chamberlain]

This does basic optimal write testing of the character device of a
conventional NVMe drive.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 tests/nvme/048     | 41 +++++++++++++++++++++++++++++++++++++++++
 tests/nvme/048.out |  2 ++
 2 files changed, 43 insertions(+)
 create mode 100755 tests/nvme/048
 create mode 100644 tests/nvme/048.out

diff --git a/tests/nvme/048 b/tests/nvme/048
new file mode 100755
index 000000000000..94329d9ed764
--- /dev/null
+++ b/tests/nvme/048
@@ -0,0 +1,41 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-3.0+
+# Copyright (C) 2022 Luis Chamberlain <mcgrof@kernel.org>
+#
+# This does basic optimal write test for the nvme character device.
+
+. tests/nvme/rc
+
+DESCRIPTION="basic optimal write io_uring_cmd engine for nvme-ns character device"
+QUICK=1
+CAN_BE_ZONED=1
+
+requires() {
+	_nvme_requires
+	_have_fio
+}
+
+device_requires() {
+	_require_test_dev_is_nvme
+}
+
+test_device() {
+	echo "Running ${TEST_NAME}"
+	local ngdev=${TEST_DEV/nvme/ng}
+	local fio_args=(
+		--size=1M
+		--cmd_type=nvme
+		--filename="$ngdev"
+		--time_based
+		--runtime=10
+	) &&
+	_run_fio_verify_iouring_cmd_write_opts "${fio_args[@]}" >>"${FULL}" 2>&1 ||
+	fail=true
+
+	if [ -z "$fail" ]; then
+		echo "Test complete"
+	else
+		echo "Test failed"
+		return 1
+	fi
+}
diff --git a/tests/nvme/048.out b/tests/nvme/048.out
new file mode 100644
index 000000000000..65ffa47e34f1
--- /dev/null
+++ b/tests/nvme/048.out
@@ -0,0 +1,2 @@
+Running nvme/048
+Test complete
-- 
2.35.1

[PATCH 5/6] tests/zbd: add new basic test for reading zone character device

[Luis Chamberlain]

This adds a basic test to read using fio against a ZNS character device.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 tests/zbd/011     | 52 +++++++++++++++++++++++++++++++++++++++++++++++
 tests/zbd/011.out |  2 ++
 2 files changed, 54 insertions(+)
 create mode 100755 tests/zbd/011
 create mode 100644 tests/zbd/011.out

diff --git a/tests/zbd/011 b/tests/zbd/011
new file mode 100755
index 000000000000..a4d46fcd6cd3
--- /dev/null
+++ b/tests/zbd/011
@@ -0,0 +1,52 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-3.0+
+# Copyright (C) 2022 Luis Chamberlain <mcgrof@kernel.org>
+#
+# This does basic read test for the zone nvme character device.
+
+. tests/zbd/rc
+
+DESCRIPTION="basic read io_uring_cmd engine for zone nvme-ns character device"
+QUICK=1
+CAN_BE_ZONED=1
+
+requires() {
+	_have_fio
+}
+
+fallback_device() {
+	_fallback_null_blk_zoned
+}
+
+cleanup_fallback_device() {
+	_exit_null_blk
+}
+
+test_device() {
+	echo "Running ${TEST_NAME}"
+	local -i bs
+	declare -a fio_args
+	local ngdev=${TEST_DEV/nvme/ng}
+
+	_get_sysfs_variable "${TEST_DEV}" || return $?
+	bs=${SYSFS_VARS[$SV_PHYS_BLK_SIZE]}
+	_put_sysfs_variable
+
+	fio_args=(
+		--bs="$bs"
+		--size=1024M
+		--cmd_type=nvme
+		--filename="$ngdev"
+		--time_based
+		--runtime=10
+	) &&
+	_run_fio_iouring_cmd_zone "${fio_args[@]}" >>"${FULL}" 2>&1 ||
+	fail=true
+
+	if [ -z "$fail" ]; then
+		echo "Test complete"
+	else
+		echo "Test failed"
+		return 1
+	fi
+}
diff --git a/tests/zbd/011.out b/tests/zbd/011.out
new file mode 100644
index 000000000000..aec7f703938a
--- /dev/null
+++ b/tests/zbd/011.out
@@ -0,0 +1,2 @@
+Running zbd/011
+Test complete
-- 
2.35.1

[PATCH 6/6] tests/zbd: add new basic test for reading zone character device

[Luis Chamberlain]

This adds a basic optimal write test using fio against a ZNS character
device.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 tests/zbd/012     | 50 +++++++++++++++++++++++++++++++++++++++++++++++
 tests/zbd/012.out |  2 ++
 2 files changed, 52 insertions(+)
 create mode 100755 tests/zbd/012
 create mode 100644 tests/zbd/012.out

diff --git a/tests/zbd/012 b/tests/zbd/012
new file mode 100755
index 000000000000..474bcb07a27b
--- /dev/null
+++ b/tests/zbd/012
@@ -0,0 +1,50 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-3.0+
+# Copyright (C) 2022 Luis Chamberlain <mcgrof@kernel.org>
+#
+# This does basic optimal write test for the zone nvme character device.
+
+. tests/zbd/rc
+
+DESCRIPTION="basic optimal write using io_uring_cmd engine for zone nvme-ns character device"
+QUICK=1
+CAN_BE_ZONED=1
+
+requires() {
+	_have_fio
+}
+
+fallback_device() {
+	_fallback_null_blk_zoned
+}
+
+cleanup_fallback_device() {
+	_exit_null_blk
+}
+
+test_device() {
+	echo "Running ${TEST_NAME}"
+	local -i bs
+	declare -a fio_args
+	local ngdev=${TEST_DEV/nvme/ng}
+
+	_get_sysfs_variable "${TEST_DEV}" || return $?
+	bs=${SYSFS_VARS[$SV_PHYS_BLK_SIZE]}
+	_put_sysfs_variable
+
+	fio_args=(
+		--bs="$bs"
+		--size=1024M
+		--cmd_type=nvme
+		--filename="$ngdev"
+	) &&
+	_run_fio_verify_iouring_cmd_write_opts_zone "${fio_args[@]}" >>"${FULL}" 2>&1 ||
+	fail=true
+
+	if [ -z "$fail" ]; then
+		echo "Test complete"
+	else
+		echo "Test failed"
+		return 1
+	fi
+}
diff --git a/tests/zbd/012.out b/tests/zbd/012.out
new file mode 100644
index 000000000000..8ff654950c5f
--- /dev/null
+++ b/tests/zbd/012.out
@@ -0,0 +1,2 @@
+Running zbd/012
+Test complete
-- 
2.35.1

Re: [PATCH 2/6] tests/nvme: add new test for rand-read on the nvme character device

[Kanchan Joshi]

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

On Wed, Dec 21, 2022 at 02:34:37AM -0800, Luis Chamberlain wrote:
>This does basic rand-read testing of the character device of a
>conventional NVMe drive.
>
>Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
>---
> tests/nvme/046     | 42 ++++++++++++++++++++++++++++++++++++++++++
> tests/nvme/046.out |  2 ++
> 2 files changed, 44 insertions(+)
> create mode 100755 tests/nvme/046
> create mode 100644 tests/nvme/046.out
>
>diff --git a/tests/nvme/046 b/tests/nvme/046
>new file mode 100755
>index 000000000000..3526ab9eedab
>--- /dev/null
>+++ b/tests/nvme/046
>@@ -0,0 +1,42 @@
>+#!/bin/bash
>+# SPDX-License-Identifier: GPL-3.0+
>+# Copyright (C) 2022 Luis Chamberlain <mcgrof@kernel.org>
>+#
>+# This does basic sanity test for the nvme character device. This is a basic
>+# test and if it fails it is probably very likely other nvme character device
>+# tests would fail.
>+#
>+. tests/nvme/rc
>+
>+DESCRIPTION="basic rand-read io_uring_cmd engine for nvme-ns character device"
>+QUICK=1
>+
>+requires() {
>+	_nvme_requires
>+	_have_fio
>+}
>+
>+device_requires() {
>+	_require_test_dev_is_nvme
>+}
>+
>+test_device() {
>+	echo "Running ${TEST_NAME}"
>+	local ngdev=${TEST_DEV/nvme/ng}
>+	local fio_args=(
>+		--size=1M
>+		--cmd_type=nvme
>+		--filename="$ngdev"
>+		--time_based
>+		--runtime=10
>+	) &&

Is this && needed?

>+	_run_fio_rand_iouring_cmd "${fio_args[@]}" >>"${FULL}" 2>&1 ||

Something to change here (and therefore in other patches too).
If we change "cmd_type = something_random", test continues to show the
success while it should show failure.

How about changing above line to:
_run_fio_rand_iouring_cmd "${fio_args[@]}" || fail=true

And thanks for the series.


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

Re: [PATCH 2/6] tests/nvme: add new test for rand-read on the nvme character device

[Luis Chamberlain]

On Fri, Dec 23, 2022 at 06:41:37PM +0530, Kanchan Joshi wrote:
> On Wed, Dec 21, 2022 at 02:34:37AM -0800, Luis Chamberlain wrote:
> > This does basic rand-read testing of the character device of a
> > conventional NVMe drive.
> > 
> > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> > ---
> > tests/nvme/046     | 42 ++++++++++++++++++++++++++++++++++++++++++
> > tests/nvme/046.out |  2 ++
> > 2 files changed, 44 insertions(+)
> > create mode 100755 tests/nvme/046
> > create mode 100644 tests/nvme/046.out
> > 
> > diff --git a/tests/nvme/046 b/tests/nvme/046
> > new file mode 100755
> > index 000000000000..3526ab9eedab
> > --- /dev/null
> > +++ b/tests/nvme/046
> > @@ -0,0 +1,42 @@
> > +#!/bin/bash
> > +# SPDX-License-Identifier: GPL-3.0+
> > +# Copyright (C) 2022 Luis Chamberlain <mcgrof@kernel.org>
> > +#
> > +# This does basic sanity test for the nvme character device. This is a basic
> > +# test and if it fails it is probably very likely other nvme character device
> > +# tests would fail.
> > +#
> > +. tests/nvme/rc
> > +
> > +DESCRIPTION="basic rand-read io_uring_cmd engine for nvme-ns character device"
> > +QUICK=1
> > +
> > +requires() {
> > +	_nvme_requires
> > +	_have_fio
> > +}
> > +
> > +device_requires() {
> > +	_require_test_dev_is_nvme
> > +}
> > +
> > +test_device() {
> > +	echo "Running ${TEST_NAME}"
> > +	local ngdev=${TEST_DEV/nvme/ng}
> > +	local fio_args=(
> > +		--size=1M
> > +		--cmd_type=nvme
> > +		--filename="$ngdev"
> > +		--time_based
> > +		--runtime=10
> > +	) &&
> 
> Is this && needed?

This form was inspired by commit 238c7e0b by Bart, but yeah you're
right, I can't see any reason for it, so we can clean zbd/010 from it too.
> 
> > +	_run_fio_rand_iouring_cmd "${fio_args[@]}" >>"${FULL}" 2>&1 ||
> 
> Something to change here (and therefore in other patches too).
> If we change "cmd_type = something_random", test continues to show the
> success while it should show failure.

Definitely no bueno.

> How about changing above line to:
> _run_fio_rand_iouring_cmd "${fio_args[@]}" || fail=true

We'd loose the 046.full log then.

If we just return $? at the end of _run_fio_rand_iouring_cmd() that
seems to fix the undetected error. Whatyda think?

I noticed an odd thing in the last two patches which work for zone
storage, if I change the runtime it doesn't take longer, so I think
something is still off there too... can you take a look?

  Luis

Re: [PATCH 0/6] blktests: char device tests with iouring-cmd fio

[Shinichiro Kawasaki]

On Dec 21, 2022 / 02:34, Luis Chamberlain wrote:
> As io-uring cmd grows there's a desire to do a bit more funky things
> with it. Add basic support with fio and add a few simple tests to
> tests the NVMe conventional drives character device as well as the
> ZNS character device.
> 
> These tests are perhaps a bit *too* basic to merge, not sure, let
> me know. But I figured that this would provide example to let us
> grow this with more complex things later as folks add support for
> more features.

It is good to have new test cases to test new features and their new code paths.
I agree to have new test cases for the NVMe character device with io-uring.

Having said that, I'm not sure if we should have all of the five test cases in
the series. The test cases nvme/046, 047, 048 are similar. They do random read,
random write, or sequential write respectively. I'm not sure how the workload
difference expands the code coverage of the code paths in the NVMe driver. Same
for zbd/011 and 012. They are intended for ZNS devices, but I do not see ZNS
unique part in the NVMe character device code paths. Then the variation of the
test cases do not look useful to find bugs in the driver. As the first step, I
think single test case will be enough which does basic read and/or write to
exercises the NVMe character device and io-uring.

-- 
Shin'ichiro Kawasaki

Re: [PATCH 1/6] common/fio: add helpers using io-uring cmd engine

[Joel Granados]

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

On Wed, Dec 21, 2022 at 02:34:36AM -0800, Luis Chamberlain wrote:
> This will allow us to add tests which use the io-uring cmd engine,
> part of fio. They are inspired by the work by Anuj Gupta and
> Ankit Kumar which added sample fio files onto fio for this exact
> purpose.
> 
> We can build on those to expand test coverage with elaborate tests.
> 
> We don't specify the cmd to allow other types of io-uring cmd
> users to use this other than nvme.
> 
> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> ---
>  common/fio | 47 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
> 
> diff --git a/common/fio b/common/fio
> index bed76d555b2a..4da90804ed21 100644
> --- a/common/fio
> +++ b/common/fio
> @@ -184,6 +184,53 @@ _run_fio_verify_io() {
>  	rm -f local*verify*state
>  }
>  
> +_run_fio_rand_iouring_cmd() {
> +	_run_fio --bs=4k --rw=randread --numjobs="$(nproc)" \
> +		--ioengine=io_uring_cmd --iodepth=32 \
> +		--thread=1 --stonewall=1 \
> +		--name=reads "$@"
> +}
> +
> +_run_fio_verify_iouring_cmd_randwrite() {
> +	_run_fio --bs=4k --rw=randwrite --numjobs="$(nproc)" \
> +		--ioengine=io_uring_cmd --iodepth=32 \
> +		--thread=1 --stonewall=1 \
> +		--sqthread_poll=1 --sqthread_poll_cpu=0 \
> +		--nonvectored=1 --registerfiles=1 \
> +		--verify=crc32c \
> +		--name=verify "$@"
> +	rm -f local*verify*state
> +}
> +
> +_run_fio_verify_iouring_cmd_write_opts() {
> +	_run_fio --bs=4k --rw=write --numjobs="$(nproc)" \
> +		--ioengine=io_uring_cmd --iodepth=32 \
> +		--thread=1 --stonewall=1 \
> +		--sqthread_poll=1 --sqthread_poll_cpu=0 \
> +		--nonvectored=1 --registerfiles=1 \
> +		--verify=crc32c \
> +		--name=verify "$@"
> +	rm -f local*verify*state
> +}
> +
> +_run_fio_iouring_cmd_zone() {
> +	_run_fio --rw=randread --numjobs="$(nproc)" \
> +		--ioengine=io_uring_cmd --iodepth=1 \
> +		--stonewall=1 \
> +		--zonemode=zbd \
> +		--name=reads "$@"
> +}
> +
> +_run_fio_verify_iouring_cmd_write_opts_zone() {
> +	_run_fio --rw=randread --numjobs="$(nproc)" \
> +		--ioengine=io_uring_cmd --iodepth=1 \
> +		--stonewall=1 \
> +		--zonemode=zbd \
> +		--sqthread_poll=1 --registerfiles=1 --sqthread_poll_cpu=0 \
> +		--verify=crc32c \
> +		--name=verify "$@"
Are you missing a "rm -f local*verify*state" here?

Joel
> +}
> +
>  _fio_perf_report() {
>  	# If there is more than one group, we don't know what to report.
>  	if [[ $(wc -l < "$TMPDIR/fio_perf") -gt 1 ]]; then
> -- 
> 2.35.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 2/6] tests/nvme: add new test for rand-read on the nvme character device

[Joel Granados]

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

On Wed, Dec 21, 2022 at 02:34:37AM -0800, Luis Chamberlain wrote:
> This does basic rand-read testing of the character device of a
> conventional NVMe drive.
> 
> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> ---
>  tests/nvme/046     | 42 ++++++++++++++++++++++++++++++++++++++++++
>  tests/nvme/046.out |  2 ++
>  2 files changed, 44 insertions(+)
>  create mode 100755 tests/nvme/046
>  create mode 100644 tests/nvme/046.out
> 
> diff --git a/tests/nvme/046 b/tests/nvme/046
> new file mode 100755
> index 000000000000..3526ab9eedab
> --- /dev/null
> +++ b/tests/nvme/046
> @@ -0,0 +1,42 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-3.0+
> +# Copyright (C) 2022 Luis Chamberlain <mcgrof@kernel.org>
> +#
> +# This does basic sanity test for the nvme character device. This is a basic
> +# test and if it fails it is probably very likely other nvme character device
> +# tests would fail.
> +#
> +. tests/nvme/rc
> +
> +DESCRIPTION="basic rand-read io_uring_cmd engine for nvme-ns character device"
> +QUICK=1
> +
> +requires() {
> +	_nvme_requires
> +	_have_fio
> +}
> +
> +device_requires() {
> +	_require_test_dev_is_nvme
> +}
> +
> +test_device() {
> +	echo "Running ${TEST_NAME}"
> +	local ngdev=${TEST_DEV/nvme/ng}
> +	local fio_args=(
> +		--size=1M
> +		--cmd_type=nvme
> +		--filename="$ngdev"
> +		--time_based
> +		--runtime=10
> +	) &&
> +	_run_fio_rand_iouring_cmd "${fio_args[@]}" >>"${FULL}" 2>&1 ||
> +	fail=true
> +
> +	if [ -z "$fail" ]; then
> +		echo "Test complete"
> +	else
> +		echo "Test failed"
> +		return 1
> +	fi
I see that several test have this structure, but would it not be better
to do:

"""
if [ -n "$fail" ]; then
  echo "Test failed"
  return 1
fi

return "Test complete"
"""

I point this out because I noticed that most nvme tests just set the
"test complete" string at the end of the test function.

> +}
> diff --git a/tests/nvme/046.out b/tests/nvme/046.out
> new file mode 100644
> index 000000000000..2b5fa6af63b1
> --- /dev/null
> +++ b/tests/nvme/046.out
> @@ -0,0 +1,2 @@
> +Running nvme/046
> +Test complete
> -- 
> 2.35.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 2/6] tests/nvme: add new test for rand-read on the nvme character device

[Chaitanya Kulkarni]

>> +
>> +	if [ -z "$fail" ]; then
>> +		echo "Test complete"
>> +	else
>> +		echo "Test failed"
>> +		return 1
>> +	fi
> I see that several test have this structure, but would it not be better
> to do:
> 
> """
> if [ -n "$fail" ]; then
>    echo "Test failed"
>    return 1
> fi
> 
> return "Test complete"
> """
> 
> I point this out because I noticed that most nvme tests just set the
> "test complete" string at the end of the test function.
> 

Yes, "Test complete" is sufficient to validate the correctness
and keeps the code small.

-ck

Re: [PATCH 2/6] tests/nvme: add new test for rand-read on the nvme character device

[Kanchan Joshi]

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

On Fri, Dec 23, 2022 at 07:56:35AM -0800, Luis Chamberlain wrote:
>On Fri, Dec 23, 2022 at 06:41:37PM +0530, Kanchan Joshi wrote:
>> On Wed, Dec 21, 2022 at 02:34:37AM -0800, Luis Chamberlain wrote:
>> > This does basic rand-read testing of the character device of a
>> > conventional NVMe drive.
>> >
>> > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
>> > ---
>> > tests/nvme/046     | 42 ++++++++++++++++++++++++++++++++++++++++++
>> > tests/nvme/046.out |  2 ++
>> > 2 files changed, 44 insertions(+)
>> > create mode 100755 tests/nvme/046
>> > create mode 100644 tests/nvme/046.out
>> >
>> > diff --git a/tests/nvme/046 b/tests/nvme/046
>> > new file mode 100755
>> > index 000000000000..3526ab9eedab
>> > --- /dev/null
>> > +++ b/tests/nvme/046
>> > @@ -0,0 +1,42 @@
>> > +#!/bin/bash
>> > +# SPDX-License-Identifier: GPL-3.0+
>> > +# Copyright (C) 2022 Luis Chamberlain <mcgrof@kernel.org>
>> > +#
>> > +# This does basic sanity test for the nvme character device. This is a basic
>> > +# test and if it fails it is probably very likely other nvme character device
>> > +# tests would fail.
>> > +#
>> > +. tests/nvme/rc
>> > +
>> > +DESCRIPTION="basic rand-read io_uring_cmd engine for nvme-ns character device"
>> > +QUICK=1
>> > +
>> > +requires() {
>> > +	_nvme_requires
>> > +	_have_fio
>> > +}
>> > +
>> > +device_requires() {
>> > +	_require_test_dev_is_nvme
>> > +}
>> > +
>> > +test_device() {
>> > +	echo "Running ${TEST_NAME}"
>> > +	local ngdev=${TEST_DEV/nvme/ng}
>> > +	local fio_args=(
>> > +		--size=1M
>> > +		--cmd_type=nvme
>> > +		--filename="$ngdev"
>> > +		--time_based
>> > +		--runtime=10
>> > +	) &&
>>
>> Is this && needed?
>
>This form was inspired by commit 238c7e0b by Bart, but yeah you're
>right, I can't see any reason for it, so we can clean zbd/010 from it too.
>>
>> > +	_run_fio_rand_iouring_cmd "${fio_args[@]}" >>"${FULL}" 2>&1 ||
>>
>> Something to change here (and therefore in other patches too).
>> If we change "cmd_type = something_random", test continues to show the
>> success while it should show failure.
>
>Definitely no bueno.
>
>> How about changing above line to:
>> _run_fio_rand_iouring_cmd "${fio_args[@]}" || fail=true
>
>We'd loose the 046.full log then.
>
>If we just return $? at the end of _run_fio_rand_iouring_cmd() that
>seems to fix the undetected error. Whatyda think?

It did not fix for me. It still shows test passed for "cmd_type=random".
How about this one instead-

_run_fio_rand_iouring_cmd "${fio_args[@]}" >>"${FULL}" || fail=true

it retains the full log, and shows the error when it occurs. 

>I noticed an odd thing in the last two patches which work for zone
>storage, if I change the runtime it doesn't take longer, so I think
>something is still off there too... can you take a look?

Runtime change works fine for first zoned test (i.e. read one). 
For the last one (i.e. zbd/012), fio fails early (that's why runtime
does not have any impact) because rw is set to randread along with
verify. It should rather be set to write.

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