[PATCH blktests 2/3] dm/003: add unmap write zeroes tests

[Zhang Yi]

On 2025/4/28 16:13, Shinichiro Kawasaki wrote:
> On Apr 28, 2025 / 12:32, Zhang Yi wrote:
>> On 2025/4/3 15:43, Shinichiro Kawasaki wrote:
> [...]
>>>> +
>>>> +setup_test_device() {
>>>> +	if ! _configure_scsi_debug "$@"; then
>>>> +		return 1
>>>> +	fi
>>>
>>> In same manner as the 1st patch, I suggest to check /queue/write_zeroes_unmap
>>> here.
>>>
>>> 	if [[ ! -f /sys/block/${SCSI_DEBUG_DEVICES[0]}/queue/write_zeroes_unmap ]]; then
>>> 		_exit_scsi_debug
>>> 		SKIP_REASONS+=("kernel does not support unmap write zeroes sysfs interface")
>>> 		return 1
>>> 	fi
>>>
>>> The caller will need to check setup_test_device() return value.
>>
>> Sure.
>>
>>>
>>>> +
>>>> +	local dev="/dev/${SCSI_DEBUG_DEVICES[0]}"
>>>> +	local blk_sz="$(blockdev --getsz "$dev")"
>>>> +	dmsetup create test --table "0 $blk_sz linear $dev 0"
>>>
>>> I suggest to call _real_dev() here, and echo back the device name.
>>>
>>> 	dpath=$(_real_dev /dev/mapper/test)
>>> 	echo ${dpath##*/}
>>>
>>> The bash parameter expansion ${xxx##*/} works in same manner as the basename
>>> command. The caller can receive the device name in a local variable. This will
>>> avoid a bit of code duplication, and allow to avoid _short_dev().
>>>
>>
>> I'm afraid this approach will not work since we may set the
>> SKIP_REASONS parameter. We cannot pass the device name in this
>> manner as it will overlook the SKIP_REASONS setting when the caller
>> invokes $(setup_test_device xxx), this function runs in a subshell.
> 
> Ah, that's right. SKIP_REASONS modification in subshell won't work.
> 
>>
>> If you don't like _short_dev(), I think we can pass dname through a
>> global variable, something like below:
>>
>> setup_test_device() {
>> 	...
>> 	dpath=$(_real_dev /dev/mapper/test)
>> 	dname=${dpath##*/}
>> }
>>
>> if ! setup_test_device lbprz=0; then
>> 	return 1
>> fi
>> umap="$(< "/sys/block/${dname}/queue/write_zeroes_unmap")"
>>
>> What do you think?
> 
> I think global variable is a bit dirty. So my suggestion is to still echo back
> the short device name from the helper, and set the SKIP_REASONS after calling
> the helper, as follows:
> 
> diff --git a/tests/dm/003 b/tests/dm/003
> index 1013eb5..e00fa99 100755
> --- a/tests/dm/003
> +++ b/tests/dm/003
> @@ -20,13 +20,23 @@ device_requries() {
>  }
>  
>  setup_test_device() {
> +	local dev blk_sz dpath
> +
>  	if ! _configure_scsi_debug "$@"; then
>  		return 1

Hmm, if we encounter an error here, the test will be skipped instead of
returning a failure. This is not the expected outcome.

Thanks,
Yi.

>  	fi
>  
> -	local dev="/dev/${SCSI_DEBUG_DEVICES[0]}"
> -	local blk_sz="$(blockdev --getsz "$dev")"
> +        if [[ ! -f /sys/block/${SCSI_DEBUG_DEVICES[0]}/queue/write_zeroes_unmap ]]; then
> +		_exit_scsi_debug
> +                return 1
> +        fi
> +
> +	dev="/dev/${SCSI_DEBUG_DEVICES[0]}"
> +	blk_sz="$(blockdev --getsz "$dev")"
>  	dmsetup create test --table "0 $blk_sz linear $dev 0"
> +
> +	dpath=$(_real_dev /dev/mapper/test)
> +	echo ${dpath##*/}
>  }
>  
>  cleanup_test_device() {
> @@ -38,17 +48,21 @@ test() {
>  	echo "Running ${TEST_NAME}"
>  
>  	# disable WRITE SAME with unmap
> -	setup_test_device lbprz=0
> -	umap="$(cat "/sys/block/$(_short_dev /dev/mapper/test)/queue/write_zeroes_unmap")"
> +	local dname
> +	if ! dname=$(setup_test_device lbprz=0); then
> +		SKIP_REASONS+=("kernel does not support unmap write zeroes sysfs interface")
> +		return 1
> +	fi
> +	umap="$(cat "/sys/block/${dname}/queue/zoned")"
>  	if [[ $umap -ne 0 ]]; then
>  		echo "Test disable WRITE SAME with unmap failed."
>  	fi
>  	cleanup_test_device
>