[PATCH] nvme: add nvme pci timeout testcase

[PATCH] nvme: add nvme pci timeout testcase

[Chaitanya Kulkarni]

Trigger and test nvme-pci timeout with concurrent fio jobs.

Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
---
 tests/nvme/050     | 43 +++++++++++++++++++++++++++++++++++++++++++
 tests/nvme/050.out |  2 ++
 2 files changed, 45 insertions(+)
 create mode 100755 tests/nvme/050
 create mode 100644 tests/nvme/050.out

diff --git a/tests/nvme/050 b/tests/nvme/050
new file mode 100755
index 0000000..ba54bcd
--- /dev/null
+++ b/tests/nvme/050
@@ -0,0 +1,43 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-3.0+
+# Copyright (C) 2024 Chaitanya Kulkarni.
+#
+# Test NVMe-PCI timeout with FIO jobs by triggering the nvme_timeout function.
+#
+
+. tests/nvme/rc
+
+DESCRIPTION="test nvme-pci timeout with fio jobs."
+
+requires() {
+	_require_nvme_trtype pci
+	_have_fio
+	_nvme_requires
+}
+
+test() {
+	local dev="/dev/nvme0n1"
+
+	echo "Running ${TEST_NAME}"
+
+	echo 1 > /sys/block/nvme0n1/io-timeout-fail
+
+	echo 99 > /sys/kernel/debug/fail_io_timeout/probability
+	echo 10 > /sys/kernel/debug/fail_io_timeout/interval
+	echo -1 > /sys/kernel/debug/fail_io_timeout/times
+	echo  0 > /sys/kernel/debug/fail_io_timeout/space
+	echo  1 > /sys/kernel/debug/fail_io_timeout/verbose
+
+	# shellcheck disable=SC2046
+	fio --bs=4k --rw=randread --norandommap --numjobs=$(nproc) \
+	    --name=reads --direct=1 --filename=${dev} --group_reporting \
+	    --time_based --runtime=1m 2>&1 | grep -q "Input/output error"
+
+	# shellcheck disable=SC2181
+	if [ $? -eq 0 ]; then
+		echo "Test complete"
+	else
+		echo "Test failed"
+	fi
+	modprobe -r nvme
+}
diff --git a/tests/nvme/050.out b/tests/nvme/050.out
new file mode 100644
index 0000000..b78b05f
--- /dev/null
+++ b/tests/nvme/050.out
@@ -0,0 +1,2 @@
+Running nvme/050
+Test complete
-- 
2.40.0

Re: [PATCH] nvme: add nvme pci timeout testcase

[Nitesh Shetty]

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

On 09/01/24 07:57PM, Chaitanya Kulkarni wrote:
>Trigger and test nvme-pci timeout with concurrent fio jobs.
>
>Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
>---
> tests/nvme/050     | 43 +++++++++++++++++++++++++++++++++++++++++++
> tests/nvme/050.out |  2 ++
> 2 files changed, 45 insertions(+)
> create mode 100755 tests/nvme/050
> create mode 100644 tests/nvme/050.out
>
>diff --git a/tests/nvme/050 b/tests/nvme/050
>new file mode 100755
>index 0000000..ba54bcd
>--- /dev/null
>+++ b/tests/nvme/050
>@@ -0,0 +1,43 @@
>+#!/bin/bash
>+# SPDX-License-Identifier: GPL-3.0+
>+# Copyright (C) 2024 Chaitanya Kulkarni.
>+#
>+# Test NVMe-PCI timeout with FIO jobs by triggering the nvme_timeout function.
>+#
>+
>+. tests/nvme/rc
>+
>+DESCRIPTION="test nvme-pci timeout with fio jobs."
>+
>+requires() {
>+	_require_nvme_trtype pci
>+	_have_fio
>+	_nvme_requires
>+}
>+
>+test() {
>+	local dev="/dev/nvme0n1"

Should this be TEST_DEV instead ?

>+
>+	echo "Running ${TEST_NAME}"
>+
>+	echo 1 > /sys/block/nvme0n1/io-timeout-fail

nvme0n1, I think this hard coding can be changed

>+
>+	echo 99 > /sys/kernel/debug/fail_io_timeout/probability
>+	echo 10 > /sys/kernel/debug/fail_io_timeout/interval
>+	echo -1 > /sys/kernel/debug/fail_io_timeout/times
>+	echo  0 > /sys/kernel/debug/fail_io_timeout/space
>+	echo  1 > /sys/kernel/debug/fail_io_timeout/verbose
>+
>+	# shellcheck disable=SC2046
>+	fio --bs=4k --rw=randread --norandommap --numjobs=$(nproc) \
>+	    --name=reads --direct=1 --filename=${dev} --group_reporting \
>+	    --time_based --runtime=1m 2>&1 | grep -q "Input/output error"
>+
>+	# shellcheck disable=SC2181
>+	if [ $? -eq 0 ]; then
>+		echo "Test complete"
>+	else
>+		echo "Test failed"
>+	fi
>+	modprobe -r nvme
>+}
>diff --git a/tests/nvme/050.out b/tests/nvme/050.out
>new file mode 100644
>index 0000000..b78b05f
>--- /dev/null
>+++ b/tests/nvme/050.out
>@@ -0,0 +1,2 @@
>+Running nvme/050
>+Test complete
>-- 
>2.40.0
>


