[PATCH v2 1/3] blktests: nvme: skip passthru tests on multipath devices

[PATCH v2 1/3] blktests: nvme: skip passthru tests on multipath devices

[Martin Wilck]

NVMe multipath devices have no associated character device that
can be used for NVMe passtrhu. Skip them.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
v2: used more expressive function name for non-multipath test (Daniel Wagner)

---
 tests/nvme/033 | 4 ++++
 tests/nvme/034 | 4 ++++
 tests/nvme/035 | 1 +
 tests/nvme/036 | 4 ++++
 tests/nvme/037 | 4 ++++
 tests/nvme/039 | 4 ++++
 tests/nvme/rc  | 8 ++++++++
 7 files changed, 29 insertions(+)

diff --git a/tests/nvme/033 b/tests/nvme/033
index 7a69b94..5e05175 100755
--- a/tests/nvme/033
+++ b/tests/nvme/033
@@ -13,6 +13,10 @@ requires() {
 	_have_kernel_option NVME_TARGET_PASSTHRU
 }
 
+device_requires() {
+	_require_test_dev_is_not_nvme_multipath
+}
+
 set_conditions() {
 	_set_nvme_trtype "$@"
 }
diff --git a/tests/nvme/034 b/tests/nvme/034
index 239757c..154fc91 100755
--- a/tests/nvme/034
+++ b/tests/nvme/034
@@ -14,6 +14,10 @@ requires() {
 	_have_fio
 }
 
+device_requires() {
+	_require_test_dev_is_not_nvme_multipath
+}
+
 set_conditions() {
 	_set_nvme_trtype "$@"
 }
diff --git a/tests/nvme/035 b/tests/nvme/035
index 8286178..ff217d6 100755
--- a/tests/nvme/035
+++ b/tests/nvme/035
@@ -17,6 +17,7 @@ requires() {
 }
 
 device_requires() {
+	_require_test_dev_is_not_nvme_multipath
 	_require_test_dev_size "${NVME_IMG_SIZE}"
 }
 
diff --git a/tests/nvme/036 b/tests/nvme/036
index ef6c29d..442ffe7 100755
--- a/tests/nvme/036
+++ b/tests/nvme/036
@@ -13,6 +13,10 @@ requires() {
 	_have_kernel_option NVME_TARGET_PASSTHRU
 }
 
+device_requires() {
+	_require_test_dev_is_not_nvme_multipath
+}
+
 set_conditions() {
 	_set_nvme_trtype "$@"
 }
diff --git a/tests/nvme/037 b/tests/nvme/037
index ef7ac59..f7ddc2d 100755
--- a/tests/nvme/037
+++ b/tests/nvme/037
@@ -12,6 +12,10 @@ requires() {
 	_have_kernel_option NVME_TARGET_PASSTHRU
 }
 
+device_requires() {
+	_require_test_dev_is_not_nvme_multipath
+}
+
 set_conditions() {
 	_set_nvme_trtype "$@"
 }
diff --git a/tests/nvme/039 b/tests/nvme/039
index a0f135c..e8020a7 100755
--- a/tests/nvme/039
+++ b/tests/nvme/039
@@ -18,6 +18,10 @@ requires() {
 	    _have_kernel_option FAULT_INJECTION_DEBUG_FS
 }
 
+device_requires() {
+	_require_test_dev_is_not_nvme_multipath
+}
+
 # Get the last dmesg lines as many as specified. Exclude the lines to indicate
 # suppression by rate limit.
 last_dmesg()
diff --git a/tests/nvme/rc b/tests/nvme/rc
index dedc412..5c554b6 100644
--- a/tests/nvme/rc
+++ b/tests/nvme/rc
@@ -130,6 +130,14 @@ _require_test_dev_is_nvme() {
 	return 0
 }
 
