* [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).