Skip to contentSkip to conversation

tests/nvme: Add admin-passthru+reset race test

PATCH Superseded Jonathan Derrick 6 participants 7 messages Nov 14, 2022 – Nov 16, 2022

Subsystems (2)

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 tests/nvme: Add admin-passthru+reset race test

Adds a test which runs many formats and reset_controllers in parallel. The intent is to expose timing holes in the controller state machine which will lead to hung task timing and the controller becoming unavailable.

Reported by https://bugzilla.kernel.org/show_bug.cgi?id=216354

---
 tests/nvme/046     | 85 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/nvme/046.out |  2 ++
 2 files changed, 87 insertions(+)
 create mode 100755 tests/nvme/046
 create mode 100644 tests/nvme/046.out
tests/nvme/046 new file +85 -0
new file mode 100755
index 0000000..4b47783
@@ -0,0 +1,85 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-3.0+
+# Copyright (C) 2022 Jonathan Derrick <[email protected]>
+#
+# Test nvme reset controller during admin passthru
+#
+# Regression for issue reported by
+# https://bugzilla.kernel.org/show_bug.cgi?id=216354
+
+. tests/nvme/rc
+
+#restrict test to nvme-pci only
+nvme_trtype=pci
+
+DESCRIPTION="test nvme reset controller during admin passthru"
+QUICK=1
+CAN_BE_ZONED=1
+
+requires() {
+	_nvme_requires
+}
+
+device_requires() {
+	_require_test_dev_is_nvme
+}
+
+test_device() {
+	echo "Running ${TEST_NAME}"
+
+	local sysfs
+	local attr
+	local m
+
+	sysfs="$TEST_DEV_SYSFS/device"
+	timeout=$(($(cat /proc/sys/kernel/hung_task_timeout_secs) / 2))
+
+	sleep 5
+
+	if [[ ! -d "$sysfs" ]]; then
+		echo "$sysfs doesn't exist"
+	fi
+
+	# do reset controller/format loops
+	# don't check status now because a timing race is desired
+	i=0
+	start=0
+	timing_out=false
+	while [[ $i -le 1000 ]]; do
+		start=$SECONDS
+		if [[ -f "$sysfs/reset_controller" ]]; then
+			echo 1 > "$sysfs/reset_controller" 2>/dev/null &
+			i=$((i+1))
+		fi
+		nvme format -l 0 -f $TEST_DEV 2>/dev/null &
+
+		#Assume the controller is hung and unrecoverable
+		if [[ $(($SECONDS - $start)) -gt $timeout ]]; then
+			echo "nvme controller timing out"
+			timing_out=true
+			break
+		fi
+	done
+
+	{ kill $!; wait; } &> /dev/null
+
+	# at this point it may have waited hung_task_timeout / 2 already, so
+	# only wait 25% longer for a total of about 75% of allowed timeout
+	m=0
+	while [[ $m -le $((timeout / 2)) ]]; do
+		if [[ $timing_out == true ]]; then
+			break
+		fi
+		if grep -q live "$sysfs/state"; then
+			break
+		fi
+		sleep 1
+		m=$((m+1))
+	done
+	if ! grep -q live "$sysfs/state"; then
+		echo "nvme still not live after $(($SECONDS - $start)) seconds!"
+	fi
+	udevadm settle
+
+	echo "Test complete"
+}
tests/nvme/046.out new file +2 -0
new file mode 100644
index 0000000..2b5fa6a
@@ -0,0 +1,2 @@
+Running nvme/046
+Test complete
Signature · 1 line
2.31.1
Reviewed-by on v1 1/1
#1 lore raw
Quoted 9 lines · Jonathan Derrick

Adds a test which runs many formats and reset_controllers in parallel. The intent is to expose timing holes in the controller state machine which will lead to hung task timing and the controller becoming unavailable.

Reported by https://bugzilla.kernel.org/show_bug.cgi?id=216354

---

Thanks a lot for submitting this, it is really useful...

Overall it looks good.

-ck

#2 lore raw
Quoted 11 lines · Chaitanya Kulkarni
Quoted 9 lines · Jonathan Derrick

Adds a test which runs many formats and reset_controllers in parallel. The intent is to expose timing holes in the controller state machine which will lead to hung task timing and the controller becoming unavailable.

Reported by https://bugzilla.kernel.org/show_bug.cgi?id=216354

---

Quoted 8 lines

Thanks a lot for submitting this, it is really useful...

Overall it looks good.

-ck

Thanks
I have v2 coming with a bit more robustness, so let's not merge this one just yet

#3 lore raw
Quoted 18 lines · Jonathan Derrick
Quoted 15 lines · Chaitanya Kulkarni
Quoted 9 lines · Jonathan Derrick

Adds a test which runs many formats and reset_controllers in parallel. The intent is to expose timing holes in the controller state machine which will lead to hung task timing and the controller becoming unavailable.

