[PATCH blktests] loop: add test for creating/deleting file-ns

[Shinichiro Kawasaki]

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 at linux.ibm.com>
> > ---
> > 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@linux.vnet.ibm.com/
> > [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.

> 
> > ---
> >   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 at linux.ibm.com)
> 
> 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?

> 
> > +	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?