diff mbox series

[blktests,v3,01/13] config: Introduce RUN_ZONED_TESTS variable and CAN_BE_ZONED flag

Message ID 20190118094453.13773-2-shinichiro.kawasaki@wdc.com (mailing list archive)
State New, archived
Headers show
Series Implement zoned block device support | expand

Commit Message

Shinichiro Kawasaki Jan. 18, 2019, 9:44 a.m. UTC 
To allow running tests using a null_blk device with the zoned mode
disabled (current setup) as well as enabled, introduce the config
the RUN_ZONED_TESTS config variable and the per-test flag CAN_BE_ZONED.

RUN_ZONED_TESTS=1 indicates that tests run against null_blk will be
executed twice, first with null_blk as a regular block device
(RUN_FOR_ZONED=0) and a second time with null_blk set as a zoned block
device (RUN_FOR_ZONED=1). This applies only to tests cases that have the
variable CAN_BE_ZONED set to 1, indicating that the test case applies to
zoned block devices. If CAN_BE_ZONED is not defined by a test case, the
test is executed only with the regular null_blk device.

_init_null_blk is modified to prepare null_blk as a zoned blocked device
if RUN_FOR_ZONED is set and as a regular block device otherwise. To avoid
"modprobe -r null_blk" failures, rmdir calls on all sysfs nullbX
directories is added.

When a zoned block device is specified in TEST_DEVS, failures of test
cases that do not set CAN_BE_ZONED are avoided by automatically skipping
the test. The new helper function _test_dev_is_zoned() is introduced to
implement this.

The use of the RUN_ZONED_TESTS variable requires that the kernel be
compiled with CONFIG_BLK_DEV_ZONED enabled.

Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
---
 Documentation/running-tests.md | 13 +++++++++++++
 check                          | 29 ++++++++++++++++++++++++++---
 common/null_blk                | 23 ++++++++++++++++++++++-
 common/shellcheck              |  2 +-
 new                            |  4 ++++
 5 files changed, 66 insertions(+), 5 deletions(-)

Comments

Omar Sandoval Jan. 25, 2019, 8:59 p.m. UTC | #1 
On Fri, Jan 18, 2019 at 06:44:41PM +0900, Shin'ichiro Kawasaki wrote:
> To allow running tests using a null_blk device with the zoned mode
> disabled (current setup) as well as enabled, introduce the config
> the RUN_ZONED_TESTS config variable and the per-test flag CAN_BE_ZONED.
> 
> RUN_ZONED_TESTS=1 indicates that tests run against null_blk will be
> executed twice, first with null_blk as a regular block device
> (RUN_FOR_ZONED=0) and a second time with null_blk set as a zoned block
> device (RUN_FOR_ZONED=1). This applies only to tests cases that have the
> variable CAN_BE_ZONED set to 1, indicating that the test case applies to
> zoned block devices. If CAN_BE_ZONED is not defined by a test case, the
> test is executed only with the regular null_blk device.
> 
> _init_null_blk is modified to prepare null_blk as a zoned blocked device
> if RUN_FOR_ZONED is set and as a regular block device otherwise. To avoid
> "modprobe -r null_blk" failures, rmdir calls on all sysfs nullbX
> directories is added.
> 
> When a zoned block device is specified in TEST_DEVS, failures of test
> cases that do not set CAN_BE_ZONED are avoided by automatically skipping
> the test. The new helper function _test_dev_is_zoned() is introduced to
> implement this.
> 
> The use of the RUN_ZONED_TESTS variable requires that the kernel be
> compiled with CONFIG_BLK_DEV_ZONED enabled.

This is much better, thanks! Some comments below.

> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> ---
>  Documentation/running-tests.md | 13 +++++++++++++
>  check                          | 29 ++++++++++++++++++++++++++---
>  common/null_blk                | 23 ++++++++++++++++++++++-
>  common/shellcheck              |  2 +-
>  new                            |  4 ++++
>  5 files changed, 66 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/running-tests.md b/Documentation/running-tests.md
> index 8f32af3..459c96b 100644
> --- a/Documentation/running-tests.md
> +++ b/Documentation/running-tests.md
> @@ -82,6 +82,19 @@ passing the `-d` command line option or setting the `DEVICE_ONLY` variable.
>  DEVICE_ONLY=1
>  ```
>  
> +### Zoned Block Device
> +
> +To run test cases for zoned block devices, set `RUN_ZONED_TESTS` variable.
> +When this variable is set and a test case can prepare a virtual devices such
> +as null_blk with zoned mode, the test case is executed twice: first with
> +non-zoned mode and second with zoned mode. The use of the RUN_ZONED_TESTS
> +variable requires that the kernel be compiled with CONFIG_BLK_DEV_ZONED
> +enabled.
> +

Minor proofreading:

To run test cases for zoned block devices, set the `RUN_ZONED_TESTS` variable.
When this variable is set and a test case can prepare a virtual device such as
`null_blk` with zoned mode, the test case is executed twice: first in non-zoned
mode and second in zoned mode. The use of the `RUN_ZONED_TESTS` variable
requires that the kernel be compiled with `CONFIG_BLK_DEV_ZONED` enabled.

> +```sh
> +RUN_ZONED_TESTS=1
> +```
> +
>  ### Custom Setup
>  
>  The `config` file is really just a bash file that is sourced at the beginning
> diff --git a/check b/check
> index 6c6d9f5..4563d62 100755
> --- a/check
> +++ b/check
> @@ -17,7 +17,7 @@ _found_test() {
>  	local test_name="$1"
>  	local explicit="$2"
>  
> -	unset DESCRIPTION QUICK TIMED requires device_requires test test_device
> +	unset DESCRIPTION QUICK TIMED CAN_BE_ZONED requires device_requires test test_device
>  
>  	# shellcheck disable=SC1090
>  	if ! . "tests/${test_name}"; then
> @@ -182,11 +182,14 @@ _write_test_run() {
>  _output_status() {
>  	local test="$1"
>  	local status="$2"
> +	local zoned=" "
> +
> +	if (( RUN_FOR_ZONED )); then zoned=" (zoned) "; fi
>  
>  	if [[ -v DESCRIPTION ]]; then
> -		printf '%-60s' "$test ($DESCRIPTION)"
> +		printf '%-60s' "${test}${zoned}($DESCRIPTION)"
>  	else
> -		printf '%-60s' "$test"
> +		printf '%-60s' "${test}${zoned}"
>  	fi
>  	if [[ -z $status ]]; then
>  		echo
> @@ -391,10 +394,19 @@ _call_test() {
>  	fi
>  }
>  
> +_test_dev_is_zoned() {
> +	if grep -qe "none" "${TEST_DEV_SYSFS}/queue/zoned" ; then
> +		SKIP_REASON="${TEST_DEV} is not a zoned block device"
> +		return 1
> +	fi
> +	return 0
> +}
> +
>  _run_test() {
>  	TEST_NAME="$1"
>  	CHECK_DMESG=1
>  	DMESG_FILTER="cat"
> +	RUN_FOR_ZONED=0
>  
>  	# shellcheck disable=SC1090
>  	. "tests/${TEST_NAME}"
> @@ -407,6 +419,11 @@ _run_test() {
>  
>  		RESULTS_DIR="$OUTPUT/nodev"
>  		_call_test test
> +		if (( RUN_ZONED_TESTS && CAN_BE_ZONED )); then
> +			RESULTS_DIR="$OUTPUT/nodev_zoned"
> +			RUN_FOR_ZONED=1
> +			_call_test test
> +		fi
>  	else
>  		if [[ ${#TEST_DEVS[@]} -eq 0 ]]; then
>  			return 0
> @@ -420,6 +437,11 @@ _run_test() {
>  		local ret=0
>  		for TEST_DEV in "${TEST_DEVS[@]}"; do
>  			TEST_DEV_SYSFS="${TEST_DEV_SYSFS_DIRS["$TEST_DEV"]}"
> +			if (( !CAN_BE_ZONED )) && _test_dev_is_zoned; then
> +				SKIP_REASON="${TEST_DEV} is a zoned block device"
> +				_output_notrun "$TEST_NAME => $(basename "$TEST_DEV")"
> +				continue
> +			fi

We should unset SKIP_REASON here from _test_dev_is_zoned just in case
device_requires forgets to set a SKIP_REASON.

>  			if declare -fF device_requires >/dev/null && ! device_requires; then
>  				_output_notrun "$TEST_NAME => $(basename "$TEST_DEV")"
>  				continue
> @@ -591,6 +613,7 @@ fi
>  # Default configuration.
>  : "${DEVICE_ONLY:=0}"
>  : "${QUICK_RUN:=0}"
> +: "${RUN_ZONED_TESTS:=0}"
>  : "${OUTPUT:=results}"
>  if [[ -v EXCLUDE ]] && ! declare -p EXCLUDE | grep -q '^declare -a'; then
>  	# If EXCLUDE was not defined as an array, convert it to one.
> diff --git a/common/null_blk b/common/null_blk
> index 937ece0..fd035b7 100644
> --- a/common/null_blk
> +++ b/common/null_blk
> @@ -8,8 +8,29 @@ _have_null_blk() {
>  	_have_modules null_blk
>  }
>  
> +_null_blk_not_zoned() {
> +	if [[ "${ZONED}" != "0" ]]; then
> +		# shellcheck disable=SC2034
> +		SKIP_REASON="null_blk zoned mode not supported"
> +		return 1
> +	fi
> +	return 0
> +}

Is this still used anywhere in this version?

>  _init_null_blk() {
> -	if ! modprobe -r null_blk || ! modprobe null_blk "$@"; then
> +	for d in /sys/kernel/config/nullb/*;
> +	do [[ -d "$d" ]] && rmdir "$d";	done

I'd prefer to do this without globbing:

        if [[ -d /sys/kernel/config/nullb ]]; then
                find /sys/kernel/config/nullb -mindepth 1 -maxdepth 1 -type d -delete
        fi

> +	local _zoned=""
> +	if [[ ${RUN_FOR_ZONED} -ne 0 ]] ; then
> +		if ! _have_kernel_option BLK_DEV_ZONED ; then
> +			echo -n "ZONED specified for kernel with "
> +			echo "CONFIG_BLK_DEV_ZONED disabled"
> +			return 1
> +		fi

Let's avoid _have_kernel_option if we can, since that requires that the
user enabled CONFIG_IKCONFIG_PROC or installed their config in /boot. We
can just skip this check and the modprobe below will fail with
"null_blk: CONFIG_BLK_DEV_ZONED not enabled" in dmesg.

While you're here, this variable name doesn't need the leading
underscore.

> +		_zoned="zoned=1"
> +	fi
> +	if ! modprobe -r null_blk || ! modprobe null_blk "$@" "${_zoned}" ; then
>  		return 1
>  	fi
>  
> diff --git a/common/shellcheck b/common/shellcheck
> index 5f46c00..b9be7d2 100644
> --- a/common/shellcheck
> +++ b/common/shellcheck
> @@ -6,5 +6,5 @@
>  
>  # Suppress unused global variable warnings.
>  _silence_sc2034() {
> -	echo "$CGROUP2_DIR $CHECK_DMESG $DESCRIPTION $DMESG_FILTER $FIO_PERF_FIELDS $FIO_PERF_PREFIX $QUICK $SKIP_REASON ${TEST_RUN[*]} $TIMED" > /dev/null
> +	echo "$CGROUP2_DIR $CHECK_DMESG $DESCRIPTION $DMESG_FILTER $FIO_PERF_FIELDS $FIO_PERF_PREFIX $QUICK $SKIP_REASON ${TEST_RUN[*]} $TIMED $CAN_BE_ZONED" > /dev/null
>  }
> diff --git a/new b/new
> index 63e36cd..d7d5f7c 100755
> --- a/new
> +++ b/new
> @@ -145,6 +145,10 @@ DESCRIPTION=""
>  # Alternatively, you can filter out any unimportant messages in dmesg like so:
>  # DMESG_FILTER="grep -v sysfs"
>  
> +# TODO: if this test can be run for both regular block devices and zoned block
> +# devices, uncomment the line below.
> +# CAN_BE_ZONED=1
> +
>  # TODO: if this test has any extra requirements, it should define a requires()
>  # function. If the test can be run, requires() should return 0. Otherwise, it
>  # should return non-zero and set the \$SKIP_REASON variable. Usually,
> -- 
> 2.20.1
>
Shinichiro Kawasaki Jan. 28, 2019, 10:07 a.m. UTC | #2 
On 1/26/19 5:59 AM, Omar Sandoval wrote:
> On Fri, Jan 18, 2019 at 06:44:41PM +0900, Shin'ichiro Kawasaki wrote:
>> To allow running tests using a null_blk device with the zoned mode
>> disabled (current setup) as well as enabled, introduce the config
>> the RUN_ZONED_TESTS config variable and the per-test flag CAN_BE_ZONED.
>>
>> RUN_ZONED_TESTS=1 indicates that tests run against null_blk will be
>> executed twice, first with null_blk as a regular block device
>> (RUN_FOR_ZONED=0) and a second time with null_blk set as a zoned block
>> device (RUN_FOR_ZONED=1). This applies only to tests cases that have the
>> variable CAN_BE_ZONED set to 1, indicating that the test case applies to
>> zoned block devices. If CAN_BE_ZONED is not defined by a test case, the
>> test is executed only with the regular null_blk device.
>>
>> _init_null_blk is modified to prepare null_blk as a zoned blocked device
>> if RUN_FOR_ZONED is set and as a regular block device otherwise. To avoid
>> "modprobe -r null_blk" failures, rmdir calls on all sysfs nullbX
>> directories is added.
>>
>> When a zoned block device is specified in TEST_DEVS, failures of test
>> cases that do not set CAN_BE_ZONED are avoided by automatically skipping
>> the test. The new helper function _test_dev_is_zoned() is introduced to
>> implement this.
>>
>> The use of the RUN_ZONED_TESTS variable requires that the kernel be
>> compiled with CONFIG_BLK_DEV_ZONED enabled.
> 
> This is much better, thanks! Some comments below.

Hi Omar, thank you again for your review.

>> +### Zoned Block Device
>> +
>> +To run test cases for zoned block devices, set `RUN_ZONED_TESTS` variable.
>> +When this variable is set and a test case can prepare a virtual devices such
>> +as null_blk with zoned mode, the test case is executed twice: first with
>> +non-zoned mode and second with zoned mode. The use of the RUN_ZONED_TESTS
>> +variable requires that the kernel be compiled with CONFIG_BLK_DEV_ZONED
>> +enabled.
>> +
> 
> Minor proofreading:

Thanks. Will apply.

>> @@ -420,6 +437,11 @@ _run_test() {
>>   		local ret=0
>>   		for TEST_DEV in "${TEST_DEVS[@]}"; do
>>   			TEST_DEV_SYSFS="${TEST_DEV_SYSFS_DIRS["$TEST_DEV"]}"
>> +			if (( !CAN_BE_ZONED )) && _test_dev_is_zoned; then
>> +				SKIP_REASON="${TEST_DEV} is a zoned block device"
>> +				_output_notrun "$TEST_NAME => $(basename "$TEST_DEV")"
>> +				continue
>> +			fi
> 
> We should unset SKIP_REASON here from _test_dev_is_zoned just in case
> device_requires forgets to set a SKIP_REASON.

OK. Will add unset.

>> diff --git a/common/null_blk b/common/null_blk
>> index 937ece0..fd035b7 100644
>> --- a/common/null_blk
>> +++ b/common/null_blk
>> @@ -8,8 +8,29 @@ _have_null_blk() {
>>   	_have_modules null_blk
>>   }
>>   
>> +_null_blk_not_zoned() {
>> +	if [[ "${ZONED}" != "0" ]]; then
>> +		# shellcheck disable=SC2034
>> +		SKIP_REASON="null_blk zoned mode not supported"
>> +		return 1
>> +	fi
>> +	return 0
>> +}
> 
> Is this still used anywhere in this version?

I left it by mistake. Will remove it.

>>   _init_null_blk() {
>> -	if ! modprobe -r null_blk || ! modprobe null_blk "$@"; then
>> +	for d in /sys/kernel/config/nullb/*;
>> +	do [[ -d "$d" ]] && rmdir "$d";	done
> 
> I'd prefer to do this without globbing:
> 
>          if [[ -d /sys/kernel/config/nullb ]]; then
>                  find /sys/kernel/config/nullb -mindepth 1 -maxdepth 1 -type d -delete
>          fi

Thanks. Will replace with the suggested code.

>> +	local _zoned=""
>> +	if [[ ${RUN_FOR_ZONED} -ne 0 ]] ; then
>> +		if ! _have_kernel_option BLK_DEV_ZONED ; then
>> +			echo -n "ZONED specified for kernel with "
>> +			echo "CONFIG_BLK_DEV_ZONED disabled"
>> +			return 1
>> +		fi
> 
> Let's avoid _have_kernel_option if we can, since that requires that the
> user enabled CONFIG_IKCONFIG_PROC or installed their config in /boot. We
> can just skip this check and the modprobe below will fail with
> "null_blk: CONFIG_BLK_DEV_ZONED not enabled" in dmesg.

OK. Will remove the kernel option check.

> While you're here, this variable name doesn't need the leading
> underscore.
> 
>> +		_zoned="zoned=1"
>> +	fi
>> +	if ! modprobe -r null_blk || ! modprobe null_blk "$@" "${_zoned}" ; then
>>   		return 1
>>   	fi

Will rename it.
diff mbox series

Patch

diff --git a/Documentation/running-tests.md b/Documentation/running-tests.md
index 8f32af3..459c96b 100644
--- a/Documentation/running-tests.md
+++ b/Documentation/running-tests.md
@@ -82,6 +82,19 @@  passing the `-d` command line option or setting the `DEVICE_ONLY` variable.
 DEVICE_ONLY=1
 ```
 
