On 04/08/2020 05:19 AM, Weiping Zhang wrote: > 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. > 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 >
