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