+_require_test_dev_is_not_nvme_multipath() {
+	if [[ "$(readlink -f "$TEST_DEV_SYSFS/device")" =~ /nvme-subsystem/ ]]; then
+		SKIP_REASONS+=("$TEST_DEV is a NVMe multipath device")
+		return 1
+	fi
+	return 0
+}
+
 _require_nvme_test_img_size() {
 	local require_sz_mb
 	local nvme_img_size_mb
-- 
2.46.0

[PATCH v2 2/3] blktests: nvme/032: skip on non-PCI devices

[Martin Wilck]

nvme/032 is a PCI-specific test.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 tests/nvme/032 | 2 +-
 tests/nvme/rc  | 8 ++++++++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/tests/nvme/032 b/tests/nvme/032
index 5353e96..512d7ea 100755
--- a/tests/nvme/032
+++ b/tests/nvme/032
@@ -24,7 +24,7 @@ requires() {
 }
 
 device_requires() {
-	_require_test_dev_is_nvme
+	_require_test_dev_is_nvme_pci
 }
 
 test_device() {
diff --git a/tests/nvme/rc b/tests/nvme/rc
index 5c554b6..b702a57 100644
--- a/tests/nvme/rc
+++ b/tests/nvme/rc
@@ -130,6 +130,14 @@ _require_test_dev_is_nvme() {
 	return 0
 }
 
+_require_test_dev_is_nvme_pci() {
+	if [[ ! "$(readlink -f "$TEST_DEV_SYSFS/device")" =~ devices/pci ]]; then
+		SKIP_REASONS+=("$TEST_DEV is not a PCI NVMe device")
+		return 1
+	fi
+	return 0
+}
+
 _require_test_dev_is_not_nvme_multipath() {
 	if [[ "$(readlink -f "$TEST_DEV_SYSFS/device")" =~ /nvme-subsystem/ ]]; then
 		SKIP_REASONS+=("$TEST_DEV is a NVMe multipath device")
-- 
2.46.0

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

[Martin Wilck]

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.

Signed-off-by: Martin Wilck <mwilck@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
+
+. 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
+@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
+
+	echo "Running ${TEST_NAME}"
+	create_rescan_script
+
+	ctrls=($(_nvme_get_ctrl_list))
+	_run_fio_verify_io --filename="$TEST_DEV" --time_based &> "$FULL" &
+
+	for i in "${!ctrls[@]}"; do
+		rescan_controller "${ctrls[$i]}" "$i"
+	done
+
+	while true; do
+		wait -n &>/dev/null
+		st=$?
+		case $st in
+			127)
+				break
+				;;
+			0)
+				;;
+			*)
+				echo "child process exited with $st!"
+				;;
+		esac
+	done
+
+	echo "Test complete"
+}
diff --git a/tests/nvme/053.out b/tests/nvme/053.out
new file mode 100644
index 0000000..e8086ce
--- /dev/null
+++ b/tests/nvme/053.out
@@ -0,0 +1,2 @@
+Running nvme/053
+Test complete
diff --git a/tests/nvme/rc b/tests/nvme/rc
index b702a57..a877de3 100644
--- a/tests/nvme/rc
+++ b/tests/nvme/rc
@@ -192,6 +192,24 @@ _test_dev_nvme_nsid() {
 	cat "${TEST_DEV_SYSFS}/nsid"
 }
 
+_nvme_get_ctrl_list() {
+	local subsys
+	local c
+
+	subsys=$(readlink  "${TEST_DEV_SYSFS}/device/subsystem")
+	case $subsys in
+		*/nvme)
+			readlink -f "${TEST_DEV_SYSFS}/device"
+			;;
+		*/nvme-subsystem)
+			for c in "${TEST_DEV_SYSFS}"/device/nvme*; do
+				[[ -L "$c" ]] || continue
+				[[ -f "$c/dev" ]] && readlink -f "$c"
+			done
+			;;
+	esac
+}
+
 _nvme_calc_rand_io_size() {
 	local img_size_mb
 	local io_size_kb
-- 
2.46.0

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

[Nilay Shroff]

On 8/24/24 01:38, 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.
> 
> Signed-off-by: Martin Wilck <mwilck@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
> +
> +. 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
> +@load "time"
> +
> +BEGIN {
> +    srand(seed);
> +    finish = gettimeofday() + strtonum(timeout);
> +    while (gettimeofday() < finish) {
> +	sleep(0.1 + 5 * rand());
> +	printf("1\n") > path;
> +	close(path);
> +    }
> +}
> +EOF
> +}
The "rand()" function in 'awk' returns a floating point value between
0 and 1 (i.e. [0, 1]). So it's possible to have sleep for some cases go
upto ~5.1 seconds. So if the intention is to sleep between 0.1 and 5 
seconds precisely then we may want to use,
 
