linux-block.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH blktests v3 0/4] Add a test that triggers the blk_mq_realloc_hw_ctxs() error path
@ 2020-03-20 22:24 Bart Van Assche
  2020-03-20 22:24 ` [PATCH blktests v3 1/4] Make _exit_null_blk remove all null_blk device instances Bart Van Assche
                   ` (3 more replies)
  0 siblings, 4 replies; 14+ messages in thread
From: Bart Van Assche @ 2020-03-20 22:24 UTC (permalink / raw)
  To: Omar Sandoval; +Cc: linux-block, Chaitanya Kulkarni, Bart Van Assche

Hi Omar,

Please consider this patch series for the upstream blktests repository.

Thanks,

Bart.

Changes compared to v2:
- Addressed Omar's review comments.

Changes between v1 and v2:
- Added three patches that refactor null_blk loading, unloading and
  configuration (Chaitanya).

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            | 21 ++++++++++++++++++-
 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, 74 insertions(+), 41 deletions(-)
 create mode 100755 tests/block/030
 create mode 100644 tests/block/030.out


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH blktests v3 1/4] Make _exit_null_blk remove all null_blk device instances
  2020-03-20 22:24 [PATCH blktests v3 0/4] Add a test that triggers the blk_mq_realloc_hw_ctxs() error path Bart Van Assche
@ 2020-03-20 22:24 ` Bart Van Assche
  2020-03-23 11:09   ` Daniel Wagner
  2020-03-20 22:24 ` [PATCH blktests v3 2/4] Use _{init,exit}_null_blk instead of open-coding these functions Bart Van Assche
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 14+ messages in thread
From: Bart Van Assche @ 2020-03-20 22:24 UTC (permalink / raw)
  To: Omar Sandoval; +Cc: linux-block, Chaitanya Kulkarni, 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().

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 common/null_blk | 7 ++++++-
 tests/block/022 | 3 ---
 tests/block/029 | 1 -
 3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/common/null_blk b/common/null_blk
index 2e300c20bbc7..a4140e365955 100644
--- a/common/null_blk
+++ b/common/null_blk
@@ -8,11 +8,15 @@ _have_null_blk() {
 	_have_modules null_blk
 }
 
-_init_null_blk() {
+_remove_null_blk_devices() {
 	if [[ -d /sys/kernel/config/nullb ]]; then
 		find /sys/kernel/config/nullb -mindepth 1 -maxdepth 1 \
 		     -type d -delete
 	fi
+}
+
+_init_null_blk() {
+	_remove_null_blk_devices
 
 	local zoned=""
 	if (( RUN_FOR_ZONED )); then zoned="zoned=1"; fi
@@ -26,6 +30,7 @@ _init_null_blk() {
 }
 
 _exit_null_blk() {
+	_remove_null_blk_devices
 	udevadm settle
 	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
 }

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH blktests v3 2/4] Use _{init,exit}_null_blk instead of open-coding these functions
  2020-03-20 22:24 [PATCH blktests v3 0/4] Add a test that triggers the blk_mq_realloc_hw_ctxs() error path Bart Van Assche
  2020-03-20 22:24 ` [PATCH blktests v3 1/4] Make _exit_null_blk remove all null_blk device instances Bart Van Assche
@ 2020-03-20 22:24 ` Bart Van Assche
  2020-03-23 11:13   ` Daniel Wagner
  2020-03-20 22:24 ` [PATCH blktests v3 3/4] Introduce the function _configure_null_blk() Bart Van Assche
  2020-03-20 22:24 ` [PATCH blktests v3 4/4] Add a test that triggers the blk_mq_realloc_hw_ctxs() error path Bart Van Assche
  3 siblings, 1 reply; 14+ messages in thread
From: Bart Van Assche @ 2020-03-20 22:24 UTC (permalink / raw)
  To: Omar Sandoval; +Cc: linux-block, Chaitanya Kulkarni, Bart Van Assche

