Chaitanya Kulkarni <Chaitanya.Kulkarni@xxxxxxx> 于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
