Re: [PATCH 06/34] common/rc: revert recursive unmount in _clear_mount_stack

[Dave Chinner <david@fromorbit.com>]

On Tue, Feb 04, 2025 at 01:23:51PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Zorro complained about the following regression in generic/589 that I
> can't reproduce here on Debian 12:
....

> > Orignal _clear_mount_stack trys to umount all of them. But Dave gave -R option to
> > umount command, so
> >   `umount -R /mnt/test/589-dst/201227_mpC` and `umount -R /mnt/test/589-src/201227_mpA`
> > already umount /mnt/test/589-dst and /mnt/test/589-src. recursively.
> > Then `umount -R /mnt/test/589-dst` shows "not mounted".
> 
> I /think/ this is a result of commit 4c6bc4565105e6 performing this
> "conversion" in _clear_mount_stack:
> 
> -		$UMOUNT_PROG $MOUNTED_POINT_STACK
> +		_unmount -R $MOUNTED_POINT_STACK
> 
> This is not a 1:1 conversion here -- previously there was no
> -R(ecursive) unmount option, and now there is.  It looks as though
> umount parses /proc/self/mountinfo to figure out what to unmount, and
> maybe on Zorro's system it balks if the argument passed is not present
> in that file?  Debian 12's umount doesn't care.

Ah, I think I added that whilst trying to work out the weird
shenanigans that the mount namespace cloning that these tests did
caused. It was cloning a shared mount namespace that many tests
interacted and so any changes to the mount namespace inside that
test were visible everywhere.

If two of these mount namespace tests ran at the same time, they
stepped on each other and they failed to unmount their own stack
cleanly because of "mount busy at unmount" errors. This would then
cascade into other test failures because the test/scratch devices
couldn't be unmounted.

The fix I came up with was to run a recursive unmount for the stack,
which seemed to clear all the mounts the test made regardless of
whatever other test could see them. It didn't through any errors on
my test machines (debian unstable) so I promptly forgot about it.

> Regardless, there was no justification for this change in behavior that
> was buried in what is otherwise a hoist patch, so revert it.  The author
> can resubmit the change with proper documentation.

I suspect that changes made later on (i.e. private mount namespaces
per runner instance) fixed the underlying namespace interaction
problem and made the recursive unmount unnecessary. The lack of
errors from it meant I likely forgot about it before I posted the
initial RFC that was merged...

> 
> Cc: <fstests@vger.kernel.org> # v2024.12.08
> Fixes: 4c6bc4565105e6 ("fstests: clean up mount and unmount operations")
> Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
> ---
>  common/rc |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> 
> diff --git a/common/rc b/common/rc
> index 4658e3b8be747f..07646927bad523 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -334,7 +334,7 @@ _put_mount()
>  _clear_mount_stack()
>  {
>  	if [ -n "$MOUNTED_POINT_STACK" ]; then
> -		_unmount -R $MOUNTED_POINT_STACK
> +		_unmount $MOUNTED_POINT_STACK
>  	fi
>  	MOUNTED_POINT_STACK=""
>  }

<tests>

That doesn't appear to cause any problems on my current
check-parallel stack, so I think the above explains how it got
there.

Reviewed-by: Dave Chinner <dchinner@redhat.com>

-- 
Dave Chinner
david@fromorbit.com