This patch reduces code duplication.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
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 1fd631445921..136163bc73ad 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() {

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH blktests v3 3/4] Introduce the function _configure_null_blk()
  2020-03-20 22:24 [PATCH blktests v3 0/4] Add a test that triggers the blk_mq_realloc_hw_ctxs() error path Bart Van Assche
  2020-03-20 22:24 ` [PATCH blktests v3 1/4] Make _exit_null_blk remove all null_blk device instances Bart Van Assche
  2020-03-20 22:24 ` [PATCH blktests v3 2/4] Use _{init,exit}_null_blk instead of open-coding these functions Bart Van Assche
@ 2020-03-20 22:24 ` Bart Van Assche
  2020-03-23 11:19   ` Daniel Wagner
  2020-03-20 22:24 ` [PATCH blktests v3 4/4] Add a test that triggers the blk_mq_realloc_hw_ctxs() error path Bart Van Assche
  3 siblings, 1 reply; 14+ messages in thread
From: Bart Van Assche @ 2020-03-20 22:24 UTC (permalink / raw)
  To: Omar Sandoval
  Cc: linux-block, Chaitanya Kulkarni, Bart Van Assche, Chaitanya Kulkarni

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

Suggested-by: Chaitanya Kulkarni <Chaitanya.Kulkarni@wdc.com>
Reviewed-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 a4140e365955..08008fca8eae 100644
--- a/common/null_blk
+++ b/common/null_blk
@@ -29,6 +29,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" || return $?
+	while [ -n "$1" ]; do
+		param="${1%%=*}"
+		val="${1#*=}"
+		shift
+		echo "$val" > "$nullb/$param" || return $?
+	done
+}
+
 _exit_null_blk() {
 	_remove_null_blk_devices
 	udevadm settle
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)) \

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH blktests v3 4/4] Add a test that triggers the blk_mq_realloc_hw_ctxs() error path
  2020-03-20 22:24 [PATCH blktests v3 0/4] Add a test that triggers the blk_mq_realloc_hw_ctxs() error path Bart Van Assche
                   ` (2 preceding siblings ...)
  2020-03-20 22:24 ` [PATCH blktests v3 3/4] Introduce the function _configure_null_blk() Bart Van Assche
@ 2020-03-20 22:24 ` Bart Van Assche
  2020-03-23 11:29   ` Daniel Wagner
  3 siblings, 1 reply; 14+ messages in thread
From: Bart Van Assche @ 2020-03-20 22:24 UTC (permalink / raw)
  To: Omar Sandoval; +Cc: linux-block, Chaitanya Kulkarni, Bart Van Assche, Ming Lei

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>
Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.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..4a17550ab8eb
--- /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
+		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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH blktests v3 1/4] Make _exit_null_blk remove all null_blk device instances
  2020-03-20 22:24 ` [PATCH blktests v3 1/4] Make _exit_null_blk remove all null_blk device instances Bart Van Assche
@ 2020-03-23 11:09   ` Daniel Wagner
  0 siblings, 0 replies; 14+ messages in thread
From: Daniel Wagner @ 2020-03-23 11:09 UTC (permalink / raw)
  To: Bart Van Assche; +Cc: Omar Sandoval, linux-block, Chaitanya Kulkarni

On Fri, Mar 20, 2020 at 03:24:10PM -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().
> 
> Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Reviewed-by: Daniel Wagner <dwagner@suse.de>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH blktests v3 2/4] Use _{init,exit}_null_blk instead of open-coding these functions
  2020-03-20 22:24 ` [PATCH blktests v3 2/4] Use _{init,exit}_null_blk instead of open-coding these functions Bart Van Assche
@ 2020-03-23 11:13   ` Daniel Wagner
  0 siblings, 0 replies; 14+ messages in thread
From: Daniel Wagner @ 2020-03-23 11:13 UTC (permalink / raw)
  To: Bart Van Assche; +Cc: Omar Sandoval, linux-block, Chaitanya Kulkarni

On Fri, Mar 20, 2020 at 03:24:11PM -0700, Bart Van Assche wrote:
> This patch reduces code duplication.
> 
> Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Reviewed-by: Daniel Wagner <dwagner@suse.de>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH blktests v3 3/4] Introduce the function _configure_null_blk()
  2020-03-20 22:24 ` [PATCH blktests v3 3/4] Introduce the function _configure_null_blk() Bart Van Assche
