* [PATCH] autofs4 doesn't expire @ 2003-08-05 22:16 Dick Streefland 2003-08-05 23:49 ` Andrew Morton 0 siblings, 1 reply; 9+ messages in thread From: Dick Streefland @ 2003-08-05 22:16 UTC (permalink / raw) To: linux-kernel 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------------------ ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] autofs4 doesn't expire 2003-08-05 22:16 [PATCH] autofs4 doesn't expire Dick Streefland @ 2003-08-05 23:49 ` Andrew Morton 2003-08-06 0:03 ` Jeremy Fitzhardinge 2003-08-06 4:28 ` Maneesh Soni 0 siblings, 2 replies; 9+ messages in thread From: Andrew Morton @ 2003-08-05 23:49 UTC (permalink / raw) To: Dick Streefland; +Cc: linux-kernel, Jeremy Fitzhardinge, Maneesh Soni 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; } _ ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] autofs4 doesn't expire 2003-08-05 23:49 ` Andrew Morton @ 2003-08-06 0:03 ` Jeremy Fitzhardinge 2003-08-06 4:28 ` Maneesh Soni 1 sibling, 0 replies; 9+ messages in thread From: Jeremy Fitzhardinge @ 2003-08-06 0:03 UTC (permalink / raw) To: Andrew Morton; +Cc: Dick Streefland, linux-kernel, Maneesh Soni 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] autofs4 doesn't expire 2003-08-05 23:49 ` Andrew Morton 2003-08-06 0:03 ` Jeremy Fitzhardinge @ 2003-08-06 4:28 ` Maneesh Soni 2003-08-06 4:34 ` Jeremy Fitzhardinge 1 sibling, 1 reply; 9+ messages in thread From: Maneesh Soni @ 2003-08-06 4:28 UTC (permalink / raw) To: Andrew Morton; +Cc: Dick Streefland, linux-kernel, Jeremy Fitzhardinge 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/ ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] autofs4 doesn't expire 2003-08-06 4:28 ` Maneesh Soni @ 2003-08-06 4:34 ` Jeremy Fitzhardinge 2003-08-06 5:00 ` Maneesh Soni 0 siblings, 1 reply; 9+ messages in thread From: Jeremy Fitzhardinge @ 2003-08-06 4:34 UTC (permalink / raw) To: maneesh; +Cc: Andrew Morton, Dick Streefland, Linux Kernel List 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] autofs4 doesn't expire 2003-08-06 4:34 ` Jeremy Fitzhardinge @ 2003-08-06 5:00 ` Maneesh Soni 2003-08-06 5:01 ` Jeremy Fitzhardinge 2003-08-06 5:08 ` Andrew Morton 0 siblings, 2 replies; 9+ messages in thread From: Maneesh Soni @ 2003-08-06 5:00 UTC (permalink / raw) To: Jeremy Fitzhardinge; +Cc: Andrew Morton, Dick Streefland, Linux Kernel List 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/ ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] autofs4 doesn't expire 2003-08-06 5:00 ` Maneesh Soni @ 2003-08-06 5:01 ` Jeremy Fitzhardinge 2003-08-06 5:08 ` Andrew Morton 1 sibling, 0 replies; 9+ messages in thread From: Jeremy Fitzhardinge @ 2003-08-06 5:01 UTC (permalink / raw) To: maneesh; +Cc: Andrew Morton, Dick Streefland, Linux Kernel List 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] autofs4 doesn't expire 2003-08-06 5:00 ` Maneesh Soni 2003-08-06 5:01 ` Jeremy Fitzhardinge @ 2003-08-06 5:08 ` Andrew Morton 2003-08-06 5:38 ` Maneesh Soni 1 sibling, 1 reply; 9+ messages in thread From: Andrew Morton @ 2003-08-06 5:08 UTC (permalink / raw) To: maneesh; +Cc: jeremy, dick.streefland, linux-kernel 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] autofs4 doesn't expire 2003-08-06 5:08 ` Andrew Morton @ 2003-08-06 5:38 ` Maneesh Soni 0 siblings, 0 replies; 9+ messages in thread From: Maneesh Soni @ 2003-08-06 5:38 UTC (permalink / raw) To: Andrew Morton; +Cc: jeremy, dick.streefland, linux-kernel 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/ ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2003-08-06 5:33 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2003-08-05 22:16 [PATCH] autofs4 doesn't expire Dick Streefland 2003-08-05 23:49 ` Andrew Morton 2003-08-06 0:03 ` Jeremy Fitzhardinge 2003-08-06 4:28 ` Maneesh Soni 2003-08-06 4:34 ` Jeremy Fitzhardinge 2003-08-06 5:00 ` Maneesh Soni 2003-08-06 5:01 ` Jeremy Fitzhardinge 2003-08-06 5:08 ` Andrew Morton 2003-08-06 5:38 ` Maneesh Soni
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).