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

[Omar Sandoval <osandov@osandov.com>]

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
>