diff mbox series

xfs/298: Add 100ms sleep before scratch_umount

Message ID 1652079417-2255-1-git-send-email-xuyang2018.jy@fujitsu.com (mailing list archive)
State New, archived
Headers show
Series xfs/298: Add 100ms sleep before scratch_umount | expand

Commit Message

Yang Xu (Fujitsu) May 9, 2022, 6:56 a.m. UTC
When testing this case on my machine, it reports the following error:
umount: /mnt/xfstests/scratch: target is busy.
xfs_db: /dev/sda11 contains a mounted filesystem

scratch_unmount failed, so  _scratch_xfs_db reports scratch_dev is a
mounted filesystem. It seems filesystem has something to be doing.

To avoid this, just add a 100ms sleep before scratch_umount.

Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
 tests/xfs/298 | 1 +
 1 file changed, 1 insertion(+)

Comments

Zorro Lang May 9, 2022, 1:42 p.m. UTC | #1
On Mon, May 09, 2022 at 02:56:57PM +0800, Yang Xu wrote:
> When testing this case on my machine, it reports the following error:
> umount: /mnt/xfstests/scratch: target is busy.
> xfs_db: /dev/sda11 contains a mounted filesystem
> 
> scratch_unmount failed, so  _scratch_xfs_db reports scratch_dev is a
> mounted filesystem. It seems filesystem has something to be doing.
> 
> To avoid this, just add a 100ms sleep before scratch_umount.
> 
> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
> ---
>  tests/xfs/298 | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/tests/xfs/298 b/tests/xfs/298
> index b0153ebf..17510379 100755
> --- a/tests/xfs/298
> +++ b/tests/xfs/298
> @@ -51,6 +51,7 @@ while [ $SIZE -lt 1024 ];do
>  	rm $SYMLINK_FILE
>  # umount and check the number of extents on the inode. Should be 0.
>  	cd /
> +	sleep 0.1

Hi

Actually I was wondering if we can add a while loop trying in _scratch_unmount,
to avoid some random "device busy" problem, likes:

Change _scratch_unmount to __scratch_unmount, then:

_scratch_unmount()
{
	local n
	local is_unmounted=1

	__scratch_unmount > $tmp.scratch_unmount.err 2>&1
	if [ $? -ne 0 -a "$MULTI_SCRATCH_UNMOUNT" = "yes" ];then
		for ((n=0; n<5; n++));do
			sleep 0.1
			__scratch_unmount >> $tmp.scratch_unmount.err 2>&1
			if [ $? -eq 0 ];then
				is_unmounted=0
				break
			fi
		done
	else
		is_unmounted=0
	fi
	if [ $is_unmounted -ne 0 ];then
		cat $tmp.scratch_unmount.err
	fi
	rm -f $tmp.scratch_unmount.err
	return $is_unmounted
}

This's just an idea out of my head, hope to hear more suggestions from other
forks.

Thanks,
Zorro

>  	_scratch_unmount >/dev/null 2>&1
>  	_scratch_xfs_db  -c "inode $inode" -c "p core.nextents"
>  
> -- 
> 2.23.0
>
Yang Xu (Fujitsu) May 10, 2022, 1:26 a.m. UTC | #2
on 2022/5/9 21:42, Zorro Lang wrote:
> On Mon, May 09, 2022 at 02:56:57PM +0800, Yang Xu wrote:
>> When testing this case on my machine, it reports the following error:
>> umount: /mnt/xfstests/scratch: target is busy.
>> xfs_db: /dev/sda11 contains a mounted filesystem
>>
>> scratch_unmount failed, so  _scratch_xfs_db reports scratch_dev is a
>> mounted filesystem. It seems filesystem has something to be doing.
>>
>> To avoid this, just add a 100ms sleep before scratch_umount.
>>
>> Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com>
>> ---
>>   tests/xfs/298 | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/tests/xfs/298 b/tests/xfs/298
>> index b0153ebf..17510379 100755
>> --- a/tests/xfs/298
>> +++ b/tests/xfs/298
>> @@ -51,6 +51,7 @@ while [ $SIZE -lt 1024 ];do
>>   	rm $SYMLINK_FILE
>>   # umount and check the number of extents on the inode. Should be 0.
>>   	cd /
>> +	sleep 0.1
>
> Hi
>
> Actually I was wondering if we can add a while loop trying in _scratch_unmount,
> to avoid some random "device busy" problem, likes:
>
> Change _scratch_unmount to __scratch_unmount, then:
>
> _scratch_unmount()
> {
> 	local n
> 	local is_unmounted=1
>
> 	__scratch_unmount>  $tmp.scratch_unmount.err 2>&1
> 	if [ $? -ne 0 -a "$MULTI_SCRATCH_UNMOUNT" = "yes" ];then
> 		for ((n=0; n<5; n++));do
> 			sleep 0.1
> 			__scratch_unmount>>  $tmp.scratch_unmount.err 2>&1
> 			if [ $? -eq 0 ];then
> 				is_unmounted=0
> 				break
> 			fi
> 		done
> 	else
> 		is_unmounted=0
> 	fi
> 	if [ $is_unmounted -ne 0 ];then
> 		cat $tmp.scratch_unmount.err
> 	fi
> 	rm -f $tmp.scratch_unmount.err
> 	return $is_unmounted
> }
>
> This's just an idea out of my head, hope to hear more suggestions from other
> forks.

I think this suggestion is meaningful and ltp also has a similar C api[1].

