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

Zorro Lang
2025-06-08T12:14:02Z

On Fri, Jun 06, 2025 at 03:30:58PM +0930, Qu Wenruo wrote:
>
>
> 在 2025/6/6 10:36, Zorro Lang 写道:
> > 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 :)
>
> I thought it was recommended to let the test continue and rely on the golden
> output to detect errors.
>
> I'm fine either way. This is only triggered because I did cause a regression
> during my development.
>
> There is no real world hit (yet), so it's not that urgent.

OK, this patch is good to me:

Reviewed-by: Zorro Lang <zlang@redhat.com>

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