On 2025/4/3 15:43, Shinichiro Kawasaki wrote:
> On Mar 18, 2025 / 15:28, Zhang Yi wrote:
>> From: Zhang Yi <yi.zh...@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.zh...@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)
