[PATCH blktests v2 0/4] Add a test that triggers the blk_mq_realloc_hw_ctxs() error path

[PATCH blktests v2 0/4] Add a test that triggers the blk_mq_realloc_hw_ctxs() error path

[Bart Van Assche]

Hi Omar,

Please consider this patch series for the upstream blktests repository.

Compared to v1, three patches have been added that refactor null_blk
loading, unloading and configuration.

Thanks,

Bart.

Bart Van Assche (4):
  Make _exit_null_blk remove all null_blk device instances
  Use _{init,exit}_null_blk instead of open-coding these functions
  Introduce the function _configure_null_blk()
  Add a test that triggers the blk_mq_realloc_hw_ctxs() error path

 common/multipath-over-rdma | 29 ++++++++------------------
 common/null_blk            | 26 +++++++++++++++++++----
 tests/block/022            |  3 ---
 tests/block/029            | 17 ++-------------
 tests/block/030            | 42 ++++++++++++++++++++++++++++++++++++++
 tests/block/030.out        |  1 +
 tests/nvmeof-mp/rc         |  2 +-
 7 files changed, 76 insertions(+), 44 deletions(-)
 create mode 100755 tests/block/030
 create mode 100644 tests/block/030.out

[PATCH blktests v2 1/4] Make _exit_null_blk remove all null_blk device instances

[Bart Van Assche]

Instead of making every test remove null_blk device instances before calling
_exit_null_blk(), move the null_blk device instance removal code into
_exit_null_blk().

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 common/null_blk | 12 ++++++++----
 tests/block/022 |  3 ---
 tests/block/029 |  1 -
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/common/null_blk b/common/null_blk
index 2e300c20bbc7..6a5f99aaae9d 100644
--- a/common/null_blk
+++ b/common/null_blk
@@ -8,11 +8,14 @@ _have_null_blk() {
 	_have_modules null_blk
 }
 
