lkml.org 
[lkml]   [2008]   [Mar]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: [patch 3/6] vfs: mountinfo stable peer group id
From
Date
> > From: Miklos Szeredi <mszeredi@suse.cz>
> >
> > Add a stable identifier for shared mounts.
>
> > +static DEFINE_SPINLOCK(mnt_pgid_lock);
>
> Um? Do you ever need to take it outside of vfsmount_lock?
>

Tried to think this through:

It's always called with namespace_sem, which is enough, no need for a
new lock. The bigger problem, is that it _is_ called with
vfsmount_lock in one case, which is bad, since the allocation may
sleep.

That is in do_change_type(). But do we really need to hold
vfsmount_lock in that case? I think not, the propagation tree has no
relevance outside namespace_sem, so that one should be sufficient.

This patch should fix it (untested, just for review).

I have a half mind to throw out the IDR allocation altogether, and
just go with a 64bit counter, some poeple would much prefer that...

Thanks,
Miklos

---
fs/namespace.c | 2 --
fs/pnode.c | 20 ++++++++------------
2 files changed, 8 insertions(+), 14 deletions(-)

Index: linux/fs/pnode.c
===================================================================
--- linux.orig/fs/pnode.c 2008-03-19 16:39:28.000000000 +0100
+++ linux/fs/pnode.c 2008-03-19 17:26:44.000000000 +0100
@@ -12,7 +12,6 @@
#include <linux/idr.h>
#include "pnode.h"

-static DEFINE_SPINLOCK(mnt_pgid_lock);
static DEFINE_IDA(mnt_pgid_ida);

/* return the next shared peer mount of @p */
@@ -37,19 +36,19 @@ static void __set_mnt_shared(struct vfsm
mnt->mnt_flags |= MNT_SHARED;
}

+/*
+ * - must hold namespace_sem to protect the mnt_pgid_ida
+ * - must not hold vfsmount_lock, because this function might sleep
+ */
void set_mnt_shared(struct vfsmount *mnt)
{
int res;

- retry:
- spin_lock(&mnt_pgid_lock);
- if (IS_MNT_SHARED(mnt)) {
- spin_unlock(&mnt_pgid_lock);
+ might_sleep();
+ if (IS_MNT_SHARED(mnt))
return;
- }
-
+ retry:
res = ida_get_new(&mnt_pgid_ida, &mnt->mnt_pgid);
- spin_unlock(&mnt_pgid_lock);
if (res == -EAGAIN) {
if (ida_pre_get(&mnt_pgid_ida, GFP_KERNEL))
goto retry;
@@ -159,11 +158,8 @@ static int do_make_slave(struct vfsmount
peer_mnt = NULL;
}

- if (IS_MNT_SHARED(mnt) && list_empty(&mnt->mnt_share)) {
- spin_lock(&mnt_pgid_lock);
+ if (IS_MNT_SHARED(mnt) && list_empty(&mnt->mnt_share))
ida_remove(&mnt_pgid_ida, mnt->mnt_pgid);
- spin_unlock(&mnt_pgid_lock);
- }
list_del_init(&mnt->mnt_share);

if (peer_mnt)
Index: linux/fs/namespace.c
===================================================================
--- linux.orig/fs/namespace.c 2008-03-19 16:39:28.000000000 +0100
+++ linux/fs/namespace.c 2008-03-19 17:23:06.000000000 +0100
@@ -1351,10 +1351,8 @@ static noinline int do_change_type(struc
return -EINVAL;

down_write(&namespace_sem);
- spin_lock(&vfsmount_lock);
for (m = mnt; m; m = (recurse ? next_mnt(m, mnt) : NULL))
change_mnt_propagation(m, type);
- spin_unlock(&vfsmount_lock);
up_write(&namespace_sem);
return 0;
}


\
 
 \ /
  Last update: 2008-03-20 01:19    [W:2.127 / U:0.172 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site