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

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

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