+_remove_null_blk_devices() {
+	local d
+
+	for d in /sys/kernel/config/nullb/*; do [ -d "$d" ] && rmdir "$d"; done
+}
+
 _init_null_blk() {
-	if [[ -d /sys/kernel/config/nullb ]]; then
-		find /sys/kernel/config/nullb -mindepth 1 -maxdepth 1 \
-		     -type d -delete
-	fi
+	_remove_null_blk_devices
 
 	local zoned=""
 	if (( RUN_FOR_ZONED )); then zoned="zoned=1"; fi
@@ -27,5 +30,6 @@ _init_null_blk() {
 
 _exit_null_blk() {
 	udevadm settle
+	_remove_null_blk_devices
 	modprobe -r null_blk
 }
diff --git a/tests/block/022 b/tests/block/022
index 1404aacef295..b2c53e266d81 100755
--- a/tests/block/022
+++ b/tests/block/022
@@ -50,9 +50,6 @@ test() {
 		wait $pid1
 	} 2>/dev/null
 
-	rmdir /sys/kernel/config/nullb/1
-	rmdir /sys/kernel/config/nullb/0
-
 	_exit_null_blk
 	echo "Test complete"
 }
diff --git a/tests/block/029 b/tests/block/029
index d298bac8db5c..0d521edb0cf6 100755
--- a/tests/block/029
+++ b/tests/block/029
@@ -58,7 +58,6 @@ test() {
 	else
 		echo "Skipping test because $sq cannot be modified" >>"$FULL"
 	fi
-	rmdir /sys/kernel/config/nullb/nullb0
 	_exit_null_blk
 	echo Passed
 }

Re: [PATCH blktests v2 1/4] Make _exit_null_blk remove all null_blk device instances

[Chaitanya Kulkarni]

Looks good, except the commit log is > 80 char, can be fixed at
the time of applying the patch.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
On 03/15/2020 03:13 PM, Bart Van Assche wrote:
> Instead of making every test remove null_blk device instances before calling
> _exit_null_blk(), move the null_blk device instance removal code into
> _exit_null_blk().
>
> Signed-off-by: Bart Van Assche<bvanassche@acm.org>

Re: [PATCH blktests v2 1/4] Make _exit_null_blk remove all null_blk device instances

[Bart Van Assche]

On 2020-03-15 16:22, Chaitanya Kulkarni wrote:
> Looks good, except the commit log is > 80 char, can be fixed at
> the time of applying the patch.
> 
> Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>

Thanks for the reviews, but I have two comments:
* The commit message is only 76 characters wide.
* Please do not top-post. From https://en.wikipedia.org/wiki/Posting_style:

A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?

Thanks,

Bart.

Re: [PATCH blktests v2 1/4] Make _exit_null_blk remove all null_blk device instances

[Omar Sandoval]

On Sun, Mar 15, 2020 at 03:13:17PM -0700, Bart Van Assche wrote:
> Instead of making every test remove null_blk device instances before calling
> _exit_null_blk(), move the null_blk device instance removal code into
> _exit_null_blk().
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  common/null_blk | 12 ++++++++----
>  tests/block/022 |  3 ---
>  tests/block/029 |  1 -
>  3 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/common/null_blk b/common/null_blk
> index 2e300c20bbc7..6a5f99aaae9d 100644
> --- a/common/null_blk
> +++ b/common/null_blk
> @@ -8,11 +8,14 @@ _have_null_blk() {
>  	_have_modules null_blk
>  }
>  
> +_remove_null_blk_devices() {
> +	local d
> +
> +	for d in /sys/kernel/config/nullb/*; do [ -d "$d" ] && rmdir "$d"; done

I'd prefer to keep the deletion code using find from _init_null_blk.

> +}
> +
>  _init_null_blk() {
> -	if [[ -d /sys/kernel/config/nullb ]]; then
> -		find /sys/kernel/config/nullb -mindepth 1 -maxdepth 1 \
> -		     -type d -delete
> -	fi
> +	_remove_null_blk_devices
>  
>  	local zoned=""
>  	if (( RUN_FOR_ZONED )); then zoned="zoned=1"; fi
> @@ -27,5 +30,6 @@ _init_null_blk() {
>  
>  _exit_null_blk() {
>  	udevadm settle
> +	_remove_null_blk_devices

This needs to happen before the udevadm settle.

[PATCH blktests v2 2/4] Use _{init,exit}_null_blk instead of open-coding these functions

[Bart Van Assche]

This patch reduces code duplication.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 common/multipath-over-rdma | 12 +++---------
 tests/nvmeof-mp/rc         |  2 +-
 2 files changed, 4 insertions(+), 10 deletions(-)

diff --git a/common/multipath-over-rdma b/common/multipath-over-rdma
index 40efc4b3aa2e..a56e7a8269db 100644
--- a/common/multipath-over-rdma
+++ b/common/multipath-over-rdma
@@ -5,6 +5,7 @@
 # Functions and global variables used by both the srp and nvmeof-mp tests.
 
 . common/shellcheck
+. common/null_blk
 
 debug=
 filesystem_type=ext4
@@ -634,13 +635,6 @@ configure_null_blk() {
 	ls -l /dev/nullb* &>>"$FULL"
 }
 
-unload_null_blk() {
-	local d
-
-	for d in /sys/kernel/config/nullb/*; do [ -d "$d" ] && rmdir "$d"; done
-	unload_module null_blk
-}
-
 setup_rdma() {
 	start_soft_rdma
 	(
@@ -662,7 +656,7 @@ teardown_uncond() {
 	rm -f /etc/multipath.conf
 	stop_target
 	stop_soft_rdma
-	unload_null_blk
+	_exit_null_blk
 }
 
 teardown() {
@@ -698,7 +692,7 @@ setup_test() {
 		[ -e "/sys/module/$m" ] || modprobe "$m" || return $?
 	done
 
-	modprobe null_blk nr_devices=0 || return $?
+	_init_null_blk nr_devices=0 || return $?
 
 	configure_null_blk || return $?
 
diff --git a/tests/nvmeof-mp/rc b/tests/nvmeof-mp/rc
index 278843a1270d..f782bb3a02ac 100755
--- a/tests/nvmeof-mp/rc
+++ b/tests/nvmeof-mp/rc
@@ -278,7 +278,7 @@ stop_nvme_target() {
 	)
 	unload_module nvmet_rdma &&
 		unload_module nvmet &&
-		unload_null_blk
+		_exit_null_blk
 }
 
 start_target() {

Re: [PATCH blktests v2 2/4] Use _{init,exit}_null_blk instead of open-coding these functions

[Chaitanya Kulkarni]

LGTM.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>

On 03/15/2020 03:13 PM, Bart Van Assche wrote:
> This patch reduces code duplication.
>
> Signed-off-by: Bart Van Assche<bvanassche@acm.org>

[PATCH blktests v2 3/4] Introduce the function _configure_null_blk()

[Bart Van Assche]

Introduce a function for creating a null_blk device instance through
configfs.

Suggested-by: Chaitanya Kulkarni <Chaitanya.Kulkarni@wdc.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 common/multipath-over-rdma | 17 +++++------------
 common/null_blk            | 14 ++++++++++++++
 tests/block/029            | 16 ++--------------
 3 files changed, 21 insertions(+), 26 deletions(-)

diff --git a/common/multipath-over-rdma b/common/multipath-over-rdma
index a56e7a8269db..7e610a0ccbbb 100644
--- a/common/multipath-over-rdma
+++ b/common/multipath-over-rdma
@@ -620,18 +620,11 @@ run_fio() {
 configure_null_blk() {
 	local i
 
-	(
-		cd /sys/kernel/config/nullb || return $?
-		for i in nullb0 nullb1; do (
-			{ mkdir -p $i &&
-				  cd $i &&
-				  echo 0 > completion_nsec &&
-				  echo 512 > blocksize &&
-				  echo $((ramdisk_size>>20)) > size &&
-				  echo 1 > memory_backed &&
-				  echo 1 > power; } || exit $?
-		) done
-	)
+	for i in nullb0 nullb1; do
+		_configure_null_blk nullb$i completion_nsec=0 blocksize=512 \
+				    size=$((ramdisk_size>>20)) memory_backed=1 \
+				    power=1 || return $?
+	done
 	ls -l /dev/nullb* &>>"$FULL"
 }
 
diff --git a/common/null_blk b/common/null_blk
index 6a5f99aaae9d..3c31db1ed4ac 100644
--- a/common/null_blk
+++ b/common/null_blk
@@ -28,6 +28,20 @@ _init_null_blk() {
 	return 0
 }
 
+# Configure one null_blk instance with name $1 and parameters $2..${$#}.
+_configure_null_blk() {
+	local nullb=/sys/kernel/config/nullb/$1 param val
+
+	shift
+	mkdir "$nullb" &&
+		while [ -n "$1" ]; do
+			param="${1//=*}"
+			val="${1#${param}=}"
+			shift
+			echo "$val" > "$nullb/$param" || return $?
+		done
+}
+
 _exit_null_blk() {
 	udevadm settle
 	_remove_null_blk_devices
diff --git a/tests/block/029 b/tests/block/029
index 0d521edb0cf6..dbb582eab473 100755
--- a/tests/block/029
+++ b/tests/block/029
@@ -14,19 +14,6 @@ requires() {
 	_have_null_blk
 }
 
-# Configure one null_blk instance.
-configure_null_blk() {
-	local nullb0="/sys/kernel/config/nullb/nullb0"
-
-	mkdir "$nullb0" &&
-	echo 0 > "$nullb0/completion_nsec" &&
-	echo 512 > "$nullb0/blocksize" &&
-	echo 16 > "$nullb0/size" &&
-	echo 1 > "$nullb0/memory_backed" &&
-	echo 1 > "$nullb0/power" &&
-	ls -l /dev/nullb* &>>"$FULL"
-}
-
 modify_nr_hw_queues() {
 	local deadline num_cpus
 
@@ -45,7 +32,8 @@ test() {
 
 	: "${TIMEOUT:=30}"
 	_init_null_blk nr_devices=0 queue_mode=2 &&
-	configure_null_blk
+	_configure_null_blk nullb0 completion_nsec=0 blocksize=512 \
+			    size=16 memory_backed=1 power=1 &&
 	if { echo 1 >$sq; } 2>/dev/null; then
 		modify_nr_hw_queues &
 		fio --rw=randwrite --bs=4K --loops=$((10**6)) \

Re: [PATCH blktests v2 3/4] Introduce the function _configure_null_blk()

[Chaitanya Kulkarni]

LGTM.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>

On 03/15/2020 03:13 PM, Bart Van Assche wrote:
> Introduce a function for creating a null_blk device instance through
> configfs.
>
> Suggested-by: Chaitanya Kulkarni<Chaitanya.Kulkarni@wdc.com>
> Signed-off-by: Bart Van Assche<bvanassche@acm.org>

Re: [PATCH blktests v2 3/4] Introduce the function _configure_null_blk()

[Omar Sandoval]

On Sun, Mar 15, 2020 at 03:13:19PM -0700, Bart Van Assche wrote:
> Introduce a function for creating a null_blk device instance through
> configfs.
> 
> Suggested-by: Chaitanya Kulkarni <Chaitanya.Kulkarni@wdc.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  common/multipath-over-rdma | 17 +++++------------
>  common/null_blk            | 14 ++++++++++++++
>  tests/block/029            | 16 ++--------------
>  3 files changed, 21 insertions(+), 26 deletions(-)
> 
> diff --git a/common/multipath-over-rdma b/common/multipath-over-rdma
> index a56e7a8269db..7e610a0ccbbb 100644
> --- a/common/multipath-over-rdma
> +++ b/common/multipath-over-rdma
> @@ -620,18 +620,11 @@ run_fio() {
>  configure_null_blk() {
>  	local i
>  
> -	(
> -		cd /sys/kernel/config/nullb || return $?
> -		for i in nullb0 nullb1; do (
> -			{ mkdir -p $i &&
> -				  cd $i &&
> -				  echo 0 > completion_nsec &&
> -				  echo 512 > blocksize &&
> -				  echo $((ramdisk_size>>20)) > size &&
> -				  echo 1 > memory_backed &&
> -				  echo 1 > power; } || exit $?
> -		) done
> -	)
> +	for i in nullb0 nullb1; do
> +		_configure_null_blk nullb$i completion_nsec=0 blocksize=512 \
> +				    size=$((ramdisk_size>>20)) memory_backed=1 \
> +				    power=1 || return $?
> +	done
>  	ls -l /dev/nullb* &>>"$FULL"
>  }
>  
> diff --git a/common/null_blk b/common/null_blk
> index 6a5f99aaae9d..3c31db1ed4ac 100644
> --- a/common/null_blk
> +++ b/common/null_blk
> @@ -28,6 +28,20 @@ _init_null_blk() {
>  	return 0
>  }
>  
> +# Configure one null_blk instance with name $1 and parameters $2..${$#}.
> +_configure_null_blk() {
> +	local nullb=/sys/kernel/config/nullb/$1 param val
> +
> +	shift
> +	mkdir "$nullb" &&

Just make this mkdir "$nullb" || return $?

> +		while [ -n "$1" ]; do

The intention is clearer with while [[ $# > 0 ]]

> +			param="${1//=*}"
> +			val="${1#${param}=}"

These are all arcane bash incantations, but the following seem slighly
clearer to me:

param="${1%%=*}"
val="${1#*=}"

I.e., for param we remove the equal sign and everything after it, and
for val we remove everything up to the equal sign.

> +			shift
> +			echo "$val" > "$nullb/$param" || return $?
> +		done
> +}
> +

[PATCH blktests v2 4/4] Add a test that triggers the blk_mq_realloc_hw_ctxs() error path

[Bart Van Assche]

Add a test that triggers the code touched by commit d0930bb8f46b ("blk-mq:
Fix a recently introduced regression in blk_mq_realloc_hw_ctxs()"). This
test only runs if a recently added fault injection feature is available,
namely commit 596444e75705 ("null_blk: Add support for init_hctx() fault
injection").

Cc: Ming Lei <ming.lei@redhat.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 tests/block/030     | 42 ++++++++++++++++++++++++++++++++++++++++++
 tests/block/030.out |  1 +
 2 files changed, 43 insertions(+)
 create mode 100755 tests/block/030
 create mode 100644 tests/block/030.out

diff --git a/tests/block/030 b/tests/block/030
new file mode 100755
index 000000000000..861c85c27f09
--- /dev/null
+++ b/tests/block/030
@@ -0,0 +1,42 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright 2020 Google LLC
+#
+# Trigger the blk_mq_realloc_hw_ctxs() error path.
+
+. tests/block/rc
+. common/null_blk
+
+DESCRIPTION="trigger the blk_mq_realloc_hw_ctxs() error path"
+QUICK=1
+
+requires() {
+	_have_null_blk || return $?
+	_have_module_param null_blk init_hctx || return $?
+}
+
+test() {
+	local i sq=/sys/kernel/config/nullb/nullb0/submit_queues
+
+	: "${TIMEOUT:=30}"
+	if ! _init_null_blk nr_devices=0 queue_mode=2 "init_hctx=$(nproc),100,0,0"; then
+		echo "Loading null_blk failed"
+		return 1
+	fi
+	if ! _configure_null_blk nullb0 completion_nsec=0 blocksize=512 size=16\
+	     submit_queues="$(nproc)" memory_backed=1 power=1; then
+		echo "Configuring null_blk failed"
+		return 1
+	fi
+	if { echo "$(<"$sq")" >$sq; } 2>/dev/null; then
+		for ((i=0;i<100;i++)); do
+			echo 1 >$sq
+			nproc >$sq
+		done
+	else
+		echo "Skipping test because $sq cannot be modified" >>"$FULL"
+	fi
+	rmdir /sys/kernel/config/nullb/nullb0
+	_exit_null_blk
+	echo Passed
+}
diff --git a/tests/block/030.out b/tests/block/030.out
new file mode 100644
index 000000000000..863339fb8ced
--- /dev/null
+++ b/tests/block/030.out
@@ -0,0 +1 @@
+Passed

Re: [PATCH blktests v2 4/4] Add a test that triggers the blk_mq_realloc_hw_ctxs() error path

[Chaitanya Kulkarni]

LGTM.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>

On 03/15/2020 03:13 PM, Bart Van Assche wrote:
> Add a test that triggers the code touched by commit d0930bb8f46b ("blk-mq:
> Fix a recently introduced regression in blk_mq_realloc_hw_ctxs()"). This
> test only runs if a recently added fault injection feature is available,
> namely commit 596444e75705 ("null_blk: Add support for init_hctx() fault
> injection").
>
> Cc: Ming Lei<ming.lei@redhat.com>
> Signed-off-by: Bart Van Assche<bvanassche@acm.org>

Re: [PATCH blktests v2 4/4] Add a test that triggers the blk_mq_realloc_hw_ctxs() error path

[Omar Sandoval]

On Sun, Mar 15, 2020 at 03:13:20PM -0700, Bart Van Assche wrote:
> Add a test that triggers the code touched by commit d0930bb8f46b ("blk-mq:
> Fix a recently introduced regression in blk_mq_realloc_hw_ctxs()"). This
> test only runs if a recently added fault injection feature is available,
> namely commit 596444e75705 ("null_blk: Add support for init_hctx() fault
> injection").
> 
> Cc: Ming Lei <ming.lei@redhat.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  tests/block/030     | 42 ++++++++++++++++++++++++++++++++++++++++++
>  tests/block/030.out |  1 +
>  2 files changed, 43 insertions(+)
>  create mode 100755 tests/block/030
>  create mode 100644 tests/block/030.out
> 
> diff --git a/tests/block/030 b/tests/block/030
> new file mode 100755
> index 000000000000..861c85c27f09
> --- /dev/null
> +++ b/tests/block/030
> @@ -0,0 +1,42 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright 2020 Google LLC
> +#
> +# Trigger the blk_mq_realloc_hw_ctxs() error path.
> +
> +. tests/block/rc
> +. common/null_blk
> +
> +DESCRIPTION="trigger the blk_mq_realloc_hw_ctxs() error path"
> +QUICK=1
> +
> +requires() {
> +	_have_null_blk || return $?
> +	_have_module_param null_blk init_hctx || return $?
> +}
> +
> +test() {
> +	local i sq=/sys/kernel/config/nullb/nullb0/submit_queues
> +
> +	: "${TIMEOUT:=30}"
> +	if ! _init_null_blk nr_devices=0 queue_mode=2 "init_hctx=$(nproc),100,0,0"; then
> +		echo "Loading null_blk failed"
> +		return 1
> +	fi
> +	if ! _configure_null_blk nullb0 completion_nsec=0 blocksize=512 size=16\
> +	     submit_queues="$(nproc)" memory_backed=1 power=1; then
> +		echo "Configuring null_blk failed"
> +		return 1
> +	fi
> +	if { echo "$(<"$sq")" >$sq; } 2>/dev/null; then
> +		for ((i=0;i<100;i++)); do
> +			echo 1 >$sq
> +			nproc >$sq
> +		done
> +	else
> +		echo "Skipping test because $sq cannot be modified" >>"$FULL"

I just pushed the support for allowing skipping from the middle of a
test, so now you could make this

	SKIP_REASON="Skipping test because $sq cannot be modified"

> +	fi
> +	rmdir /sys/kernel/config/nullb/nullb0
> +	_exit_null_blk
> +	echo Passed
> +}
> diff --git a/tests/block/030.out b/tests/block/030.out
> new file mode 100644
> index 000000000000..863339fb8ced
> --- /dev/null
> +++ b/tests/block/030.out
> @@ -0,0 +1 @@
> +Passed

Re: [PATCH blktests v2 4/4] Add a test that triggers the blk_mq_realloc_hw_ctxs() error path

[Shinichiro Kawasaki]

On Mar 20, 2020 / 14:42, Omar Sandoval wrote:
> On Sun, Mar 15, 2020 at 03:13:20PM -0700, Bart Van Assche wrote:
> > Add a test that triggers the code touched by commit d0930bb8f46b ("blk-mq:
> > Fix a recently introduced regression in blk_mq_realloc_hw_ctxs()"). This
> > test only runs if a recently added fault injection feature is available,
> > namely commit 596444e75705 ("null_blk: Add support for init_hctx() fault
> > injection").
> > 
> > Cc: Ming Lei <ming.lei@redhat.com>
> > Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> > ---
> >  tests/block/030     | 42 ++++++++++++++++++++++++++++++++++++++++++
> >  tests/block/030.out |  1 +
> >  2 files changed, 43 insertions(+)
> >  create mode 100755 tests/block/030
> >  create mode 100644 tests/block/030.out
> > 
> > diff --git a/tests/block/030 b/tests/block/030
> > new file mode 100755
> > index 000000000000..861c85c27f09
> > --- /dev/null
> > +++ b/tests/block/030
> > @@ -0,0 +1,42 @@
> > +#!/bin/bash
> > +# SPDX-License-Identifier: GPL-2.0
> > +# Copyright 2020 Google LLC
> > +#
> > +# Trigger the blk_mq_realloc_hw_ctxs() error path.
> > +
> > +. tests/block/rc
> > +. common/null_blk
> > +
> > +DESCRIPTION="trigger the blk_mq_realloc_hw_ctxs() error path"
> > +QUICK=1
> > +
> > +requires() {
> > +	_have_null_blk || return $?
> > +	_have_module_param null_blk init_hctx || return $?
> > +}
> > +
> > +test() {
> > +	local i sq=/sys/kernel/config/nullb/nullb0/submit_queues
> > +
> > +	: "${TIMEOUT:=30}"
> > +	if ! _init_null_blk nr_devices=0 queue_mode=2 "init_hctx=$(nproc),100,0,0"; then
> > +		echo "Loading null_blk failed"
> > +		return 1
> > +	fi
> > +	if ! _configure_null_blk nullb0 completion_nsec=0 blocksize=512 size=16\
> > +	     submit_queues="$(nproc)" memory_backed=1 power=1; then
> > +		echo "Configuring null_blk failed"
> > +		return 1
> > +	fi
> > +	if { echo "$(<"$sq")" >$sq; } 2>/dev/null; then
> > +		for ((i=0;i<100;i++)); do
> > +			echo 1 >$sq
> > +			nproc >$sq
> > +		done
> > +	else
> > +		echo "Skipping test because $sq cannot be modified" >>"$FULL"
> 
> I just pushed the support for allowing skipping from the middle of a
> test, so now you could make this
> 
> 	SKIP_REASON="Skipping test because $sq cannot be modified"
>

Hi Omar,

I noticed the commit cd11d001fe86 ("Support skipping tests from
test{,_device}()") have side effects to some test cases. The unexpected run
result "not run" is reported to test cases as follows:

- block/005 ... for non-rotational drives such as nullb
- zbd/00[1-5]
- zbd/007   ... for dm-linear device

Some helper functions _test_dev_is_rotational, _test_dev_is_partition or so
are called within test_device() context, and left SKIP_REASON with values. It
resulted in the "not run" result.

To fix this, I can think of two approaches. One is to unset SKIP_REASON after
calling _test_dev_is_X helper functions within the test cases affected. This fix
is straight forward but it will require similar care for future test case
additions and changes. The other approach is to flag the test cases which judge
skip during test run. For example, SKIP_DURING_RUN=1 can be defined in those
test cases (block/030). The check script validates SKIP_REASON only if the test
case is flagged. This second approach looks less costly for me.

Please let me know your thoughts about the fix approach. Thanks!

-- 
Best Regards,
Shin'ichiro Kawasaki

Re: [PATCH blktests v2 4/4] Add a test that triggers the blk_mq_realloc_hw_ctxs() error path

[Shinichiro Kawasaki]

On Apr 21, 2020 / 02:35, Shinichiro Kawasaki wrote:
> On Mar 20, 2020 / 14:42, Omar Sandoval wrote:
> > On Sun, Mar 15, 2020 at 03:13:20PM -0700, Bart Van Assche wrote:
> > > Add a test that triggers the code touched by commit d0930bb8f46b ("blk-mq:
> > > Fix a recently introduced regression in blk_mq_realloc_hw_ctxs()"). This
> > > test only runs if a recently added fault injection feature is available,
> > > namely commit 596444e75705 ("null_blk: Add support for init_hctx() fault
> > > injection").
> > > 
> > > Cc: Ming Lei <ming.lei@redhat.com>
> > > Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> > > ---
> > >  tests/block/030     | 42 ++++++++++++++++++++++++++++++++++++++++++
> > >  tests/block/030.out |  1 +
> > >  2 files changed, 43 insertions(+)
> > >  create mode 100755 tests/block/030
> > >  create mode 100644 tests/block/030.out
> > > 
> > > diff --git a/tests/block/030 b/tests/block/030
> > > new file mode 100755
> > > index 000000000000..861c85c27f09
> > > --- /dev/null
> > > +++ b/tests/block/030
> > > @@ -0,0 +1,42 @@
> > > +#!/bin/bash
> > > +# SPDX-License-Identifier: GPL-2.0
> > > +# Copyright 2020 Google LLC
> > > +#
> > > +# Trigger the blk_mq_realloc_hw_ctxs() error path.
> > > +
> > > +. tests/block/rc
> > > +. common/null_blk
> > > +
> > > +DESCRIPTION="trigger the blk_mq_realloc_hw_ctxs() error path"
> > > +QUICK=1
> > > +
> > > +requires() {
> > > +	_have_null_blk || return $?
> > > +	_have_module_param null_blk init_hctx || return $?
> > > +}
> > > +
> > > +test() {
> > > +	local i sq=/sys/kernel/config/nullb/nullb0/submit_queues
> > > +
> > > +	: "${TIMEOUT:=30}"
> > > +	if ! _init_null_blk nr_devices=0 queue_mode=2 "init_hctx=$(nproc),100,0,0"; then
> > > +		echo "Loading null_blk failed"
> > > +		return 1
> > > +	fi
> > > +	if ! _configure_null_blk nullb0 completion_nsec=0 blocksize=512 size=16\
> > > +	     submit_queues="$(nproc)" memory_backed=1 power=1; then
> > > +		echo "Configuring null_blk failed"
> > > +		return 1
> > > +	fi
> > > +	if { echo "$(<"$sq")" >$sq; } 2>/dev/null; then
> > > +		for ((i=0;i<100;i++)); do
> > > +			echo 1 >$sq
> > > +			nproc >$sq
> > > +		done
> > > +	else
> > > +		echo "Skipping test because $sq cannot be modified" >>"$FULL"
> > 
> > I just pushed the support for allowing skipping from the middle of a
> > test, so now you could make this
> > 
> > 	SKIP_REASON="Skipping test because $sq cannot be modified"
> >
> 
> Hi Omar,
> 
> I noticed the commit cd11d001fe86 ("Support skipping tests from
> test{,_device}()") have side effects to some test cases. The unexpected run
> result "not run" is reported to test cases as follows:
> 
> - block/005 ... for non-rotational drives such as nullb
> - zbd/00[1-5]
> - zbd/007   ... for dm-linear device
> 
> Some helper functions _test_dev_is_rotational, _test_dev_is_partition or so
> are called within test_device() context, and left SKIP_REASON with values. It
> resulted in the "not run" result.
> 
> To fix this, I can think of two approaches. One is to unset SKIP_REASON after
> calling _test_dev_is_X helper functions within the test cases affected. This fix
> is straight forward but it will require similar care for future test case
> additions and changes. The other approach is to flag the test cases which judge
> skip during test run. For example, SKIP_DURING_RUN=1 can be defined in those
> test cases (block/030). The check script validates SKIP_REASON only if the test
> case is flagged. This second approach looks less costly for me.
> 
> Please let me know your thoughts about the fix approach. Thanks!

Now I found that Klaus Jensen already reported this and posted his fix patch.
My report was not necessary. Sorry about this noise.

-- 
Best Regards,
Shin'ichiro Kawasaki