[PATCH] nvme: add nvmf reset/disconnect during traffic test

[Shinichiro Kawasaki]

On Jun 29, 2022 / 11:54, Sagi Grimberg wrote:
> Test traffic controller reset and disconnect during traffic.

Hi Sagi, thanks for the patch. This test case looks valuable. Is there any
specific kernel bug or fix which motivated to add this test case? If so, it
would be useful to note it in this commit message and the header comment of
the test case.

> 
> Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
> ---
>  tests/nvme/040     | 55 ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/nvme/040.out |  6 +++++
>  tests/nvme/rc      |  8 +++++++
>  3 files changed, 69 insertions(+)
>  create mode 100755 tests/nvme/040
>  create mode 100644 tests/nvme/040.out
> 
> diff --git a/tests/nvme/040 b/tests/nvme/040
> new file mode 100755
> index 000000000000..88d255a8d0e0
> --- /dev/null
> +++ b/tests/nvme/040
> @@ -0,0 +1,55 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-3.0+
> +# Copyright (C) 2019 Yi Zhang <yi.zhang at redhat.com>

Was this authored by Yi in 2019?

> +#
> +# Test nvme fabrics controller reset/disconnect/reconnect operation during I/O
> +# This test is somewhat similar to test 032 but for fabrics controllers.

Ah, the copyright was copied from nvme/032, probably.

> +
> +. tests/nvme/rc
> +
> +DESCRIPTION="test nvme fabrics controller reset/disconnect operation during I/O"
> +
> +requires() {
> +	_nvme_requires
> +	_have_loop
> +	_have_fio
> +	_require_nvme_trtype_is_fabrics
> +}
> +
> +test() {
> +	local subsys="blktests-subsystem-"

As for the sub-system name, other test cases have a number after
"blktests-subsystem-". "blktests-subsystem-1" would be the better for this test
case also, just for consistency.

> +	local port
> +	local loop_dev
> +	local nvmedev
> +
> +	echo "Running ${TEST_NAME}"
> +
> +	_setup_nvmet
> +	truncate -s 1G "$TMPDIR/img"
> +	loop_dev="$(losetup -f --show "$TMPDIR/img")"
> +
> +	port="$(_create_nvmet_port "${nvme_trtype}")"
> +	_create_nvmet_subsystem "${subsys}" "${loop_dev}"
> +	_add_nvmet_subsys_to_port "${port}" "${subsys}"
> +	_nvme_connect_subsys "${nvme_trtype}" "${subsys}"
> +	udevadm settle
> +	nvmedev=$(_find_nvme_dev "${subsys}")
> +
> +	# start fio job
> +	echo "starting background fio on ${nvmedev}n1"
> +	_run_fio_rand_io --filename="${nvmedev}n1" --size=1g \
> +		--group_reporting --ramp_time=5  &> /dev/null &
> +	sleep 5
> +
> +	# do reset/remove operation
> +	echo "resetting ${nvmedev}"
> +	_nvme_reset_ctrl ${nvmedev}
> +	sleep 1
> +	echo "deleting ${nvmedev}"
> +	_nvme_delete_ctrl ${nvmedev}
> +
> +	echo "stopping background fio on ${nvmedev}n1"
> +	{ kill $!; wait; } &> /dev/null

It looks like that clean up helper function calls for the port and the
sybsystem are missing. When I ran this test case, I observed WARNINGS as
follows, which _cleanup_nvmet() in nvme/rc prints out.

WARNING: Test did not clean up port: 0
WARNING: Test did not clean up subsystem: blktests-subsystem-

> +
> +	echo "Test complete"
> +}
> diff --git a/tests/nvme/040.out b/tests/nvme/040.out
> new file mode 100644
> index 000000000000..f14ae8db1ddb
> --- /dev/null
> +++ b/tests/nvme/040.out
> @@ -0,0 +1,6 @@
> +Running nvme/040
> +starting background fio on nvme0n1
> +resetting nvme0
> +deleting nvme0
> +stopping background fio on nvme0n1

The nvme controller name "nvme0" is assumed in the out file. However, when the
test system has nvme devices, the name of the nvme controller created by this
test case is not "nvme0". Then this test case fails. I suggest not to record the
nvme device names in the out file.

> +Test complete
> diff --git a/tests/nvme/rc b/tests/nvme/rc
> index 4bebbc762cbb..684d56b184e5 100644
> --- a/tests/nvme/rc
> +++ b/tests/nvme/rc
> @@ -447,3 +447,11 @@ _nvme_disable_err_inject()
>          echo 0 > /sys/kernel/debug/"$1"/fault_inject/probability
>          echo 0 > /sys/kernel/debug/"$1"/fault_inject/times
>  }
> +
> +_nvme_reset_ctrl() {
> +	echo 1 > /sys/class/nvme/$1/reset_controller
> +}
> +
> +_nvme_delete_ctrl() {
> +	echo 1 > /sys/class/nvme/$1/delete_controller
> +}
> -- 
> 2.34.1
> 

And shellcheck complains that some variable references lack double quotes.

$ make check
shellcheck -x -e SC2119 -f gcc check new common/* \
        tests/*/rc tests/*/[0-9]*[0-9]
tests/nvme/rc:452:27: note: Double quote to prevent globbing and word splitting. [SC2086]
tests/nvme/rc:456:27: note: Double quote to prevent globbing and word splitting. [SC2086]
tests/nvme/040:46:19: note: Double quote to prevent globbing and word splitting. [SC2086]
tests/nvme/040:49:20: note: Double quote to prevent globbing and word splitting. [SC2086]
make: *** [Makefile:21: check] Error 1

-- 
Shin'ichiro Kawasaki