[PATCH blktests 0/3] blktest: add unmap write zeroes tests

[PATCH blktests 0/3] blktest: add unmap write zeroes tests

[Zhang Yi]

From: Zhang Yi <yi.zhang@huawei.com>

The Linux kernel is planning to support FALLOC_FL_WRITE_ZEROES in
fallocate(2). Add tests for the newly added BLK_FEAT_WRITE_ZEROES_UNMAP
feature flag on the block device queue limit. These tests test block
device unmap write zeroes sysfs interface

        /sys/block/<disk>/queue/write_zeroes_unmap

with various SCSI/NVMe/device-mapper devices.

The /sys/block/<disk>/queue/write_zeroes_unmap interface should return
1 if the block device supports unmap write zeroes command, and it should
return 0 otherwise.

 - scsi/010 test SCSI devices.
 - dm/003 test device mapper stacked devices.
 - nvme/060 test NVMe devices.

Thanks,
Yi.

Zhang Yi (3):
  scsi/010: add unmap write zeroes tests
  dm/003: add unmap write zeroes tests
  nvme/060: add unmap write zeroes tests

 common/rc          | 16 +++++++++++
 tests/dm/003       | 57 ++++++++++++++++++++++++++++++++++++++
 tests/dm/003.out   |  2 ++
 tests/nvme/060     | 68 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/nvme/060.out |  4 +++
 tests/scsi/010     | 56 ++++++++++++++++++++++++++++++++++++++
 tests/scsi/010.out |  2 ++
 7 files changed, 205 insertions(+)
 create mode 100755 tests/dm/003
 create mode 100644 tests/dm/003.out
 create mode 100755 tests/nvme/060
 create mode 100644 tests/nvme/060.out
 create mode 100755 tests/scsi/010
 create mode 100644 tests/scsi/010.out

-- 
2.46.1

[PATCH blktests 1/3] scsi/010: add unmap write zeroes tests

[Zhang Yi]

From: Zhang Yi <yi.zhang@huawei.com>

Test block device unmap write zeroes sysfs interface with various SCSI
debug devices. The /sys/block/<disk>/queue/write_zeroes_unmap interface
should return 1 if the SCSI device enable the WRITE SAME command with
unmap functionality, and it should return 0 otherwise.

Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
---
 tests/scsi/010     | 56 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/scsi/010.out |  2 ++
 2 files changed, 58 insertions(+)
 create mode 100755 tests/scsi/010
 create mode 100644 tests/scsi/010.out

diff --git a/tests/scsi/010 b/tests/scsi/010
new file mode 100755
index 0000000..27a672c
--- /dev/null
+++ b/tests/scsi/010
@@ -0,0 +1,56 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-3.0+
+# Copyright (C) 2025 Huawei.
+#
+# Test block device unmap write zeroes sysfs interface with various scsi
+# devices.
+
+. tests/scsi/rc
+. common/scsi_debug
+
+DESCRIPTION="test unmap write zeroes sysfs interface with scsi devices"
+QUICK=1
+
+requires() {
+	_have_scsi_debug
+}
+
+device_requries() {
+	_require_test_dev_sysfs queue/write_zeroes_unmap
+}
+
+test() {
+	echo "Running ${TEST_NAME}"
+
+	# disable WRITE SAME with unmap
+	if ! _configure_scsi_debug lbprz=0; then
+		return 1
+	fi
+	umap="$(cat "/sys/block/${SCSI_DEBUG_DEVICES[0]}/queue/write_zeroes_unmap")"
+	if [[ $umap -ne 0 ]]; then
+		echo "Test disable WRITE SAME with unmap failed."
+	fi
+	_exit_scsi_debug
+
+	# enable WRITE SAME(16) with unmap
+	if ! _configure_scsi_debug lbprz=1 lbpws=1; then
+		return 1
+	fi
+	umap="$(cat "/sys/block/${SCSI_DEBUG_DEVICES[0]}/queue/write_zeroes_unmap")"
+	if [[ $umap -ne 1 ]]; then
+		echo "Test enable WRITE SAME(16) with unmap failed."
+	fi
+	_exit_scsi_debug
+
+	# enable WRITE SAME(10) with unmap
+	if ! _configure_scsi_debug lbprz=1 lbpws10=1; then
+		return 1
+	fi
+	umap="$(cat "/sys/block/${SCSI_DEBUG_DEVICES[0]}/queue/write_zeroes_unmap")"
+	if [[ $umap -ne 1 ]]; then
+		echo "Test enable WRITE SAME(10) with unmap failed."
+	fi
+	_exit_scsi_debug
+
+	echo "Test complete"
+}
diff --git a/tests/scsi/010.out b/tests/scsi/010.out
new file mode 100644
index 0000000..6581d5e
--- /dev/null
+++ b/tests/scsi/010.out
@@ -0,0 +1,2 @@
+Running scsi/010
+Test complete
-- 
2.46.1

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

