| 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
| ||
|
Message-ID: <pbuxtmwqk5bdmxckwwkjob3lh6mg3et52w3xdvsnjjwmofschm@pkotcbnp4ypt>
Date: Mon, 28 Apr 2025 09:25:13 +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 / 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
Powered by blists - more mailing lists