> > From: Miklos Szeredi <mszeredi@xxxxxxx>
> >
> > 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;
}
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