+### Zoned Block Device
+
+To run test cases for zoned block devices, set `RUN_ZONED_TESTS` variable.
+When this variable is set and a test case can prepare a virtual devices such
+as null_blk with zoned mode, the test case is executed twice: first with
+non-zoned mode and second with zoned mode. The use of the RUN_ZONED_TESTS
+variable requires that the kernel be compiled with CONFIG_BLK_DEV_ZONED
+enabled.
+
+```sh
+RUN_ZONED_TESTS=1
+```
+
 ### Custom Setup
 
 The `config` file is really just a bash file that is sourced at the beginning
diff --git a/check b/check
index 6c6d9f5..4563d62 100755
--- a/check
+++ b/check
@@ -17,7 +17,7 @@  _found_test() {
 	local test_name="$1"
 	local explicit="$2"
 
-	unset DESCRIPTION QUICK TIMED requires device_requires test test_device
+	unset DESCRIPTION QUICK TIMED CAN_BE_ZONED requires device_requires test test_device
 
 	# shellcheck disable=SC1090
 	if ! . "tests/${test_name}"; then
@@ -182,11 +182,14 @@  _write_test_run() {
 _output_status() {
 	local test="$1"
 	local status="$2"
+	local zoned=" "
+
+	if (( RUN_FOR_ZONED )); then zoned=" (zoned) "; fi
 
 	if [[ -v DESCRIPTION ]]; then
-		printf '%-60s' "$test ($DESCRIPTION)"
+		printf '%-60s' "${test}${zoned}($DESCRIPTION)"
 	else
-		printf '%-60s' "$test"
+		printf '%-60s' "${test}${zoned}"
 	fi
 	if [[ -z $status ]]; then
 		echo
@@ -391,10 +394,19 @@  _call_test() {
 	fi
 }
 
+_test_dev_is_zoned() {
+	if grep -qe "none" "${TEST_DEV_SYSFS}/queue/zoned" ; then
+		SKIP_REASON="${TEST_DEV} is not a zoned block device"
+		return 1
+	fi
+	return 0
+}
+
 _run_test() {
 	TEST_NAME="$1"
 	CHECK_DMESG=1
 	DMESG_FILTER="cat"
+	RUN_FOR_ZONED=0
 
 	# shellcheck disable=SC1090
 	. "tests/${TEST_NAME}"
@@ -407,6 +419,11 @@  _run_test() {
 
 		RESULTS_DIR="$OUTPUT/nodev"
 		_call_test test
+		if (( RUN_ZONED_TESTS && CAN_BE_ZONED )); then
+			RESULTS_DIR="$OUTPUT/nodev_zoned"
+			RUN_FOR_ZONED=1
+			_call_test test
+		fi
 	else
 		if [[ ${#TEST_DEVS[@]} -eq 0 ]]; then
 			return 0
@@ -420,6 +437,11 @@  _run_test() {
 		local ret=0
 		for TEST_DEV in "${TEST_DEVS[@]}"; do
 			TEST_DEV_SYSFS="${TEST_DEV_SYSFS_DIRS["$TEST_DEV"]}"
+			if (( !CAN_BE_ZONED )) && _test_dev_is_zoned; then
+				SKIP_REASON="${TEST_DEV} is a zoned block device"
+				_output_notrun "$TEST_NAME => $(basename "$TEST_DEV")"
+				continue
+			fi
 			if declare -fF device_requires >/dev/null && ! device_requires; then
 				_output_notrun "$TEST_NAME => $(basename "$TEST_DEV")"
 				continue
@@ -591,6 +613,7 @@  fi
 # Default configuration.
 : "${DEVICE_ONLY:=0}"
 : "${QUICK_RUN:=0}"
+: "${RUN_ZONED_TESTS:=0}"
 : "${OUTPUT:=results}"
 if [[ -v EXCLUDE ]] && ! declare -p EXCLUDE | grep -q '^declare -a'; then
 	# If EXCLUDE was not defined as an array, convert it to one.
diff --git a/common/null_blk b/common/null_blk
index 937ece0..fd035b7 100644
--- a/common/null_blk
+++ b/common/null_blk
@@ -8,8 +8,29 @@  _have_null_blk() {
 	_have_modules null_blk
 }
 
+_null_blk_not_zoned() {
+	if [[ "${ZONED}" != "0" ]]; then
+		# shellcheck disable=SC2034
+		SKIP_REASON="null_blk zoned mode not supported"
+		return 1
+	fi
+	return 0
+}
+
 _init_null_blk() {
-	if ! modprobe -r null_blk || ! modprobe null_blk "$@"; then
+	for d in /sys/kernel/config/nullb/*;
+	do [[ -d "$d" ]] && rmdir "$d";	done
+
+	local _zoned=""
+	if [[ ${RUN_FOR_ZONED} -ne 0 ]] ; then
+		if ! _have_kernel_option BLK_DEV_ZONED ; then
+			echo -n "ZONED specified for kernel with "
+			echo "CONFIG_BLK_DEV_ZONED disabled"
+			return 1
+		fi
+		_zoned="zoned=1"
+	fi
+	if ! modprobe -r null_blk || ! modprobe null_blk "$@" "${_zoned}" ; then
 		return 1
 	fi
 
diff --git a/common/shellcheck b/common/shellcheck
index 5f46c00..b9be7d2 100644
--- a/common/shellcheck
+++ b/common/shellcheck
@@ -6,5 +6,5 @@ 
 
 # Suppress unused global variable warnings.
 _silence_sc2034() {
-	echo "$CGROUP2_DIR $CHECK_DMESG $DESCRIPTION $DMESG_FILTER $FIO_PERF_FIELDS $FIO_PERF_PREFIX $QUICK $SKIP_REASON ${TEST_RUN[*]} $TIMED" > /dev/null
+	echo "$CGROUP2_DIR $CHECK_DMESG $DESCRIPTION $DMESG_FILTER $FIO_PERF_FIELDS $FIO_PERF_PREFIX $QUICK $SKIP_REASON ${TEST_RUN[*]} $TIMED $CAN_BE_ZONED" > /dev/null
 }
diff --git a/new b/new
index 63e36cd..d7d5f7c 100755
--- a/new
+++ b/new
@@ -145,6 +145,10 @@  DESCRIPTION=""
 # Alternatively, you can filter out any unimportant messages in dmesg like so:
 # DMESG_FILTER="grep -v sysfs"
 
+# TODO: if this test can be run for both regular block devices and zoned block
+# devices, uncomment the line below.
+# CAN_BE_ZONED=1
+
 # TODO: if this test has any extra requirements, it should define a requires()
 # function. If the test can be run, requires() should return 0. Otherwise, it
 # should return non-zero and set the \$SKIP_REASON variable. Usually,