diff mbox series

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

Message ID 20221221103441.3216600-3-mcgrof@kernel.org (mailing list archive)
State New, archived
Headers show
Series blktests: char device tests with iouring-cmd fio | expand

Commit Message

Luis Chamberlain Dec. 21, 2022, 10:34 a.m. UTC 
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

Comments

Kanchan Joshi Dec. 23, 2022, 1:11 p.m. UTC | #1 
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.
Luis Chamberlain Dec. 23, 2022, 3:56 p.m. UTC | #2 
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
Joel Granados Dec. 30, 2022, 10:37 a.m. UTC | #3 
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
>
Chaitanya Kulkarni Jan. 3, 2023, 5:48 a.m. UTC | #4 
>> +
>> +	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
Kanchan Joshi Jan. 17, 2023, 7:46 a.m. UTC | #5 
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.
diff mbox series

Patch

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