autofs4 doesn't expire

PATCH In review Dick Streefland 4 participants 9 messages Aug 5, 2003 – Aug 6, 2003

Share & tools

Dick Streefland

author

2003-08-05 22:17 UTC #0 lore raw

PATCH 1/1 autofs4 doesn't expire

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:

fs/autofs4/expire.c renamed from fs/autofs4/expire.c.orig +1 -0

@@ -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));

Signature · 3 lines

Dick Streefland                    ////               De Bilt
[email protected]         (@ @)       The Netherlands
------------------------------oOO--(_)--OOo------------------

Andrew Morton

2003-08-05 23:47 UTC #1 lore raw

Quoted 7 lines · [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?

fs/autofs4/expire.c renamed from fs/autofs4/expire.c~autofs4-expiry-fix +6 -3

diff -puN fs/autofs4/expire.c~autofs4-expiry-fix fs/autofs4/expire.c
@@ -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;
 }

Jeremy Fitzhardinge

2003-08-06 00:05 UTC #2 lore raw

Quoted 2 lines · Andrew Morton

Probably we should hold onto that ref because we play with the vfsmount later on. So something like this?

We're of one mind.

fs/autofs4/expire.c static int check_vfsmnt(struct vfsmount · line 77

Quoted 5 lines

fs/autofs4/expire.c renamed from fs/autofs4/expire.c~autofs4-expiry-fix +5 -0
+	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.

Maneesh Soni

2003-08-06 04:23 UTC #3 lore raw

In reply to Andrew Morton

Quoted 42 lines · Andrew Morton

Quoted 7 lines · [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
fs/autofs4/expire.c renamed from fs/autofs4/expire.c~autofs4-expiry-fix +6 -3
@@ -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;
 }

Quoted 1 line

_

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

fs/autofs4/expire.c renamed from fs/autofs4/expire.c~autofs4-vfsmount-fix +14 -4

diff -puN fs/autofs4/expire.c~autofs4-vfsmount-fix fs/autofs4/expire.c
@@ -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++;

_
_

Signature · 5 lines

Maneesh Soni
IBM Linux Technology Center, 
IBM India Software Lab, Bangalore.
Phone: +91-80-5044999 email: [email protected]
http://lse.sourceforge.net/

Jeremy Fitzhardinge

2003-08-06 04:34 UTC #4 lore raw

Quoted 2 lines · Maneesh Soni

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.

fs/autofs4/expire.c static int check_vfsmnt(struct vfsmount · line 77

Quoted 6 lines

fs/autofs4/expire.c renamed from fs/autofs4/expire.c~autofs4-vfsmount-fix +1 -1

@@ -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?

Maneesh Soni

2003-08-06 04:55 UTC #5 lore raw

Quoted 10 lines · Jeremy Fitzhardinge

Quoted 2 lines · Maneesh Soni
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.

Quoted 10 lines

Review static int check_vfsmnt(struct vfsmount · line 77

Quoted 7 lines

(quoted diff) +1 -1

@@ -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(-)

fs/autofs4/expire.c renamed from fs/autofs4/expire.c~autofs4-vfsmount-fix +12 -3

diff -puN fs/autofs4/expire.c~autofs4-vfsmount-fix fs/autofs4/expire.c
@@ -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;
 }

Signature · 5 lines

Maneesh Soni
IBM Linux Technology Center, 
IBM India Software Lab, Bangalore.
Phone: +91-80-5044999 email: [email protected]
http://lse.sourceforge.net/

Jeremy Fitzhardinge

2003-08-06 05:01 UTC #6 lore raw

fs/autofs4/expire.c static int check_vfsmnt(struct vfsmount · line 82

Quoted 7 lines · Maneesh Soni

fs/autofs4/expire.c renamed from fs/autofs4/expire.c~autofs4-vfsmount-fix +7 -0

+	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.

Andrew Morton

2003-08-06 05:07 UTC #7 lore raw

In reply to Maneesh Soni

Quoted 9 lines · Maneesh Soni

+ 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.

Maneesh Soni

2003-08-06 05:33 UTC #8 lore raw

Quoted 11 lines · Andrew Morton

Quoted 9 lines · Maneesh Soni
+ 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);
+ }

Quoted 3 lines

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

Signature · 5 lines

Maneesh Soni
IBM Linux Technology Center, 
IBM India Software Lab, Bangalore.
Phone: +91-80-5044999 email: [email protected]
http://lse.sourceforge.net/