Re: [PATCH blktest] nvme/033: add test case for nvme update hardware queue count

[Omar Sandoval <osandov@osandov.com>]

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.