[PATCH blktests v2 0/2] block/031: allow to run with built-in null_blk driver

[PATCH blktests v2 0/2] block/031: allow to run with built-in null_blk driver

[Shin'ichiro Kawasaki]

The test case block/031 now requires loadable null_blk driver. This series
allows it to run with built-in null_blk driver. The first patch prepares for it.
The second patch does the change.

Changes from v1:
* Added null_blk driver status check in _have_null_blk_feature()

Shin'ichiro Kawasaki (2):
  common/null_blk: introduce _have_null_blk_feature
  block/031: allow to run with built-in null_blk driver

 common/null_blk | 15 +++++++++++++++
 tests/block/031 | 27 +++++++++++++++++++--------
 2 files changed, 34 insertions(+), 8 deletions(-)

-- 
2.43.0

[PATCH blktests v2 1/2] common/null_blk: introduce _have_null_blk_feature

[Shin'ichiro Kawasaki]

Introduce a helper function _have_null_blk_feature which checks
/sys/kernel/config/features. It allows test cases to adapt to null_blk
feature support status.

Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
---
 common/null_blk | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/common/null_blk b/common/null_blk
index 91b78d4..164125d 100644
--- a/common/null_blk
+++ b/common/null_blk
@@ -10,6 +10,21 @@ _have_null_blk() {
 	_have_driver null_blk
 }
 
+_have_null_blk_feature() {
+	# Ensure that null_blk driver is built-in or loaded
+	if ! [[ -d /sys/module/null_blk ]]; then
+		if ! modprobe -q null_blk; then
+			return 1
+		fi
+		if [[ ! "${MODULES_TO_UNLOAD[*]}" =~ null_blk ]]; then
+			MODULES_TO_UNLOAD+=(null_blk)
+		fi
+	fi
+
+	# Check that null_blk has the specified feature
+	grep -qe "$1" /sys/kernel/config/nullb/features
+}
+
 _remove_null_blk_devices() {
 	if [[ -d /sys/kernel/config/nullb ]]; then
 		find /sys/kernel/config/nullb -mindepth 1 -maxdepth 1 \
-- 
2.43.0

Re: [PATCH blktests v2 1/2] common/null_blk: introduce _have_null_blk_feature

[Chaitanya Kulkarni]

On 1/9/24 02:44, Shin'ichiro Kawasaki wrote:
> Introduce a helper function _have_null_blk_feature which checks
> /sys/kernel/config/features. It allows test cases to adapt to null_blk
> feature support status.
>
> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> ---
>

much needed helper for a driver since features can change
across the release of kernel for validation can also be used
to skip certain tests ...

Looks good.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

Re: [PATCH blktests v2 1/2] common/null_blk: introduce _have_null_blk_feature

[Bart Van Assche]

On 1/9/24 02:44, Shin'ichiro Kawasaki wrote:
> Introduce a helper function _have_null_blk_feature which checks
> /sys/kernel/config/features. It allows test cases to adapt to null_blk
> feature support status.
> 
> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> ---
>   common/null_blk | 15 +++++++++++++++
>   1 file changed, 15 insertions(+)
> 
> diff --git a/common/null_blk b/common/null_blk
> index 91b78d4..164125d 100644
> --- a/common/null_blk
> +++ b/common/null_blk
> @@ -10,6 +10,21 @@ _have_null_blk() {
>   	_have_driver null_blk
>   }
>   
> +_have_null_blk_feature() {
> +	# Ensure that null_blk driver is built-in or loaded
> +	if ! [[ -d /sys/module/null_blk ]]; then
> +		if ! modprobe -q null_blk; then
> +			return 1
> +		fi
> +		if [[ ! "${MODULES_TO_UNLOAD[*]}" =~ null_blk ]]; then
> +			MODULES_TO_UNLOAD+=(null_blk)
> +		fi
> +	fi
> +
> +	# Check that null_blk has the specified feature
> +	grep -qe "$1" /sys/kernel/config/nullb/features
> +}
> +
>   _remove_null_blk_devices() {
>   	if [[ -d /sys/kernel/config/nullb ]]; then
>   		find /sys/kernel/config/nullb -mindepth 1 -maxdepth 1 \

Shouldn't _have_null_blk_feature() unload the null_blk kernel module if it
loads that kernel module? That will allow to simplify the next patch in this
series.

Thanks,

Bart.

[PATCH blktests v2 2/2] block/031: allow to run with built-in null_blk driver

[Shin'ichiro Kawasaki]

The test case block/031 sets null_blk parameter shared_tag_bitmap=1 for
testing. The parameter has been set as a module parameter then null_blk
driver must be loadable. However, null_blk allows to set
shared_tag_bitmap as a configfs parameter since the kernel commit
7012eef520cb ("null_blk: add configfs variables for 2 options"). Then
the test case now can be run with built-in null_blk driver by specifying
shared_tag_bitmap through configfs.

Modify the test case for that purpose. Refer null_blk feature list and
check if shared_tag_bitmap can be specified through configfs. If so,
specify the parameter as an option of _configure_null_blk and set it
through configfs. If not, check in requires() that shared_tag_bitmap can
be specified as a module parameter. Then call _init_null_blk() in test()
and specify shared_tag_bitmap=1 at null_blk module load.

Also change null_blk device name from nullb0 to nullb1 since the default
null_blk device name nullb0 is not usable with built-in null_blk driver.

Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
---
 tests/block/031 | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/tests/block/031 b/tests/block/031
index d253af8..61a3502 100755
--- a/tests/block/031
+++ b/tests/block/031
@@ -11,30 +11,41 @@ DESCRIPTION="do IO on null-blk with a host tag set"
 TIMED=1
 
 requires() {
-	_have_fio && _have_null_blk && _have_module_param null_blk shared_tag_bitmap
+	_have_fio
+	_have_null_blk
+	if ! _have_null_blk_feature shared_tag_bitmap; then
+		_have_module_param null_blk shared_tag_bitmap
+	fi
 }
 
 test() {
 	local fio_status bs=512
+	local -a opts=(nullb1 completion_nsec=0 blocksize="$bs" size=1 \
+			      submit_queues="$(nproc)" memory_backed=1)
 
 	: "${TIMEOUT:=30}"
-	if ! _init_null_blk nr_devices=0 shared_tag_bitmap=1; then
-		echo "Loading null_blk failed"
-		return 1
+	if _have_null_blk_feature shared_tag_bitmap; then
+		opts+=(shared_tag_bitmap=1)
+	else
+		# Old kernel requires shared_tag_bitmap as a module parameter
+		# instead of configfs parameter.
+		if ! _init_null_blk shared_tag_bitmap=1; then
+			echo "Loading null_blk failed"
+			return 1
+		fi
 	fi
-	if ! _configure_null_blk nullb0 completion_nsec=0 blocksize=$bs size=1\
-	     submit_queues="$(nproc)" memory_backed=1 power=1; then
+	if ! _configure_null_blk "${opts[@]}" power=1; then
 		echo "Configuring null_blk failed"
 		return 1
 	fi
 	fio --verify=md5 --rw=randwrite --bs=$bs --loops=$((10**6)) \
 		--iodepth=64 --group_reporting --sync=1 --direct=1 \
 		--ioengine=libaio --runtime="${TIMEOUT}" --thread \
-		--name=block-031 --filename=/dev/nullb0 \
+		--name=block-031 --filename=/dev/nullb1 \
 		--output="${RESULTS_DIR}/block/fio-output-031.txt" \
 		>>"$FULL"
 	fio_status=$?
-	rmdir /sys/kernel/config/nullb/nullb0
+	rmdir /sys/kernel/config/nullb/nullb1
 	_exit_null_blk
 	case $fio_status in
 		0) echo "Passed";;
-- 
2.43.0

Re: [PATCH blktests v2 2/2] block/031: allow to run with built-in null_blk driver

[Chaitanya Kulkarni]

On 1/9/24 02:44, Shin'ichiro Kawasaki wrote:
> The test case block/031 sets null_blk parameter shared_tag_bitmap=1 for
> testing. The parameter has been set as a module parameter then null_blk
> driver must be loadable. However, null_blk allows to set
> shared_tag_bitmap as a configfs parameter since the kernel commit
> 7012eef520cb ("null_blk: add configfs variables for 2 options"). Then
> the test case now can be run with built-in null_blk driver by specifying
> shared_tag_bitmap through configfs.
>
> Modify the test case for that purpose. Refer null_blk feature list and
> check if shared_tag_bitmap can be specified through configfs. If so,
> specify the parameter as an option of _configure_null_blk and set it
> through configfs. If not, check in requires() that shared_tag_bitmap can
> be specified as a module parameter. Then call _init_null_blk() in test()
> and specify shared_tag_bitmap=1 at null_blk module load.
>
> Also change null_blk device name from nullb0 to nullb1 since the default
> null_blk device name nullb0 is not usable with built-in null_blk driver.
>
> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> ---
>

Looks good.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

Re: [PATCH blktests v2 2/2] block/031: allow to run with built-in null_blk driver

[Bart Van Assche]

On 1/9/24 02:44, Shin'ichiro Kawasaki wrote:
> The test case block/031 sets null_blk parameter shared_tag_bitmap=1 for
> testing.
> The parameter has been set as a module parameter then null_blk
> driver must be loadable.

It seems like the word "If" is missing from the start of the above sentence?

> -	if ! _init_null_blk nr_devices=0 shared_tag_bitmap=1; then
> -		echo "Loading null_blk failed"
> -		return 1
> +	if _have_null_blk_feature shared_tag_bitmap; then
> +		opts+=(shared_tag_bitmap=1)
> +	else
> +		# Old kernel requires shared_tag_bitmap as a module parameter
> +		# instead of configfs parameter.
> +		if ! _init_null_blk shared_tag_bitmap=1; then
> +			echo "Loading null_blk failed"
> +			return 1
> +		fi
>   	fi

_have_null_blk_feature loads the null_blk kernel module as a side effect. The
above code relies on that side effect. I think that _have_null_blk_feature either
should unload the null_blk kernel module or that a comment should be added above
the above if-statement that explains this side effect. Otherwise readers of this
code will wonder why there is an _init_null_blk call in one branch of the
if-statement and not in the other branch.

Thanks,

Bart.

Re: [PATCH blktests v2 2/2] block/031: allow to run with built-in null_blk driver

[Shinichiro Kawasaki]

On Jan 09, 2024 / 12:19, Bart Van Assche wrote:
> On 1/9/24 02:44, Shin'ichiro Kawasaki wrote:
> > The test case block/031 sets null_blk parameter shared_tag_bitmap=1 for
> > testing.
> > The parameter has been set as a module parameter then null_blk
> > driver must be loadable.
> 
> It seems like the word "If" is missing from the start of the above sentence?

With this sentence, I wanted to describe current implmenetation fact. Before
applying this patch, shared_tag_bitmap=1 was set as a module parameter, not
through a configfs file. So I don't think "If" is missing.

> 
> > -	if ! _init_null_blk nr_devices=0 shared_tag_bitmap=1; then
> > -		echo "Loading null_blk failed"
> > -		return 1
> > +	if _have_null_blk_feature shared_tag_bitmap; then
> > +		opts+=(shared_tag_bitmap=1)
> > +	else
> > +		# Old kernel requires shared_tag_bitmap as a module parameter
> > +		# instead of configfs parameter.
> > +		if ! _init_null_blk shared_tag_bitmap=1; then
> > +			echo "Loading null_blk failed"
> > +			return 1
> > +		fi
> >   	fi
> 
> _have_null_blk_feature loads the null_blk kernel module as a side effect. The
> above code relies on that side effect.

No. Regardless of the null_blk module load status, _init_null_blk() should work
in same manner. Actually _init_null_blk() unload null_blk (modprobe -r null_blk)
then load null_blk (modprobe null_blk).

> I think that _have_null_blk_feature either
> should unload the null_blk kernel module or that a comment should be added above
> the above if-statement that explains this side effect. Otherwise readers of this
> code will wonder why there is an _init_null_blk call in one branch of the
> if-statement and not in the other branch.

I think the inline comment above was not clear enough and caused the confusion.
How about to improve the comment as follows? I hope it explains why
_init_null_blk is called in the if-statement.

        # _configure_null_blk() sets null_blk parameters via configfs, while
	# _init_null_blk() sets null_blk parameters as module parameters.
        # Old kernel requires shared_tag_bitmap as a module parameter. In that
	# case, call _init_null_blk() for shared_tag_bitmap.

Re: [PATCH blktests v2 2/2] block/031: allow to run with built-in null_blk driver

[Bart Van Assche]

On 1/9/24 18:19, Shinichiro Kawasaki wrote:
> On Jan 09, 2024 / 12:19, Bart Van Assche wrote:
>> On 1/9/24 02:44, Shin'ichiro Kawasaki wrote:
>>> The parameter has been set as a module parameter then null_blk
>>> driver must be loadable.
>>
>> It seems like the word "If" is missing from the start of the above sentence?
> 
> With this sentence, I wanted to describe current implmenetation fact. Before
> applying this patch, shared_tag_bitmap=1 was set as a module parameter, not
> through a configfs file. So I don't think "If" is missing.

If I upload the sentence that I quoted to a free online grammar checker
(https://quillbot.com/grammar-check) then it tells me that there is a
grammatical issue with that sentence. That's why I asked whether a word is
perhaps missing. I'm not trying to be pedantic - I raised my question because
the above sentence is incomprehensible to me.

> I think the inline comment above was not clear enough and caused the confusion.
> How about to improve the comment as follows? I hope it explains why
> _init_null_blk is called in the if-statement.
> 
>          # _configure_null_blk() sets null_blk parameters via configfs, while
> 	# _init_null_blk() sets null_blk parameters as module parameters.
>          # Old kernel requires shared_tag_bitmap as a module parameter. In that
> 	# case, call _init_null_blk() for shared_tag_bitmap.

That sounds better to me.

Thanks,

Bart.

Re: [PATCH blktests v2 2/2] block/031: allow to run with built-in null_blk driver

[Shinichiro Kawasaki]

On Jan 10, 2024 / 09:50, Bart Van Assche wrote:
> On 1/9/24 18:19, Shinichiro Kawasaki wrote:
> > On Jan 09, 2024 / 12:19, Bart Van Assche wrote:
> > > On 1/9/24 02:44, Shin'ichiro Kawasaki wrote:
> > > > The parameter has been set as a module parameter then null_blk
> > > > driver must be loadable.
> > > 
> > > It seems like the word "If" is missing from the start of the above sentence?
> > 
> > With this sentence, I wanted to describe current implmenetation fact. Before
> > applying this patch, shared_tag_bitmap=1 was set as a module parameter, not
> > through a configfs file. So I don't think "If" is missing.
> 
> If I upload the sentence that I quoted to a free online grammar checker
> (https://quillbot.com/grammar-check) then it tells me that there is a
> grammatical issue with that sentence. That's why I asked whether a word is
> perhaps missing. I'm not trying to be pedantic - I raised my question because
> the above sentence is incomprehensible to me.

Thanks for pointing out the incomprehensible sentence. The grammar checker looks
useful for improving my writing. I will use it to revise the commit message as
well as the code block comments.

> 
> > I think the inline comment above was not clear enough and caused the confusion.
> > How about to improve the comment as follows? I hope it explains why
> > _init_null_blk is called in the if-statement.
> > 
> >          # _configure_null_blk() sets null_blk parameters via configfs, while
> > 	# _init_null_blk() sets null_blk parameters as module parameters.
> >          # Old kernel requires shared_tag_bitmap as a module parameter. In that
> > 	# case, call _init_null_blk() for shared_tag_bitmap.
> 
> That sounds better to me.
> 
> Thanks,
> 
> Bart.
> 
>