[Zhang Yi]

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.1

[PATCH blktests 3/3] nvme/060: add unmap write zeroes tests

[Zhang Yi]

From: Zhang Yi <yi.zhang@huawei.com>

Test block device unmap write zeroes sysfs interface with NVMeT devices
which are based on various SCSI debug devices. The
/sys/block/<disk>/queue/write_zeroes_unmap interface should return 1 if
the NVMeT devices support the unmap write zeroes command, and it should
return 0 otherwise.

Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
---
 tests/nvme/060     | 68 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/nvme/060.out |  4 +++
 2 files changed, 72 insertions(+)
 create mode 100755 tests/nvme/060
 create mode 100644 tests/nvme/060.out

diff --git a/tests/nvme/060 b/tests/nvme/060
new file mode 100755
index 0000000..524176f
--- /dev/null
+++ b/tests/nvme/060
@@ -0,0 +1,68 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-3.0+
+# Copyright (C) 2025 Huawei.
+#
+# Test block device unmap write zeroes sysfs interface with nvmet scsi
+# debug devices.
+
+. tests/nvme/rc
+. common/scsi_debug
+
+DESCRIPTION="test unmap write zeroes sysfs interface with nvmet devices"
+QUICK=1
+
+nvme_trtype=loop
+nvmet_blkdev_type="device"
+
+requires() {
+	_have_scsi_debug
+	_nvme_requires
+	_require_nvme_trtype_is_loop
+}
+
+device_requries() {
+	_require_test_dev_sysfs queue/write_zeroes_unmap
+}
+
+setup_test_device() {
+	if ! _configure_scsi_debug "$@"; then
+		return 1
+	fi
+
+	local port="$(_create_nvmet_port)"
+	_create_nvmet_subsystem --blkdev "/dev/${SCSI_DEBUG_DEVICES[0]}"
+	_add_nvmet_subsys_to_port "${port}" "${def_subsysnqn}"
+
+	_create_nvmet_host "${def_subsysnqn}" "${def_hostnqn}"
+	_nvme_connect_subsys
+}
+
+cleanup_test_device() {
+	_nvme_disconnect_subsys
+	_nvmet_target_cleanup --subsysnqn "${def_subsysnqn}"
+	_exit_scsi_debug
+}
+
+test() {
+	echo "Running ${TEST_NAME}"
+
+	_setup_nvmet
+
+	# disable WRITE SAME with unmap
+	setup_test_device lbprz=0
+	umap="$(cat "/sys/block/$(_find_nvme_ns "${def_subsys_uuid}")/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/$(_find_nvme_ns "${def_subsys_uuid}")/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/nvme/060.out b/tests/nvme/060.out
new file mode 100644
index 0000000..e60714d
--- /dev/null
+++ b/tests/nvme/060.out
@@ -0,0 +1,4 @@
+Running nvme/060
+disconnected 1 controller(s)
+disconnected 1 controller(s)
+Test complete
-- 
2.46.1

Re: [PATCH blktests 1/3] scsi/010: add unmap write zeroes tests

[Shinichiro Kawasaki]

Hello Zhang, thank you for the patches.

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 various SCSI
> debug devices. The /sys/block/<disk>/queue/write_zeroes_unmap interface
> should return 1 if the SCSI device enable the WRITE SAME command with
> unmap functionality, and it should return 0 otherwise.
> 
> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
> ---
>  tests/scsi/010     | 56 ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/scsi/010.out |  2 ++
>  2 files changed, 58 insertions(+)
>  create mode 100755 tests/scsi/010
>  create mode 100644 tests/scsi/010.out
> 
> diff --git a/tests/scsi/010 b/tests/scsi/010
> new file mode 100755
> index 0000000..27a672c
> --- /dev/null
> +++ b/tests/scsi/010
> @@ -0,0 +1,56 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-3.0+
> +# Copyright (C) 2025 Huawei.
> +#
> +# Test block device unmap write zeroes sysfs interface with various scsi
> +# devices.
> +
> +. tests/scsi/rc
> +. common/scsi_debug
> +
> +DESCRIPTION="test unmap write zeroes sysfs interface with scsi devices"
> +QUICK=1
> +
> +requires() {
> +	_have_scsi_debug
> +}
> +
> +device_requries() {
> +	_require_test_dev_sysfs queue/write_zeroes_unmap
> +}

The device_requries() hook does not work for test cases which implement test().
It is rather dirty, but I think we need to delay the check for
write_zeroes_unmap sysfs attribute availability until test() gets called.
See below for my idea.

> +
> +test() {
> +	echo "Running ${TEST_NAME}"
> +
> +	# disable WRITE SAME with unmap
> +	if ! _configure_scsi_debug lbprz=0; then
> +		return 1
> +	fi

I suggest to check queue/write_zeroes_unmap here. If it's not available, set
SKIP_REASONS and return like this (totally untested):

	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

> +	umap="$(cat "/sys/block/${SCSI_DEBUG_DEVICES[0]}/queue/write_zeroes_unmap")"
> +	if [[ $umap -ne 0 ]]; then
> +		echo "Test disable WRITE SAME with unmap failed."
> +	fi
> +	_exit_scsi_debug
> +
> +	# enable WRITE SAME(16) with unmap
> +	if ! _configure_scsi_debug lbprz=1 lbpws=1; then
> +		return 1
> +	fi
> +	umap="$(cat "/sys/block/${SCSI_DEBUG_DEVICES[0]}/queue/write_zeroes_unmap")"
> +	if [[ $umap -ne 1 ]]; then
> +		echo "Test enable WRITE SAME(16) with unmap failed."
> +	fi
> +	_exit_scsi_debug
> +
> +	# enable WRITE SAME(10) with unmap
> +	if ! _configure_scsi_debug lbprz=1 lbpws10=1; then
> +		return 1
> +	fi
> +	umap="$(cat "/sys/block/${SCSI_DEBUG_DEVICES[0]}/queue/write_zeroes_unmap")"
> +	if [[ $umap -ne 1 ]]; then
> +		echo "Test enable WRITE SAME(10) with unmap failed."
> +	fi
> +	_exit_scsi_debug
> +
> +	echo "Test complete"
> +}
> diff --git a/tests/scsi/010.out b/tests/scsi/010.out
> new file mode 100644
> index 0000000..6581d5e
> --- /dev/null
> +++ b/tests/scsi/010.out
> @@ -0,0 +1,2 @@
> +Running scsi/010
> +Test complete
> -- 
> 2.46.1
> 

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

[Shinichiro Kawasaki]

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.

> 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)

