In linux-2.6.0-test1, lookup_mnt() was changed to increment the ref
count of the returned vfsmount struct. This breaks expiration of
autofs4 mounts, because lookup_mnt() is called in check_vfsmnt()
without decrementing the ref count afterwards. The following patch
fixes this:
--- linux-2.6.0-test2/fs/autofs4/expire.c.orig 2003-07-14 05:36:30.000000000 +0200
+++ linux-2.6.0-test2/fs/autofs4/expire.c 2003-08-05 20:51:57.000000000 +0200
@@ -70,6 +70,7 @@
int ret = dentry->d_mounted;
struct vfsmount *vfs = lookup_mnt(mnt, dentry);
+ mntput(vfs);
if (vfs && is_vfsmnt_tree_busy(vfs))
ret--;
DPRINTK(("check_vfsmnt: ret=%d\n", ret));
--
Dick Streefland //// De Bilt
[email protected] (@ @) The Netherlands
------------------------------oOO--(_)--OOo------------------
[email protected] (Dick Streefland) wrote:
>
> In linux-2.6.0-test1, lookup_mnt() was changed to increment the ref
> count of the returned vfsmount struct. This breaks expiration of
> autofs4 mounts, because lookup_mnt() is called in check_vfsmnt()
> without decrementing the ref count afterwards. The following patch
> fixes this:
>
Neat, thanks.
Probably we should hold onto that ref because we play with the vfsmount
later on. So something like this?
diff -puN fs/autofs4/expire.c~autofs4-expiry-fix fs/autofs4/expire.c
--- 25/fs/autofs4/expire.c~autofs4-expiry-fix 2003-08-05 16:44:41.000000000 -0700
+++ 25-akpm/fs/autofs4/expire.c 2003-08-05 16:48:20.000000000 -0700
@@ -25,7 +25,7 @@ static inline int is_vfsmnt_tree_busy(st
struct list_head *next;
int count;
- count = atomic_read(&mnt->mnt_count) - 1;
+ count = atomic_read(&mnt->mnt_count) - 1 - 1;
repeat:
next = this_parent->mnt_mounts.next;
@@ -70,8 +70,11 @@ static int check_vfsmnt(struct vfsmount
int ret = dentry->d_mounted;
struct vfsmount *vfs = lookup_mnt(mnt, dentry);
- if (vfs && is_vfsmnt_tree_busy(vfs))
- ret--;
+ if (vfs) {
+ if (is_vfsmnt_tree_busy(vfs))
+ ret = 0;
+ mntput(vfs);
+ }
DPRINTK(("check_vfsmnt: ret=%d\n", ret));
return ret;
}
_
On Tue, 2003-08-05 at 16:49, Andrew Morton wrote:
> Probably we should hold onto that ref because we play with the vfsmount
> later on. So something like this?
We're of one mind.
> + if (vfs) {
> + if (is_vfsmnt_tree_busy(vfs))
> + ret = 0;
> + mntput(vfs);
> + }
mntput will cheerfully ignore a NULL vfs, so I don't think the code
needs this much mashing.
J
On Tue, Aug 05, 2003 at 04:49:04PM -0700, Andrew Morton wrote:
> [email protected] (Dick Streefland) wrote:
> >
> > In linux-2.6.0-test1, lookup_mnt() was changed to increment the ref
> > count of the returned vfsmount struct. This breaks expiration of
> > autofs4 mounts, because lookup_mnt() is called in check_vfsmnt()
> > without decrementing the ref count afterwards. The following patch
> > fixes this:
> >
>
> Neat, thanks.
>
> Probably we should hold onto that ref because we play with the vfsmount
> later on. So something like this?
>
>
> diff -puN fs/autofs4/expire.c~autofs4-expiry-fix fs/autofs4/expire.c
> --- 25/fs/autofs4/expire.c~autofs4-expiry-fix 2003-08-05 16:44:41.000000000 -0700
> +++ 25-akpm/fs/autofs4/expire.c 2003-08-05 16:48:20.000000000 -0700
> @@ -25,7 +25,7 @@ static inline int is_vfsmnt_tree_busy(st
> struct list_head *next;
> int count;
>
> - count = atomic_read(&mnt->mnt_count) - 1;
> + count = atomic_read(&mnt->mnt_count) - 1 - 1;
>
> repeat:
> next = this_parent->mnt_mounts.next;
> @@ -70,8 +70,11 @@ static int check_vfsmnt(struct vfsmount
> int ret = dentry->d_mounted;
> struct vfsmount *vfs = lookup_mnt(mnt, dentry);
>
> - if (vfs && is_vfsmnt_tree_busy(vfs))
> - ret--;
> + if (vfs) {
> + if (is_vfsmnt_tree_busy(vfs))
> + ret = 0;
> + mntput(vfs);
> + }
> DPRINTK(("check_vfsmnt: ret=%d\n", ret));
> return ret;
> }
>
> _
Sorry, I don't think it is correct. This code is called under dcache_lock,
taken in is_tree_busy(). mntput() calls dput() and which can lead to deadlock.
I was thinking of some clean solution and trying to understand autofs4 but
some what lost in user mode utility, version 3 and 4, etc etc.
Dick, can you test the appended patch whether it works for you or not.
Thanks
Maneesh
diff -puN fs/autofs4/expire.c~autofs4-vfsmount-fix fs/autofs4/expire.c
--- linux-2.6.0-test2/fs/autofs4/expire.c~autofs4-vfsmount-fix 2003-08-06 09:10:49.000000000 +0530
+++ linux-2.6.0-test2-maneesh/fs/autofs4/expire.c 2003-08-06 09:24:07.000000000 +0530
@@ -25,7 +25,10 @@ static inline int is_vfsmnt_tree_busy(st
struct list_head *next;
int count;
- count = atomic_read(&mnt->mnt_count) - 1;
+ /* -1 for vfsmount's normal count,
+ * -1 for ref taken in lookup_mnt()
+ */
+ count = atomic_read(&mnt->mnt_count) - 1 - 1;
repeat:
next = this_parent->mnt_mounts.next;
@@ -71,7 +74,8 @@ static int check_vfsmnt(struct vfsmount
struct vfsmount *vfs = lookup_mnt(mnt, dentry);
if (vfs && is_vfsmnt_tree_busy(vfs))
- ret--;
+ ret = 0;
+ mntput(vfs);
DPRINTK(("check_vfsmnt: ret=%d\n", ret));
return ret;
}
@@ -96,8 +100,11 @@ static int is_tree_busy(struct vfsmount
DPRINTK(("is_tree_busy: autofs; count=%d\n", count));
}
- if (d_mountpoint(top))
+ if (d_mountpoint(top)) {
+ spin_unlock(&dcache_lock);
count -= check_vfsmnt(topmnt, top);
+ spin_lock(&dcache_lock);
+ }
repeat:
next = this_parent->d_subdirs.next;
@@ -110,8 +117,11 @@ static int is_tree_busy(struct vfsmount
count += atomic_read(&dentry->d_count) - 1;
- if (d_mountpoint(dentry))
+ if (d_mountpoint(dentry)) {
+ spin_unlock(&dcache_lock);
adj += check_vfsmnt(topmnt, dentry);
+ spin_lock(&dcache_lock);
+ }
if (is_autofs4_dentry(dentry)) {
adj++;
_
_
--
Maneesh Soni
IBM Linux Technology Center,
IBM India Software Lab, Bangalore.
Phone: +91-80-5044999 email: [email protected]
http://lse.sourceforge.net/
On Tue, 2003-08-05 at 21:28, Maneesh Soni wrote:
> Sorry, I don't think it is correct. This code is called under dcache_lock,
> taken in is_tree_busy(). mntput() calls dput() and which can lead to deadlock.
Urk. On the other hand, it only calls dput if the refcount drops to
zero, which it can't because there's already a reference (hence the -2
in is_vfsmnt_tree_busy).
I'm not too keen on releasing dcache lock, since the whole point is to
keep the dcache tree stable while we traverse it.
> @@ -71,7 +74,8 @@ static int check_vfsmnt(struct vfsmount
> struct vfsmount *vfs = lookup_mnt(mnt, dentry);
>
> if (vfs && is_vfsmnt_tree_busy(vfs))
> - ret--;
> + ret = 0;
Erm, why?
J
On Tue, 2003-08-05 at 22:00, Maneesh Soni wrote:
> + if (vfs) {
> + if (is_vfsmnt_tree_busy(vfs))
> + ret--;
> + /* just to reduce ref count taken in lookup_mnt
> + * cannot call mntput() here
> + */
> + atomic_dec(&vfs->mnt_count);
I wonder if we shouldn't make this atomic_dec_and_test with a BUG, just
for paranoia's sake.
J
On Tue, Aug 05, 2003 at 09:34:14PM -0700, Jeremy Fitzhardinge wrote:
> On Tue, 2003-08-05 at 21:28, Maneesh Soni wrote:
> > Sorry, I don't think it is correct. This code is called under dcache_lock,
> > taken in is_tree_busy(). mntput() calls dput() and which can lead to deadlock.
>
> Urk. On the other hand, it only calls dput if the refcount drops to
> zero, which it can't because there's already a reference (hence the -2
> in is_vfsmnt_tree_busy).
>
> I'm not too keen on releasing dcache lock, since the whole point is to
> keep the dcache tree stable while we traverse it.
yeah.. that is the problem in release dcache_lock there. How about just
doing atomic_dec(&vfs->mnt_count) in place of mntput()? This is also ugly,
but otherwise we have to re-write the entire is_tree_busy() thing.
>
> > @@ -71,7 +74,8 @@ static int check_vfsmnt(struct vfsmount
> > struct vfsmount *vfs = lookup_mnt(mnt, dentry);
> >
> > if (vfs && is_vfsmnt_tree_busy(vfs))
> > - ret--;
> > + ret = 0;
>
> Erm, why?
>
oh.. it should be ret--. I just copied Andrew's code. Following is the
corrected patch
fs/autofs4/expire.c | 15 ++++++++++++---
1 files changed, 12 insertions(+), 3 deletions(-)
diff -puN fs/autofs4/expire.c~autofs4-vfsmount-fix fs/autofs4/expire.c
--- linux-2.6.0-test2/fs/autofs4/expire.c~autofs4-vfsmount-fix 2003-08-06 09:10:49.000000000 +0530
+++ linux-2.6.0-test2-maneesh/fs/autofs4/expire.c 2003-08-06 10:25:58.000000000 +0530
@@ -25,7 +25,10 @@ static inline int is_vfsmnt_tree_busy(st
struct list_head *next;
int count;
- count = atomic_read(&mnt->mnt_count) - 1;
+ /* -1 for vfsmount's normal count,
+ * -1 for ref taken in lookup_mnt()
+ */
+ count = atomic_read(&mnt->mnt_count) - 1 - 1;
repeat:
next = this_parent->mnt_mounts.next;
@@ -70,8 +73,14 @@ static int check_vfsmnt(struct vfsmount
int ret = dentry->d_mounted;
struct vfsmount *vfs = lookup_mnt(mnt, dentry);
- if (vfs && is_vfsmnt_tree_busy(vfs))
- ret--;
+ if (vfs) {
+ if (is_vfsmnt_tree_busy(vfs))
+ ret--;
+ /* just to reduce ref count taken in lookup_mnt
+ * cannot call mntput() here
+ */
+ atomic_dec(&vfs->mnt_count);
+ }
DPRINTK(("check_vfsmnt: ret=%d\n", ret));
return ret;
}
_
--
Maneesh Soni
IBM Linux Technology Center,
IBM India Software Lab, Bangalore.
Phone: +91-80-5044999 email: [email protected]
http://lse.sourceforge.net/
Maneesh Soni <[email protected]> wrote:
>
> + if (vfs) {
> + if (is_vfsmnt_tree_busy(vfs))
> + ret--;
> + /* just to reduce ref count taken in lookup_mnt
> + * cannot call mntput() here
> + */
> + atomic_dec(&vfs->mnt_count);
> + }
Doesn't work, does it? If someone else does a mntput() just beforehand,
__mntput() never gets run.
On Tue, Aug 05, 2003 at 10:08:48PM -0700, Andrew Morton wrote:
> Maneesh Soni <[email protected]> wrote:
> >
> > + if (vfs) {
> > + if (is_vfsmnt_tree_busy(vfs))
> > + ret--;
> > + /* just to reduce ref count taken in lookup_mnt
> > + * cannot call mntput() here
> > + */
> > + atomic_dec(&vfs->mnt_count);
> > + }
>
> Doesn't work, does it? If someone else does a mntput() just beforehand,
> __mntput() never gets run.
>
no.. it will not work. looks like we have to unlock and lock dcache_lock and
use mntput as I did earlier. But I think, it will be really nice if Jermey
can revisit is_tree_busy() code.
There can be other problems like the checking d_count for dentries under
dcache_lock(). As users can do dget() or dput() manipulating d_count without
dcache_lock().
Maneesh
--
Maneesh Soni
IBM Linux Technology Center,
IBM India Software Lab, Bangalore.
Phone: +91-80-5044999 email: [email protected]
http://lse.sourceforge.net/