Re: [PATCH] common/rc: fix _try_scratch_mount() and _test_mount() when mount fails

["Darrick J. Wong" <djwong@kernel.org>]

On Mon, Apr 11, 2022 at 03:08:38PM +0100, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> After the recent commit 4a7b35d7a76cd9 ("common: allow to run all tests
> on idmapped mounts"), some test that use _try_scratch_mount started to
> fail. For example:
> 
> $ ./check btrfs/131 btrfs/220
> FSTYP         -- btrfs
> PLATFORM      -- Linux/x86_64 debian9 5.17.0-rc8-btrfs-next-114 (...)
> MKFS_OPTIONS  -- /dev/sdc
> MOUNT_OPTIONS -- /dev/sdc /home/fdmanana/btrfs-tests/scratch_1
> 
> btrfs/131 2s ... - output mismatch (see .../results//btrfs/131.out.bad)
>     --- tests/btrfs/131.out	2020-06-10 19:29:03.818519162 +0100
>     +++ /home/fdmanana/git/hub/xfstests/results//btrfs/131.out.bad (...)
>     @@ -6,8 +6,6 @@
>      Disabling free space cache and enabling free space tree
>      free space tree is enabled
>      Trying to mount without free space tree
>     -mount failed
>     -mount failed
>      Mounting existing free space tree
>      free space tree is enabled
>     ...
>     (Run 'diff -u /home/fdmanana/git/hub/xfstests/tests/btrfs/131.out ...
> btrfs/220 7s ... - output mismatch (see .../results//btrfs/220.out.bad)
>     --- tests/btrfs/220.out	2020-10-16 23:13:46.802162554 +0100
>     +++ /home/fdmanana/git/hub/xfstests/results//btrfs/220.out.bad (...)
>     @@ -1,2 +1,32 @@
>      QA output created by 220
>     +Option fragment=invalid should fail to mount
>     +umount: /home/fdmanana/btrfs-tests/scratch_1: not mounted.
>     +Option nologreplay should fail to mount
>     +umount: /home/fdmanana/btrfs-tests/scratch_1: not mounted.
>     +Option norecovery should fail to mount
>     +umount: /home/fdmanana/btrfs-tests/scratch_1: not mounted.
>     ...
>     (Run 'diff -u /home/fdmanana/git/hub/xfstests/tests/btrfs/220.out ...
> Ran: btrfs/131 btrfs/220
> Failures: btrfs/131 btrfs/220
> Failed 2 of 2 tests
> 
> The reason is that if _try_scratch_mount() fails to mount the filesystem,
> we don't return the failure, instead we call _idmapped_mount(), which
> can succeed and make _try_scratch_mount() return 0 (success). The same
> happens for _test_mount(), however a quick search revealed no tests
> currently relying on the return value of _test_mount().
> 
> So fix that by making _try_scratch_mount() return immediately if it gets
> a mount failure. Also do the same for _test_mount().
> 
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

Aha, that's why the test cloud reported a 4% test failure rate.

Well, this fixes things, so:
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  common/rc | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/common/rc b/common/rc
> index 17629801..37d18599 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -329,11 +329,15 @@ _supports_filetype()
>  # mount scratch device with given options but don't check mount status
>  _try_scratch_mount()
>  {
> +	local mount_ret
> +
>  	if [ "$FSTYP" == "overlay" ]; then
>  		_overlay_scratch_mount $*
>  		return $?
>  	fi
>  	_mount -t $FSTYP `_scratch_mount_options $*`
> +	mount_ret=$?
> +	[ $mount_ret -ne 0 ] && return $mount_ret
>  	_idmapped_mount $SCRATCH_DEV $SCRATCH_MNT
>  }
>  
> @@ -494,12 +498,16 @@ _idmapped_mount()
>  
>  _test_mount()
>  {
> +    local mount_ret
> +
>      if [ "$FSTYP" == "overlay" ]; then
>          _overlay_test_mount $*
>          return $?
>      fi
>      _test_options mount
>      _mount -t $FSTYP $TEST_OPTIONS $TEST_FS_MOUNT_OPTS $SELINUX_MOUNT_OPTIONS $* $TEST_DEV $TEST_DIR
> +    mount_ret=$?
> +    [ $mount_ret -ne 0 ] && return $mount_ret
>      _idmapped_mount $TEST_DEV $TEST_DIR
>  }
>  
> -- 
> 2.35.1
>