Date: Tue, 2 Jul 2024 06:50:02 +0200
From: Christian Brauner <brauner@...nel.org>
To: Ian Kent <raven@...maw.net>
Cc: Ian Kent <ikent@...hat.com>, Alexander Larsson <alexl@...hat.com>, 
	Jan Kara <jack@...e.cz>, Matthew Wilcox <willy@...radead.org>, 
	Lucas Karpinski <lkarpins@...hat.com>, viro@...iv.linux.org.uk, linux-fsdevel@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Eric Chanudet <echanude@...hat.com>
Subject: Re: [RFC v3 1/1] fs/namespace: remove RCU sync for MNT_DETACH umount

On Tue, Jul 02, 2024 at 09:29:49AM GMT, Ian Kent wrote:
> On 1/7/24 13:50, Christian Brauner wrote:
> > > I always thought the rcu delay was to ensure concurrent path walks "see" the
> > > 
> > > umount not to ensure correct operation of the following mntput()(s).
> > > 
> > > 
> > > Isn't the sequence of operations roughly, resolve path, lock, deatch,
> > > release
> > > 
> > > lock, rcu wait, mntput() subordinate mounts, put path.
> 
> Sorry but I'm still having trouble understanding the role of the rcu wait.

Ok, maybe I'm missing what you're after.

> 
> 
> > The crucial bit is really that synchronize_rcu_expedited() ensures that
> > the final mntput() won't happen until path walk leaves RCU mode.
> 
> Sure, that's easily seen, even for me, but the rcu read lock is held for
> 
> the duration of the rcu walk and not released until leaving rcu walk more
> 
> and, on fail, switches to ref walk mode and restarts. So the mount struct
> 
> won't be freed from under the process in rcu walk mode, correct?

Yes.

> 
> 
> > 
> > This allows caller's like legitimize_mnt() which are called with only
> > the RCU read-lock during lazy path walk to simple check for
> > MNT_SYNC_UMOUNT and see that the mnt is about to be killed. If they see
> > that this mount is MNT_SYNC_UMOUNT then they know that the mount won't
> > be freed until an RCU grace period is up and so they know that they can
> > simply put the reference count they took _without having to actually
> > call mntput()_.
> > 
> > Because if they did have to call mntput() they might end up shutting the
> > filesystem down instead of umount() and that will cause said EBUSY
> > errors I mentioned in my earlier mails.
> 
> Again, I get this too, but where is the need for the rcu wait in this?

The rcu wait is there to allow lazy path walk to not call mntput().
Otherwise you'll see these EBUSY issues. Maybe I'm confused now but you
just said that you got it. 

One thing that I had misremembered though is that a lazy umount won't
set MNT_SYNC_UMOUNT on the mounts it kills. So really for that case
synchronize_rcu_expedited() won't matter.

> Originally I had the notion that it was to ensure any path walkers had seen
> 
> the mount become invalid before tearing down things that enable the
> detection
> 
> but suddenly I don't get that any more ...

For a regular umount concurrent lazy path walkers want to be able to not
steal the last umount. So the synchronize_*() ensures that they all see
MNT_SYNC_UMOUNT and don't need to call mntput().

Afaict, what you're thinking about is handled by call_rcu(&mnt->mnt_rcu,
delayed-free_vfsmnt() in cleanup_mnt() which is always called and makes
sure that anyone still holding an rcu_read_lock() over that mount can
access the data.

That's how I had always understood it. 

> 
> 
> Please help me out here, I just don't get the need (and I'm sure there is
> 
> one) for the rcu wait.
> 
> 
> Ian
> 
> > 
> > > 
> > > So the mount gets detached in the critical section, then we wait followed by
> > > 
> > > the mntput()(s). The catch is that not waiting might increase the likelyhood
> > > 
> > > that concurrent path walks don't see the umount (so that possibly the umount
> > > 
> > > goes away before the walks see the umount) but I'm not certain. What looks
> > > to
> > > 
> > > be as much of a problem is mntput() racing with a concurrent mount beacase
> > > while
> > > 
> > > the detach is done in the critical section the super block instance list
> > > deletion
> > > 
> > > is not and the wait will make the race possibility more likely. What's more
> > Concurrent mounters of the same filesystem will wait for each other via
> > grab_super(). That has it's own logic based on sb->s_active which goes
> > to zero when all mounts are gone.
> > 
> > > mntput() delegates the mount cleanup (which deletes the list instance) to a
> > > 
> > > workqueue job so this can also occur serially in a following mount command.
> > No, that only happens when it's a kthread. Regular umount() call goes
> > via task work which finishes before the caller returns to userspace
> > (same as closing files work).
> > 
> > > 
> > > In fact I might have seen exactly this behavior in a recent xfs-tests run
> > > where I
> > > 
> > > was puzzled to see occasional EBUSY return on mounting of mounts that should
> > > not
> > > 
> > > have been in use following their umount.
> > That's usually very much other bugs. See commit 2ae4db5647d8 ("fs: don't
> > misleadingly warn during thaw operations") in vfs.fixes for example.
> > 
> > > 
> > > So I think there are problems here but I don't think the removal of the wait
> > > for
> > > 
> > > lazy umount is the worst of it.
> > > 
> > > 
> > > The question then becomes, to start with, how do we resolve this unjustified
> > > EBUSY
> > > 
> > > return. Perhaps a completion (used between the umount and mount system
> > > calls) would
> > > 
> > > work well here?
> > Again, this already exists deeper down the stack...

Powered by blists - more mailing lists