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

Qu Wenruo
2025-06-06T06:01:07Z

在 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
>>>>
>>>>
>>>
>>>
>>
>