On 6/18/24 14:58, Shinichiro Kawasaki wrote: > On Jun 18, 2024 / 01:35, Chaitanya Kulkarni wrote: >> On 6/17/24 02:17, Nilay Shroff wrote: >> >> I think subject line should start with nvme ? >> >> nvme: add test for creating/deleting file-ns >> >>> This is regression test for commit be647e2c76b2 >>> (nvme: use srcu for iterating namespace list) >>> >>> This test uses a regulare file backed loop device >>> for creating and then deleting an NVMe namespace >>> in a loop. >> >> >> s/regulare/regular/ ? >> >> nit:- commit log looks a bit short :- >> >> This is regression test for commit be647e2c76b2 >> (nvme: use srcu for iterating namespace list) >> >> This test uses a regulare file backed loop device for creating and >> then deleting an NVMe namespace in a loop. >> >>> Signed-off-by: Nilay Shroff <nilay@xxxxxxxxxxxxx> >>> --- >>> This regression was first reported[1], and now it's >>> fixed in 6.10-rc4[2] >>> >>> [1] https://lore.kernel.org/all/2312e6c3-a069-4388-a863-df7e261b9d70@xxxxxxxxxxxxxxxxxx/ >>> [2] commit ff0ffe5b7c3c (nvme: fix namespace removal list) >> >> it will be helpful in long run to add above information >> into the commit log, Shinichiro any thoughts ? > > Agreed. It is helpful to record the kernel side fix commit and the link to the > discussion threads in the blktests side commit log. Yeah I am going to updated the commit message with relevant information as suggested in the next patch. > >> >>> --- >>> tests/nvme/051 | 65 ++++++++++++++++++++++++++++++++++++++++++++++ >>> tests/nvme/051.out | 2 ++ >>> 2 files changed, 67 insertions(+) >>> create mode 100755 tests/nvme/051 >>> create mode 100644 tests/nvme/051.out >>> >>> diff --git a/tests/nvme/051 b/tests/nvme/051 >>> new file mode 100755 >>> index 0000000..0de5c56 >>> --- /dev/null >>> +++ b/tests/nvme/051 >>> @@ -0,0 +1,65 @@ >>> +#!/bin/bash >>> +# SPDX-License-Identifier: GPL-3.0+ >>> +# Copyright (C) 2024 Nilay Shroff(nilay@xxxxxxxxxxxxx) >> >> not sure we need to have email address here as it's a part of >> commit log anyways ... >> >>> +# >>> +# Regression test for commit be647e2c76b2(nvme: use srcu for iterating >>> +# namespace list) > > It is also good to enrich this header comment section. Especially, the kernel > side fix commit will be helpful. > >>> + >>> +. tests/nvme/rc >>> + >>> +DESCRIPTION="Test file-ns creation/deletion under one subsystem" >>> + >>> +requires() { >>> + _nvme_requires >>> + _have_loop >>> + _require_nvme_trtype_is_loop >>> +} >>> + >>> +set_conditions() { >>> + _set_nvme_trtype "$@" >>> +} >>> + >>> +test() { >>> + echo "Running ${TEST_NAME}" >>> + >>> + _setup_nvmet >>> + >>> + local subsys="blktests-subsystem-1" >>> + local iterations="${NVME_NUM_ITER}" >> >> no need for above var, I think direct use of NVME_NUM_ITER is >> clear here ... > > I ran this test case on my baremetal test node and QEMU test node using the > kernel without the fix. On the baremetal test node, the kernel Oops was > created soon. Good. On the QEMU test node, the Oops was not recreated, and > the test case passed. For this pass case, it took more than 15 minutes to > complete the test case. I think the default NVME_NUM_ITER=1000 is too much. > Can we reduce it to 10 or 20? > I also tested this case on my baremetal machine and I could recreate this crash on the first iteration. However I didn't test it on QEMU. But agrees the default iteration value of 1000 is too big and I think it's reasonable to make it 20. I will change it in the next patch. >> >>> + local loop_dev >>> + local port >>> + >>> + truncate -s "${NVME_IMG_SIZE}" "$(_nvme_def_file_path)" >>> + >>> + loop_dev="$(losetup -f --show "$(_nvme_def_file_path)")" >>> + >>> + port="$(_create_nvmet_port "${nvme_trtype}")" >>> + >>> + _nvmet_target_setup --subsysnqn "${subsys}" --blkdev "${loop_dev}" >>> + >>> + _nvme_connect_subsys --subsysnqn "${subsys}" >>> + >>> + for ((i = 2; i <= iterations; i++)); do { >> >> small comment would be useful to explain why are starting at 2 ... >> >>> + truncate -s "${NVME_IMG_SIZE}" "$(_nvme_def_file_path).$i" >>> + _create_nvmet_ns "${subsys}" "${i}" "$(_nvme_def_file_path).$i" >>> + >>> + # allow async request to be processed >>> + sleep 1 >>> + >>> + _remove_nvmet_ns "${subsys}" "${i}" >>> + rm "$(_nvme_def_file_path).$i" >>> + } >>> + done >>> + >>> + _nvme_disconnect_subsys --subsysnqn "${subsys}" >> "${FULL}" 2>&1 >>> + >>> + _nvmet_target_cleanup --subsysnqn "${subsys}" --blkdev "${loop_dev}" >>> + >>> + _remove_nvmet_port "${port}" >>> + >>> + losetup -d "$loop_dev" >>> + >>> + rm "$(_nvme_def_file_path)" >>> + >>> + echo "Test complete" >>> +} >>> diff --git a/tests/nvme/051.out b/tests/nvme/051.out >>> new file mode 100644 >>> index 0000000..156f068 >>> --- /dev/null >>> +++ b/tests/nvme/051.out >>> @@ -0,0 +1,2 @@ >>> +Running nvme/051 >>> +Test complete >> >> thanks for the test, I think this is much needed test especially with >> recent reported issues ... > > I also appreciate this patch. Thanks! > > One more request, recent commit added a test case to the nvme group and it has > the number nvme/051. Could you renumber this test case to nvme/052? Yes I will rebase my tree to the latest and update the test case to nvme/052. And thank you for testing my test case on your machine and your review comments! Much appreciated.... I will soon post the patch v2. Thanks, --Nilay