@ 2020-03-23 11:19   ` Daniel Wagner
  2020-03-24  2:58     ` Bart Van Assche
  0 siblings, 1 reply; 14+ messages in thread
From: Daniel Wagner @ 2020-03-23 11:19 UTC (permalink / raw)
  To: Bart Van Assche; +Cc: Omar Sandoval, linux-block, Chaitanya Kulkarni

Hi Bart,

On Fri, Mar 20, 2020 at 03:24:12PM -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>
> Reviewed-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 \

I think this should be:

		_configure_null_blk $i completion_nsec=0 blocksize=512 \

Rest looks good, feel free do add my Reviewed-by when spinning a new version.

Thanks,
Daniel

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH blktests v3 4/4] Add a test that triggers the blk_mq_realloc_hw_ctxs() error path
  2020-03-20 22:24 ` [PATCH blktests v3 4/4] Add a test that triggers the blk_mq_realloc_hw_ctxs() error path Bart Van Assche
@ 2020-03-23 11:29   ` Daniel Wagner
  2020-03-24  3:09     ` Bart Van Assche
  0 siblings, 1 reply; 14+ messages in thread
From: Daniel Wagner @ 2020-03-23 11:29 UTC (permalink / raw)
  To: Bart Van Assche; +Cc: Omar Sandoval, linux-block, Chaitanya Kulkarni, Ming Lei

Hi Bart,

On Fri, Mar 20, 2020 at 03:24:13PM -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").

[...]

> +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

Doesn't make the $(nproc) the test subtil depending on the execution
environment?

Thanks,
Daniel

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH blktests v3 3/4] Introduce the function _configure_null_blk()
  2020-03-23 11:19   ` Daniel Wagner
@ 2020-03-24  2:58     ` Bart Van Assche
  0 siblings, 0 replies; 14+ messages in thread
From: Bart Van Assche @ 2020-03-24  2:58 UTC (permalink / raw)
  To: Daniel Wagner; +Cc: Omar Sandoval, linux-block, Chaitanya Kulkarni

On 2020-03-23 04:19, Daniel Wagner wrote:
> On Fri, Mar 20, 2020 at 03:24:12PM -0700, Bart Van Assche wrote:
>> 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 \
> 
> I think this should be:
> 
> 		_configure_null_blk $i completion_nsec=0 blocksize=512 \

Good catch, will fix.

Thanks,

Bart.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH blktests v3 4/4] Add a test that triggers the blk_mq_realloc_hw_ctxs() error path
  2020-03-23 11:29   ` Daniel Wagner
@ 2020-03-24  3:09     ` Bart Van Assche
  2020-03-24 10:41       ` Daniel Wagner
  0 siblings, 1 reply; 14+ messages in thread
From: Bart Van Assche @ 2020-03-24  3:09 UTC (permalink / raw)
  To: Daniel Wagner; +Cc: Omar Sandoval, linux-block, Chaitanya Kulkarni, Ming Lei

On 2020-03-23 04:29, Daniel Wagner wrote:
> On Fri, Mar 20, 2020 at 03:24:13PM -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").
> 
> [...]
> 
>> +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
> 
> Doesn't make the $(nproc) the test subtil depending on the execution
> environment?

Hi Daniel,

The value $(nproc) has been chosen on purpose. The following code from
the test script:

+			echo 1 >$sq
+			nproc >$sq

triggers (nproc + 1) calls to null_init_hctx(). So injecting a failure
after (nproc) null_init_hctx() calls triggers the following pattern:
* The first blk_mq_realloc_hw_ctxs() call fails after (nproc - 1)
null_init_hctx() calls.
* The second blk_mq_realloc_hw_ctxs() call fails after (nproc - 2)
null_init_hctx() calls.
...
* The (nproc) th blk_mq_realloc_hw_ctxs() call fails after one
null_init_hctx() call.
* The (nproc + 1) th blk_mq_realloc_hw_ctxs() call succeeds.

I'm not sure to trigger this behavior without using the $(nproc) value?

Thanks,

