Re: [PATCH] fstests: generic/741: make cleanup to handle test failure properly

Zorro Lang
2025-06-06T01:06:41Z

On Fri, Jun 06, 2025 at 07:39:56AM +0930, Qu Wenruo wrote:
>
>
> 在 2025/6/6 02:40, Zorro Lang 写道:
> > On Thu, Jun 05, 2025 at 09:25:24AM +0930, Qu Wenruo wrote:
> > > [BUG]
> > > When I was tinkering the bdev open holder parameter, it caused a bug
> > > that it no longer rejects mounting the underlying device of a
> > > device-mapper.
> > >
> > > And the test case properly detects the regression:
> > >
> > > generic/741 1s ... umount: /mnt/test: target is busy.
> > > _check_btrfs_filesystem: filesystem on /dev/mapper/test-test is inconsistent
> > > (see /home/adam/xfstests/results//generic/741.full for details)
> > > Trying to repair broken TEST_DEV file system
> > > _check_btrfs_filesystem: filesystem on /dev/mapper/test-scratch1 is inconsistent
> > > (see /home/adam/xfstests/results//generic/741.full for details)
> > > - output mismatch (see /home/adam/xfstests/results//generic/741.out.bad)
> > > --- tests/generic/741.out 2024-04-06 08:10:44.773333344 +1030
> > > +++ /home/adam/xfstests/results//generic/741.out.bad 2025-06-05 09:18:03.675049206 +0930
> > > @@ -1,3 +1,2 @@
> > > QA output created by 741
> > > -mount: TEST_DIR/extra_mnt: SCRATCH_DEV already mounted or mount point busy
> > > -mount: TEST_DIR/extra_mnt: SCRATCH_DEV already mounted or mount point busy
> > > +rm: cannot remove '/mnt/test/extra_mnt': Device or resource busy
> > > ...
> > > (Run 'diff -u /home/adam/xfstests/tests/generic/741.out /home/adam/xfstests/results//generic/741.out.bad' to see the entire diff)
> > >
> > > The problem is, all later test will fail, because the $SCRATCH_DEV is
> > > still mounted at $extra_mnt:
> > >
> > > TEST_DEV=/dev/mapper/test-test is mounted but not on TEST_DIR=/mnt/test - aborting
> > > Already mounted result:
> > > /dev/mapper/test-test /mnt/test /dev/mapper/test-test /mnt/test
> > >
> > > [CAUSE]
> > > The test case itself is doing two expected-to-fail mounts, but the
> > > cleanup function is only doing unmount once, if the mount succeeded
> > > unexpectedly, the $SCRATCH_DEV will be mounted at $extra_mnt forever.
> > >
> > > [ENHANCEMENT]
> > > To avoid screwing up later test cases, do the $extra_mnt cleanup twice
> > > to handle the unexpected mount success.
> > >
> > > Signed-off-by: Qu Wenruo <wqu@suse.com>
> > > ---
> > > tests/generic/741 | 4 ++++
> > > 1 file changed, 4 insertions(+)
> > >
> > > diff --git a/tests/generic/741 b/tests/generic/741
> > > index cac7045e..c15dc434 100755
> > > --- a/tests/generic/741
> > > +++ b/tests/generic/741
> > > @@ -13,6 +13,10 @@ _begin_fstest auto quick volume tempfsid
> > > # Override the default cleanup function.
> > > _cleanup()
> > > {
> > > + # If by somehow the fs mounted the underlying device (twice), we have
> > > + # to make sure $extra_mnt is not mounted, or TEST_DEV can not be
> > > + # unmounted for fsck.
> > > + _unmount $extra_mnt &> /dev/null
> >
> > Hmm... I'm not sure a "double" (even treble) umount is good solution for
> > temporary "Device or resource busy" after umount. Many other cases might
> > hit this problem sometimes.
> > Maybe we can have a helper to do a certain umount which make sure the fs
> > is truely umounted before returning :)
>
> This is not about the umount after EBUSY.
>
> The problem is, the test case itself is expecting two mounts to fail.
> But if the mount succeeded, it will mount twice and need to be unmounted
> twice.
>
> It's to make the cleanup to match the test case's worst scenario.

Oh, sorry I didn't get your point. If so, how about _fail the case directly if
the first mount (which should be failed) passed, e.g.

diff --git a/tests/generic/741 b/tests/generic/741
index cac7045eb..5538b3a1b 100755
--- a/tests/generic/741
+++ b/tests/generic/741
@@ -44,6 +44,9 @@ mkdir -p $extra_mnt
# Filters alter the return code of the mount.
_mount $SCRATCH_DEV $extra_mnt 2>&1 | \
_filter_testdir_and_scratch | _filter_error_mount
+if [ ${PIPESTATUS[0]} -eq 0 ];then
+ _fail "Unexpected mount pass"
+fi

Anyway, I can't say which way is better, both are good to me, depends on your
choice :)

Thanks,
Zorro

>
> Thanks,
> Qu
>
> >
> > Thanks,
> > Zorro
> >
> > > _unmount $extra_mnt &> /dev/null
> > > rm -rf $extra_mnt
> > > _unmount_flakey
> > > --
> > > 2.49.0
> > >
> > >
> >
> >
>