Re: [PATCH 2/4] fstests: make sure to unfreeze test and scratch mounts

[Dave Chinner <david@fromorbit.com>]

On Sun, Jun 19, 2022 at 04:46:55PM +0300, Amir Goldstein wrote:
> Almost all of the tests that _require_freeze() fail to unfreeze
> scratch mount in case the test is interrupted while fs is frozen.
> 
> Move the handling of unfreeze to generic check code.
> For now, tests only freeze scratch fs, but to be more robust, unfreeze
> both test and scratch fs following a call to _require_freeze().
> 
> Tests could still hang if thier private _cleanup() routine tries
> to modify the frozen fs or wait for a blocked process. Fix the
> _cleanup() routine of xfs/011 to avoid that.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  check             | 14 ++++++++------
>  common/rc         |  5 +++--
>  tests/generic/390 |  2 --
>  tests/xfs/011     |  2 --
>  tests/xfs/517     |  1 -
>  5 files changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/check b/check
> index de11b37e..d6ee71aa 100755
> --- a/check
> +++ b/check
> @@ -527,17 +527,21 @@ _check_filesystems()
>  {
>  	local ret=0
>  
> +	# Make sure both test and scratch are unfrozen post _require_freeze()
> +	if [ -f ${RESULT_DIR}/require_freeze ]; then
> +		xfs_freeze -u "$TEST_DIR" >/dev/null 2>&1
> +		xfs_freeze -u "$SCRATCH_MNT" >/dev/null 2>&1
> +	fi

A test leaving a filesystem frozen on exit is a test bug. There can
still be background test processes sitting blocked on a frozen
filesystem when the test exits with a frozen filesystem, and that
has the potential to cause problems in the next few operations
because of "busy filesystem" errors trying to unmount the fs...

IOWs, think this is the wrong way to address this problem. tests
that freeze filesystems need to ensure that everything is cleaned up
properly in the test _cleanup() function where the right thing can
be done and blocked processes can be waited on once the fs has been
thawed.

> diff --git a/tests/generic/390 b/tests/generic/390
> index 20c66e22..0f2b86fa 100755
> --- a/tests/generic/390
> +++ b/tests/generic/390
> @@ -14,8 +14,6 @@ _begin_fstest auto freeze stress
>  _cleanup()
>  {
>  	cd /
> -	# Make sure $SCRATCH_MNT is unfreezed
> -	xfs_freeze -u $SCRATCH_MNT 2>/dev/null
>  	rm -f $tmp.*
>  }

This test is already doing the right thing.

> diff --git a/tests/xfs/011 b/tests/xfs/011
> index d6e9099e..351a574e 100755
> --- a/tests/xfs/011
> +++ b/tests/xfs/011
> @@ -17,9 +17,7 @@ _begin_fstest auto freeze log metadata quick
>  _cleanup()
>  {
>  	$KILLALL_PROG -9 fsstress 2>/dev/null
> -	wait
>  	cd /
> -	_scratch_unmount 2>/dev/null
>  	rm -f $tmp.*
>  }

This is wrong. We have to wait for background fsstress processes to
exit, otherwise unmount can fail randomly. What it is missing is the
thaw before killing the fsstress processes and waiting for them to
complete.

> diff --git a/tests/xfs/517 b/tests/xfs/517
> index f7f9a8a2..961668e3 100755
> --- a/tests/xfs/517
> +++ b/tests/xfs/517
> @@ -15,7 +15,6 @@ _register_cleanup "_cleanup" BUS
>  _cleanup()
>  {
>  	cd /
> -	$XFS_IO_PROG -x -c 'thaw' $SCRATCH_MNT > /dev/null 2>&1
>  	rm -rf $tmp.*
>  }

This is doing the right thing, too.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com