| Message ID | 20240823200822.129867-3-mwilck@suse.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/3] blktests: nvme: skip passthru tests on multipath devices | expand |
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)
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
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)
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
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 >
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
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
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