[PATCH blktests 0/4] Four blktests patches

[PATCH blktests 0/4] Four blktests patches

[Bart Van Assche]

Hi Omar,

This series includes one improvement for the NVMe tests, two improvements for
the NVMeOF-multipath tests and version two of the SRP test that triggers a SCSI
reset while I/O is ongoing. Please consider these for the official blktests
git repository.

Thanks,

Bart.

Bart Van Assche (4):
  tests/nvme/rc: Modify the approach for disabling and re-enabling
    Ctrl-C
  tests/nvmeof-mp/rc: Make simulate_network_failure_loop() more robust
  tests/nvmeof-mp/rc: Make the NVMeOF multipath tests more reliable
  tests/srp/014: Add a test that triggers a SCSI reset while I/O is
    ongoing

 tests/nvme/rc      |   4 +-
 tests/nvmeof-mp/rc |   7 +--
 tests/srp/014      | 107 +++++++++++++++++++++++++++++++++++++++++++++
 tests/srp/014.out  |   2 +
 4 files changed, 115 insertions(+), 5 deletions(-)
 create mode 100755 tests/srp/014
 create mode 100644 tests/srp/014.out

-- 
2.22.0.770.g0f2c4a37fd-goog

[PATCH blktests 1/4] tests/nvme/rc: Modify the approach for disabling and re-enabling Ctrl-C

[Bart Van Assche]

Avoid that the following error messages are reported when redirecting stdin:

stty: 'standard input': Inappropriate ioctl for device
stty: 'standard input': Inappropriate ioctl for device

