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

[Zhang Yi]

On 2025/4/28 17:25, Shinichiro Kawasaki wrote:
> On Apr 28, 2025 / 17:04, Zhang Yi wrote:
>> 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.
> 
> Ah, rigth. That's not good.
> How about to return differnt values for the failure case above,
> 
>>
>> 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
> 
> and this "should skip" case?
> 
> 
> diff --git a/tests/dm/003 b/tests/dm/003
> index 1013eb5..5e617fd 100755
> --- a/tests/dm/003
> +++ b/tests/dm/003
> @@ -19,14 +19,26 @@ device_requries() {
>  	_require_test_dev_sysfs queue/write_zeroes_unmap
>  }
>  
> +readonly TO_SKIP=255
> +
>  setup_test_device() {
> +	local dev blk_sz dpath
> +
>  	if ! _configure_scsi_debug "$@"; then
>  		return 1
>  	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 $TO_SKIP
> +	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 +50,25 @@ 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
> +	dname=$(setup_test_device lbprz=0)
> +	ret=$?
> +	if ((ret)); then
> +		if ((ret == TO_SKIP)); then
> +			SKIP_REASONS+=("kernel does not support unmap write zeroes sysfs interface")
> +		fi
> +		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
>  

Yeah, I believe this will work, and it looks good to me. Thank you for
the suggestion！

Thanks,
Yi.