All of lore.kernel.org
 help / color / mirror / Atom feed
From: Christian Brauner <brauner@kernel.org>
To: Jeff Layton <jlayton@kernel.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>,
	Dave Wysochanski <dwysocha@redhat.com>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>,
	linux-nfs <linux-nfs@vger.kernel.org>,
	David Howells <dhowells@redhat.com>, NeilBrown <neilb@suse.de>,
	Christoph Hellwig <hch@lst.de>
Subject: Re: allowing for a completely cached umount(2) pathwalk
Date: Fri, 14 Apr 2023 14:18:09 +0200	[thread overview]
Message-ID: <20230414-insignien-fordern-07551443dccd@brauner> (raw)
In-Reply-To: <8d2c619d2a91f3ac925fbc8e4fc467c6b137ab14.camel@kernel.org>

On Fri, Apr 14, 2023 at 06:01:59AM -0400, Jeff Layton wrote:
> On Fri, 2023-04-14 at 03:32 +0100, Al Viro wrote:
> > On Thu, Apr 13, 2023 at 06:00:42PM -0400, Jeff Layton wrote:
> > 
> > > It describes a situation where there are nested NFS mounts on a client,
> > > and one of the intermediate mounts ends up being unexported from the
> > > server. In a situation like this, we end up being unable to pathwalk
> > > down to the child mount of these unreachable dentries and can't unmount
> > > anything, even as root.
> > 
> > So umount -l the stuck sucker.  What's the problem with that?
> > 
> 
> You mean lazy umount the parent that is stuck? What happens to the child
> mount in that case? Is it also eventually cleaned up?

I hope it's ok I barge in to answer this but due to the mount beneath
patches I was working on I did spend even more time in that code then I
already did. So this is good chance to get yelled at if I analyzed these
codepaths wrong.

The child mount would be unmounted in that case. umount_tree() is what
you want to be looking at.

If you perform a regular umount _without_ MNT_DETACH you can see that
umount_tree() is effectively guarded by a call to propagate_mount_busy().
It checks wether the direct umount target has any child mounts and if so
refuses the umount with EBUSY:

        mkdir -p /mnt/a/b /mnt/c /mnt/d

	# Create parent mount of a@c
        mount --bind /mnt/a /mnt/c

	# create child d@b which as child mount of a@c
        mount --bind /mnt/d /mnt/c/b

If you call umount /mnt/c it will fail because a@c has child mounts.
If you do a lazy umount via MNT_DETACH through umount -l /mnt/c then it
will also unmount all children of a@c. In fact it will even include
children of children...

	mkdir /mnt/c/b/e
	mount --bind /mnt/a/b/ /mnt/c/b/e
	umount -l /mnt/c

That's basically what the next_mnt() loop at the beginning of
umount_tree() is doing where it collects all direct targets to umount.

However, if mount propagation is in play things get a lot nastier as you
can fail a non-MNT_DETACH umount because of it as well (Note that umount
propagation is always triggered if the parent mount of your direct
umount target is a shared mount. IOW, you can't easily opt out of it
unless you make the parent mount of your immediate umount target a
non-shared mount.).

A trivial reason that comes to mind where you would fail the umount due
to mount propagation would where a propagated mount is kept busy and not
the original mount. So similar to above on the host do:

        mkdir -p /mnt/a/b /mnt/c /mnt/d
        mount --bind /mnt/a /mnt/c
        umount /mnt/c

and you would expect the umount /mnt/c to work. But you realize it fails
with EBUSY but noone is referencing that mount anymore at least not in
an obvious way.

But assume someone had a mount namespace open that receives mount
propagation from /. In that case the a@c mount would have propagated
into that mount namespace. So someone could've cd /mnt/c into that
propagated mount and the umount /mnt/c would fail.

In that case propagate_mount_busy() would detect the increased refcount
when it tries to check whether the umount could be propagated and give
you EBUSY. So here you also need a lazy umount to get rid of that
mount... And there are other nice scenarios where that's hard to figure
out.

  reply	other threads:[~2023-04-14 12:18 UTC|newest]

Thread overview: 48+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-04-13 22:00 allowing for a completely cached umount(2) pathwalk Jeff Layton
2023-04-13 22:25 ` Andreas Dilger
2023-04-13 22:41 ` NeilBrown
2023-04-14  2:43   ` Al Viro
2023-04-14  3:28     ` Trond Myklebust
2023-04-14  3:51       ` Al Viro
2023-04-14  4:06         ` Trond Myklebust
2023-04-14  4:21           ` Al Viro
2023-04-14  9:41         ` Christian Brauner
2023-04-14 10:09           ` Jeff Layton
2023-04-14 11:16             ` Christian Brauner
2023-04-14 12:33               ` Jeff Layton
2023-04-14 12:51                 ` Christian Brauner
2023-04-15  9:51             ` Amir Goldstein
2023-04-14 10:06     ` Jeff Layton
2023-04-14 13:41       ` Christian Brauner
2023-04-14 14:21         ` Trond Myklebust
2023-04-14 15:13           ` Christian Brauner
2023-04-14 15:30             ` Trond Myklebust
2023-04-14 15:57               ` Trond Myklebust
2023-04-14 16:22                 ` Al Viro
2023-04-14 16:41                   ` Trond Myklebust
2023-04-14 19:01                     ` Benjamin Coddington
2023-04-17  8:22                       ` Christian Brauner
2023-04-14 16:32               ` Christian Brauner
2023-04-14  2:32 ` Al Viro
2023-04-14 10:01   ` Jeff Layton
2023-04-14 12:18     ` Christian Brauner [this message]
2023-04-14 14:57     ` Al Viro
2023-04-14 13:16   ` David Wysochanski
2023-04-16 23:13 ` [PATCH/RFC] VFS: LOOKUP_MOUNTPOINT should used cached info whenever possible NeilBrown
2023-04-17  1:08   ` kernel test robot
2023-04-17  3:51   ` kernel test robot
2023-04-17 11:55   ` Christian Brauner
2023-04-17 12:25     ` Jeff Layton
2023-04-17 14:24       ` Christian Brauner
2023-04-17 15:21         ` Jeff Layton
2023-04-17 21:34           ` NeilBrown
2023-04-18  8:10             ` Christian Brauner
2023-04-18  3:25           ` Andreas Dilger
2023-04-18  8:04             ` Christian Brauner
2023-04-20 13:05               ` Jeff Layton
2023-04-20 15:41                 ` Christian Brauner
2023-04-17 21:26     ` NeilBrown
2023-04-20 21:35       ` Al Viro
2023-04-20 22:01         ` NeilBrown
2023-04-20 22:27           ` Al Viro
2023-04-17 12:09   ` Jeff Layton

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20230414-insignien-fordern-07551443dccd@brauner \
    --to=brauner@kernel.org \
    --cc=dhowells@redhat.com \
    --cc=dwysocha@redhat.com \
    --cc=hch@lst.de \
    --cc=jlayton@kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-nfs@vger.kernel.org \
    --cc=neilb@suse.de \
    --cc=viro@zeniv.linux.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.