Re: [PATCH blktests 3/3] nvme/060: add unmap write zeroes tests

[Shinichiro Kawasaki]

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 NVMeT devices
> which are based on various SCSI debug devices. The
> /sys/block/<disk>/queue/write_zeroes_unmap interface should return 1 if
> the NVMeT devices support the unmap write zeroes command, and it should
> return 0 otherwise.
> 
> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
> ---
>  tests/nvme/060     | 68 ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/nvme/060.out |  4 +++
>  2 files changed, 72 insertions(+)
>  create mode 100755 tests/nvme/060
>  create mode 100644 tests/nvme/060.out
> 
> diff --git a/tests/nvme/060 b/tests/nvme/060
> new file mode 100755
> index 0000000..524176f
> --- /dev/null
> +++ b/tests/nvme/060
> @@ -0,0 +1,68 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-3.0+
> +# Copyright (C) 2025 Huawei.
> +#
> +# Test block device unmap write zeroes sysfs interface with nvmet scsi
> +# debug devices.
> +
> +. tests/nvme/rc
> +. common/scsi_debug
> +
> +DESCRIPTION="test unmap write zeroes sysfs interface with nvmet devices"
> +QUICK=1
> +
> +nvme_trtype=loop
> +nvmet_blkdev_type="device"
> +
> +requires() {
> +	_have_scsi_debug
> +	_nvme_requires
> +	_require_nvme_trtype_is_loop
> +}
> +
> +device_requries() {
> +	_require_test_dev_sysfs queue/write_zeroes_unmap
> +}

Same comment as the 1st and the 2nd patches.

> +
> +setup_test_device() {
> +	if ! _configure_scsi_debug "$@"; then
> +		return 1
> +	fi

I suggest the same change as the 2nd patch to check
queue/write_zeroes_unmap here...

> +
> +	local port="$(_create_nvmet_port)"
> +	_create_nvmet_subsystem --blkdev "/dev/${SCSI_DEBUG_DEVICES[0]}"
> +	_add_nvmet_subsys_to_port "${port}" "${def_subsysnqn}"
> +
> +	_create_nvmet_host "${def_subsysnqn}" "${def_hostnqn}"
> +	_nvme_connect_subsys
> +}
> +
> +cleanup_test_device() {
> +	_nvme_disconnect_subsys
> +	_nvmet_target_cleanup --subsysnqn "${def_subsysnqn}"
> +	_exit_scsi_debug
> +}
> +
> +test() {
> +	echo "Running ${TEST_NAME}"
> +
> +	_setup_nvmet
> +
> +	# disable WRITE SAME with unmap
> +	setup_test_device lbprz=0

and here, to check the setup_test_device() return value.

Re: [PATCH blktests 0/3] blktest: add unmap write zeroes tests

[Shinichiro Kawasaki]

On Mar 18, 2025 / 15:28, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@huawei.com>
> 
> The Linux kernel is planning to support FALLOC_FL_WRITE_ZEROES in
> fallocate(2). Add tests for the newly added BLK_FEAT_WRITE_ZEROES_UNMAP
> feature flag on the block device queue limit. These tests test block
> device unmap write zeroes sysfs interface
> 
>         /sys/block/<disk>/queue/write_zeroes_unmap
> 
> with various SCSI/NVMe/device-mapper devices.
> 
> The /sys/block/<disk>/queue/write_zeroes_unmap interface should return
> 1 if the block device supports unmap write zeroes command, and it should
> return 0 otherwise.
> 
>  - scsi/010 test SCSI devices.
>  - dm/003 test device mapper stacked devices.
>  - nvme/060 test NVMe devices.

Zhang, thank you again for the patches. The test contents look meaningful
for me :)  When the kernel side changes get ready, I will run the test cases
and do further review.

One thing I noticed is that the patches trigger shellcheck warnings. When you
respin the patches, please run "make check" and address the warnings.

