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

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

[-- 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 --]