* [PATCH blktest] nvme/033: add test case for nvme update hardware queue count
@ 2020-04-07 14:16 Weiping Zhang
2020-04-07 15:29 ` Chaitanya Kulkarni
2020-04-08 21:40 ` Omar Sandoval
0 siblings, 2 replies; 6+ messages in thread
From: Weiping Zhang @ 2020-04-07 14:16 UTC (permalink / raw)
To: osandov; +Cc: linux-block
Modify nvme module parameter write_queues to change hardware
queue count, then reset nvme controller to reinitialize nvme
with different queue count.
Attention, this test case may trigger a kernel panic.
Signed-off-by: Weiping Zhang <zhangweiping@didiglobal.com>
---
tests/nvme/033 | 87 ++++++++++++++++++++++++++++++++++++++++++++++
tests/nvme/033.out | 2 ++
2 files changed, 89 insertions(+)
create mode 100755 tests/nvme/033
create mode 100644 tests/nvme/033.out
diff --git a/tests/nvme/033 b/tests/nvme/033
new file mode 100755
index 0000000..e3b9211
--- /dev/null
+++ b/tests/nvme/033
@@ -0,0 +1,87 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-3.0+
+# Copyright (C) 2020 Weiping Zhang <zwp10758@gmail.com>
+#
+# Test nvme update hardware queue count larger than system cpu count
+#
+
+. tests/nvme/rc
+
+DESCRIPTION="test nvme update hardware queue count larger than system cpu count"
+QUICK=1
+
+requires() {
+ _have_program dd
+}
+
+device_requires() {
+ _test_dev_is_nvme
+}
+
+test_device() {
+ echo "Running ${TEST_NAME}"
+
+ local old_write_queues
+ local cur_hw_io_queues
+ local file
+ local sys_dev=$TEST_DEV_SYSFS/device
+
+ # backup old module parameter: write_queues
+ file=/sys/module/nvme/parameters/write_queues
+ if [[ ! -e "$file" ]]; then
+ echo "$file does not exist"
+ return 1
+ fi
+ old_write_queues="$(cat $file)"
+
+ # get current hardware queue count
+ file="$sys_dev/queue_count"
+ if [[ ! -e "$file" ]]; then
+ echo "$file does not exist"
+ return 1
+ fi
+ cur_hw_io_queues="$(cat "$file")"
+ # minus admin queue
+ cur_hw_io_queues=$((cur_hw_io_queues - 1))
+
+ # set write queues count to increase more hardware queues
+ file=/sys/module/nvme/parameters/write_queues
+ echo "$cur_hw_io_queues" > "$file"
+
+ # reset controller, make it effective
+ file="$sys_dev/reset_controller"
+ if [[ ! -e "$file" ]]; then
+ echo "$file does not exist"
+ return 1
+ fi
+ echo 1 > "$file"
+
+ # wait nvme reinitialized
+ for ((m = 0; m < 10; m++)); do
+ if [[ -b "${TEST_DEV}" ]]; then
+ break
+ fi
+ sleep 0.5
+ done
+ if (( m > 9 )); then
+ echo "nvme still not reinitialized after 5 seconds!"
+ return 1
+ fi
+
+ # read data from device (may kernel panic)
+ dd if="${TEST_DEV}" of=/dev/null bs=4096 count=1 status=none
+
+ # If all work well restore hardware queue to default
+ file=/sys/module/nvme/parameters/write_queues
+ echo "$old_write_queues" > "$file"
+
+ # reset controller
+ file="$sys_dev/reset_controller"
+ echo 1 > "$file"
+
+ # read data from device (may kernel panic)
+ dd if="${TEST_DEV}" of=/dev/null bs=4096 count=1 iflag=direct status=none
+ dd if=/dev/zero of="${TEST_DEV}" bs=4096 count=1 oflag=direct status=none
+
+ echo "Test complete"
+}
diff --git a/tests/nvme/033.out b/tests/nvme/033.out
new file mode 100644
index 0000000..9648c73
--- /dev/null
+++ b/tests/nvme/033.out
@@ -0,0 +1,2 @@
+Running nvme/033
+Test complete
--
2.18.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH blktest] nvme/033: add test case for nvme update hardware queue count
2020-04-07 14:16 [PATCH blktest] nvme/033: add test case for nvme update hardware queue count Weiping Zhang
@ 2020-04-07 15:29 ` Chaitanya Kulkarni
2020-04-08 12:19 ` Weiping Zhang
2020-04-08 21:40 ` Omar Sandoval
1 sibling, 1 reply; 6+ messages in thread
From: Chaitanya Kulkarni @ 2020-04-07 15:29 UTC (permalink / raw)
To: Weiping Zhang, osandov; +Cc: linux-block
On 04/07/2020 07:17 AM, Weiping Zhang wrote:
> Modify nvme module parameter write_queues to change hardware
> queue count, then reset nvme controller to reinitialize nvme
> with different queue count.
>
> Attention, this test case may trigger a kernel panic.
>
> Signed-off-by: Weiping Zhang <zhangweiping@didiglobal.com>
> ---
> tests/nvme/033 | 87 ++++++++++++++++++++++++++++++++++++++++++++++
> tests/nvme/033.out | 2 ++
> 2 files changed, 89 insertions(+)
> create mode 100755 tests/nvme/033
> create mode 100644 tests/nvme/033.out
>
> diff --git a/tests/nvme/033 b/tests/nvme/033
> new file mode 100755
> index 0000000..e3b9211
> --- /dev/null
> +++ b/tests/nvme/033
> @@ -0,0 +1,87 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-3.0+
> +# Copyright (C) 2020 Weiping Zhang <zwp10758@gmail.com>
> +#
> +# Test nvme update hardware queue count larger than system cpu count
> +#
> +
> +. tests/nvme/rc
> +
> +DESCRIPTION="test nvme update hardware queue count larger than system cpu count"
> +QUICK=1
> +
> +requires() {
> + _have_program dd
> +}
> +
> +device_requires() {
> + _test_dev_is_nvme
> +}
> +
> +test_device() {
> + echo "Running ${TEST_NAME}"
> +
> + local old_write_queues
> + local cur_hw_io_queues
> + local file
> + local sys_dev=$TEST_DEV_SYSFS/device
> +
> + # backup old module parameter: write_queues
> + file=/sys/module/nvme/parameters/write_queues
> + if [[ ! -e "$file" ]]; then
> + echo "$file does not exist"
> + return 1
> + fi
can we skip the test ? I think Omar added a feature to skip the test.
> + old_write_queues="$(cat $file)"
> +
> + # get current hardware queue count
> + file="$sys_dev/queue_count"
> + if [[ ! -e "$file" ]]; then
> + echo "$file does not exist"
> + return 1
> + fi
Same here.
> + cur_hw_io_queues="$(cat "$file")"
> + # minus admin queue
> + cur_hw_io_queues=$((cur_hw_io_queues - 1))
> +
> + # set write queues count to increase more hardware queues
> + file=/sys/module/nvme/parameters/write_queues
> + echo "$cur_hw_io_queues" > "$file"
> +
Shouldn't we check if new queue count is set here by reading
write_queues ?
> + # reset controller, make it effective
> + file="$sys_dev/reset_controller"
> + if [[ ! -e "$file" ]]; then
> + echo "$file does not exist"
> + return 1
> + fi
I think we need to add a helper to verify all the files and skip the
test required file doesn't exists. Also, please use different variables
representing different files.
> + echo 1 > "$file"
> +
> + # wait nvme reinitialized
> + for ((m = 0; m < 10; m++)); do
> + if [[ -b "${TEST_DEV}" ]]; then
> + break
> + fi
> + sleep 0.5
> + done
> + if (( m > 9 )); then
> + echo "nvme still not reinitialized after 5 seconds!"
> + return 1
> + fi
> +
> + # read data from device (may kernel panic)
> + dd if="${TEST_DEV}" of=/dev/null bs=4096 count=1 status=none
> +
This should not lead to the kernel panic. Do you have a patch to fix
the panic ? If not can you please provide information so that we can
fix the panic and make test a test which will not result in panic ?
> + # If all work well restore hardware queue to default
> + file=/sys/module/nvme/parameters/write_queues
> + echo "$old_write_queues" > "$file"
> +
> + # reset controller
> + file="$sys_dev/reset_controller"
> + echo 1 > "$file"
> +
> + # read data from device (may kernel panic)
> + dd if="${TEST_DEV}" of=/dev/null bs=4096 count=1 iflag=direct status=none
> + dd if=/dev/zero of="${TEST_DEV}" bs=4096 count=1 oflag=direct status=none
Just a write a helper for dd command instead of hard-coding it.
> +
> + echo "Test complete"
> +}
> diff --git a/tests/nvme/033.out b/tests/nvme/033.out
> new file mode 100644
> index 0000000..9648c73
> --- /dev/null
> +++ b/tests/nvme/033.out
> @@ -0,0 +1,2 @@
> +Running nvme/033
> +Test complete
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH blktest] nvme/033: add test case for nvme update hardware queue count
2020-04-07 15:29 ` Chaitanya Kulkarni
@ 2020-04-08 12:19 ` Weiping Zhang
2020-04-08 16:27 ` Chaitanya Kulkarni
0 siblings, 1 reply; 6+ messages in thread
From: Weiping Zhang @ 2020-04-08 12:19 UTC (permalink / raw)
To: Chaitanya Kulkarni; +Cc: Weiping Zhang, osandov, linux-block
Chaitanya Kulkarni <Chaitanya.Kulkarni@wdc.com> 于2020年4月7日周二 下午11:32写道：
>
Hi Chaitanya,
Thanks your review
> > + # backup old module parameter: write_queues
> > + file=/sys/module/nvme/parameters/write_queues
> > + if [[ ! -e "$file" ]]; then
> > + echo "$file does not exist"
> > + return 1
> > + fi
> can we skip the test ? I think Omar added a feature to skip the test.
What feature can be used here, I don't see any rc file "set -e".
> > + old_write_queues="$(cat $file)"
> > +
> > + # get current hardware queue count
> > + file="$sys_dev/queue_count"
> > + if [[ ! -e "$file" ]]; then
> > + echo "$file does not exist"
> > + return 1
> > + fi
> Same here.
> > + cur_hw_io_queues="$(cat "$file")"
> > + # minus admin queue
> > + cur_hw_io_queues=$((cur_hw_io_queues - 1))
> > +
> > + # set write queues count to increase more hardware queues
> > + file=/sys/module/nvme/parameters/write_queues
> > + echo "$cur_hw_io_queues" > "$file"
> > +
> Shouldn't we check if new queue count is set here by reading
> write_queues ?
Most of time, this parameter will not equal to io queue_count,
if really so, it will makes this test case be more complicated.
> > + # reset controller, make it effective
> > + file="$sys_dev/reset_controller"
> > + if [[ ! -e "$file" ]]; then
> > + echo "$file does not exist"
> > + return 1
> > + fi
> I think we need to add a helper to verify all the files and skip the
> test required file doesn't exists. Also, please use different variables
> representing different files.
The reason why use same variable name $file, is that copy and paste
code(check file exist or not).
If common/rc include "set -e", all these checks can be removed.
> > + echo 1 > "$file"
> > +
> > + # wait nvme reinitialized
> > + for ((m = 0; m < 10; m++)); do
> > + if [[ -b "${TEST_DEV}" ]]; then
> > + break
> > + fi
> > + sleep 0.5
> > + done
> > + if (( m > 9 )); then
> > + echo "nvme still not reinitialized after 5 seconds!"
> > + return 1
> > + fi
> > +
> > + # read data from device (may kernel panic)
> > + dd if="${TEST_DEV}" of=/dev/null bs=4096 count=1 status=none
> > +
> This should not lead to the kernel panic. Do you have a patch to fix
> the panic ? If not can you please provide information so that we can
> fix the panic and make test a test which will not result in panic ?
>
The patch is under the review, for more detail please visit:
https://patchwork.kernel.org/patch/11476409/
> > + # If all work well restore hardware queue to default
> > + file=/sys/module/nvme/parameters/write_queues
> > + echo "$old_write_queues" > "$file"
> > +
> > + # reset controller
> > + file="$sys_dev/reset_controller"
> > + echo 1 > "$file"
> > +
> > + # read data from device (may kernel panic)
> > + dd if="${TEST_DEV}" of=/dev/null bs=4096 count=1 iflag=direct status=none
> > + dd if=/dev/zero of="${TEST_DEV}" bs=4096 count=1 oflag=direct status=none
> Just a write a helper for dd command instead of hard-coding it.
I think dd is simple enough.
> > +
> > + echo "Test complete"
> > +}
> > diff --git a/tests/nvme/033.out b/tests/nvme/033.out
> > new file mode 100644
> > index 0000000..9648c73
> > --- /dev/null
> > +++ b/tests/nvme/033.out
> > @@ -0,0 +1,2 @@
> > +Running nvme/033
> > +Test complete
> >
>
Thanks
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH blktest] nvme/033: add test case for nvme update hardware queue count
2020-04-08 12:19 ` Weiping Zhang
@ 2020-04-08 16:27 ` Chaitanya Kulkarni
2020-04-08 21:42 ` Omar Sandoval
0 siblings, 1 reply; 6+ messages in thread
From: Chaitanya Kulkarni @ 2020-04-08 16:27 UTC (permalink / raw)
To: Weiping Zhang, osandov; +Cc: Weiping Zhang, linux-block
On 04/08/2020 05:19 AM, Weiping Zhang wrote:
> Chaitanya Kulkarni <Chaitanya.Kulkarni@wdc.com> 于2020年4月7日周二 下午11:32写道：
>>
> Hi Chaitanya,
> Thanks your review
>
>>> + # backup old module parameter: write_queues
>>> + file=/sys/module/nvme/parameters/write_queues
>>> + if [[ ! -e "$file" ]]; then
>>> + echo "$file does not exist"
>>> + return 1
>>> + fi
>> can we skip the test ? I think Omar added a feature to skip the test.
>
Please have a look this discussion :-
https://www.spinics.net/lists/linux-block/msg50491.html
> What feature can be used here, I don't see any rc file "set -e".
>>> + old_write_queues="$(cat $file)"
>>> +
>>> + # get current hardware queue count
>>> + file="$sys_dev/queue_count"
>>> + if [[ ! -e "$file" ]]; then
>>> + echo "$file does not exist"
>>> + return 1
>>> + fi
>> Same here.
>>> + cur_hw_io_queues="$(cat "$file")"
>>> + # minus admin queue
>>> + cur_hw_io_queues=$((cur_hw_io_queues - 1))
>>> +
>>> + # set write queues count to increase more hardware queues
>>> + file=/sys/module/nvme/parameters/write_queues
>>> + echo "$cur_hw_io_queues" > "$file"
>>> +
>> Shouldn't we check if new queue count is set here by reading
>> write_queues ?
> Most of time, this parameter will not equal to io queue_count,
> if really so, it will makes this test case be more complicated.
>
>>> + # reset controller, make it effective
>>> + file="$sys_dev/reset_controller"
>>> + if [[ ! -e "$file" ]]; then
>>> + echo "$file does not exist"
>>> + return 1
>>> + fi
>> I think we need to add a helper to verify all the files and skip the
>> test required file doesn't exists. Also, please use different variables
>> representing different files.
> The reason why use same variable name $file, is that copy and paste
> code(check file exist or not).
>
> If common/rc include "set -e", all these checks can be removed.
>
>>> + echo 1 > "$file"
>>> +
>>> + # wait nvme reinitialized
>>> + for ((m = 0; m < 10; m++)); do
>>> + if [[ -b "${TEST_DEV}" ]]; then
>>> + break
>>> + fi
>>> + sleep 0.5
>>> + done
>>> + if (( m > 9 )); then
>>> + echo "nvme still not reinitialized after 5 seconds!"
>>> + return 1
>>> + fi
>>> +
>>> + # read data from device (may kernel panic)
>>> + dd if="${TEST_DEV}" of=/dev/null bs=4096 count=1 status=none
>>> +
>> This should not lead to the kernel panic. Do you have a patch to fix
>> the panic ? If not can you please provide information so that we can
>> fix the panic and make test a test which will not result in panic ?
>>
> The patch is under the review, for more detail please visit:
> https://patchwork.kernel.org/patch/11476409/
We need to wait for the patch to get in first before we add test with
fixes tag. I'm not sure is it a good idea to have a test which results
in panic when patch in discussion is not in the tree, in that case are
testing the known failure.
We need to add a test to validate and pass the fix and make sure as
development goes on fixed code is stable.
Tests in blktests should always pass based on the feature or a fix,
unless further development adds a regression or has an indirect effect.
Omar any thoughts ?
>>> + # If all work well restore hardware queue to default
>>> + file=/sys/module/nvme/parameters/write_queues
>>> + echo "$old_write_queues" > "$file"
>>> +
>>> + # reset controller
>>> + file="$sys_dev/reset_controller"
>>> + echo 1 > "$file"
>>> +
>>> + # read data from device (may kernel panic)
>>> + dd if="${TEST_DEV}" of=/dev/null bs=4096 count=1 iflag=direct status=none
>>> + dd if=/dev/zero of="${TEST_DEV}" bs=4096 count=1 oflag=direct status=none
>> Just a write a helper for dd command instead of hard-coding it.
> I think dd is simple enough.
Fine.
>>> +
>>> + echo "Test complete"
>>> +}
>>> diff --git a/tests/nvme/033.out b/tests/nvme/033.out
>>> new file mode 100644
>>> index 0000000..9648c73
>>> --- /dev/null
>>> +++ b/tests/nvme/033.out
>>> @@ -0,0 +1,2 @@
>>> +Running nvme/033
>>> +Test complete
>>>
>>
> Thanks
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH blktest] nvme/033: add test case for nvme update hardware queue count
2020-04-07 14:16 [PATCH blktest] nvme/033: add test case for nvme update hardware queue count Weiping Zhang
2020-04-07 15:29 ` Chaitanya Kulkarni
@ 2020-04-08 21:40 ` Omar Sandoval
1 sibling, 0 replies; 6+ messages in thread
From: Omar Sandoval @ 2020-04-08 21:40 UTC (permalink / raw)
To: linux-block
On Tue, Apr 07, 2020 at 10:16:33PM +0800, Weiping Zhang wrote:
> Modify nvme module parameter write_queues to change hardware
> queue count, then reset nvme controller to reinitialize nvme
> with different queue count.
>
> Attention, this test case may trigger a kernel panic.
>
> Signed-off-by: Weiping Zhang <zhangweiping@didiglobal.com>
> ---
> tests/nvme/033 | 87 ++++++++++++++++++++++++++++++++++++++++++++++
> tests/nvme/033.out | 2 ++
> 2 files changed, 89 insertions(+)
> create mode 100755 tests/nvme/033
> create mode 100644 tests/nvme/033.out
>
> diff --git a/tests/nvme/033 b/tests/nvme/033
> new file mode 100755
> index 0000000..e3b9211
> --- /dev/null
> +++ b/tests/nvme/033
> @@ -0,0 +1,87 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-3.0+
> +# Copyright (C) 2020 Weiping Zhang <zwp10758@gmail.com>
> +#
> +# Test nvme update hardware queue count larger than system cpu count
> +#
> +
> +. tests/nvme/rc
> +
> +DESCRIPTION="test nvme update hardware queue count larger than system cpu count"
> +QUICK=1
> +
> +requires() {
> + _have_program dd
> +}
> +
> +device_requires() {
> + _test_dev_is_nvme
> +}
> +
> +test_device() {
> + echo "Running ${TEST_NAME}"
> +
> + local old_write_queues
> + local cur_hw_io_queues
> + local file
> + local sys_dev=$TEST_DEV_SYSFS/device
> +
> + # backup old module parameter: write_queues
> + file=/sys/module/nvme/parameters/write_queues
> + if [[ ! -e "$file" ]]; then
> + echo "$file does not exist"
> + return 1
> + fi
Tests shouldn't fail if kernel support is missing. You can check this
with `_have_module_param nvme write_queues` in requires().
> + old_write_queues="$(cat $file)"
> +
> + # get current hardware queue count
> + file="$sys_dev/queue_count"
> + if [[ ! -e "$file" ]]; then
> + echo "$file does not exist"
> + return 1
> + fi
Again, this should be checked before the test is run (device_requires(),
in this case). Something like
if [[ ! -e "$TEST_DEV_SYSFS/device/queue_count" ]]; then
SKIP_REASON="device does have queue_count sysfs attribute"
return
fi
> + cur_hw_io_queues="$(cat "$file")"
> + # minus admin queue
> + cur_hw_io_queues=$((cur_hw_io_queues - 1))
> +
> + # set write queues count to increase more hardware queues
> + file=/sys/module/nvme/parameters/write_queues
> + echo "$cur_hw_io_queues" > "$file"
> +
> + # reset controller, make it effective
> + file="$sys_dev/reset_controller"
> + if [[ ! -e "$file" ]]; then
> + echo "$file does not exist"
> + return 1
> + fi
Same thing here.
> + echo 1 > "$file"
> +
> + # wait nvme reinitialized
> + for ((m = 0; m < 10; m++)); do
> + if [[ -b "${TEST_DEV}" ]]; then
> + break
> + fi
> + sleep 0.5
> + done
> + if (( m > 9 )); then
> + echo "nvme still not reinitialized after 5 seconds!"
> + return 1
You're not resetting the write_queues parameter here.
> + fi
> +
> + # read data from device (may kernel panic)
> + dd if="${TEST_DEV}" of=/dev/null bs=4096 count=1 status=none
> +
> + # If all work well restore hardware queue to default
> + file=/sys/module/nvme/parameters/write_queues
> + echo "$old_write_queues" > "$file"
> +
> + # reset controller
> + file="$sys_dev/reset_controller"
> + echo 1 > "$file"
Does this need the same logic to wait for the device to reappear?
> + # read data from device (may kernel panic)
> + dd if="${TEST_DEV}" of=/dev/null bs=4096 count=1 iflag=direct status=none
> + dd if=/dev/zero of="${TEST_DEV}" bs=4096 count=1 oflag=direct status=none
> +
> + echo "Test complete"
> +}
> diff --git a/tests/nvme/033.out b/tests/nvme/033.out
> new file mode 100644
> index 0000000..9648c73
> --- /dev/null
> +++ b/tests/nvme/033.out
> @@ -0,0 +1,2 @@
> +Running nvme/033
> +Test complete
> --
> 2.18.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH blktest] nvme/033: add test case for nvme update hardware queue count
2020-04-08 16:27 ` Chaitanya Kulkarni
@ 2020-04-08 21:42 ` Omar Sandoval
0 siblings, 0 replies; 6+ messages in thread
From: Omar Sandoval @ 2020-04-08 21:42 UTC (permalink / raw)
To: Chaitanya Kulkarni; +Cc: Weiping Zhang, Weiping Zhang, linux-block
On Wed, Apr 08, 2020 at 04:27:58PM +0000, Chaitanya Kulkarni wrote:
> On 04/08/2020 05:19 AM, Weiping Zhang wrote:
> > Chaitanya Kulkarni <Chaitanya.Kulkarni@wdc.com> 于2020年4月7日周二 下午11:32写道：
> >>
> > Hi Chaitanya,
> > Thanks your review
> >
> >>> + # backup old module parameter: write_queues
> >>> + file=/sys/module/nvme/parameters/write_queues
> >>> + if [[ ! -e "$file" ]]; then
> >>> + echo "$file does not exist"
> >>> + return 1
> >>> + fi
> >> can we skip the test ? I think Omar added a feature to skip the test.
> >
> Please have a look this discussion :-
> https://www.spinics.net/lists/linux-block/msg50491.html
> > What feature can be used here, I don't see any rc file "set -e".
> >>> + old_write_queues="$(cat $file)"
> >>> +
> >>> + # get current hardware queue count
> >>> + file="$sys_dev/queue_count"
> >>> + if [[ ! -e "$file" ]]; then
> >>> + echo "$file does not exist"
> >>> + return 1
> >>> + fi
> >> Same here.
> >>> + cur_hw_io_queues="$(cat "$file")"
> >>> + # minus admin queue
> >>> + cur_hw_io_queues=$((cur_hw_io_queues - 1))
> >>> +
> >>> + # set write queues count to increase more hardware queues
> >>> + file=/sys/module/nvme/parameters/write_queues
> >>> + echo "$cur_hw_io_queues" > "$file"
> >>> +
> >> Shouldn't we check if new queue count is set here by reading
> >> write_queues ?
> > Most of time, this parameter will not equal to io queue_count,
> > if really so, it will makes this test case be more complicated.
> >
> >>> + # reset controller, make it effective
> >>> + file="$sys_dev/reset_controller"
> >>> + if [[ ! -e "$file" ]]; then
> >>> + echo "$file does not exist"
> >>> + return 1
> >>> + fi
> >> I think we need to add a helper to verify all the files and skip the
> >> test required file doesn't exists. Also, please use different variables
> >> representing different files.
> > The reason why use same variable name $file, is that copy and paste
> > code(check file exist or not).
> >
> > If common/rc include "set -e", all these checks can be removed.
> >
> >>> + echo 1 > "$file"
> >>> +
> >>> + # wait nvme reinitialized
> >>> + for ((m = 0; m < 10; m++)); do
> >>> + if [[ -b "${TEST_DEV}" ]]; then
> >>> + break
> >>> + fi
> >>> + sleep 0.5
> >>> + done
> >>> + if (( m > 9 )); then
> >>> + echo "nvme still not reinitialized after 5 seconds!"
> >>> + return 1
> >>> + fi
> >>> +
> >>> + # read data from device (may kernel panic)
> >>> + dd if="${TEST_DEV}" of=/dev/null bs=4096 count=1 status=none
> >>> +
> >> This should not lead to the kernel panic. Do you have a patch to fix
> >> the panic ? If not can you please provide information so that we can
> >> fix the panic and make test a test which will not result in panic ?
> >>
> > The patch is under the review, for more detail please visit:
> > https://patchwork.kernel.org/patch/11476409/
>
> We need to wait for the patch to get in first before we add test with
> fixes tag. I'm not sure is it a good idea to have a test which results
> in panic when patch in discussion is not in the tree, in that case are
> testing the known failure.
>
> We need to add a test to validate and pass the fix and make sure as
> development goes on fixed code is stable.
>
> Tests in blktests should always pass based on the feature or a fix,
> unless further development adds a regression or has an indirect effect.
>
> Omar any thoughts ?
Yeah, it's probably a good idea to wait to merge this until the commit
has hit mainline.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-04-08 21:42 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-07 14:16 [PATCH blktest] nvme/033: add test case for nvme update hardware queue count Weiping Zhang
2020-04-07 15:29 ` Chaitanya Kulkarni
2020-04-08 12:19 ` Weiping Zhang
2020-04-08 16:27 ` Chaitanya Kulkarni
2020-04-08 21:42 ` Omar Sandoval
2020-04-08 21:40 ` Omar Sandoval
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.