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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <6p2dh577oiqe7lfaexv4fzct4aqhc56lxrz2ecwwctvbuxrjx3@oual7hmxfiqc>
Date: Mon, 28 Apr 2025 08:13:41 +0000
From: Shinichiro Kawasaki <shinichiro.kawasaki@....com>
To: Zhang Yi <yi.zhang@...weicloud.com>
CC: "linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
	"linux-ext4@...r.kernel.org" <linux-ext4@...r.kernel.org>,
	"linux-block@...r.kernel.org" <linux-block@...r.kernel.org>,
	"dm-devel@...ts.linux.dev" <dm-devel@...ts.linux.dev>,
	"linux-nvme@...ts.infradead.org" <linux-nvme@...ts.infradead.org>,
	"linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>,
	"linux-xfs@...r.kernel.org" <linux-xfs@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, hch
	<hch@....de>, "tytso@....edu" <tytso@....edu>, "djwong@...nel.org"
	<djwong@...nel.org>, "john.g.garry@...cle.com" <john.g.garry@...cle.com>,
	"bmarzins@...hat.com" <bmarzins@...hat.com>, "chaitanyak@...dia.com"
	<chaitanyak@...dia.com>, "yi.zhang@...wei.com" <yi.zhang@...wei.com>,
	"chengzhihao1@...wei.com" <chengzhihao1@...wei.com>, "yukuai3@...wei.com"
	<yukuai3@...wei.com>, "yangerkun@...wei.com" <yangerkun@...wei.com>
Subject: Re: [PATCH blktests 2/3] dm/003: add unmap write zeroes tests

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