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@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 ? Yeah, it's probably a good idea to wait to merge this until the commit has hit mainline.