Reported by https://bugzilla.kernel.org/show_bug.cgi?id=216354

---

Thanks a lot for submitting this, it is really useful...

Overall it looks good.

Quoted 7 lines
Quoted 4 lines

-ck

Thanks
I have v2 coming with a bit more robustness, so let's not merge this one just yet

This test case looks valuable. Thanks. When you post v2, would you mind renumbering the test case from nvme/046 to nvme/047? Another new test case of the nvme test group was posted [1]. Also, your check with "make check" command will be appreciated. Shellcheck has some complaints:

$ make check
shellcheck -x -e SC2119 -f gcc check new common/* \
        tests/*/rc tests/*/[0-9]*[0-9]
tests/nvme/046:31:8: warning: attr appears unused. Verify use (or export if used externally). [SC2034]
tests/nvme/046:54:23: note: Double quote to prevent globbing and word splitting. [SC2086]
tests/nvme/046:57:12: note: $/${} is unnecessary on arithmetic variables. [SC2004]
tests/nvme/046:57:23: note: $/${} is unnecessary on arithmetic variables. [SC2004]
tests/nvme/046:80:38: note: $/${} is unnecessary on arithmetic variables. [SC2004]
tests/nvme/046:80:49: note: $/${} is unnecessary on arithmetic variables. [SC2004]
make: *** [Makefile:21: check] Error 1

[1] https://lore.kernel.org/linux-nvme/[email protected]/

Signature · 1 line
Shin'ichiro Kawasaki
#4 lore raw
In reply to Jonathan Derrick
I love your patch! Perhaps something to improve:

Hi Jonathan,

I love your patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v6.1-rc5 next-20221114]
[If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Jonathan-Derrick/tests-nvme-Add-admin-passthru-reset-race-test/20221115-043752
patch link:    https://lore.kernel.org/r/20221114203412.383-1-jonathan.derrick%40linux.dev
patch subject: [PATCH] tests/nvme: Add admin-passthru+reset race test

reproduce:
scripts/spdxcheck.py

If you fix the issue, kindly add following tag where applicable | Reported-by: kernel test robot <[email protected]>

spdxcheck warnings: (new ones prefixed by >>)
drivers/cpufreq/amd-pstate-ut.c: 1:28 Invalid License ID: GPL-1.0-or-later

>> tests/nvme/046: 2:27 Invalid License ID: GPL-3.0+
Signature · 2 lines
0-DAY CI Kernel Test Service
https://01.org/lkp
maintainer
#5 lore raw
In reply to Jonathan Derrick

tests/nvme/046 line 34

Quoted 7 lines · Jonathan Derrick
tests/nvme/046 new file +7 -0
+	echo "Running ${TEST_NAME}"
+
+	local sysfs
+	local attr
+	local m
+
+	sysfs="$TEST_DEV_SYSFS/device"

That's not the correct directory when the device is using native nvme-multipath.

Quoted 18 lines
tests/nvme/046 new file +18 -0
+	timeout=$(($(cat /proc/sys/kernel/hung_task_timeout_secs) / 2))
+
+	sleep 5
+
+	if [[ ! -d "$sysfs" ]]; then
+		echo "$sysfs doesn't exist"
+	fi
+
+	# do reset controller/format loops
+	# don't check status now because a timing race is desired
+	i=0
+	start=0
+	timing_out=false
+	while [[ $i -le 1000 ]]; do
+		start=$SECONDS
+		if [[ -f "$sysfs/reset_controller" ]]; then
+			echo 1 > "$sysfs/reset_controller" 2>/dev/null &
+			i=$((i+1))

tests/nvme/046 line 62

Quoted 10 lines
tests/nvme/046 new file +10 -0
+		fi
+		nvme format -l 0 -f $TEST_DEV 2>/dev/null &
+
+		#Assume the controller is hung and unrecoverable
+		if [[ $(($SECONDS - $start)) -gt $timeout ]]; then
+			echo "nvme controller timing out"
+			timing_out=true
+			break
+		fi
+	done

If the controller is already undergoing a reset, then writing to the reset_controller file becomes a no-op. Unless your "reset_controller" completes near instantaneously, I find that this loop tears through 1000 iterations, forks 1000 formats, and only 1 reset_controller actually gets through.

If I remove the upper limit, then I can also see the stalled task, but it is only temporary and gets itself out of it after the admin timeout (1 minute). Is that also your observation, or is it stuck forever?

#6 lore raw
In reply to Jonathan Derrick
Quoted 4 lines · Jonathan Derrick

Adds a test which runs many formats and reset_controllers in parallel. The intent is to expose timing holes in the controller state machine which will lead to hung task timing and the controller becoming unavailable.

This passes just fine for me both on qemu and a thunderbolt attached m.2 SSD. So it seems to be hardware / timing dependent.