But we don't use this api in everywhere in ltp. So I don't use this api 
in everywhere in xfstests and just create a new function name to use it 
in situation that use many times mount/umount.

Also, xfstests some cases need to filter the stderr ie xfs/444.

So I think we can create a api that try to umount many times(5 or10) and
people can use this api if they meet the random ebusy problem instead of 
breaking the now _scratch_unmount api.

What do you think about it?

[1]https://github.com/linux-test-project/ltp/blob/master/lib/tst_device.c#L382

Best Regards
Yang Xu
>
> Thanks,
> Zorro
>
>>   	_scratch_unmount>/dev/null 2>&1
>>   	_scratch_xfs_db  -c "inode $inode" -c "p core.nextents"
>>
>> --
>> 2.23.0
>>
Yang Xu (Fujitsu) May 10, 2022, 1:38 a.m. UTC | #3
on 2022/5/9 21:42, Zorro Lang wrote:
> On Mon, May 09, 2022 at 02:56:57PM +0800, Yang Xu wrote:
>> When testing this case on my machine, it reports the following error:
>> umount: /mnt/xfstests/scratch: target is busy.
>> xfs_db: /dev/sda11 contains a mounted filesystem
>>
>> scratch_unmount failed, so  _scratch_xfs_db reports scratch_dev is a
>> mounted filesystem. It seems filesystem has something to be doing.
>>
>> To avoid this, just add a 100ms sleep before scratch_umount.
>>
>> Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com>
>> ---
>>   tests/xfs/298 | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/tests/xfs/298 b/tests/xfs/298
>> index b0153ebf..17510379 100755
>> --- a/tests/xfs/298
>> +++ b/tests/xfs/298
>> @@ -51,6 +51,7 @@ while [ $SIZE -lt 1024 ];do
>>   	rm $SYMLINK_FILE
>>   # umount and check the number of extents on the inode. Should be 0.
>>   	cd /
>> +	sleep 0.1
>
> Hi
>
> Actually I was wondering if we can add a while loop trying in _scratch_unmount,
> to avoid some random "device busy" problem, likes:
>
> Change _scratch_unmount to __scratch_unmount, then:
>
> _scratch_unmount()
> {
> 	local n
> 	local is_unmounted=1
>
> 	__scratch_unmount>  $tmp.scratch_unmount.err 2>&1
> 	if [ $? -ne 0 -a "$MULTI_SCRATCH_UNMOUNT" = "yes" ];then
> 		for ((n=0; n<5; n++));do
> 			sleep 0.1
> 			__scratch_unmount>>  $tmp.scratch_unmount.err 2>&1
> 			if [ $? -eq 0 ];then
> 				is_unmounted=0
> 				break
> 			fi
> 		done
> 	else
> 		is_unmounted=0
> 	fi
> 	if [ $is_unmounted -ne 0 ];then
> 		cat $tmp.scratch_unmount.err
> 	fi
> 	rm -f $tmp.scratch_unmount.err
> 	return $is_unmounted
> }
>
> This's just an idea out of my head, hope to hear more suggestions from other
> forks.

Another way is that introdued a environment variable, so people can 
enable this functionality in _scratch_unmount when meet ebusy problem.
By default, _scratch_unmount work as it did before.

Best Regards
Yang Xu
>
> Thanks,
> Zorro
>
>>   	_scratch_unmount>/dev/null 2>&1
>>   	_scratch_xfs_db  -c "inode $inode" -c "p core.nextents"
>>
>> --
>> 2.23.0
>>
Dave Chinner May 11, 2022, 5:29 a.m. UTC | #4
On Mon, May 09, 2022 at 02:56:57PM +0800, Yang Xu wrote:
> When testing this case on my machine, it reports the following error:
> umount: /mnt/xfstests/scratch: target is busy.
> xfs_db: /dev/sda11 contains a mounted filesystem
> 
> scratch_unmount failed, so  _scratch_xfs_db reports scratch_dev is a
> mounted filesystem. It seems filesystem has something to be doing.
> 
> To avoid this, just add a 100ms sleep before scratch_umount.
> 
> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
> ---
>  tests/xfs/298 | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/tests/xfs/298 b/tests/xfs/298
> index b0153ebf..17510379 100755
> --- a/tests/xfs/298
> +++ b/tests/xfs/298
> @@ -51,6 +51,7 @@ while [ $SIZE -lt 1024 ];do
>  	rm $SYMLINK_FILE
>  # umount and check the number of extents on the inode. Should be 0.
>  	cd /
> +	sleep 0.1
>  	_scratch_unmount >/dev/null 2>&1

What? No.

Never put random undocument sleeps in tests to hide failures - they
are almost always covering up a problem that needs to be understood
and fixed.

First thing to do here is understand why the filesystem is busy and
can't unmount. What is holding a reference that prevents unmount?

Is systemd or udev doing something stupid on your system and so
racing with unmount? Or is something else going on?

Cheers,

Dave.
diff mbox series

Patch

diff --git a/tests/xfs/298 b/tests/xfs/298
index b0153ebf..17510379 100755
--- a/tests/xfs/298
+++ b/tests/xfs/298
@@ -51,6 +51,7 @@  while [ $SIZE -lt 1024 ];do
 	rm $SYMLINK_FILE
 # umount and check the number of extents on the inode. Should be 0.
 	cd /
+	sleep 0.1
 	_scratch_unmount >/dev/null 2>&1
 	_scratch_xfs_db  -c "inode $inode" -c "p core.nextents"