Cc: Logan Gunthorpe <logang@deltatee.com>
Cc: Johannes Thumshirn <jthumshirn@suse.de>
Fixes: a987b10bc179 ("nvme: Ensure all ports and subsystems are removed on cleanup")
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 tests/nvme/rc | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/nvme/rc b/tests/nvme/rc
index d4e18e635dea..40f0413d32d2 100644
--- a/tests/nvme/rc
+++ b/tests/nvme/rc
@@ -36,7 +36,7 @@ _cleanup_nvmet() {
 	fi
 
 	# Don't let successive Ctrl-Cs interrupt the cleanup processes
-	stty -isig
+	trap '' SIGINT
 
 	shopt -s nullglob
 
@@ -66,7 +66,7 @@ _cleanup_nvmet() {
 	done
 
 	shopt -u nullglob
-	stty isig
+	trap SIGINT
 
 	modprobe -r nvme-loop 2>/dev/null
 	modprobe -r nvmet 2>/dev/null
-- 
2.22.0.770.g0f2c4a37fd-goog

Re: [PATCH blktests 1/4] tests/nvme/rc: Modify the approach for disabling and re-enabling Ctrl-C

[Logan Gunthorpe]

On 2019-08-08 2:05 p.m., Bart Van Assche wrote:
> Avoid that the following error messages are reported when redirecting stdin:
> 
> stty: 'standard input': Inappropriate ioctl for device
> stty: 'standard input': Inappropriate ioctl for device
> 
> Cc: Logan Gunthorpe <logang@deltatee.com>
> Cc: Johannes Thumshirn <jthumshirn@suse.de>
> Fixes: a987b10bc179 ("nvme: Ensure all ports and subsystems are removed on cleanup")
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  tests/nvme/rc | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/nvme/rc b/tests/nvme/rc
> index d4e18e635dea..40f0413d32d2 100644
> --- a/tests/nvme/rc
> +++ b/tests/nvme/rc
> @@ -36,7 +36,7 @@ _cleanup_nvmet() {
>  	fi
>  
>  	# Don't let successive Ctrl-Cs interrupt the cleanup processes
> -	stty -isig
> +	trap '' SIGINT

Did you test this? Pretty sure I tried using trap and it didn't work,
probably because it's already running inside a trapped SIGINT.

Maybe it'd be better to just ignore any errors stty produces and pipe to
/dev/null?

Logan

Re: [PATCH blktests 1/4] tests/nvme/rc: Modify the approach for disabling and re-enabling Ctrl-C

[Bart Van Assche]

On 8/8/19 1:08 PM, Logan Gunthorpe wrote:
> On 2019-08-08 2:05 p.m., Bart Van Assche wrote:
>> Avoid that the following error messages are reported when redirecting stdin:
>>
>> stty: 'standard input': Inappropriate ioctl for device
>> stty: 'standard input': Inappropriate ioctl for device
>>
>> Cc: Logan Gunthorpe <logang@deltatee.com>
>> Cc: Johannes Thumshirn <jthumshirn@suse.de>
>> Fixes: a987b10bc179 ("nvme: Ensure all ports and subsystems are removed on cleanup")
>> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>> ---
>>   tests/nvme/rc | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/tests/nvme/rc b/tests/nvme/rc
>> index d4e18e635dea..40f0413d32d2 100644
>> --- a/tests/nvme/rc
>> +++ b/tests/nvme/rc
>> @@ -36,7 +36,7 @@ _cleanup_nvmet() {
>>   	fi
>>   
>>   	# Don't let successive Ctrl-Cs interrupt the cleanup processes
>> -	stty -isig
>> +	trap '' SIGINT
> 
> Did you test this? Pretty sure I tried using trap and it didn't work,
> probably because it's already running inside a trapped SIGINT.
> 
> Maybe it'd be better to just ignore any errors stty produces and pipe to
> /dev/null?

Hi Logan,

I don't think that redirecting the stty errors would be sufficient 
because Ctrl-C still works even if stdin, stdout and stderr are 
redirected. A command like sleep 60 </dev/null >&/dev/null can be 
interrupted with Ctrl-C but stty -isig >&/dev/null does not suppress 
Ctrl-C if stdin is redirected.

Are there other trap SIGINT statements in the blktests code? Does that 
mean that I overlooked something?

Thanks,

Bart.

Re: [PATCH blktests 1/4] tests/nvme/rc: Modify the approach for disabling and re-enabling Ctrl-C

[Logan Gunthorpe]

On 2019-08-08 3:00 p.m., Bart Van Assche wrote:
> On 8/8/19 1:08 PM, Logan Gunthorpe wrote:
>> On 2019-08-08 2:05 p.m., Bart Van Assche wrote:
>>> Avoid that the following error messages are reported when redirecting
>>> stdin:
>>>
>>> stty: 'standard input': Inappropriate ioctl for device
>>> stty: 'standard input': Inappropriate ioctl for device
>>>
>>> Cc: Logan Gunthorpe <logang@deltatee.com>
>>> Cc: Johannes Thumshirn <jthumshirn@suse.de>
>>> Fixes: a987b10bc179 ("nvme: Ensure all ports and subsystems are
>>> removed on cleanup")
>>> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>>> ---
>>>   tests/nvme/rc | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/tests/nvme/rc b/tests/nvme/rc
>>> index d4e18e635dea..40f0413d32d2 100644
>>> --- a/tests/nvme/rc
>>> +++ b/tests/nvme/rc
>>> @@ -36,7 +36,7 @@ _cleanup_nvmet() {
>>>       fi
>>>         # Don't let successive Ctrl-Cs interrupt the cleanup processes
>>> -    stty -isig
>>> +    trap '' SIGINT
>>
>> Did you test this? Pretty sure I tried using trap and it didn't work,
>> probably because it's already running inside a trapped SIGINT.
>>
>> Maybe it'd be better to just ignore any errors stty produces and pipe to
>> /dev/null?
> 
> Hi Logan,
> 
> I don't think that redirecting the stty errors would be sufficient
> because Ctrl-C still works even if stdin, stdout and stderr are
> redirected. A command like sleep 60 </dev/null >&/dev/null can be
> interrupted with Ctrl-C but stty -isig >&/dev/null does not suppress
> Ctrl-C if stdin is redirected.

Ok, actually I just tested your change and it does work. I must have
done something slightly differently when I first tried it (I think I
added a handler which echoed messages which still let the SIGINTs
through to child processes but it works with the null string).

So:

Reviewed-by: Logan Gunthorpe <logang@deltatee.com>

> Are there other trap SIGINT statements in the blktests code? Does that
> mean that I overlooked something?

The main code traps EXIT which calls the cleanup handler that this trap
then overrides SIGINT with, so I'm not really sure how they interact.

Thanks,

Logan

Re: [PATCH blktests 1/4] tests/nvme/rc: Modify the approach for disabling and re-enabling Ctrl-C

[Bart Van Assche]

On 8/8/19 2:11 PM, Logan Gunthorpe wrote:
> The main code traps EXIT which calls the cleanup handler that this trap
> then overrides SIGINT with, so I'm not really sure how they interact.

Hi Logan,

You may want to know that there is already code in the blktests project 
that modifies a trap handler from inside a trap handler:

	trap 'trap "" EXIT; teardown' EXIT

Bart.

[PATCH blktests 2/4] tests/nvmeof-mp/rc: Make simulate_network_failure_loop() more robust

[Bart Van Assche]

Avoid that the following is logged in the nvmeof-mp .full log files:

ls: cannot access '/sys/class/nvme/*/device/*/nvme0n1/reset_controller': No such
file or directory
tests/nvmeof-mp/rc: line 124: : No such file or directory

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 tests/nvmeof-mp/rc | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tests/nvmeof-mp/rc b/tests/nvmeof-mp/rc
index 9324dd1e8e4f..7352b1628cd3 100755
--- a/tests/nvmeof-mp/rc
+++ b/tests/nvmeof-mp/rc
@@ -117,8 +117,9 @@ simulate_network_failure_loop() {
 	while [ $rc = 0 ]; do
 		sleep_until 5 ${deadline} || break
 		for d in $(held_by "$dev"); do
-			sf=$(ls -d /sys/class/nvme/*/device/*/"${d#/dev/}/reset_controller")
-			echo 1 > "$sf"
+			for sf in /sys/class/nvme/*/device/*/"${d#/dev/}/reset_controller"; do
+				[ -e "$sf" ] && echo 1 > "$sf"
+			done
 		done
 	done 2>>"$FULL"
 
-- 
2.22.0.770.g0f2c4a37fd-goog

[PATCH blktests 3/4] tests/nvmeof-mp/rc: Make the NVMeOF multipath tests more reliable

[Bart Van Assche]

Fix the following failure for tests 002, 003, 004 and 011:

+tests/nvmeof-mp/rc: line 99: echo: write error: Operation already in progress

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 tests/nvmeof-mp/rc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/nvmeof-mp/rc b/tests/nvmeof-mp/rc
index 7352b1628cd3..2493fcee12de 100755
--- a/tests/nvmeof-mp/rc
+++ b/tests/nvmeof-mp/rc
@@ -124,7 +124,7 @@ simulate_network_failure_loop() {
 	done 2>>"$FULL"
 
 	for ((i=0;i<5;i++)); do
-		log_in && break
+		log_in 2>/dev/null && break
 		sleep 1
 	done
 }
-- 
2.22.0.770.g0f2c4a37fd-goog

[PATCH blktests 4/4] tests/srp/014: Add a test that triggers a SCSI reset while I/O is ongoing

[Bart Van Assche]

This test triggers the task management function handling code in the SRP
initiator and target drivers.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 tests/srp/014     | 107 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/srp/014.out |   2 +
 2 files changed, 109 insertions(+)
 create mode 100755 tests/srp/014
 create mode 100644 tests/srp/014.out

diff --git a/tests/srp/014 b/tests/srp/014
new file mode 100755
index 000000000000..8ecd8a439a82
--- /dev/null
+++ b/tests/srp/014
@@ -0,0 +1,107 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0+
+# Copyright (c) 2016-2018 Western Digital Corporation or its affiliates
+#
+# Submit SCSI resets while a fio --verify job is running. This is a regression
+# test for Linux kernel commit fd5614124406 ("scsi: RDMA/srp: Fix a
+# sleep-in-invalid-context bug") # v5.3.
+
+. tests/srp/rc
+
+DESCRIPTION="Run sg_reset while I/O is ongoing"
+TIMED=1
+
+# $1: SCSI device of which to change the state to running, e.g. sdc.
+make_running() {
+	local dev=$1 sp state
+
+	for sp in /sys/class/scsi_device/*/device/block/"$dev"; do
+		if [ -e "$sp" ]; then
+			break
+		else
+			return 1
+		fi
+	done
+	sp=$(dirname "$(dirname "$sp")")/state
+	# If the SCSI error handler changed the device state to offline,
+	# change the state back to running.
+	state=$(<"$sp")
+	if [ "$state" = offline ]; then
+		echo running > "$sp"
+		echo "$dev: state $state -> running" >>"$FULL"
+	else
+		echo "$dev: state $state" >>"$FULL"
+	fi
+}
+
+# $1: dm device to examine, e.g.
+# /dev/disk/by-id/dm-uuid-mpath-360014056e756c6c62300000000000000
+make_all_running() {
+	local d h dev=$1
+
+	while [ -L "$dev" ]; do
+		dev=$(realpath "$dev")
+	done
+	dev=${dev#/dev/}
+	for h in /sys/class/block/*/holders/"$dev"; do
+		[ -e "$h" ] || continue
+		d=$(basename "$(dirname "$(dirname "$h")")")
+		make_running "$d"
+	done
+}
+
+# $1: dm device to act on.
+set_running_loop() {
+	local dev="$1"
+
+	[ -e "$dev" ] || return $?
+	while true; do
+		sleep 1
+		make_all_running "$dev"
+	done
+	echo "set_running_loop $dev finished" >>"$FULL"
+}
+
+# $1: dm device to reset periodically; $2: how long the reset loop should run.
+sg_reset_loop() {
+	local cmd dev="$1" duration="$2" deadline i=0 reset_type
+
+	[ -e "$dev" ] || return $?
+	[ -n "$duration" ] || return $?
+	reset_type=(-d -b)
+	deadline=$(($(uptime_s) + duration))
+	while true; do
+		sleep_until 1 ${deadline} || break
+		cmd="sg_reset --no-esc ${reset_type[i++ % 2]} $dev"
+		{ echo "+ $cmd"; eval "$cmd"; } >>"$FULL" 2>&1
+	done
+	echo "sg_reset_loop $dev finished" >>"$FULL"
+}
+
+test_sg_reset() {
+	local dev fio_status m job
+
+	use_blk_mq y y || return $?
+	dev=$(get_bdev 0) || return $?
+	sg_reset_loop "$dev" "$TIMEOUT" &
+	# Redirect stderr to suppress the bash "Terminated" message.
+	set_running_loop "$dev" 2>/dev/null &
+	job=$!
+	run_fio --verify=md5 --rw=randwrite --bs=64K --loops=$((10**6)) \
+		--iodepth=16 --group_reporting --sync=1 --direct=1 \
+		--ioengine=libaio --runtime="${TIMEOUT}" \
+		--filename="$dev" --name=sg-reset-test --thread --numjobs=1 \
+		--output="${RESULTS_DIR}/srp/fio-output-014.txt" \
+		>>"$FULL"
+	fio_status=$?
+	kill "$job"
+	make_all_running "$dev"
+	wait
+	return $fio_status
+}
+
+test() {
+	: "${TIMEOUT:=30}"
+	trap 'trap "" EXIT; teardown' EXIT
+	setup && test_sg_reset && echo Passed
+}
diff --git a/tests/srp/014.out b/tests/srp/014.out
new file mode 100644
index 000000000000..5e25d8e8672d
--- /dev/null
+++ b/tests/srp/014.out
@@ -0,0 +1,2 @@
+Configured SRP target driver
+Passed
-- 
2.22.0.770.g0f2c4a37fd-goog

Re: [PATCH blktests 0/4] Four blktests patches

[Bart Van Assche]

On 8/8/19 1:05 PM, Bart Van Assche wrote:
> Hi Omar,
> 
> This series includes one improvement for the NVMe tests, two improvements for
> the NVMeOF-multipath tests and version two of the SRP test that triggers a SCSI
> reset while I/O is ongoing. Please consider these for the official blktests
> git repository.

(replying to my own e-mail)

Hi Omar,

This patch series was posted about one month ago. Feedback about how to
proceed with this patch series is still welcome.

Thanks,

Bart.

Re: [PATCH blktests 0/4] Four blktests patches

[Omar Sandoval]

On Wed, Sep 04, 2019 at 10:49:13AM -0700, Bart Van Assche wrote:
> On 8/8/19 1:05 PM, Bart Van Assche wrote:
> > Hi Omar,
> > 
> > This series includes one improvement for the NVMe tests, two improvements for
> > the NVMeOF-multipath tests and version two of the SRP test that triggers a SCSI
> > reset while I/O is ongoing. Please consider these for the official blktests
> > git repository.
> 
> (replying to my own e-mail)
> 
> Hi Omar,
> 
> This patch series was posted about one month ago. Feedback about how to
> proceed with this patch series is still welcome.

I was actually just in the middle of merging it. Pushed now, thanks and
sorry for the delay.