$ make check
shellcheck -x -e SC2119 -f gcc check common/* \
        tests/*/rc tests/*/[0-9]*[0-9] src/*.sh
common/rc:624:7: note: Use $(...) notation instead of legacy backticks `...`. [SC2006]
common/rc:626:7: note: Double quote to prevent globbing and word splitting. [SC2086]
common/rc:632:7: warning: Quote this to prevent word splitting. [SC2046]
common/rc:632:7: note: Useless echo? Instead of 'echo $(cmd)', just use 'cmd'. [SC2005]
common/rc:632:7: note: Use $(...) notation instead of legacy backticks `...`. [SC2006]
common/rc:632:17: warning: Quote this to prevent word splitting. [SC2046]
common/rc:632:29: note: Double quote to prevent globbing and word splitting. [SC2086]
tests/dm/003:28:8: warning: Declare and assign separately to avoid masking return values. [SC2155]
tests/nvme/060:32:8: warning: Declare and assign separately to avoid masking return values. [SC2155]
make: *** [Makefile:21: check] Error 1

Re: [PATCH blktests 1/3] scsi/010: add unmap write zeroes tests

[Zhang Yi]

Hello Shinichiro！

I apologize for the significant delay, and I greatly appreciate your
review and suggestions.

On 2025/4/3 15:26, Shinichiro Kawasaki wrote:
> Hello Zhang, thank you for the patches.
> 
> 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 various SCSI
>> debug devices. The /sys/block/<disk>/queue/write_zeroes_unmap interface
>> should return 1 if the SCSI device enable the WRITE SAME command with
>> unmap functionality, and it should return 0 otherwise.
>>
>> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
>> ---
>>  tests/scsi/010     | 56 ++++++++++++++++++++++++++++++++++++++++++++++
>>  tests/scsi/010.out |  2 ++
>>  2 files changed, 58 insertions(+)
>>  create mode 100755 tests/scsi/010
>>  create mode 100644 tests/scsi/010.out
>>
>> diff --git a/tests/scsi/010 b/tests/scsi/010
>> new file mode 100755
>> index 0000000..27a672c
>> --- /dev/null
>> +++ b/tests/scsi/010
>> @@ -0,0 +1,56 @@
>> +#!/bin/bash
>> +# SPDX-License-Identifier: GPL-3.0+
>> +# Copyright (C) 2025 Huawei.
>> +#
>> +# Test block device unmap write zeroes sysfs interface with various scsi
>> +# devices.
>> +
>> +. tests/scsi/rc
>> +. common/scsi_debug
>> +
>> +DESCRIPTION="test unmap write zeroes sysfs interface with scsi devices"
>> +QUICK=1
>> +
>> +requires() {
>> +	_have_scsi_debug
>> +}
>> +
>> +device_requries() {
>> +	_require_test_dev_sysfs queue/write_zeroes_unmap
>> +}
> 
> The device_requries() hook does not work for test cases which implement test().
> It is rather dirty, but I think we need to delay the check for
> write_zeroes_unmap sysfs attribute availability until test() gets called.
> See below for my idea.
> 

Indeed, I completely missed that.

>> +
>> +test() {
>> +	echo "Running ${TEST_NAME}"
>> +
>> +	# disable WRITE SAME with unmap
>> +	if ! _configure_scsi_debug lbprz=0; then
>> +		return 1
>> +	fi
> 
> I suggest to check queue/write_zeroes_unmap here. If it's not available, set
> SKIP_REASONS and return like this (totally untested):
> 
> 	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
> 

Yeah, I agree with you. For now, there is no helper available for
checking the sysfs interface of the SCSI debugging device.

I will add a new helper setup_test_device() in this test as the
following two patches do, and put this check into that helper.

Thanks,
Yi.

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

[Zhang Yi]

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)

Re: [PATCH blktests 0/3] blktest: add unmap write zeroes tests

[Zhang Yi]

On 2025/4/3 15:55, Shinichiro Kawasaki wrote:
> On Mar 18, 2025 / 15:28, Zhang Yi wrote:
>> From: Zhang Yi <yi.zhang@huawei.com>
>>
>> The Linux kernel is planning to support FALLOC_FL_WRITE_ZEROES in
>> fallocate(2). Add tests for the newly added BLK_FEAT_WRITE_ZEROES_UNMAP
>> feature flag on the block device queue limit. These tests test block
>> device unmap write zeroes sysfs interface
>>
>>         /sys/block/<disk>/queue/write_zeroes_unmap
>>
>> with various SCSI/NVMe/device-mapper devices.
>>
>> The /sys/block/<disk>/queue/write_zeroes_unmap interface should return
>> 1 if the block device supports unmap write zeroes command, and it should
>> return 0 otherwise.
>>
>>  - scsi/010 test SCSI devices.
>>  - dm/003 test device mapper stacked devices.
>>  - nvme/060 test NVMe devices.
> 
> Zhang, thank you again for the patches. The test contents look meaningful
> for me :)  When the kernel side changes get ready, I will run the test cases
> and do further review.
> 
> One thing I noticed is that the patches trigger shellcheck warnings. When you
> respin the patches, please run "make check" and address the warnings.
> 
> $ make check
> shellcheck -x -e SC2119 -f gcc check common/* \
>         tests/*/rc tests/*/[0-9]*[0-9] src/*.sh
> common/rc:624:7: note: Use $(...) notation instead of legacy backticks `...`. [SC2006]
> common/rc:626:7: note: Double quote to prevent globbing and word splitting. [SC2086]
> common/rc:632:7: warning: Quote this to prevent word splitting. [SC2046]
> common/rc:632:7: note: Useless echo? Instead of 'echo $(cmd)', just use 'cmd'. [SC2005]
> common/rc:632:7: note: Use $(...) notation instead of legacy backticks `...`. [SC2006]
> common/rc:632:17: warning: Quote this to prevent word splitting. [SC2046]
> common/rc:632:29: note: Double quote to prevent globbing and word splitting. [SC2086]
> tests/dm/003:28:8: warning: Declare and assign separately to avoid masking return values. [SC2155]
> tests/nvme/060:32:8: warning: Declare and assign separately to avoid masking return values. [SC2155]
> make: *** [Makefile:21: check] Error 1

Sure, I'll do this check in my next iteration.

Thanks,
Yi,

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

[Shinichiro Kawasaki]

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

Re: [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
> 

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

[Shinichiro Kawasaki]

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
 

Re: [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.