[PATCH v2 3/3] nvme: add test for controller rescan under I/O load

[Shinichiro Kawasaki]

On Aug 23, 2024 / 22:08, Martin Wilck wrote:
> Add a test that repeatedly rescans nvme controllers while doing IO
> on an nvme namespace connected to these controllers. The purpose
> of the test is to make sure that no I/O errors or data corruption
> occurs because of the rescan operations. The test uses sub-second
> sleeps, which can't be easily accomplished in bash because of
> missing floating-point arithmetic (and because usleep(1) isn't
> portable). Therefore an awk program is used to trigger the
> device rescans.

Hello Martin, thanks for the series. I think this test case is good since it
looks extending the code coverage. As for the rationale discussion for the v1
series, I suggest to add a "Link:" tag here:

Link: https://lore.kernel.org/linux-nvme/20240822201413.112268-1-mwilck@suse.com/

> 
> Signed-off-by: Martin Wilck <mwilck at suse.com>
> ---
> v2: - don't use usleep (Nilay Shroff). Use an awk program to do floating
>       point arithmetic and achieve more accurate sub-second sleep times.
>     - add 053.out (Nilay Shroff).
> ---
>  tests/nvme/053     | 70 ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/nvme/053.out |  2 ++
>  tests/nvme/rc      | 18 ++++++++++++
>  3 files changed, 90 insertions(+)
>  create mode 100755 tests/nvme/053
>  create mode 100644 tests/nvme/053.out
> 
> diff --git a/tests/nvme/053 b/tests/nvme/053
> new file mode 100755
> index 0000000..d32484c
> --- /dev/null
> +++ b/tests/nvme/053
> @@ -0,0 +1,70 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-3.0+
> +# Copyright (C) 2024 Martin Wilck, SUSE LLC

I suggest to describe the test content here, something like,

#
# Repeatedly rescans nvme controllers while doing IO on an nvme namespace
# connected to these controllers, and make sure that no I/O errors or data
# corruption occurs.

> +
> +. tests/nvme/rc
> +
> +DESCRIPTION="test controller rescan under I/O load"
> +TIMED=1
> +: "${TIMEOUT:=60}"
> +
> +rescan_controller() {
> +	local path
> +	path="$1/rescan_controller"
> +
> +	[[ -f "$path" ]] || {
> +		echo "cannot rescan $1"
> +		return 1
> +	}
> +
> +	awk -f "$TMPDIR/rescan.awk" \
> +	    -v path="$path" -v timeout="$TIMEOUT" -v seed="$2" &
> +}
> +
> +create_rescan_script() {
> +	cat >"$TMPDIR/rescan.awk" <<EOF
> + at load "time"
> +
> +BEGIN {
> +    srand(seed);
> +    finish = gettimeofday() + strtonum(timeout);
> +    while (gettimeofday() < finish) {
> +	sleep(0.1 + 5 * rand());
> +	printf("1\n") > path;
> +	close(path);
> +    }
> +}
> +EOF
> +}
> +
> +test_device() {
> +	local -a ctrls
> +	local i

Nit: 'st' can be decalread as the local variable too. I suggest to add another
new local variable 'line' below, so I suggset "local i line st" here.

> +
> +	echo "Running ${TEST_NAME}"
> +	create_rescan_script
> +
> +	ctrls=($(_nvme_get_ctrl_list))

Shellcheck reports a warning here:

  tests/nvme/053:47:9: warning: Prefer mapfile or read -a to split command output (or quote to avoid splitting). [SC2207]

It is a bit lengthy, but let's replace the line above with this:

        while read -r line ; do ctrls+=("$line"); done < <(_nvme_get_ctrl_list)