Bart.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH blktests v3 4/4] Add a test that triggers the blk_mq_realloc_hw_ctxs() error path
  2020-03-24  3:09     ` Bart Van Assche
@ 2020-03-24 10:41       ` Daniel Wagner
  2020-03-25  2:10         ` Bart Van Assche
  0 siblings, 1 reply; 14+ messages in thread
From: Daniel Wagner @ 2020-03-24 10:41 UTC (permalink / raw)
  To: Bart Van Assche; +Cc: Omar Sandoval, linux-block, Chaitanya Kulkarni, Ming Lei

Hi Bart,

On Mon, Mar 23, 2020 at 08:09:27PM -0700, Bart Van Assche wrote:
> On 2020-03-23 04:29, Daniel Wagner wrote:
> > On Fri, Mar 20, 2020 at 03:24:13PM -0700, Bart Van Assche wrote:
> >> +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

From the kernel code:

	/* "<interval>,<probability>,<space>,<times>" */

Don't you need to set the times attribute to -1 in order to inject the
everytime the interval is reached? If I understood it correctly,
with times=0 no failure is injected.

BTW, I've had to change it to init_hctx=$(($(nproc)+1)) to pass
the initial __configure_null_blk call before the first fail hits.

> > Doesn't make the $(nproc) the test subtil depending on the execution
> > environment?
> The value $(nproc) has been chosen on purpose. The following code from
> the test script:
>
> +			echo 1 >$sq
> +			nproc >$sq
>
> triggers (nproc + 1) calls to null_init_hctx().So injecting a failure
> after (nproc) null_init_hctx() calls triggers the following pattern:
> * The first blk_mq_realloc_hw_ctxs() call fails after (nproc - 1)
> null_init_hctx() calls.
> * The second blk_mq_realloc_hw_ctxs() call fails after (nproc - 2)
> null_init_hctx() calls.
> ...
> * The (nproc) th blk_mq_realloc_hw_ctxs() call fails after one
> null_init_hctx() call.
> * The (nproc + 1) th blk_mq_realloc_hw_ctxs() call succeeds.
>
> I'm not sure to trigger this behavior without using the $(nproc) value?

Okay, I get the idea how you want to test.

Is the dependency on nproc because null_blk expects submit_queue <= online
cpus?

Though why the 100?

		for ((i=0;i<100;i++)); do
			echo 1 >$sq
			nproc >$sq
		done

And shouldn't be there a test for error?

Thanks,
Daniel

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH blktests v3 4/4] Add a test that triggers the blk_mq_realloc_hw_ctxs() error path
  2020-03-24 10:41       ` Daniel Wagner
@ 2020-03-25  2:10         ` Bart Van Assche
  2020-03-25  8:18           ` Daniel Wagner
  0 siblings, 1 reply; 14+ messages in thread
From: Bart Van Assche @ 2020-03-25  2:10 UTC (permalink / raw)
  To: Daniel Wagner; +Cc: Omar Sandoval, linux-block, Chaitanya Kulkarni, Ming Lei

On 2020-03-24 03:41, Daniel Wagner wrote:
> Hi Bart,
> 
> On Mon, Mar 23, 2020 at 08:09:27PM -0700, Bart Van Assche wrote:
>> On 2020-03-23 04:29, Daniel Wagner wrote:
>>> On Fri, Mar 20, 2020 at 03:24:13PM -0700, Bart Van Assche wrote:
>>>> +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
> 
>>From the kernel code:
> 
> 	/* "<interval>,<probability>,<space>,<times>" */
> 
> Don't you need to set the times attribute to -1 in order to inject the
> everytime the interval is reached? If I understood it correctly,
> with times=0 no failure is injected.
> 
> BTW, I've had to change it to init_hctx=$(($(nproc)+1)) to pass
> the initial __configure_null_blk call before the first fail hits.

Hi Daniel,

I will make both changes in the init_hctx string. Not sure how this
escaped from my attention.