-- Nitesh Shetty

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

Re: [PATCH] nvme: add nvme pci timeout testcase

[Chaitanya Kulkarni]

On 1/9/24 22:17, Nitesh Shetty wrote:
> On 09/01/24 07:57PM, Chaitanya Kulkarni wrote:
>> Trigger and test nvme-pci timeout with concurrent fio jobs.
>>
>> Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
>> ---
>> tests/nvme/050     | 43 +++++++++++++++++++++++++++++++++++++++++++
>> tests/nvme/050.out |  2 ++
>> 2 files changed, 45 insertions(+)
>> create mode 100755 tests/nvme/050
>> create mode 100644 tests/nvme/050.out
>>
>> diff --git a/tests/nvme/050 b/tests/nvme/050
>> new file mode 100755
>> index 0000000..ba54bcd
>> --- /dev/null
>> +++ b/tests/nvme/050
>> @@ -0,0 +1,43 @@
>> +#!/bin/bash
>> +# SPDX-License-Identifier: GPL-3.0+
>> +# Copyright (C) 2024 Chaitanya Kulkarni.
>> +#
>> +# Test NVMe-PCI timeout with FIO jobs by triggering the nvme_timeout 
>> function.
>> +#
>> +
>> +. tests/nvme/rc
>> +
>> +DESCRIPTION="test nvme-pci timeout with fio jobs."
>> +
>> +requires() {
>> +    _require_nvme_trtype pci
>> +    _have_fio
>> +    _nvme_requires
>> +}
>> +
>> +test() {
>> +    local dev="/dev/nvme0n1"
>
> Should this be TEST_DEV instead ?

why ?

>
>
>> +
>> +    echo "Running ${TEST_NAME}"
>> +
>> +    echo 1 > /sys/block/nvme0n1/io-timeout-fail
>
> nvme0n1, I think this hard coding can be changed
>

why ?

-ck

Re: [PATCH] nvme: add nvme pci timeout testcase

[Nitesh Shetty]

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

On 10/01/24 07:40AM, Chaitanya Kulkarni wrote:
>On 1/9/24 22:17, Nitesh Shetty wrote:
>> On 09/01/24 07:57PM, Chaitanya Kulkarni wrote:
>>> Trigger and test nvme-pci timeout with concurrent fio jobs.
>>>
>>> Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
>>> ---
>>> tests/nvme/050     | 43 +++++++++++++++++++++++++++++++++++++++++++
>>> tests/nvme/050.out |  2 ++
>>> 2 files changed, 45 insertions(+)
>>> create mode 100755 tests/nvme/050
>>> create mode 100644 tests/nvme/050.out
>>>
>>> diff --git a/tests/nvme/050 b/tests/nvme/050
>>> new file mode 100755
>>> index 0000000..ba54bcd
>>> --- /dev/null
>>> +++ b/tests/nvme/050
>>> @@ -0,0 +1,43 @@
>>> +#!/bin/bash
>>> +# SPDX-License-Identifier: GPL-3.0+
>>> +# Copyright (C) 2024 Chaitanya Kulkarni.
>>> +#
>>> +# Test NVMe-PCI timeout with FIO jobs by triggering the nvme_timeout
>>> function.
>>> +#
>>> +
>>> +. tests/nvme/rc
>>> +
>>> +DESCRIPTION="test nvme-pci timeout with fio jobs."
>>> +
>>> +requires() {
>>> +    _require_nvme_trtype pci
>>> +    _have_fio
>>> +    _nvme_requires
>>> +}
>>> +
>>> +test() {
>>> +    local dev="/dev/nvme0n1"
>>
>> Should this be TEST_DEV instead ?
>
>why ?
>
My understanding of blktests is, add device which we want to test in
config files under TEST_DEV (except null-blk and nvme-fabrics loopback
devices, which are usually populated inside the tests).
In this case, if someone do not want to disturb nvme0n1 device,
this test doesn't allow it.

Regards,
Nitesh Shetty

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

Re: [PATCH] nvme: add nvme pci timeout testcase

[Chaitanya Kulkarni]

>>>
>>> Should this be TEST_DEV instead ?
>>
>> why ?
>>
> My understanding of blktests is, add device which we want to test in
> config files under TEST_DEV (except null-blk and nvme-fabrics loopback
> devices, which are usually populated inside the tests).
> In this case, if someone do not want to disturb nvme0n1 device,
> this test doesn't allow it.
>
> Regards,
> Nitesh Shetty
>

it is clearly stated in the documentation that blktests are destructive
to the entire system and including any devices you have, if your
system has sensitive data then _don't run these tests_ simple, when
you are running blktests you are bound to disturb system you can't
prevent that by using TEST_DEV.

-ck

Re: [PATCH] nvme: add nvme pci timeout testcase

[Shinichiro Kawasaki]

Hi Chaitanya, thanks for this new test case. I will comment on the whole patch
later. Here I share my comment on the TEST_DEV topic.

On Jan 10, 2024 / 08:12, Chaitanya Kulkarni wrote:
> 
> >>>
> >>> Should this be TEST_DEV instead ?
> >>
> >> why ?
> >>
> > My understanding of blktests is, add device which we want to test in
> > config files under TEST_DEV (except null-blk and nvme-fabrics loopback
> > devices, which are usually populated inside the tests).
> > In this case, if someone do not want to disturb nvme0n1 device,
> > this test doesn't allow it.
> >
> > Regards,
> > Nitesh Shetty
> >
> 
> it is clearly stated in the documentation that blktests are destructive
> to the entire system and including any devices you have, if your
> system has sensitive data then _don't run these tests_ simple, when
> you are running blktests you are bound to disturb system you can't
> prevent that by using TEST_DEV.

It's true that blktests is destructive to the test system and the test devices,
but I still think it's good to reduce the level of destructive risk. It is
likely that /dev/nvme0n1 is the system disk of the test system and the test case
puts some risk on it. By using TEST_DEV, blktests users can specify non-system
disk for testing and reduce the risk. We can run the test case (and its
following test cases to be added in the future) more reliably.

On top of that, TEST_DEV will help blktests users to control which device to
take the destructive risk. It also helps to check that the test target device
exists and it is a block, nvme device.

So I recommend to use TEST_DEV in place of /dev/nvme0n1, and replace test() with
test_device(). TEST_DEV_SYSFS will help to access the io-timeout-fail sysfs
attribute.

Re: [PATCH] nvme: add nvme pci timeout testcase

[Hannes Reinecke]

On 1/10/24 09:12, Chaitanya Kulkarni wrote:
> 
>>>>
>>>> Should this be TEST_DEV instead ?
>>>
>>> why ?
>>>
>> My understanding of blktests is, add device which we want to test in
>> config files under TEST_DEV (except null-blk and nvme-fabrics loopback
>> devices, which are usually populated inside the tests).
>> In this case, if someone do not want to disturb nvme0n1 device,
>> this test doesn't allow it.
>>
>> Regards,
>> Nitesh Shetty
>>
> 
> it is clearly stated in the documentation that blktests are destructive
> to the entire system and including any devices you have, if your
> system has sensitive data then _don't run these tests_ simple, when
> you are running blktests you are bound to disturb system you can't
> prevent that by using TEST_DEV.
> 
I don't think this is the direction we want to go.
NVMe for internal drives is becoming more and more prevalent (especially
on laptops), making them unusable for running blktests.

We have been putting quite some effort into nvme-cli to ensure that
blktest _can_ run concurrently with other NVMe drives, so really we
should not hard-code any device names.

Or we discuss this at LSF/MM; Daniel or me will be happy to give an
overview about concurrent NVMe-oF management applications on the same
system (nvme-cli, nvme-stas, blktests all running at the same time).

Cheers,

Hannes

Re: [PATCH] nvme: add nvme pci timeout testcase

[Chaitanya Kulkarni]

On 1/10/2024 1:24 AM, Hannes Reinecke wrote:
> On 1/10/24 09:12, Chaitanya Kulkarni wrote:
>>
>>>>>
>>>>> Should this be TEST_DEV instead ?
>>>>
>>>> why ?
>>>>
>>> My understanding of blktests is, add device which we want to test in
>>> config files under TEST_DEV (except null-blk and nvme-fabrics loopback
>>> devices, which are usually populated inside the tests).
>>> In this case, if someone do not want to disturb nvme0n1 device,
>>> this test doesn't allow it.
>>>
>>> Regards,
>>> Nitesh Shetty
>>>
>>
>> it is clearly stated in the documentation that blktests are destructive
>> to the entire system and including any devices you have, if your
>> system has sensitive data then _don't run these tests_ simple, when
>> you are running blktests you are bound to disturb system you can't
>> prevent that by using TEST_DEV.
>>
> I don't think this is the direction we want to go.
> NVMe for internal drives is becoming more and more prevalent (especially
> on laptops), making them unusable for running blktests.
> 
> We have been putting quite some effort into nvme-cli to ensure that
> blktest _can_ run concurrently with other NVMe drives, so really we
> should not hard-code any device names.
> 

Okay ...

> Or we discuss this at LSF/MM; Daniel or me will be happy to give an
> overview about concurrent NVMe-oF management applications on the same
> system (nvme-cli, nvme-stas, blktests all running at the same time).
> 

would be a great topic for the blktests session ...

> Cheers,
> 
> Hannes
> 

I'll make it TEST_DEV change ...

-ck

Re: [PATCH] nvme: add nvme pci timeout testcase

[Jens Axboe]

On 1/10/24 1:12 AM, Chaitanya Kulkarni wrote:
> 
>>>>
>>>> Should this be TEST_DEV instead ?
>>>
>>> why ?
>>>
>> My understanding of blktests is, add device which we want to test in
>> config files under TEST_DEV (except null-blk and nvme-fabrics loopback
>> devices, which are usually populated inside the tests).
>> In this case, if someone do not want to disturb nvme0n1 device,
>> this test doesn't allow it.
>>
>> Regards,
>> Nitesh Shetty
>>
> 
> it is clearly stated in the documentation that blktests are destructive
> to the entire system and including any devices you have, if your
> system has sensitive data then _don't run these tests_ simple, when
> you are running blktests you are bound to disturb system you can't
> prevent that by using TEST_DEV.

It should very much be possible to run blktests on specific devices
ONLY, the intent of a test suite is not to potentially mess up
everything around you. It should ONLY operate on the devices given, not
poke at random devices in the system.

-- 
Jens Axboe

Re: [PATCH] nvme: add nvme pci timeout testcase

[Chaitanya Kulkarni]

On 1/10/2024 6:59 AM, Jens Axboe wrote:
> On 1/10/24 1:12 AM, Chaitanya Kulkarni wrote:
>>
>>>>>
>>>>> Should this be TEST_DEV instead ?
>>>>
>>>> why ?
>>>>
>>> My understanding of blktests is, add device which we want to test in
>>> config files under TEST_DEV (except null-blk and nvme-fabrics loopback
>>> devices, which are usually populated inside the tests).
>>> In this case, if someone do not want to disturb nvme0n1 device,
>>> this test doesn't allow it.
>>>
>>> Regards,
>>> Nitesh Shetty
>>>
>>
>> it is clearly stated in the documentation that blktests are destructive
>> to the entire system and including any devices you have, if your
>> system has sensitive data then _don't run these tests_ simple, when
>> you are running blktests you are bound to disturb system you can't
>> prevent that by using TEST_DEV.
> 
> It should very much be possible to run blktests on specific devices
> ONLY, the intent of a test suite is not to potentially mess up
> everything around you. It should ONLY operate on the devices given, not
> poke at random devices in the system.
> 
I'll make the TEST_DEV change ..

-ck

Re: [PATCH] nvme: add nvme pci timeout testcase

[Shinichiro Kawasaki]

Thanks again for this test case. Please find my review comments. Tomorrow, I
will try to run this test case.

On Jan 09, 2024 / 19:57, Chaitanya Kulkarni wrote:
> Trigger and test nvme-pci timeout with concurrent fio jobs.

Is there any related kernel commit which motivated this test case? If so,
can we add a kernel commit or e-mail discussion link as a reference?

> 
> Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
> ---
>  tests/nvme/050     | 43 +++++++++++++++++++++++++++++++++++++++++++
>  tests/nvme/050.out |  2 ++
>  2 files changed, 45 insertions(+)
>  create mode 100755 tests/nvme/050
>  create mode 100644 tests/nvme/050.out
> 
> diff --git a/tests/nvme/050 b/tests/nvme/050
> new file mode 100755
> index 0000000..ba54bcd
> --- /dev/null
> +++ b/tests/nvme/050
> @@ -0,0 +1,43 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-3.0+
> +# Copyright (C) 2024 Chaitanya Kulkarni.

Nit: you may want to remove the last '.'

> +#
> +# Test NVMe-PCI timeout with FIO jobs by triggering the nvme_timeout function.
> +#
> +
> +. tests/nvme/rc
> +
> +DESCRIPTION="test nvme-pci timeout with fio jobs."

To be consitent with other test cases, let's remove the last '.'.

> +
> +requires() {
> +	_require_nvme_trtype pci
> +	_have_fio
> +	_nvme_requires

This test case depends on the kernel config FAIL_IO_TIMEOUT. Let's add

    _have_kernel_option FAIL_IO_TIMEOUT

> +}
> +
> +test() {
> +	local dev="/dev/nvme0n1"
> +
> +	echo "Running ${TEST_NAME}"
> +
> +	echo 1 > /sys/block/nvme0n1/io-timeout-fail
> +
> +	echo 99 > /sys/kernel/debug/fail_io_timeout/probability
> +	echo 10 > /sys/kernel/debug/fail_io_timeout/interval
> +	echo -1 > /sys/kernel/debug/fail_io_timeout/times
> +	echo  0 > /sys/kernel/debug/fail_io_timeout/space
> +	echo  1 > /sys/kernel/debug/fail_io_timeout/verbose

To avoid impact on following test cases, I suggest to restore the original
values of the fail_io_timeout/* sysfs attributes above at the end of this
test case. _nvme_err_inject_setup() and _nvme_err_inject_cleanup() in
test/nvme/rc do similar thing. We can add new helper functions in same manner.

> +
> +	# shellcheck disable=SC2046
> +	fio --bs=4k --rw=randread --norandommap --numjobs=$(nproc) \

Double quotes for "$(nproc)" will allow to remove the shellcheck exception,
probably.

> +	    --name=reads --direct=1 --filename=${dev} --group_reporting \
> +	    --time_based --runtime=1m 2>&1 | grep -q "Input/output error"
> +
> +	# shellcheck disable=SC2181
> +	if [ $? -eq 0 ]; then
> +		echo "Test complete"
> +	else
> +		echo "Test failed"
> +	fi
> +	modprobe -r nvme

If nvme driver is built-in, this unload command does not work.
Do we really need to unload nvme driver here?

> +}
> diff --git a/tests/nvme/050.out b/tests/nvme/050.out
> new file mode 100644
> index 0000000..b78b05f
> --- /dev/null
> +++ b/tests/nvme/050.out
> @@ -0,0 +1,2 @@
> +Running nvme/050
> +Test complete
> -- 
> 2.40.0
> 
> 

Re: [PATCH] nvme: add nvme pci timeout testcase

[Chaitanya Kulkarni]

On 1/10/2024 4:23 AM, Shinichiro Kawasaki wrote:
> Thanks again for this test case. Please find my review comments. Tomorrow, I
> will try to run this test case.
> 
> On Jan 09, 2024 / 19:57, Chaitanya Kulkarni wrote:
>> Trigger and test nvme-pci timeout with concurrent fio jobs.
> 
> Is there any related kernel commit which motivated this test case? If so,
> can we add a kernel commit or e-mail discussion link as a reference?
> 

no, this part if never tested on the regular basis as it has some
complicated logic I believe it is much needed test ..

>>
>> Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
>> ---
>>   tests/nvme/050     | 43 +++++++++++++++++++++++++++++++++++++++++++
>>   tests/nvme/050.out |  2 ++
>>   2 files changed, 45 insertions(+)
>>   create mode 100755 tests/nvme/050
>>   create mode 100644 tests/nvme/050.out
>>
>> diff --git a/tests/nvme/050 b/tests/nvme/050
>> new file mode 100755
>> index 0000000..ba54bcd
>> --- /dev/null
>> +++ b/tests/nvme/050
>> @@ -0,0 +1,43 @@
>> +#!/bin/bash
>> +# SPDX-License-Identifier: GPL-3.0+
>> +# Copyright (C) 2024 Chaitanya Kulkarni.
> 
> Nit: you may want to remove the last '.'

okay ...

> 
>> +#
>> +# Test NVMe-PCI timeout with FIO jobs by triggering the nvme_timeout function.
>> +#
>> +
>> +. tests/nvme/rc
>> +
>> +DESCRIPTION="test nvme-pci timeout with fio jobs."
> 
> To be consitent with other test cases, let's remove the last '.'.

okay ...

> 
>> +
>> +requires() {
>> +	_require_nvme_trtype pci
>> +	_have_fio
>> +	_nvme_requires
> 
> This test case depends on the kernel config FAIL_IO_TIMEOUT. Let's add
> 
>      _have_kernel_option FAIL_IO_TIMEOUT

indeed, I'll add that ...

> 
>> +}
>> +
>> +test() {
>> +	local dev="/dev/nvme0n1"
>> +
>> +	echo "Running ${TEST_NAME}"
>> +
>> +	echo 1 > /sys/block/nvme0n1/io-timeout-fail
>> +
>> +	echo 99 > /sys/kernel/debug/fail_io_timeout/probability
>> +	echo 10 > /sys/kernel/debug/fail_io_timeout/interval
>> +	echo -1 > /sys/kernel/debug/fail_io_timeout/times
>> +	echo  0 > /sys/kernel/debug/fail_io_timeout/space
>> +	echo  1 > /sys/kernel/debug/fail_io_timeout/verbose
> 
> To avoid impact on following test cases, I suggest to restore the original
> values of the fail_io_timeout/* sysfs attributes above at the end of this
> test case. _nvme_err_inject_setup() and _nvme_err_inject_cleanup() in
> test/nvme/rc do similar thing. We can add new helper functions in same manner.

I can add the code for config and restore, but at this point there are
no other testcases using this ?(correct me if I'm wrong), so instead of
bloating the code in nvme/rc let's keep it for this testcase only ?
and add common helpers code later once we have more users for this
case ?

> 
>> +
>> +	# shellcheck disable=SC2046
>> +	fio --bs=4k --rw=randread --norandommap --numjobs=$(nproc) \
> 
> Double quotes for "$(nproc)" will allow to remove the shellcheck exception,
> probably.

not sure I understand this comment, can you please elaborate ?

> 
>> +	    --name=reads --direct=1 --filename=${dev} --group_reporting \
>> +	    --time_based --runtime=1m 2>&1 | grep -q "Input/output error"
>> +
>> +	# shellcheck disable=SC2181
>> +	if [ $? -eq 0 ]; then
>> +		echo "Test complete"
>> +	else
>> +		echo "Test failed"
>> +	fi
>> +	modprobe -r nvme
> 
> If nvme driver is built-in, this unload command does not work.
> Do we really need to unload nvme driver here?
> 

Yes, post timeout handler execution we need to make sure that device
can be removed at the time of module unload, perhaps there is a way to
avoid this when nvme is a built-in module so that test will not execute
above ? any suggestions ?

-ck

Re: [PATCH] nvme: add nvme pci timeout testcase

[Shinichiro Kawasaki]

On Jan 11, 2024 / 00:00, Chaitanya Kulkarni wrote:
> On 1/10/2024 4:23 AM, Shinichiro Kawasaki wrote:
> > Thanks again for this test case. Please find my review comments. Tomorrow, I
> > will try to run this test case.
> > 
> > On Jan 09, 2024 / 19:57, Chaitanya Kulkarni wrote:
> >> Trigger and test nvme-pci timeout with concurrent fio jobs.
> > 
> > Is there any related kernel commit which motivated this test case? If so,
> > can we add a kernel commit or e-mail discussion link as a reference?
> > 
> 
> no, this part if never tested on the regular basis as it has some
> complicated logic I believe it is much needed test ..

Okay, it's good to expand the test coverage.

...

> >> +}
> >> +
> >> +test() {
> >> +	local dev="/dev/nvme0n1"
> >> +
> >> +	echo "Running ${TEST_NAME}"
> >> +
> >> +	echo 1 > /sys/block/nvme0n1/io-timeout-fail
> >> +
> >> +	echo 99 > /sys/kernel/debug/fail_io_timeout/probability
> >> +	echo 10 > /sys/kernel/debug/fail_io_timeout/interval
> >> +	echo -1 > /sys/kernel/debug/fail_io_timeout/times
> >> +	echo  0 > /sys/kernel/debug/fail_io_timeout/space
> >> +	echo  1 > /sys/kernel/debug/fail_io_timeout/verbose
> > 
> > To avoid impact on following test cases, I suggest to restore the original
> > values of the fail_io_timeout/* sysfs attributes above at the end of this
> > test case. _nvme_err_inject_setup() and _nvme_err_inject_cleanup() in
> > test/nvme/rc do similar thing. We can add new helper functions in same manner.
> 
> I can add the code for config and restore, but at this point there are
> no other testcases using this ?(correct me if I'm wrong),

This new test case is the only one user of fail_io_timeout, I believe.

> so instead of
> bloating the code in nvme/rc let's keep it for this testcase only ?
> and add common helpers code later once we have more users for this
> case ?

I think either way is good.

> 
> > 
> >> +
> >> +	# shellcheck disable=SC2046
> >> +	fio --bs=4k --rw=randread --norandommap --numjobs=$(nproc) \
> > 
> > Double quotes for "$(nproc)" will allow to remove the shellcheck exception,
> > probably.
> 
> not sure I understand this comment, can you please elaborate ?

I suggest this:

diff --git a/tests/nvme/050 b/tests/nvme/050
index ba54bcd..8e5acb2 100755
--- a/tests/nvme/050
+++ b/tests/nvme/050
@@ -28,8 +28,7 @@ test() {
 	echo  0 > /sys/kernel/debug/fail_io_timeout/space
 	echo  1 > /sys/kernel/debug/fail_io_timeout/verbose
 
-	# shellcheck disable=SC2046
-	fio --bs=4k --rw=randread --norandommap --numjobs=$(nproc) \
+	fio --bs=4k --rw=randread --norandommap --numjobs="$(nproc)" \
 	    --name=reads --direct=1 --filename=${dev} --group_reporting \
 	    --time_based --runtime=1m 2>&1 | grep -q "Input/output error"
 

> 
> > 
> >> +	    --name=reads --direct=1 --filename=${dev} --group_reporting \
> >> +	    --time_based --runtime=1m 2>&1 | grep -q "Input/output error"
> >> +
> >> +	# shellcheck disable=SC2181
> >> +	if [ $? -eq 0 ]; then
> >> +		echo "Test complete"
> >> +	else
> >> +		echo "Test failed"
> >> +	fi
> >> +	modprobe -r nvme
> > 
> > If nvme driver is built-in, this unload command does not work.
> > Do we really need to unload nvme driver here?
> > 
> 
> Yes, post timeout handler execution we need to make sure that device
> can be removed at the time of module unload, perhaps there is a way to
> avoid this when nvme is a built-in module so that test will not execute
> above ? any suggestions ?

I see... One idea I can think of is to remove the device using PCI device
sysfs. How about the code below? block/011 does similar thing. To do that
for TEST_DEV, we can use _get_pci_from_dev_sysfs() in place of
_get_pci_from_dev_sysfs(). Does this meet your intent?

diff --git a/tests/nvme/050 b/tests/nvme/050
index 8e5acb2..592f167 100755
--- a/tests/nvme/050
+++ b/tests/nvme/050
@@ -17,6 +17,9 @@ requires() {
 
 test() {
 	local dev="/dev/nvme0n1"
+	local pdev
+
+	pdev="$(_get_pci_from_dev_sysfs /dev/nvme0n1)"
 
 	echo "Running ${TEST_NAME}"
 
@@ -38,5 +41,11 @@ test() {
 	else
 		echo "Test failed"
 	fi
-	modprobe -r nvme
+
+	# Remove and rescan the NVME device to ensure that it has come back
+	echo 1 > "/sys/bus/pci/devices/$pdev/remove"
+	echo 1 > /sys/bus/pci/rescan
+	if [[ ! -b $TEST_DEV ]]; then
+		echo "Failed to regain $TEST_DEV"
+	fi
 }


BTW, when I ran the test case, I observed the test case failed. I took a quick
look, and saw the nvme driver reported I/O timeout in kernel messages. But fio
did not report the expected I/O error. Not sure why. I will look closer when I
check v2 patch.

Re: [PATCH] nvme: add nvme pci timeout testcase

[Chaitanya Kulkarni]

> BTW, when I ran the test case, I observed the test case failed. I took a quick
> look, and saw the nvme driver reported I/O timeout in kernel messages. But fio
> did not report the expected I/O error. Not sure why. I will look closer when I
> check v2 patch.

Ohh really ? I've consistently seen I/O timeout error in the kernel message
and with fio output giving I/O error.

Thanks a lot for looking into this, all the suggestions look good to me,
meanwhile I've raised a question that ideal scenario should be in order to
pass the testcase see [1].

As I'm getting device with 0 capacity at the end of testcase and there are
still open questions. As soon as I get reply will modify the testcase 
passing
scenario and send V2.

-ck

[1]
http://lists.infradead.org/pipermail/linux-nvme/2024-January/044263.html

Re: [PATCH] nvme: add nvme pci timeout testcase

[Yi Zhang]

On Wed, Jan 10, 2024 at 11:58 AM Chaitanya Kulkarni <kch@nvidia.com> wrote:
>
> Trigger and test nvme-pci timeout with concurrent fio jobs.
>
> Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
> ---
>  tests/nvme/050     | 43 +++++++++++++++++++++++++++++++++++++++++++
>  tests/nvme/050.out |  2 ++
>  2 files changed, 45 insertions(+)
>  create mode 100755 tests/nvme/050
>  create mode 100644 tests/nvme/050.out
>
> diff --git a/tests/nvme/050 b/tests/nvme/050
> new file mode 100755
> index 0000000..ba54bcd
> --- /dev/null
> +++ b/tests/nvme/050
> @@ -0,0 +1,43 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-3.0+
> +# Copyright (C) 2024 Chaitanya Kulkarni.
> +#
> +# Test NVMe-PCI timeout with FIO jobs by triggering the nvme_timeout function.
> +#
> +
> +. tests/nvme/rc

How about restricting this test to pci only? just like nvme/032 did.
nvme_trtype=pci

> +
> +DESCRIPTION="test nvme-pci timeout with fio jobs."
> +
> +requires() {
> +       _require_nvme_trtype pci
> +       _have_fio
> +       _nvme_requires
> +}

Check the test dev is nvme:
device_requires() {
        _require_test_dev_is_nvme
}

> +
> +test() {

Use test_device() here.

> +       local dev="/dev/nvme0n1"
> +
> +       echo "Running ${TEST_NAME}"
> +
> +       echo 1 > /sys/block/nvme0n1/io-timeout-fail
> +
> +       echo 99 > /sys/kernel/debug/fail_io_timeout/probability
> +       echo 10 > /sys/kernel/debug/fail_io_timeout/interval
> +       echo -1 > /sys/kernel/debug/fail_io_timeout/times
> +       echo  0 > /sys/kernel/debug/fail_io_timeout/space
> +       echo  1 > /sys/kernel/debug/fail_io_timeout/verbose
> +
> +       # shellcheck disable=SC2046
> +       fio --bs=4k --rw=randread --norandommap --numjobs=$(nproc) \
> +           --name=reads --direct=1 --filename=${dev} --group_reporting \
> +           --time_based --runtime=1m 2>&1 | grep -q "Input/output error"
> +
> +       # shellcheck disable=SC2181
> +       if [ $? -eq 0 ]; then
> +               echo "Test complete"
> +       else
> +               echo "Test failed"
> +       fi
> +       modprobe -r nvme
> +}
> diff --git a/tests/nvme/050.out b/tests/nvme/050.out
> new file mode 100644
> index 0000000..b78b05f
> --- /dev/null
> +++ b/tests/nvme/050.out
> @@ -0,0 +1,2 @@
> +Running nvme/050
> +Test complete
> --
> 2.40.0
>
>


-- 
Best Regards,
  Yi Zhang

Re: [PATCH] nvme: add nvme pci timeout testcase

[Chaitanya Kulkarni]

On 1/10/24 17:25, Yi Zhang wrote:
> On Wed, Jan 10, 2024 at 11:58 AM Chaitanya Kulkarni <kch@nvidia.com> wrote:
>> Trigger and test nvme-pci timeout with concurrent fio jobs.
>>
>> Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
>> ---
>>   tests/nvme/050     | 43 +++++++++++++++++++++++++++++++++++++++++++
>>   tests/nvme/050.out |  2 ++
>>   2 files changed, 45 insertions(+)
>>   create mode 100755 tests/nvme/050
>>   create mode 100644 tests/nvme/050.out
>>
>> diff --git a/tests/nvme/050 b/tests/nvme/050
>> new file mode 100755
>> index 0000000..ba54bcd
>> --- /dev/null
>> +++ b/tests/nvme/050
>> @@ -0,0 +1,43 @@
>> +#!/bin/bash
>> +# SPDX-License-Identifier: GPL-3.0+
>> +# Copyright (C) 2024 Chaitanya Kulkarni.
>> +#
>> +# Test NVMe-PCI timeout with FIO jobs by triggering the nvme_timeout function.
>> +#
>> +
>> +. tests/nvme/rc
> How about restricting this test to pci only? just like nvme/032 did.
> nvme_trtype=pci

Yes it has to now, since I'm adding TEST_DEV, thanks for mentioning that
already added that change to v2.

>> +
>> +DESCRIPTION="test nvme-pci timeout with fio jobs."
>> +
>> +requires() {
>> +       _require_nvme_trtype pci
>> +       _have_fio
>> +       _nvme_requires
>> +}
> Check the test dev is nvme:
> device_requires() {
>          _require_test_dev_is_nvme
> }
>
>> +
>> +test() {
> Use test_device() here.

okay ..


-ck