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