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

[Zhang Yi <yi.zhang@huaweicloud.com>]

On 2025/4/3 15:43, Shinichiro Kawasaki wrote:
> On Mar 18, 2025 / 15:28, Zhang Yi wrote:
>> From: Zhang Yi <yi.zhang@huawei.com>
>>
>> Test block device unmap write zeroes sysfs interface with device-mapper
>> stacked devices. The /sys/block/<disk>/queue/write_zeroes_unmap
>> interface should return 1 if the underlying devices support the unmap
>> write zeroes command, and it should return 0 otherwise.
>>
>> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
>> ---
>>  common/rc        | 16 ++++++++++++++
>>  tests/dm/003     | 57 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  tests/dm/003.out |  2 ++
>>  3 files changed, 75 insertions(+)
>>  create mode 100755 tests/dm/003
>>  create mode 100644 tests/dm/003.out
>>
>> diff --git a/common/rc b/common/rc
>> index bc6c2e4..60c21f2 100644
>> --- a/common/rc
>> +++ b/common/rc
>> @@ -615,3 +615,19 @@ _io_uring_restore()
>>  		echo "$IO_URING_DISABLED" > /proc/sys/kernel/io_uring_disabled
>>  	fi
>>  }
>> +
>> +# get real device path name by following link
>> +_real_dev()
>> +{
>> +	local dev=$1
>> +	if [ -b "$dev" ] && [ -L "$dev" ]; then
>> +		dev=`readlink -f "$dev"`
>> +	fi
>> +	echo $dev
>> +}
> 
> This helper function looks useful, and it looks reasonable to add it.
> 
>> +
>> +# basename of a device
>> +_short_dev()
>> +{
>> +	echo `basename $(_real_dev $1)`
>> +}
> 
> But I'm not sure about this one. The name "_short_dev" is not super
> clear for me.
> 

I copied these two helpers form the xfstests. :)

>> diff --git a/tests/dm/003 b/tests/dm/003
>> new file mode 100755
>> index 0000000..1013eb5
>> --- /dev/null
>> +++ b/tests/dm/003
>> @@ -0,0 +1,57 @@
>> +#!/bin/bash
>> +# SPDX-License-Identifier: GPL-3.0+
>> +# Copyright (C) 2025 Huawei.
>> +#
>> +# Test block device unmap write zeroes sysfs interface with device-mapper
>> +# stacked devices.
>> +
>> +. tests/dm/rc
>> +. common/scsi_debug
>> +
>> +DESCRIPTION="test unmap write zeroes sysfs interface with dm devices"
>> +QUICK=1
>> +
>> +requires() {
>> +	_have_scsi_debug
>> +}
>> +
>> +device_requries() {
>> +	_require_test_dev_sysfs queue/write_zeroes_unmap
>> +}
> 
> Same comment as the 1st patch: device_requries() does not work here.
> 
>> +
>> +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.

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?

Thanks,
Yi.

>> +}
>> +
>> +cleanup_test_device() {
>> +	dmsetup remove test
>> +	_exit_scsi_debug
>> +}
>> +
>> +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")"
> 
> I suggest to modify the two lines above as follows, to match with the other
> suggested changes:
> 
> 	local dname umap
> 	if ! dname=$(setup_test_device lbprz=0); then
> 		return 1
> 	fi
> 	umap="$(< "/sys/block/${dname}/queue/write_zeroes_unmap")"
> 
> (Please note that the suggested changes are untested)