[PATCH] autofs4 doesn't expire

[PATCH] autofs4 doesn't expire

[Dick Streefland]

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
dick.streefland@xs4all.nl         (@ @)       The Netherlands
------------------------------oOO--(_)--OOo------------------

Re: [PATCH] autofs4 doesn't expire

[Andrew Morton]

spam@streefland.xs4all.nl (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;
 }

_

Re: [PATCH] autofs4 doesn't expire

[Jeremy Fitzhardinge]

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

Re: [PATCH] autofs4 doesn't expire

[Maneesh Soni]

On Tue, Aug 05, 2003 at 04:49:04PM -0700, Andrew Morton wrote:
> spam@streefland.xs4all.nl (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: maneesh@in.ibm.com
http://lse.sourceforge.net/

Re: [PATCH] autofs4 doesn't expire

[Jeremy Fitzhardinge]

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

Re: [PATCH] autofs4 doesn't expire

[Maneesh Soni]

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: maneesh@in.ibm.com
http://lse.sourceforge.net/

Re: [PATCH] autofs4 doesn't expire

[Jeremy Fitzhardinge]

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

Re: [PATCH] autofs4 doesn't expire

[Andrew Morton]

Maneesh Soni <maneesh@in.ibm.com> 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.

Re: [PATCH] autofs4 doesn't expire

[Maneesh Soni]

On Tue, Aug 05, 2003 at 10:08:48PM -0700, Andrew Morton wrote:
> Maneesh Soni <maneesh@in.ibm.com> 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: maneesh@in.ibm.com
http://lse.sourceforge.net/