* [PATCH] fstests: generic/741: make cleanup to handle test failure properly
@ 2025-06-04 23:55 Qu Wenruo
2025-06-05 17:10 ` Zorro Lang
0 siblings, 1 reply; 6+ messages in thread
From: Qu Wenruo @ 2025-06-04 23:55 UTC (permalink / raw)
To: linux-btrfs, fstests
[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
_unmount $extra_mnt &> /dev/null
rm -rf $extra_mnt
_unmount_flakey
--
2.49.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] fstests: generic/741: make cleanup to handle test failure properly 2025-06-04 23:55 [PATCH] fstests: generic/741: make cleanup to handle test failure properly Qu Wenruo @ 2025-06-05 17:10 ` Zorro Lang 2025-06-05 22:09 ` Qu Wenruo 0 siblings, 1 reply; 6+ messages in thread From: Zorro Lang @ 2025-06-05 17:10 UTC (permalink / raw) To: Qu Wenruo; +Cc: linux-btrfs, fstests 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 :) Thanks, Zorro > _unmount $extra_mnt &> /dev/null > rm -rf $extra_mnt > _unmount_flakey > -- > 2.49.0 > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] fstests: generic/741: make cleanup to handle test failure properly 2025-06-05 17:10 ` Zorro Lang @ 2025-06-05 22:09 ` Qu Wenruo 2025-06-06 1:06 ` Zorro Lang 0 siblings, 1 reply; 6+ messages in thread From: Qu Wenruo @ 2025-06-05 22:09 UTC (permalink / raw) To: Zorro Lang, Qu Wenruo; +Cc: linux-btrfs, fstests 在 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. Thanks, Qu > > Thanks, > Zorro > >> _unmount $extra_mnt &> /dev/null >> rm -rf $extra_mnt >> _unmount_flakey >> -- >> 2.49.0 >> >> > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] fstests: generic/741: make cleanup to handle test failure properly 2025-06-05 22:09 ` Qu Wenruo @ 2025-06-06 1:06 ` Zorro Lang 2025-06-06 6:00 ` Qu Wenruo 0 siblings, 1 reply; 6+ messages in thread From: Zorro Lang @ 2025-06-06 1:06 UTC (permalink / raw) To: Qu Wenruo; +Cc: Qu Wenruo, linux-btrfs, fstests 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 > > > > > > > > > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] fstests: generic/741: make cleanup to handle test failure properly 2025-06-06 1:06 ` Zorro Lang @ 2025-06-06 6:00 ` Qu Wenruo 2025-06-08 12:13 ` Zorro Lang 0 siblings, 1 reply; 6+ messages in thread From: Qu Wenruo @ 2025-06-06 6:00 UTC (permalink / raw) To: Zorro Lang; +Cc: Qu Wenruo, linux-btrfs, fstests 在 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. Thanks, Qu > > Thanks, > Zorro > >> >> Thanks, >> Qu >> >>> >>> Thanks, >>> Zorro >>> >>>> _unmount $extra_mnt &> /dev/null >>>> rm -rf $extra_mnt >>>> _unmount_flakey >>>> -- >>>> 2.49.0 >>>> >>>> >>> >>> >> > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] fstests: generic/741: make cleanup to handle test failure properly 2025-06-06 6:00 ` Qu Wenruo @ 2025-06-08 12:13 ` Zorro Lang 0 siblings, 0 replies; 6+ messages in thread From: Zorro Lang @ 2025-06-08 12:13 UTC (permalink / raw) To: Qu Wenruo; +Cc: Qu Wenruo, linux-btrfs, fstests 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 > > > > > > > > > > > > > > > > > > > > > > > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-06-08 12:14 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-06-04 23:55 [PATCH] fstests: generic/741: make cleanup to handle test failure properly Qu Wenruo 2025-06-05 17:10 ` Zorro Lang 2025-06-05 22:09 ` Qu Wenruo 2025-06-06 1:06 ` Zorro Lang 2025-06-06 6:00 ` Qu Wenruo 2025-06-08 12:13 ` Zorro Lang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox