[PATCH blktests 00/12] Fix nvme block test issues

[PATCH blktests 00/12] Fix nvme block test issues

[Logan Gunthorpe]

Hi,

This patchset cleans up a number of issues and pain points
I've had with getting the nvme blktests to pass and run cleanly.

The first three patches are meant to fix the Generation Counter
issue that's been discussed before but hasn't been fixed in months.
I primarily use a slightly fixed up patch posted by Michael Moese[1]
but add another patch to properly test the Generation Counter that
would no longer be tested otherwise.

I've also taken it a step further to filter out more of the discovery
information so that we are less fragile against churn and less dependant
on the version of nvme-cli in use. This should also fix the issue discussed
in [2].

Patches 4 through 7 fix a number of smaller issues I've found with
individual tests.

Patches 8 through 10 implement a system to ensure the nvme tests
clean themselves up properly especially when ctrl-c is used to
interrupt a test (working with the tests before this was a huge
pain seeing,  when you ctrl-c, you have to either manually clean
up the nvmet configuration or reboot to get the system in a state
where it's sane to run the tests again).

Patches 11 and 12 make some minor changes that allow running the
tests with the nvme modules built into the kernel.

With these patches, plus a couple I've sent to the nvme list for the
kernel, I can consistently pass the entire nvme suite with the
exception of the lockdep false-positive with nvme/012 that still
seems to be in a bit of contention[3].

Thanks,

Logan

[1] https://github.com/osandov/blktests/pull/34
[2] https://lore.kernel.org/linux-block/20190505150611.15776-4-minwoo.im.dev@gmail.com/
[3] https://lore.kernel.org/lkml/20190214230058.196511-22-bvanassche@acm.org/

--

Logan Gunthorpe (11):
  nvme: More agressively filter the discovery output
  nvme: Add new test to verify the generation counter
  nvme/003,004: Add missing calls to nvme disconnect
  nvme/005: Don't rely on modprobing to set the multipath paramater
  nvme/015: Ensure the namespace is flushed not the char device
  nvme/018: Ignore error message generated by nvme read
  check: Add the ability to call a cleanup function after a test ends
  nvme: Cleanup modprobe lines into helper functions
  nvme: Ensure all ports and subsystems are removed on cleanup
  common: Use sysfs instead of modinfo for _have_module_param()
  nvme: Ignore errors when removing modules

Michael Moese (1):
  Add filter function for nvme discover

 check              |    9 +
 common/rc          |   18 +-
 tests/nvme/002     |   10 +-
 tests/nvme/002.out | 6003 +-------------------------------------------
 tests/nvme/003     |    6 +-
 tests/nvme/003.out |    1 +
 tests/nvme/004     |    6 +-
 tests/nvme/004.out |    1 +
 tests/nvme/005     |   16 +-
 tests/nvme/006     |    6 +-
 tests/nvme/007     |    6 +-
 tests/nvme/008     |    6 +-
 tests/nvme/009     |    5 +-
 tests/nvme/010     |    6 +-
 tests/nvme/011     |    6 +-
 tests/nvme/012     |    6 +-
 tests/nvme/013     |    6 +-
 tests/nvme/014     |    6 +-
 tests/nvme/015     |    5 +-
 tests/nvme/016     |    6 +-
 tests/nvme/016.out |    9 +-
 tests/nvme/017     |    8 +-
 tests/nvme/017.out |    9 +-
 tests/nvme/018     |    8 +-
 tests/nvme/019     |    6 +-
 tests/nvme/020     |    5 +-
 tests/nvme/021     |    6 +-
 tests/nvme/022     |    6 +-
 tests/nvme/023     |    6 +-
 tests/nvme/024     |    6 +-
 tests/nvme/025     |    6 +-
 tests/nvme/026     |    6 +-
 tests/nvme/027     |    6 +-
 tests/nvme/028     |    6 +-
 tests/nvme/029     |    6 +-
 tests/nvme/030     |   72 +
 tests/nvme/030.out |    2 +
 tests/nvme/rc      |   64 +
 38 files changed, 208 insertions(+), 6163 deletions(-)
 create mode 100755 tests/nvme/030
 create mode 100644 tests/nvme/030.out

--
2.17.1

[PATCH blktests 01/12] Add filter function for nvme discover

[Logan Gunthorpe]

From: Michael Moese <mmoese@suse.de>

Several NVMe tests (002, 016, 017) used a pipe to a sed call filtering
the output. This call is moved to a new filter function nvme/rc and
the calls to sed are replaced by this function.

Additionally, the test nvme/016 failed for me due to the Generation
counter being greater than 1, so the new filter function was expanded to
replace the Generation counter with 'X'.

Signed-off-by: Michael Moese <mmoese@suse.de>
[logang@deltatee.com: added missing changes to 002.out and 017.out]
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 tests/nvme/002     | 2 +-
 tests/nvme/002.out | 2 +-
 tests/nvme/016     | 2 +-
 tests/nvme/016.out | 2 +-
 tests/nvme/017     | 2 +-
 tests/nvme/017.out | 2 +-
 tests/nvme/rc      | 5 +++++
 7 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/tests/nvme/002 b/tests/nvme/002
index 106a5a8395f2..ceac1c677bd4 100755
--- a/tests/nvme/002
+++ b/tests/nvme/002
@@ -32,7 +32,7 @@ test() {
 		_add_nvmet_subsys_to_port "${port}" "blktests-subsystem-$i"
 	done
 
-	nvme discover -t loop | sed -r -e "s/portid:  [0-9]+/portid:  X/"
+	nvme discover -t loop | _filter_discovery
 
 	for ((i = iterations - 1; i >= 0; i--)); do
 		_remove_nvmet_subsystem_from_port "${port}" "blktests-subsystem-$i"
diff --git a/tests/nvme/002.out b/tests/nvme/002.out
index aa71d8f5f5f8..7437a21f60a9 100644
--- a/tests/nvme/002.out
+++ b/tests/nvme/002.out
@@ -1,6 +1,6 @@
 Running nvme/002
 
-Discovery Log Number of Records 1000, Generation counter 1000
+Discovery Log Number of Records 1000, Generation counter X
 =====Discovery Log Entry 0======
 trtype:  loop
 adrfam:  pci
diff --git a/tests/nvme/016 b/tests/nvme/016
index 966d5dc0a4a2..dd1b84a16daa 100755
--- a/tests/nvme/016
+++ b/tests/nvme/016
@@ -34,7 +34,7 @@ test() {
 	port="$(_create_nvmet_port "loop")"
 	_add_nvmet_subsys_to_port "$port" "${subsys_nqn}"
 
-	nvme discover -t loop | sed -r -e "s/portid:  [0-9]+/portid:  X/"
+	nvme discover -t loop | _filter_discovery
 	_remove_nvmet_subsystem_from_port "${port}" "${subsys_nqn}"
 	_remove_nvmet_port "${port}"
 
diff --git a/tests/nvme/016.out b/tests/nvme/016.out
index 59bd2935346f..b70603144d5c 100644
--- a/tests/nvme/016.out
+++ b/tests/nvme/016.out
@@ -1,6 +1,6 @@
 Running nvme/016
 
-Discovery Log Number of Records 1, Generation counter 1
+Discovery Log Number of Records 1, Generation counter X
 =====Discovery Log Entry 0======
 trtype:  loop
 adrfam:  pci
diff --git a/tests/nvme/017 b/tests/nvme/017
index 0b86bece9520..5f8d60907293 100755
--- a/tests/nvme/017
+++ b/tests/nvme/017
@@ -37,7 +37,7 @@ test() {
 	port="$(_create_nvmet_port "loop")"
 	_add_nvmet_subsys_to_port "${port}" "${subsys_name}"
 
-	nvme discover -t loop | sed -r -e "s/portid:  [0-9]+/portid:  X/"
+	nvme discover -t loop | _filter_discovery
 	_remove_nvmet_subsystem_from_port "${port}" "${subsys_name}"
 	_remove_nvmet_port "${port}"
 
diff --git a/tests/nvme/017.out b/tests/nvme/017.out
index 4b0877aaf3ca..cf212971d180 100644
--- a/tests/nvme/017.out
+++ b/tests/nvme/017.out
@@ -1,6 +1,6 @@
 Running nvme/017
 
-Discovery Log Number of Records 1, Generation counter 1
+Discovery Log Number of Records 1, Generation counter X
 =====Discovery Log Entry 0======
 trtype:  loop
 adrfam:  pci
diff --git a/tests/nvme/rc b/tests/nvme/rc
index eff1dd992460..22833d8ef9bb 100644
--- a/tests/nvme/rc
+++ b/tests/nvme/rc
@@ -118,3 +118,8 @@ _find_nvme_loop_dev() {
 		fi
 	done
 }
+
+_filter_discovery() {
+	sed -r  -e "s/portid:  [0-9]+/portid:  X/" \
+		-e "s/Generation counter [0-9]+/Generation counter X/"
+}
-- 
2.17.1

[PATCH blktests 03/12] nvme: Add new test to verify the generation counter

[Logan Gunthorpe]

Now that the other discovery tests ignore the generation counter value,
create a new test to specifically check that it increments when
subsystems are added or removed from ports and when allow_any_host
is set/unset.

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 tests/nvme/030     | 76 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/nvme/030.out |  2 ++
 tests/nvme/rc      |  5 +++
 3 files changed, 83 insertions(+)
 create mode 100755 tests/nvme/030
 create mode 100644 tests/nvme/030.out

diff --git a/tests/nvme/030 b/tests/nvme/030
new file mode 100755
index 000000000000..963e1ad7118c
--- /dev/null
+++ b/tests/nvme/030
@@ -0,0 +1,76 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-3.0+
+# Copyright (C) 2019 Logan Gunthorpe
+#
+# Test nvme discovery generation counter
+
+. tests/nvme/rc
+
+DESCRIPTION="ensure the discovery generation counter is updated appropriately"
+QUICK=1
+
+requires() {
+	_have_program nvme &&
+	_have_modules loop nvme-loop nvmet &&
+	_have_configfs
+}
+
+
+checkgenctr() {
+	local last=$1
+	local msg=$2
+	local genctr
+
+	genctr=$(_discovery_genctr)
+	if (( "${genctr}" <= "${last}" )); then
+		echo "Generation counter not incremented when ${msg} (${genctr} <= ${last})"
+	fi
+
+	echo "${genctr}"
+}
+
+test() {
+	local port
+	local genctr
+	local subsys="blktests-subsystem-"
+
+	echo "Running ${TEST_NAME}"
+
+	modprobe nvmet
+	modprobe nvme-loop
+
+	port="$(_create_nvmet_port "loop")"
+
+	_create_nvmet_subsystem "${subsys}1" "$(losetup -f)"
+	_add_nvmet_subsys_to_port "${port}" "${subsys}1"
+
+	genctr=$(_discovery_genctr)
+
+	_create_nvmet_subsystem "${subsys}2" "$(losetup -f)"
+	_add_nvmet_subsys_to_port "${port}" "${subsys}2"
+
+	genctr=$(checkgenctr "${genctr}" "adding a subsystem to a port")
+
+	echo 0 > "${NVMET_CFS}/subsystems/${subsys}2/attr_allow_any_host"
+
+	genctr=$(checkgenctr "${genctr}" "clearing attr_allow_any_host")
+
+	echo 1 > "${NVMET_CFS}/subsystems/${subsys}2/attr_allow_any_host"
+
+	genctr=$(checkgenctr "${genctr}" "setting attr_allow_any_host")
+
+	_remove_nvmet_subsystem_from_port "${port}" "${subsys}2"
+	_remove_nvmet_subsystem "${subsys}2"
+
+	genctr=$(checkgenctr "${genctr}" "removing a subsystem from a port")
+
+	_remove_nvmet_subsystem_from_port "${port}" "${subsys}1"
+	_remove_nvmet_subsystem "${subsys}1"
+
+	_remove_nvmet_port "${port}"
+
+	modprobe -r nvme-loop
+	modprobe -r nvmet
+
+	echo "Test complete"
+}
diff --git a/tests/nvme/030.out b/tests/nvme/030.out
new file mode 100644
index 000000000000..93e51905b5d4
--- /dev/null
+++ b/tests/nvme/030.out
@@ -0,0 +1,2 @@
+Running nvme/030
+Test complete
diff --git a/tests/nvme/rc b/tests/nvme/rc
index 60dc05869726..39b2c2e2b91c 100644
--- a/tests/nvme/rc
+++ b/tests/nvme/rc
@@ -123,3 +123,8 @@ _filter_discovery() {
 	sed -r -e "s/Generation counter [0-9]+/Generation counter X/" |
 		grep 'Discovery Log Number\|Log Entry\|trtype\|subnqn'
 }
+
+_discovery_genctr() {
+	nvme discover -t loop |
+		sed -n -e 's/^.*Generation counter \([0-9]\+\).*$/\1/p'
+}
-- 
2.17.1

[PATCH blktests 04/12] nvme/003,004: Add missing calls to nvme disconnect

[Logan Gunthorpe]

Tests 003 and 004 do not call  nvme disconnect. In most cases it is
cleaned up by removing the modules but it should be made explicit.

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 tests/nvme/003     | 1 +
 tests/nvme/003.out | 1 +
 tests/nvme/004     | 1 +
 tests/nvme/004.out | 1 +
 4 files changed, 4 insertions(+)

diff --git a/tests/nvme/003 b/tests/nvme/003
index c6b3d4037aa6..374e6af0ca6f 100755
--- a/tests/nvme/003
+++ b/tests/nvme/003
@@ -42,6 +42,7 @@ test() {
 		echo "Fail"
 	fi
 
+	nvme disconnect -n nqn.2014-08.org.nvmexpress.discovery
 	_remove_nvmet_subsystem_from_port "${port}" "blktests-subsystem-1"
 	_remove_nvmet_subsystem "blktests-subsystem-1"
 	_remove_nvmet_port "${port}"
diff --git a/tests/nvme/003.out b/tests/nvme/003.out
index 01b275612159..beb356128c9d 100644
--- a/tests/nvme/003.out
+++ b/tests/nvme/003.out
@@ -1,2 +1,3 @@
 Running nvme/003
+NQN:nqn.2014-08.org.nvmexpress.discovery disconnected 1 controller(s)
 Test complete
diff --git a/tests/nvme/004 b/tests/nvme/004
index 0506fa220de3..767aedaa0263 100755
--- a/tests/nvme/004
+++ b/tests/nvme/004
@@ -40,6 +40,7 @@ test() {
 	cat "/sys/block/${nvmedev}n1/uuid"
 	cat "/sys/block/${nvmedev}n1/wwid"
 
+	nvme disconnect -n "blktests-subsystem-1"
 	_remove_nvmet_subsystem_from_port "${port}" "blktests-subsystem-1"
 	_remove_nvmet_subsystem "blktests-subsystem-1"
 	_remove_nvmet_port "${port}"
diff --git a/tests/nvme/004.out b/tests/nvme/004.out
index 53f911ecf329..51f605227320 100644
--- a/tests/nvme/004.out
+++ b/tests/nvme/004.out
@@ -1,4 +1,5 @@
 Running nvme/004
 91fdba0d-f87b-4c25-b80f-db7be1418b9e
 uuid.91fdba0d-f87b-4c25-b80f-db7be1418b9e
+NQN:blktests-subsystem-1 disconnected 1 controller(s)
 Test complete
-- 
2.17.1

[PATCH blktests 05/12] nvme/005: Don't rely on modprobing to set the multipath paramater

[Logan Gunthorpe]

On test systems with existing nvme drives or built-in modules it may not
be possible to remove nvme-core in order to re-probe it with
multipath=1.

Instead, skip the test if the multipath parameter is not already set
ahead of time.

Note: the multipath parameter of nvme-core is set by default if
CONFIG_NVME_MULTIPATH is set so this will only affect systems
that explicitly disable it via the module parameter.

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 common/rc      | 16 ++++++++++++++++
 tests/nvme/005 | 10 ++--------
 2 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/common/rc b/common/rc
index 5dd2c9512fd2..49050c71dabf 100644
--- a/common/rc
+++ b/common/rc
@@ -55,6 +55,22 @@ _have_module_param() {
 	return 0
 }
 
+_have_module_param_value() {
+	local value
+
+	if ! _have_module_param "$1" "$2"; then
+		return 1
+	fi
+
+	value=$(cat "/sys/module/$1/parameters/$2")
+	if [[ "${value}" != "$3" ]]; then
+		SKIP_REASON="$1 module parameter $2 must be set to $3"
+		return 1
+	fi
+
+	return 0
+}
+
 _have_program() {
 	if command -v "$1" >/dev/null 2>&1; then
 		return 0
diff --git a/tests/nvme/005 b/tests/nvme/005
index e72fc809c936..91c164de73e6 100755
--- a/tests/nvme/005
+++ b/tests/nvme/005
@@ -12,18 +12,13 @@ QUICK=1
 
 requires() {
 	_have_modules loop nvme-core nvme-loop nvmet && \
-		_have_module_param nvme-core multipath && _have_configfs
+		_have_module_param_value nvme_core multipath Y && \
+		_have_configfs
 }
 
 test() {
 	echo "Running ${TEST_NAME}"
 
-	# Clean up all stale modules
-	modprobe -r nvme-loop
-	modprobe -r nvme-core
-	modprobe -r nvmet
-
-	modprobe nvme-core multipath=1
 	modprobe nvmet
 	modprobe nvme-loop
 
@@ -57,7 +52,6 @@ test() {
 	rm "$TMPDIR/img"
 
 	modprobe -r nvme-loop
-	modprobe -r nvme-core
 	modprobe -r nvmet
 
 	echo "Test complete"
-- 
2.17.1

[PATCH blktests 06/12] nvme/015: Ensure the namespace is flushed not the char device

[Logan Gunthorpe]

Flushing the char device now results in the warning:

   nvme nvme1: using deprecated NVME_IOCTL_IO_CMD ioctl on the char
	device!

Instead, call the flush on the namespace.

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 tests/nvme/015 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/nvme/015 b/tests/nvme/015
index 47e1b048e16d..ca1216163e16 100755
--- a/tests/nvme/015
+++ b/tests/nvme/015
@@ -39,7 +39,7 @@ test() {
 
 	dd if=/dev/urandom of="/dev/${nvmedev}n1" count=128000 bs=4k status=none
 
-	nvme flush "/dev/${nvmedev}" -n 1
+	nvme flush "/dev/${nvmedev}n1" -n 1
 
 	nvme disconnect -n "${subsys_name}"
 
-- 
2.17.1

[PATCH blktests 07/12] nvme/018: Ignore error message generated by nvme read

[Logan Gunthorpe]

nvme-cli at some point started printing the error message:

  NVMe status: CAP_EXCEEDED: The execution of the command has caused the
	capacity of the namespace to be exceeded(0x6081)

This was not accounted for by test 018 and caused it to fail.

This test does not need to test the error message content, it's
only important that a read past the end of the file fails. Therefore,
pipe stderr of nvme-cli to /dev/null.

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 tests/nvme/018 | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/tests/nvme/018 b/tests/nvme/018
index e29fa92e8153..7b5ade5d3c40 100755
--- a/tests/nvme/018
+++ b/tests/nvme/018
@@ -44,9 +44,8 @@ test() {
 	sectors="$(blockdev --getsz "/dev/${nvmedev}n1")"
 	bs="$(blockdev --getbsz "/dev/${nvmedev}n1")"
 
-	if nvme read "/dev/${nvmedev}n1" -s "$sectors" -c 0 -z "$bs"; then
-		echo "ERROR: Successfully read out of device lba range"
-	fi
+	nvme read "/dev/${nvmedev}n1" -s "$sectors" -c 0 -z "$bs" 2>/dev/null \
+		&& echo "ERROR: Successfully read out of device lba range"
 
 	nvme disconnect -n "${subsys_name}"
 
-- 
2.17.1

[PATCH blktests 08/12] check: Add the ability to call a cleanup function after a test ends

[Logan Gunthorpe]

In order to ensure tests properly clean themselves up, even if
they are subject to interruption, add the ability to call a test
specified function at cleanup time.

Any test can call _register_test_cleanup with the first argument
as a function to call after the test ends or is interrupted
(similar to using 'trap <func> EXIT').

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 check | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/check b/check
index 029095e7cf38..981058c59c12 100755
--- a/check
+++ b/check
@@ -288,7 +288,15 @@ _output_test_run() {
 	) | column -t -s $'\t'
 }
 
+_register_test_cleanup() {
+	TEST_CLEANUP=$1
+}
+
 _cleanup() {
+	if [[ -v TEST_CLEANUP ]]; then
+		${TEST_CLEANUP}
+	fi
+
 	if [[ "${TMPDIR:-}" ]]; then
 		rm -rf "$TMPDIR"
 		unset TMPDIR
@@ -337,6 +345,7 @@ _call_test() {
 	fi
 	$LOGGER_PROG "run blktests $TEST_NAME"
 
+	unset TEST_CLEANUP
 	trap _cleanup EXIT
 	if ! TMPDIR="$(mktemp --tmpdir -p "$OUTPUT" -d "tmpdir.${TEST_NAME//\//.}.XXX")"; then
 		return
-- 
2.17.1

[PATCH blktests 09/12] nvme: Cleanup modprobe lines into helper functions

[Logan Gunthorpe]

Move all the lines to modprobe nvmet and nvme-loop
into _setup_nvmet() and _cleanup_nvmet() helper functions
and call _cleanup_nvmet() using _register_test_cleanup()
to ensure it's always called after the test terminates.

This will allow us to improve the cleanup of these tests and
not leave the system in an inconsistent state when tests
are aborted.

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 tests/nvme/002 |  8 ++------
 tests/nvme/003 |  5 +----
 tests/nvme/004 |  5 +----
 tests/nvme/005 |  6 +-----
 tests/nvme/006 |  6 +-----
 tests/nvme/007 |  6 +-----
 tests/nvme/008 |  6 +-----
 tests/nvme/009 |  5 +----
 tests/nvme/010 |  6 +-----
 tests/nvme/011 |  6 +-----
 tests/nvme/012 |  6 +-----
 tests/nvme/013 |  6 +-----
 tests/nvme/014 |  6 +-----
 tests/nvme/015 |  3 +--
 tests/nvme/016 |  4 +---
 tests/nvme/017 |  6 +-----
 tests/nvme/018 |  3 +--
 tests/nvme/019 |  6 +-----
 tests/nvme/020 |  5 +----
 tests/nvme/021 |  6 +-----
 tests/nvme/022 |  6 +-----
 tests/nvme/023 |  6 +-----
 tests/nvme/024 |  6 +-----
 tests/nvme/025 |  6 +-----
 tests/nvme/026 |  6 +-----
 tests/nvme/027 |  6 +-----
 tests/nvme/028 |  6 +-----
 tests/nvme/029 |  6 +-----
 tests/nvme/030 |  6 +-----
 tests/nvme/rc  | 11 +++++++++++
 30 files changed, 41 insertions(+), 134 deletions(-)

diff --git a/tests/nvme/002 b/tests/nvme/002
index ceac1c677bd4..07b7fdae2d39 100755
--- a/tests/nvme/002
+++ b/tests/nvme/002
@@ -16,11 +16,9 @@ requires() {
 test() {
 	echo "Running ${TEST_NAME}"
 
-	local iterations=1000
-
-	modprobe nvmet
-	modprobe nvme-loop
+	_setup_nvmet
 
+	local iterations=1000
 	local port
 	port="$(_create_nvmet_port "loop")"
 
@@ -41,7 +39,5 @@ test() {
 
 	_remove_nvmet_port "${port}"
 
-	modprobe -r nvme-loop
-	modprobe -r nvmet
 	echo "Test complete"
 }
diff --git a/tests/nvme/003 b/tests/nvme/003
index 374e6af0ca6f..ed0feca3cac7 100755
--- a/tests/nvme/003
+++ b/tests/nvme/003
@@ -17,8 +17,7 @@ requires() {
 test() {
 	echo "Running ${TEST_NAME}"
 
-	modprobe nvmet
-	modprobe nvme-loop
+	_setup_nvmet
 
 	local port
 	port="$(_create_nvmet_port "loop")"
@@ -47,7 +46,5 @@ test() {
 	_remove_nvmet_subsystem "blktests-subsystem-1"
 	_remove_nvmet_port "${port}"
 
-	modprobe -r nvme-loop
-	modprobe -r nvmet
 	echo "Test complete"
 }
diff --git a/tests/nvme/004 b/tests/nvme/004
index 767aedaa0263..0debcd9c7049 100755
--- a/tests/nvme/004
+++ b/tests/nvme/004
@@ -18,8 +18,7 @@ requires() {
 test() {
 	echo "Running ${TEST_NAME}"
 
-	modprobe nvmet
-	modprobe nvme-loop
+	_setup_nvmet
 
 	local port
 	port="$(_create_nvmet_port "loop")"
@@ -47,7 +46,5 @@ test() {
 	losetup -d "$loop_dev"
 	rm "$TMPDIR/img"
 
-	modprobe -r nvme-loop
-	modprobe -r nvmet
 	echo "Test complete"
 }
diff --git a/tests/nvme/005 b/tests/nvme/005
index 91c164de73e6..8c79d234bb1d 100755
--- a/tests/nvme/005
+++ b/tests/nvme/005
@@ -19,8 +19,7 @@ requires() {
 test() {
 	echo "Running ${TEST_NAME}"
 
-	modprobe nvmet
-	modprobe nvme-loop
+	_setup_nvmet
 
 	local port
 	port="$(_create_nvmet_port "loop")"
@@ -51,8 +50,5 @@ test() {
 	losetup -d "$loop_dev"
 	rm "$TMPDIR/img"
 
-	modprobe -r nvme-loop
-	modprobe -r nvmet
-
 	echo "Test complete"
 }
diff --git a/tests/nvme/006 b/tests/nvme/006
index d12d66bc5a8d..6c8e18560264 100755
--- a/tests/nvme/006
+++ b/tests/nvme/006
@@ -21,8 +21,7 @@ test() {
 	local loop_dev
 	local subsys_name="blktests-subsystem-1"
 
-	modprobe nvmet
-	modprobe nvme-loop
+	_setup_nvmet
 
 	truncate -s 1G "$TMPDIR/img"
 
@@ -41,8 +40,5 @@ test() {
 
 	rm "$TMPDIR/img"
 
-	modprobe -r nvme-loop
-	modprobe -r nvmet
-
 	echo "Test complete"
 }
diff --git a/tests/nvme/007 b/tests/nvme/007
index 6a57b7bf7e0d..58f4bf8808a1 100755
--- a/tests/nvme/007
+++ b/tests/nvme/007
@@ -20,8 +20,7 @@ test() {
 	local file_path
 	local subsys_name="blktests-subsystem-1"
 
-	modprobe nvmet
-	modprobe nvme-loop
+	_setup_nvmet
 
 	file_path="${TMPDIR}/img"
 
@@ -38,8 +37,5 @@ test() {
 
 	rm "${file_path}"
 
-	modprobe -r nvme-loop
-	modprobe -r nvmet
-
 	echo "Test complete"
 }
diff --git a/tests/nvme/008 b/tests/nvme/008
index 04ff0bda42e3..71ff4d962b00 100755
--- a/tests/nvme/008
+++ b/tests/nvme/008
@@ -17,8 +17,7 @@ requires() {
 test() {
 	echo "Running ${TEST_NAME}"
 
-	modprobe nvmet
-	modprobe nvme-loop
+	_setup_nvmet
 
 	local port
 	local nvmedev
@@ -53,8 +52,5 @@ test() {
 
 	rm "${file_path}"
 
-	modprobe -r nvme-loop
-	modprobe -r nvmet
-
 	echo "Test complete"
 }
diff --git a/tests/nvme/009 b/tests/nvme/009
index 81e61f99aba5..25c7da2ab854 100755
--- a/tests/nvme/009
+++ b/tests/nvme/009
@@ -16,8 +16,7 @@ requires() {
 test() {
 	echo "Running ${TEST_NAME}"
 
-	modprobe nvmet
-	modprobe nvme-loop
+	_setup_nvmet
 
 	local port
 	local nvmedev
@@ -47,7 +46,5 @@ test() {
 
 	rm "${file_path}"
 
-	modprobe -r nvme-loop
-	modprobe -r nvmet
 	echo "Test complete"
 }
diff --git a/tests/nvme/010 b/tests/nvme/010
index ed7c95af2853..2ed0f4871a30 100755
--- a/tests/nvme/010
+++ b/tests/nvme/010
@@ -17,8 +17,7 @@ requires() {
 test() {
 	echo "Running ${TEST_NAME}"
 
-	modprobe nvmet
-	modprobe nvme-loop
+	_setup_nvmet
 
 	local port
 	local nvmedev
@@ -54,8 +53,5 @@ test() {
 	rm "${file_path}"
 	rm -f local*verify*state
 
-	modprobe -r nvme_loop
-	modprobe -r nvmet
-
 	echo "Test complete"
 }
diff --git a/tests/nvme/011 b/tests/nvme/011
index fa638a193bad..974b33745b99 100755
--- a/tests/nvme/011
+++ b/tests/nvme/011
@@ -17,8 +17,7 @@ requires() {
 test() {
 	echo "Running ${TEST_NAME}"
 
-	modprobe nvmet
-	modprobe nvme-loop
+	_setup_nvmet
 
 	local port
 	local nvmedev
@@ -50,8 +49,5 @@ test() {
 	rm "${file_path}"
 	rm -f local-write-and-verify*state
 
-	modprobe -r nvme_loop
-	modprobe -r nvmet
-
 	echo "Test complete"
 }
diff --git a/tests/nvme/012 b/tests/nvme/012
index d7a8751ec752..27981e903c58 100755
--- a/tests/nvme/012
+++ b/tests/nvme/012
@@ -17,8 +17,7 @@ requires() {
 test() {
 	echo "Running ${TEST_NAME}"
 
-	modprobe nvmet
-	modprobe nvme-loop
+	_setup_nvmet
 
 	local port
 	local nvmedev
@@ -66,8 +65,5 @@ test() {
 	rm "${file_path}"
 	rm -fr "${mount_dir}"
 
-	modprobe -r nvme_loop
-	modprobe -r nvmet
-
 	echo "Test complete"
 }
diff --git a/tests/nvme/013 b/tests/nvme/013
index 131855298f0f..af5f3730a2fc 100755
--- a/tests/nvme/013
+++ b/tests/nvme/013
@@ -17,8 +17,7 @@ requires() {
 test() {
 	echo "Running ${TEST_NAME}"
 
-	modprobe nvmet
-	modprobe nvme-loop
+	_setup_nvmet
 
 	local port
 	local nvmedev
@@ -62,8 +61,5 @@ test() {
 	rm "${file_path}"
 	rm -fr "${mount_dir}"
 
-	modprobe -r nvme_loop
-	modprobe -r nvmet
-
 	echo "Test complete"
 }
diff --git a/tests/nvme/014 b/tests/nvme/014
index 7de568faeff2..c255d5f12205 100755
--- a/tests/nvme/014
+++ b/tests/nvme/014
@@ -17,8 +17,7 @@ requires() {
 test() {
 	echo "Running ${TEST_NAME}"
 
-	modprobe nvmet
-	modprobe nvme-loop
+	_setup_nvmet
 
 	local port
 	local nvmedev
@@ -55,8 +54,5 @@ test() {
 
 	rm "${file_path}"
 
-	modprobe -r nvme-loop
-	modprobe -r nvmet
-
 	echo "Test complete"
 }
diff --git a/tests/nvme/015 b/tests/nvme/015
index ca1216163e16..a8497a2ba400 100755
--- a/tests/nvme/015
+++ b/tests/nvme/015
@@ -16,8 +16,7 @@ requires() {
 test() {
 	echo "Running ${TEST_NAME}"
 
-	modprobe nvmet
-	modprobe nvme-loop
+	_setup_nvmet
 
 	local port
 	local nvmedev
diff --git a/tests/nvme/016 b/tests/nvme/016
index dd1b84a16daa..9e670e7f6bcd 100755
--- a/tests/nvme/016
+++ b/tests/nvme/016
@@ -20,8 +20,7 @@ test() {
 	local loop_dev
 	local subsys_nqn="blktests-subsystem-1"
 
-	modprobe nvmet
-	modprobe nvme-loop
+	_setup_nvmet
 
 	loop_dev="$(losetup -f)"
 
@@ -44,6 +43,5 @@ test() {
 
 	_remove_nvmet_subsystem "${subsys_nqn}"
 
-	modprobe -r nvme-loop nvmet
 	echo "Test complete"
 }
diff --git a/tests/nvme/017 b/tests/nvme/017
index 5f8d60907293..ef27de65cf2e 100755
--- a/tests/nvme/017
+++ b/tests/nvme/017
@@ -20,8 +20,7 @@ test() {
 	local iterations=1000
 	local subsys_name="blktests-subsystem-1"
 
-	modprobe nvmet
-	modprobe nvme-loop
+	_setup_nvmet
 
 	file_path="${TMPDIR}/img"
 
@@ -49,8 +48,5 @@ test() {
 
 	rm "${file_path}"
 
-	modprobe -r nvme-loop
-	modprobe -r nvmet
-
 	echo "Test complete"
 }
diff --git a/tests/nvme/018 b/tests/nvme/018
index 7b5ade5d3c40..c1231c3de172 100755
--- a/tests/nvme/018
+++ b/tests/nvme/018
@@ -18,8 +18,7 @@ requires() {
 test() {
 	echo "Running ${TEST_NAME}"
 
-	modprobe nvmet
-	modprobe nvme-loop
+	_setup_nvmet
 
 	local port
 	local nvmedev
diff --git a/tests/nvme/019 b/tests/nvme/019
index 4a167361c42c..a8b0204ec0eb 100755
--- a/tests/nvme/019
+++ b/tests/nvme/019
@@ -17,8 +17,7 @@ requires() {
 test() {
 	echo "Running ${TEST_NAME}"
 
-	modprobe nvmet
-	modprobe nvme-loop
+	_setup_nvmet
 
 	local port
 	local nvmedev
@@ -55,8 +54,5 @@ test() {
 
 	rm -f "${file_path}"
 
-	modprobe -r nvme-loop
-	modprobe -r nvmet
-
 	echo "Test complete"
 }
diff --git a/tests/nvme/020 b/tests/nvme/020
index 6bd8075b1f4d..b480ee1b92d0 100755
--- a/tests/nvme/020
+++ b/tests/nvme/020
@@ -16,8 +16,7 @@ requires() {
 test() {
 	echo "Running ${TEST_NAME}"
 
-	modprobe nvmet
-	modprobe nvme-loop
+	_setup_nvmet
 
 	local port
 	local nvmedev
@@ -49,7 +48,5 @@ test() {
 
 	rm -f "${file_path}"
 
-	modprobe -r nvme-loop
-	modprobe -r nvmet
 	echo "Test complete"
 }
diff --git a/tests/nvme/021 b/tests/nvme/021
index fef31b080c99..bbee54d16ff1 100755
--- a/tests/nvme/021
+++ b/tests/nvme/021
@@ -17,8 +17,7 @@ requires() {
 test() {
 	echo "Running ${TEST_NAME}"
 
-	modprobe nvmet
-	modprobe nvme-loop
+	_setup_nvmet
 
 	local port
 	local nvmedev
@@ -50,8 +49,5 @@ test() {
 
 	rm -f "${file_path}"
 
-	modprobe -r nvme-loop
-	modprobe -r nvmet
-
 	echo "Test complete"
 }
diff --git a/tests/nvme/022 b/tests/nvme/022
index 8f7492c0ff15..9ba07c1cc50f 100755
--- a/tests/nvme/022
+++ b/tests/nvme/022
@@ -17,8 +17,7 @@ requires() {
 test() {
 	echo "Running ${TEST_NAME}"
 
-	modprobe nvmet
-	modprobe nvme-loop
+	_setup_nvmet
 
 	local port
 	local nvmedev
@@ -50,8 +49,5 @@ test() {
 
 	rm -f "${file_path}"
 
-	modprobe -r nvme-loop
-	modprobe -r nvmet
-
 	echo "Test complete"
 }
diff --git a/tests/nvme/023 b/tests/nvme/023
index de1a6bc0f975..ed2a5ad7653f 100755
--- a/tests/nvme/023
+++ b/tests/nvme/023
@@ -17,8 +17,7 @@ requires() {
 test() {
 	echo "Running ${TEST_NAME}"
 
-	modprobe nvmet
-	modprobe nvme-loop
+	_setup_nvmet
 
 	local port
 	local nvmedev
@@ -55,8 +54,5 @@ test() {
 
 	rm -f "${file_path}"
 
-	modprobe -r nvme-loop
-	modprobe -r nvmet
-
 	echo "Test complete"
 }
diff --git a/tests/nvme/024 b/tests/nvme/024
index c88fbd8a3663..538580947c5c 100755
--- a/tests/nvme/024
+++ b/tests/nvme/024
@@ -17,8 +17,7 @@ requires() {
 test() {
 	echo "Running ${TEST_NAME}"
 
-	modprobe nvmet
-	modprobe nvme-loop
+	_setup_nvmet
 
 	local port
 	local nvmedev
@@ -49,8 +48,5 @@ test() {
 
 	rm -f "${file_path}"
 
-	modprobe -r nvme-loop
-	modprobe -r nvmet
-
 	echo "Test complete"
 }
diff --git a/tests/nvme/025 b/tests/nvme/025
index 6468f59d9dd9..0039fefa5007 100755
--- a/tests/nvme/025
+++ b/tests/nvme/025
@@ -17,8 +17,7 @@ requires() {
 test() {
 	echo "Running ${TEST_NAME}"
 
-	modprobe nvmet
-	modprobe nvme-loop
+	_setup_nvmet
 
 	local port
 	local nvmedev
@@ -50,8 +49,5 @@ test() {
 
 	rm -f "${file_path}"
 
-	modprobe -r nvme-loop
-	modprobe -r nvmet
-
 	echo "Test complete"
 }
diff --git a/tests/nvme/026 b/tests/nvme/026
index 2e9655bdd40f..7e89d840529c 100755
--- a/tests/nvme/026
+++ b/tests/nvme/026
@@ -17,8 +17,7 @@ requires() {
 test() {
 	echo "Running ${TEST_NAME}"
 
-	modprobe nvmet
-	modprobe nvme-loop
+	_setup_nvmet
 
 	local port
 	local nvmedev
@@ -50,8 +49,5 @@ test() {
 
 	rm -f "${file_path}"
 
-	modprobe -r nvme-loop
-	modprobe -r nvmet
-
 	echo "Test complete"
 }
diff --git a/tests/nvme/027 b/tests/nvme/027
index db732425db21..4d293beb8b47 100755
--- a/tests/nvme/027
+++ b/tests/nvme/027
@@ -17,8 +17,7 @@ requires() {
 test() {
 	echo "Running ${TEST_NAME}"
 
-	modprobe nvmet
-	modprobe nvme-loop
+	_setup_nvmet
 
 	local port
 	local nvmedev
@@ -49,8 +48,5 @@ test() {
 
 	rm -f "${file_path}"
 
-	modprobe -r nvme-loop
-	modprobe -r nvmet
-
 	echo "Test complete"
 }
diff --git a/tests/nvme/028 b/tests/nvme/028
index 10be8fb4ba9c..1280107ed5df 100755
--- a/tests/nvme/028
+++ b/tests/nvme/028
@@ -17,8 +17,7 @@ requires() {
 test() {
 	echo "Running ${TEST_NAME}"
 
-	modprobe nvmet
-	modprobe nvme-loop
+	_setup_nvmet
 
 	local port
 	local nvmedev
@@ -49,8 +48,5 @@ test() {
 
 	rm -f "${file_path}"
 
-	modprobe -r nvme-loop
-	modprobe -r nvmet
-
 	echo "Test complete"
 }
diff --git a/tests/nvme/029 b/tests/nvme/029
index e63dfc166f26..65eb40031888 100755
--- a/tests/nvme/029
+++ b/tests/nvme/029
@@ -50,8 +50,7 @@ test_user_io()
 test() {
 	echo "Running ${TEST_NAME}"
 
-	modprobe nvmet
-	modprobe nvme-loop
+	_setup_nvmet
 
 	local port
 	local nvmedev
@@ -92,8 +91,5 @@ test() {
 
 	rm "${file_path}"
 
-	modprobe -r nvme-loop
-	modprobe -r nvmet
-
 	echo "Test complete"
 }
diff --git a/tests/nvme/030 b/tests/nvme/030
index 963e1ad7118c..94020f47411e 100755
--- a/tests/nvme/030
+++ b/tests/nvme/030
@@ -36,8 +36,7 @@ test() {
 
 	echo "Running ${TEST_NAME}"
 
-	modprobe nvmet
-	modprobe nvme-loop
+    _setup_nvmet
 
 	port="$(_create_nvmet_port "loop")"
 
@@ -69,8 +68,5 @@ test() {
 
 	_remove_nvmet_port "${port}"
 
-	modprobe -r nvme-loop
-	modprobe -r nvmet
-
 	echo "Test complete"
 }
diff --git a/tests/nvme/rc b/tests/nvme/rc
index 39b2c2e2b91c..b8f7571c7170 100644
--- a/tests/nvme/rc
+++ b/tests/nvme/rc
@@ -24,6 +24,17 @@ _test_dev_is_nvme() {
 	return 0
 }
 
+_cleanup_nvmet() {
+	modprobe -r nvme-loop
+	modprobe -r nvmet
+}
+
+_setup_nvmet() {
+	_register_test_cleanup _cleanup_nvmet
+	modprobe nvmet
+	modprobe nvme-loop
+}
+
 _create_nvmet_port() {
 	local trtype="$1"
 
-- 
2.17.1

[PATCH blktests 10/12] nvme: Ensure all ports and subsystems are removed on cleanup

[Logan Gunthorpe]

This ensures any test that fails or is interrupted will cleanup
their subsystems. This will prevent the system from being left
in an inconsistent state that will fail subsequent tests.

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 tests/nvme/rc | 43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/tests/nvme/rc b/tests/nvme/rc
index b8f7571c7170..1c9e4af0cbe5 100644
--- a/tests/nvme/rc
+++ b/tests/nvme/rc
@@ -25,6 +25,49 @@ _test_dev_is_nvme() {
 }
 
 _cleanup_nvmet() {
+	local dev
+	local port
+	local subsys
+	local transport
+	local name
+
+	if [[ ! -d "${NVMET_CFS}" ]]; then
+		return 0
+	fi
+
+	# Don't let successive Ctrl-Cs interrupt the cleanup processes
+	stty -isig
+
+	shopt -s nullglob
+
+	for dev in /sys/class/nvme/nvme*; do
+		dev="$(basename "$dev")"
+		transport="$(cat "/sys/class/nvme/${dev}/transport")"
+		if [[ "$transport" == "loop" ]]; then
+			echo "WARNING: Test did not clean up loop device: ${dev}"
+			nvme disconnect -d "${dev}"
+		fi
+	done
+
+	for port in "${NVMET_CFS}"/ports/*; do
+		name=$(basename "${port}")
+		echo "WARNING: Test did not clean up port: ${name}"
+		rm -f "${port}"/subsystems/*
+		rmdir "${port}"
+	done
+
+	for subsys in "${NVMET_CFS}"/subsystems/*; do
+		name=$(basename "${subsys}")
+		echo "WARNING: Test did not clean up subsystem: ${name}"
+		for ns in "${subsys}"/namespaces/*; do
+			rmdir "${ns}"
+		done
+		rmdir "${subsys}"
+	done
+
+	shopt -u nullglob
+	stty isig
+
 	modprobe -r nvme-loop
 	modprobe -r nvmet
 }
-- 
2.17.1

[PATCH blktests 11/12] common: Use sysfs instead of modinfo for _have_module_param()

[Logan Gunthorpe]

Using modinfo fails if the given module is built-in. Instead,
just check for the parameter's existence in sysfs.

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 common/rc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/common/rc b/common/rc
index 49050c71dabf..d48f73c5bf3d 100644
--- a/common/rc
+++ b/common/rc
@@ -48,7 +48,7 @@ _have_modules() {
 }
 
 _have_module_param() {
-	if ! modinfo -F parm -0 "$1" | grep -q -z "^$2:"; then
+	if ! [ -e "/sys/module/$1/parameters/$2" ]; then
 		SKIP_REASON="$1 module does not have parameter $2"
 		return 1
 	fi
-- 
2.17.1

[PATCH blktests 12/12] nvme: Ignore errors when removing modules

[Logan Gunthorpe]

It is no longer important for correct test functionality to
remove the modules between tests. Therefore, we ignore errors
if the modules are not removed (ie. if they are builtin).

With this patch, it is now safe to run the tests with the nvmet
modules built-in. This will be more convienent for developers
that want to run the tests in a simple VM.

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 tests/nvme/rc | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/nvme/rc b/tests/nvme/rc
index 1c9e4af0cbe5..0b98b89d0634 100644
--- a/tests/nvme/rc
+++ b/tests/nvme/rc
@@ -68,8 +68,8 @@ _cleanup_nvmet() {
 	shopt -u nullglob
 	stty isig
 
-	modprobe -r nvme-loop
-	modprobe -r nvmet
+	modprobe -r nvme-loop 2>/dev/null
+	modprobe -r nvmet 2>/dev/null
 }
 
 _setup_nvmet() {
-- 
2.17.1

Re: [PATCH blktests 00/12] Fix nvme block test issues

[Logan Gunthorpe]

Oh, I forgot to mention there is a git branch of these patches available
here:

https://github.com/Eideticom/blktests nvme_fixes

Logan

Re: [PATCH blktests 07/12] nvme/018: Ignore error message generated by nvme read]

[Johannes Thumshirn]

On Fri, Jul 12, 2019 at 05:57:37PM -0600, Logan Gunthorpe wrote:
> nvme-cli at some point started printing the error message:
> 
>   NVMe status: CAP_EXCEEDED: The execution of the command has caused the
> 	capacity of the namespace to be exceeded(0x6081)
> 
> This was not accounted for by test 018 and caused it to fail.
> 
> This test does not need to test the error message content, it's
> only important that a read past the end of the file fails. Therefore,
> pipe stderr of nvme-cli to /dev/null.

How about redirecting all of the output to $FULL?

-- 
Johannes Thumshirn                            SUSE Labs Filesystems
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH blktests 11/12] common: Use sysfs instead of modinfo for _have_module_param()

[Johannes Thumshirn]

On Fri, Jul 12, 2019 at 05:57:41PM -0600, Logan Gunthorpe wrote:
> Using modinfo fails if the given module is built-in. Instead,
> just check for the parameter's existence in sysfs.
> 
> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
> ---
>  common/rc | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/common/rc b/common/rc
> index 49050c71dabf..d48f73c5bf3d 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -48,7 +48,7 @@ _have_modules() {
>  }
>  
>  _have_module_param() {
> -	if ! modinfo -F parm -0 "$1" | grep -q -z "^$2:"; then
> +	if ! [ -e "/sys/module/$1/parameters/$2" ]; then
>  		SKIP_REASON="$1 module does not have parameter $2"
>  		return 1
>  	fi

But this now fails if the module isn't loaded yet. IMHO we'll need to check if
"/sys/module/$1" exists and if it does check for
"/sys/module/$1/parameters/$2", if not try modinfo.

Does that make sense?

Byte,
	Johannes
-- 
Johannes Thumshirn                            SUSE Labs Filesystems
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH blktests 07/12] nvme/018: Ignore error message generated by nvme read]

[Logan Gunthorpe]

On 2019-07-15 1:15 a.m., Johannes Thumshirn wrote:
> On Fri, Jul 12, 2019 at 05:57:37PM -0600, Logan Gunthorpe wrote:
>> nvme-cli at some point started printing the error message:
>>
>>   NVMe status: CAP_EXCEEDED: The execution of the command has caused the
>> 	capacity of the namespace to be exceeded(0x6081)
>>
>> This was not accounted for by test 018 and caused it to fail.
>>
>> This test does not need to test the error message content, it's
>> only important that a read past the end of the file fails. Therefore,
>> pipe stderr of nvme-cli to /dev/null.
> 
> How about redirecting all of the output to $FULL?

Sure, I'll update it and send a v2.

Logan

Re: [PATCH blktests 11/12] common: Use sysfs instead of modinfo for _have_module_param()

[Logan Gunthorpe]

On 2019-07-15 1:21 a.m., Johannes Thumshirn wrote:
> On Fri, Jul 12, 2019 at 05:57:41PM -0600, Logan Gunthorpe wrote:
>> Using modinfo fails if the given module is built-in. Instead,
>> just check for the parameter's existence in sysfs.
>>
>> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
>> ---
>>  common/rc | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/common/rc b/common/rc
>> index 49050c71dabf..d48f73c5bf3d 100644
>> --- a/common/rc
>> +++ b/common/rc
>> @@ -48,7 +48,7 @@ _have_modules() {
>>  }
>>  
>>  _have_module_param() {
>> -	if ! modinfo -F parm -0 "$1" | grep -q -z "^$2:"; then
>> +	if ! [ -e "/sys/module/$1/parameters/$2" ]; then
>>  		SKIP_REASON="$1 module does not have parameter $2"
>>  		return 1
>>  	fi
> 
> But this now fails if the module isn't loaded yet. IMHO we'll need to check if
> "/sys/module/$1" exists and if it does check for
> "/sys/module/$1/parameters/$2", if not try modinfo.
> 
> Does that make sense?

Yup, will fix for v2.

Thanks,

Logan

Re: [PATCH blktests 00/12] Fix nvme block test issues

[Chaitanya Kulkarni]

Thanks Logan for doing this. I am overall okay with overall with
these changes.

Once you post V2 I'll test it on my machine.

On 07/12/2019 04:58 PM, Logan Gunthorpe wrote:
> Hi,
>
> This patchset cleans up a number of issues and pain points
> I've had with getting the nvme blktests to pass and run cleanly.
>
> The first three patches are meant to fix the Generation Counter
> issue that's been discussed before but hasn't been fixed in months.
> I primarily use a slightly fixed up patch posted by Michael Moese[1]
> but add another patch to properly test the Generation Counter that
> would no longer be tested otherwise.
>
> I've also taken it a step further to filter out more of the discovery
> information so that we are less fragile against churn and less dependant
> on the version of nvme-cli in use. This should also fix the issue discussed
> in [2].
>
> Patches 4 through 7 fix a number of smaller issues I've found with
> individual tests.
>
> Patches 8 through 10 implement a system to ensure the nvme tests
> clean themselves up properly especially when ctrl-c is used to
> interrupt a test (working with the tests before this was a huge
> pain seeing,  when you ctrl-c, you have to either manually clean
> up the nvmet configuration or reboot to get the system in a state
> where it's sane to run the tests again).
>
> Patches 11 and 12 make some minor changes that allow running the
> tests with the nvme modules built into the kernel.
>
> With these patches, plus a couple I've sent to the nvme list for the
> kernel, I can consistently pass the entire nvme suite with the
> exception of the lockdep false-positive with nvme/012 that still
> seems to be in a bit of contention[3].
>
> Thanks,
>
> Logan
>
> [1] https://github.com/osandov/blktests/pull/34
> [2] https://lore.kernel.org/linux-block/20190505150611.15776-4-minwoo.im.dev@gmail.com/
> [3] https://lore.kernel.org/lkml/20190214230058.196511-22-bvanassche@acm.org/
>
> --
>
> Logan Gunthorpe (11):
>    nvme: More agressively filter the discovery output
>    nvme: Add new test to verify the generation counter
>    nvme/003,004: Add missing calls to nvme disconnect
>    nvme/005: Don't rely on modprobing to set the multipath paramater
>    nvme/015: Ensure the namespace is flushed not the char device
>    nvme/018: Ignore error message generated by nvme read
>    check: Add the ability to call a cleanup function after a test ends
>    nvme: Cleanup modprobe lines into helper functions
>    nvme: Ensure all ports and subsystems are removed on cleanup
>    common: Use sysfs instead of modinfo for _have_module_param()
>    nvme: Ignore errors when removing modules
>
> Michael Moese (1):
>    Add filter function for nvme discover
>
>   check              |    9 +
>   common/rc          |   18 +-
>   tests/nvme/002     |   10 +-
>   tests/nvme/002.out | 6003 +-------------------------------------------
>   tests/nvme/003     |    6 +-
>   tests/nvme/003.out |    1 +
>   tests/nvme/004     |    6 +-
>   tests/nvme/004.out |    1 +
>   tests/nvme/005     |   16 +-
>   tests/nvme/006     |    6 +-
>   tests/nvme/007     |    6 +-
>   tests/nvme/008     |    6 +-
>   tests/nvme/009     |    5 +-
>   tests/nvme/010     |    6 +-
>   tests/nvme/011     |    6 +-
>   tests/nvme/012     |    6 +-
>   tests/nvme/013     |    6 +-
>   tests/nvme/014     |    6 +-
>   tests/nvme/015     |    5 +-
>   tests/nvme/016     |    6 +-
>   tests/nvme/016.out |    9 +-
>   tests/nvme/017     |    8 +-
>   tests/nvme/017.out |    9 +-
>   tests/nvme/018     |    8 +-
>   tests/nvme/019     |    6 +-
>   tests/nvme/020     |    5 +-
>   tests/nvme/021     |    6 +-
>   tests/nvme/022     |    6 +-
>   tests/nvme/023     |    6 +-
>   tests/nvme/024     |    6 +-
>   tests/nvme/025     |    6 +-
>   tests/nvme/026     |    6 +-
>   tests/nvme/027     |    6 +-
>   tests/nvme/028     |    6 +-
>   tests/nvme/029     |    6 +-
>   tests/nvme/030     |   72 +
>   tests/nvme/030.out |    2 +
>   tests/nvme/rc      |   64 +
>   38 files changed, 208 insertions(+), 6163 deletions(-)
>   create mode 100755 tests/nvme/030
>   create mode 100644 tests/nvme/030.out
>
> --
> 2.17.1
>

Re: [PATCH blktests 02/12] nvme: More agressively filter the discovery output

[Omar Sandoval]

On Fri, Jul 12, 2019 at 05:57:32PM -0600, Logan Gunthorpe wrote:
> Comparing the entire output of nvme-cli for discovery is fragile
> and error prone as things change. There's already been the
> long standing issue of the generation counter mismatching
> and also some versions of nvme-cli print an extra "sq flow control
> disable supported" text[1].
> 
> Instead, filter out all but a few key values from the discovery
> text which should still be sufficient for this test and much
> less likely to be subject to churn.
> 
> [1] https://lore.kernel.org/linux-block/20190505150611.15776-4-minwoo.im.dev@gmail.com/
> 
> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
> ---
>  tests/nvme/002.out | 6001 --------------------------------------------
>  tests/nvme/016.out |    7 -
>  tests/nvme/017.out |    7 -
>  tests/nvme/rc      |    4 +-
>  4 files changed, 2 insertions(+), 6017 deletions(-)

[snip]

> diff --git a/tests/nvme/rc b/tests/nvme/rc
> index 22833d8ef9bb..60dc05869726 100644
> --- a/tests/nvme/rc
> +++ b/tests/nvme/rc
> @@ -120,6 +120,6 @@ _find_nvme_loop_dev() {
>  }
>  
>  _filter_discovery() {
> -	sed -r  -e "s/portid:  [0-9]+/portid:  X/" \
> -		-e "s/Generation counter [0-9]+/Generation counter X/"
> +	sed -r -e "s/Generation counter [0-9]+/Generation counter X/" |
> +		grep 'Discovery Log Number\|Log Entry\|trtype\|subnqn'
>  }

This can be done in a single sed command instead of sed + grep:

sed -rn -e 's/Generation counter [0-9]+/Generation counter X/' \
	-e '/Discovery Log Number|Log Entry|trtype|subnqn/p'

Re: [PATCH blktests 00/12] Fix nvme block test issues

[Omar Sandoval]

On Fri, Jul 12, 2019 at 05:57:30PM -0600, Logan Gunthorpe wrote:
> Hi,
> 
> This patchset cleans up a number of issues and pain points
> I've had with getting the nvme blktests to pass and run cleanly.
> 
> The first three patches are meant to fix the Generation Counter
> issue that's been discussed before but hasn't been fixed in months.
> I primarily use a slightly fixed up patch posted by Michael Moese[1]
> but add another patch to properly test the Generation Counter that
> would no longer be tested otherwise.
> 
> I've also taken it a step further to filter out more of the discovery
> information so that we are less fragile against churn and less dependant
> on the version of nvme-cli in use. This should also fix the issue discussed
> in [2].
> 
> Patches 4 through 7 fix a number of smaller issues I've found with
> individual tests.
> 
> Patches 8 through 10 implement a system to ensure the nvme tests
> clean themselves up properly especially when ctrl-c is used to
> interrupt a test (working with the tests before this was a huge
> pain seeing,  when you ctrl-c, you have to either manually clean
> up the nvmet configuration or reboot to get the system in a state
> where it's sane to run the tests again).
> 
> Patches 11 and 12 make some minor changes that allow running the
> tests with the nvme modules built into the kernel.
> 
> With these patches, plus a couple I've sent to the nvme list for the
> kernel, I can consistently pass the entire nvme suite with the
> exception of the lockdep false-positive with nvme/012 that still
> seems to be in a bit of contention[3].
> 
> Thanks,
> 
> Logan
> 
> [1] https://github.com/osandov/blktests/pull/34
> [2] https://lore.kernel.org/linux-block/20190505150611.15776-4-minwoo.im.dev@gmail.com/
> [3] https://lore.kernel.org/lkml/20190214230058.196511-22-bvanassche@acm.org/
> 
> --
> 
> Logan Gunthorpe (11):
>   nvme: More agressively filter the discovery output
>   nvme: Add new test to verify the generation counter
>   nvme/003,004: Add missing calls to nvme disconnect
>   nvme/005: Don't rely on modprobing to set the multipath paramater
>   nvme/015: Ensure the namespace is flushed not the char device
>   nvme/018: Ignore error message generated by nvme read
>   check: Add the ability to call a cleanup function after a test ends
>   nvme: Cleanup modprobe lines into helper functions
>   nvme: Ensure all ports and subsystems are removed on cleanup
>   common: Use sysfs instead of modinfo for _have_module_param()
>   nvme: Ignore errors when removing modules
> 
> Michael Moese (1):
>   Add filter function for nvme discover
> 
>  check              |    9 +
>  common/rc          |   18 +-
>  tests/nvme/002     |   10 +-
>  tests/nvme/002.out | 6003 +-------------------------------------------
>  tests/nvme/003     |    6 +-
>  tests/nvme/003.out |    1 +
>  tests/nvme/004     |    6 +-
>  tests/nvme/004.out |    1 +
>  tests/nvme/005     |   16 +-
>  tests/nvme/006     |    6 +-
>  tests/nvme/007     |    6 +-
>  tests/nvme/008     |    6 +-
>  tests/nvme/009     |    5 +-
>  tests/nvme/010     |    6 +-
>  tests/nvme/011     |    6 +-
>  tests/nvme/012     |    6 +-
>  tests/nvme/013     |    6 +-
>  tests/nvme/014     |    6 +-
>  tests/nvme/015     |    5 +-
>  tests/nvme/016     |    6 +-
>  tests/nvme/016.out |    9 +-
>  tests/nvme/017     |    8 +-
>  tests/nvme/017.out |    9 +-
>  tests/nvme/018     |    8 +-
>  tests/nvme/019     |    6 +-
>  tests/nvme/020     |    5 +-
>  tests/nvme/021     |    6 +-
>  tests/nvme/022     |    6 +-
>  tests/nvme/023     |    6 +-
>  tests/nvme/024     |    6 +-
>  tests/nvme/025     |    6 +-
>  tests/nvme/026     |    6 +-
>  tests/nvme/027     |    6 +-
>  tests/nvme/028     |    6 +-
>  tests/nvme/029     |    6 +-
>  tests/nvme/030     |   72 +
>  tests/nvme/030.out |    2 +
>  tests/nvme/rc      |   64 +
>  38 files changed, 208 insertions(+), 6163 deletions(-)
>  create mode 100755 tests/nvme/030
>  create mode 100644 tests/nvme/030.out

Thanks for cleaning this up! I replied with one nitpick, and besides
that and comments from the other reviewers, I'm happy with it overall
(assuming it passes shellcheck).

Re: [PATCH blktests 00/12] Fix nvme block test issues

[Logan Gunthorpe]

On 2019-07-15 5:14 p.m., Omar Sandoval wrote:
> 
> Thanks for cleaning this up! I replied with one nitpick, and besides
> that and comments from the other reviewers, I'm happy with it overall
> (assuming it passes shellcheck).

Thanks, yes my sed skills are obviously not as good as yours. I'll fix
that nit up for v2 which I'll send in a couple days.

I have run "make check" on the series and it does pass.

Logan