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