sleep(0.1 + 4.9 * rand());

However this is not a major problem and we may ignore. 
Otherwise, code looks good to me.
    
Reviewed-by: Nilay Shroff (nilay@linux.ibm.com)

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

[Martin Wilck]

On Mon, 2024-08-26 at 11:07 +0530, Nilay Shroff wrote:
> > 
> The "rand()" function in 'awk' returns a floating point value between
> 0 and 1 (i.e. [0, 1]). So it's possible to have sleep for some cases
> go
> upto ~5.1 seconds. So if the intention is to sleep between 0.1 and 5 
> seconds precisely then we may want to use,
>  
> sleep(0.1 + 4.9 * rand());
> 

Yes, I know. I thought it didn't really matter as the 5s limit was
arbitrary anyway.

> However this is not a major problem and we may ignore. 
> Otherwise, code looks good to me.
>     
> Reviewed-by: Nilay Shroff (nilay@linux.ibm.com)
> 

Thanks!
Martin

Re: [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@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
> +@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)

Re: [PATCH v2 1/3] blktests: nvme: skip passthru tests on multipath devices

[Shinichiro Kawasaki]

A nit comment: "blktests:" prefix in the commit title is not so meaningful (Same
comment for the 2nd patch). Also, it's the better to note the affected test
case. How about this?

    nvme/{033-037,039}: skip passthru tests on multipath devices

Other than that, the 1st and 2nd patches look good to me.

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

[Martin Wilck]

On Thu, 2024-08-29 at 07:36 +0000, Shinichiro Kawasaki wrote:


> 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)

Thanks for the review. If you don't mind, I'll just use "mapfile -t",
as suggested by shellcheck.

Martin

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

[Shinichiro Kawasaki]

On Sep 02, 2024 / 15:53, Martin Wilck wrote:
> On Thu, 2024-08-29 at 07:36 +0000, Shinichiro Kawasaki wrote:
> 
> 
> > 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)
> 
> Thanks for the review. If you don't mind, I'll just use "mapfile -t",
> as suggested by shellcheck.

The reason I did not suggest the "mapfile -t" solution was that SC2207
recommends it for bash versions 4.4+ [1]. On the other hand, blktests README.md
requires bash version >= 4.2. So I assume that some blktests users use bash
version 4.2 or 4.3. I took a glance in the bash change history [2], and there
are some mpafile related bug fixes between bash version v4.2 an v4.4. So I think
it's the better to not use "mpafile -t" in blktests.

Do you have any specific reason to use "mapfile -t"?

[1] https://www.shellcheck.net/wiki/SC2207
[2] https://git.savannah.gnu.org/cgit/bash.git/tree/CHANGES

> 
> Martin
> 

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

[Martin Wilck]

On Tue, 2024-09-03 at 00:16 +0000, Shinichiro Kawasaki wrote:
> On Sep 02, 2024 / 15:53, Martin Wilck wrote:
> > On Thu, 2024-08-29 at 07:36 +0000, Shinichiro Kawasaki wrote:
> > 
> > 
> > > 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)
> > 
> > Thanks for the review. If you don't mind, I'll just use "mapfile -
> > t",
> > as suggested by shellcheck.
> 
> The reason I did not suggest the "mapfile -t" solution was that
> SC2207
> recommends it for bash versions 4.4+ [1]. On the other hand, blktests
> README.md
> requires bash version >= 4.2. So I assume that some blktests users
> use bash
> version 4.2 or 4.3. I took a glance in the bash change history [2],
> and there
> are some mpafile related bug fixes between bash version v4.2 an v4.4.
> So I think
> it's the better to not use "mpafile -t" in blktests.

Sorry, I forgot the requirement to be compatible with bash 4.2.

> Do you have any specific reason to use "mapfile -t"?

Just the fact that the code looks simpler and more elegant.

Side note: as we know that _nvme_get_ctrl_list() prints paths from
sysfs, and that these won't contain spaces or glob characters, we might
as well just ignore SC2207 here.

Anyway, I agree that it's better to be on the safe side.
I'll send a new version.

Thanks,
Martin