diff mbox series

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

Message ID 20200315221320.613-5-bvanassche@acm.org (mailing list archive)
State New, archived
Headers show
Series Add a test that triggers the blk_mq_realloc_hw_ctxs() error path | expand

Commit Message

Bart Van Assche March 15, 2020, 10:13 p.m. UTC 
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

Comments

Chaitanya Kulkarni March 15, 2020, 11:36 p.m. UTC | #1 
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>
Omar Sandoval March 20, 2020, 9:42 p.m. UTC | #2 
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
Shinichiro Kawasaki April 21, 2020, 2:35 a.m. UTC | #3 
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!
Shinichiro Kawasaki April 21, 2020, 9:10 a.m. UTC | #4 
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.
diff mbox series

Patch

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