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: <a5d847d1-9799-4294-ac8f-e78d73e3733d@huaweicloud.com>
Date: Mon, 28 Apr 2025 17:04:34 +0800
From: Zhang Yi <yi.zhang@...weicloud.com>
To: Shinichiro Kawasaki <shinichiro.kawasaki@....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 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
>