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.
--
Best Regards,
Shin'ichiro Kawasaki
