On 2/7/24 12:58, Christian Brauner wrote:
On Fri, Jun 28, 2024 at 01:13:45PM GMT, Jan Kara wrote:
On Fri 28-06-24 10:58:54, Ian Kent wrote:
On 27/6/24 19:54, Jan Kara wrote:
On Thu 27-06-24 09:11:14, Ian Kent wrote:
On 27/6/24 04:47, Matthew Wilcox wrote:
On Wed, Jun 26, 2024 at 04:07:49PM -0400, Lucas Karpinski wrote:
+++ b/fs/namespace.c
@@ -78,6 +78,7 @@ static struct kmem_cache *mnt_cache __ro_after_init;
static DECLARE_RWSEM(namespace_sem);
static HLIST_HEAD(unmounted); /* protected by namespace_sem */
static LIST_HEAD(ex_mountpoints); /* protected by namespace_sem */
+static bool lazy_unlock = false; /* protected by namespace_sem */
That's a pretty ugly way of doing it. How about this?
Ha!
That was my original thought but I also didn't much like changing all the
callers.
I don't really like the proliferation of these small helper functions either
but if everyone
is happy to do this I think it's a great idea.
So I know you've suggested removing synchronize_rcu_expedited() call in
your comment to v2. But I wonder why is it safe? I *thought*
synchronize_rcu_expedited() is there to synchronize the dropping of the
last mnt reference (and maybe something else) - see the comment at the
beginning of mntput_no_expire() - and this change would break that?
Interesting, because of the definition of lazy umount I didn't look closely
enough at that.
But I wonder, how exactly would that race occur, is holding the rcu read
lock sufficient since the rcu'd mount free won't be done until it's
released (at least I think that's how rcu works).
I'm concerned about a race like:
[path lookup] [umount -l]
...
path_put()
mntput(mnt)
mntput_no_expire(m)
rcu_read_lock();
if (likely(READ_ONCE(mnt->mnt_ns))) {
do_umount()
umount_tree()
...
mnt->mnt_ns = NULL;
...
namespace_unlock()
mntput(&m->mnt)
mntput_no_expire(mnt)
smp_mb();
mnt_add_count(mnt, -1);
count = mnt_get_count(mnt);
if (count != 0) {
...
return;
mnt_add_count(mnt, -1);
rcu_read_unlock();
return;
-> KABOOM, mnt->mnt_count dropped to 0 but nobody cleaned up the mount!
}
Yeah, I think that's a valid concern. mntput_no_expire() requires that
the last reference is dropped after an rcu grace period and that can
only be done by synchronize_rcu_*() (It could be reworked but that would
be quite ugly.). See also mnt_make_shortterm() caller's for kernel
initiated unmounts.
I've thought about this a couple of times now.
Isn't it the case here that the path lookup thread will have taken a
reference
(because it's calling path_put()) and the umount will have taken a
reference on
system call entry.
So for the mount being umounted the starting count will be at lest three
then if
the umount mntput() is called from namespace_unlock() it will correctly see
count != 0 and the path lookup mntput() to release it's reference
finally leaving
the mntput() of the path_put() from the top level system call function
to release
the last reference.
Once again I find myself thinking this should be independent of the rcu
wait because
only path walks done before the mount being detached can be happening
and the lockless
walks are done holding the rcu read lock and how likely is it a ref-walk
path lookup
(that should follow a failed rcu-walk in this case) has been able to
grab a reference
anyway?
I think the only reason the wait could be significant is to prevent
changes to the
structures concerned causing problems because they happen earlier than
can be
tolerated. That I can understand.
Mmm ... I feel like I'm starting to sound like a broken record ... oops!
Ian