>>> Doesn't make the $(nproc) the test subtil depending on the execution
>>> environment?
>> The value $(nproc) has been chosen on purpose. The following code from
>> the test script:
>>
>> +			echo 1 >$sq
>> +			nproc >$sq
>>
>> triggers (nproc + 1) calls to null_init_hctx().So injecting a failure
>> after (nproc) null_init_hctx() calls triggers the following pattern:
>> * The first blk_mq_realloc_hw_ctxs() call fails after (nproc - 1)
>> null_init_hctx() calls.
>> * The second blk_mq_realloc_hw_ctxs() call fails after (nproc - 2)
>> null_init_hctx() calls.
>> ...
>> * The (nproc) th blk_mq_realloc_hw_ctxs() call fails after one
>> null_init_hctx() call.
>> * The (nproc + 1) th blk_mq_realloc_hw_ctxs() call succeeds.
>>
>> I'm not sure to trigger this behavior without using the $(nproc) value?
> 
> Okay, I get the idea how you want to test.
> 
> Is the dependency on nproc because null_blk expects submit_queue <= online
> cpus?

That's correct. I want to test with the maximum number of submit queues
allowed, hence the use of $(nproc).

> Though why the 100?
> 
> 		for ((i=0;i<100;i++)); do
> 			echo 1 >$sq
> 			nproc >$sq
> 		done

No particular reason other than "a significant number of iterations".

> And shouldn't be there a test for error?

All I want to test is the absence of kernel crashes. The blktests
framework already inspects dmesg for the absence of kernel crashes. So I
don't think that I have to check whether or not the quoted sysfs writes
succeed.

Thanks,

Bart.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH blktests v3 4/4] Add a test that triggers the blk_mq_realloc_hw_ctxs() error path
  2020-03-25  2:10         ` Bart Van Assche
@ 2020-03-25  8:18           ` Daniel Wagner
  0 siblings, 0 replies; 14+ messages in thread
From: Daniel Wagner @ 2020-03-25  8:18 UTC (permalink / raw)
  To: Bart Van Assche; +Cc: Omar Sandoval, linux-block, Chaitanya Kulkarni, Ming Lei

Hi Bart,

> > Is the dependency on nproc because null_blk expects submit_queue <= online
> > cpus?
> 
> That's correct. I want to test with the maximum number of submit queues
> allowed, hence the use of $(nproc).

Ok, I'd probably add this info as comment or have it in the commit
message. Just in case someone in the future wonders why nproc.

> > Though why the 100?
> > 
> > 		for ((i=0;i<100;i++)); do
> > 			echo 1 >$sq
> > 			nproc >$sq
> > 		done
> 
> No particular reason other than "a significant number of iterations".
> 
> > And shouldn't be there a test for error?

Ah ok. I was getting paranoid about numbers :)

> All I want to test is the absence of kernel crashes. The blktests
> framework already inspects dmesg for the absence of kernel crashes. So I
> don't think that I have to check whether or not the quoted sysfs writes
> succeed.

Maybe adding this info to the commit message what this test tries to
cover wouldn't hurt.

Thanks,
Daniel

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2020-03-25  8:18 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-20 22:24 [PATCH blktests v3 0/4] Add a test that triggers the blk_mq_realloc_hw_ctxs() error path Bart Van Assche
2020-03-20 22:24 ` [PATCH blktests v3 1/4] Make _exit_null_blk remove all null_blk device instances Bart Van Assche
2020-03-23 11:09   ` Daniel Wagner
2020-03-20 22:24 ` [PATCH blktests v3 2/4] Use _{init,exit}_null_blk instead of open-coding these functions Bart Van Assche
2020-03-23 11:13   ` Daniel Wagner
2020-03-20 22:24 ` [PATCH blktests v3 3/4] Introduce the function _configure_null_blk() Bart Van Assche
2020-03-23 11:19   ` Daniel Wagner
2020-03-24  2:58     ` Bart Van Assche
2020-03-20 22:24 ` [PATCH blktests v3 4/4] Add a test that triggers the blk_mq_realloc_hw_ctxs() error path Bart Van Assche
2020-03-23 11:29   ` Daniel Wagner
2020-03-24  3:09     ` Bart Van Assche
2020-03-24 10:41       ` Daniel Wagner
2020-03-25  2:10         ` Bart Van Assche
2020-03-25  8:18           ` Daniel Wagner

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).