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
+}
+
+# basename of a device
+_short_dev()
+{
+ echo `basename $(_real_dev $1)`
+}
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
+}
+
+setup_test_device() {
+ if ! _configure_scsi_debug "$@"; then
+ return 1
+ fi
+
+ local dev="/dev/${SCSI_DEBUG_DEVICES[0]}"
+ local blk_sz="$(blockdev --getsz "$dev")"
+ dmsetup create test --table "0 $blk_sz linear $dev 0"
+}
+
+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")"
+ if [[ $umap -ne 0 ]]; then
+ echo "Test disable WRITE SAME with unmap failed."
+ fi
+ cleanup_test_device
+
+ # enable WRITE SAME with unmap
+ setup_test_device lbprz=1 lbpws=1
+ umap="$(cat "/sys/block/$(_short_dev /dev/mapper/test)/queue/write_zeroes_unmap")"
+ if [[ $umap -ne 1 ]]; then
+ echo "Test enable WRITE SAME with unmap failed."
+ fi
+ cleanup_test_device
+
+ echo "Test complete"
+}
diff --git a/tests/dm/003.out b/tests/dm/003.out
new file mode 100644
index 0000000..51a5405
--- /dev/null
+++ b/tests/dm/003.out
@@ -0,0 +1,2 @@
+Running dm/003
+Test complete
--
2.46.1On 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.
> 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.
> +
> + 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().
> +}
> +
> +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)
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)
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
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
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
>
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
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.
© 2016 - 2025 Red Hat, Inc.