linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] autofs4 needs to force fail return revalidate
@ 2006-06-21  6:18 Ian Kent
  2006-06-21  6:39 ` Andrew Morton
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Ian Kent @ 2006-06-21  6:18 UTC (permalink / raw)
  To: akpm; +Cc: autofs, linux-fsdevel, linux-kernel


Hi Andrew,

I didn't get any adverse (or other feedback) for this patch after posting
an RFC to LKML so here it is.

For a long time now I have had a problem with not being able to return a 
lookup failure on an existsing directory. In autofs this corresponds to a 
mount failure on a autofs managed mount entry that is browsable (and so 
the mount point directory exists).

While this problem has been present for a long time I've avoided resolving 
it because it was not very visible. But now that autofs v5 has "mount and 
expire on demand" of nested multiple mounts, such as is found when 
mounting an export list from a server, solving the problem cannot be 
avoided any longer.

I've tried very hard to find a way to do this entirely within the 
autofs4 module but have not been able to find a satisfactory way to 
achieve it.

So, I need to propose a change to the VFS.

Signed-off-by: Ian Kent <raven@themaw.net>

Ian

--

--- linux-2.6.17/include/linux/dcache.h.dcache-revalidate-return-fail	2006-06-19 13:26:27.000000000 +0800
+++ linux-2.6.17/include/linux/dcache.h	2006-06-19 13:29:25.000000000 +0800
@@ -163,6 +163,7 @@ d_iput:		no		no		no       yes
 #define DCACHE_UNHASHED		0x0010	
 
 #define DCACHE_INOTIFY_PARENT_WATCHED	0x0020 /* Parent inode is watched */
+#define DCACHE_REVAL_FORCE_FAIL 0x0040	/* Force revalidate fail on valid dentry */
 
 extern spinlock_t dcache_lock;
 
--- linux-2.6.17/fs/autofs4/root.c.dcache-revalidate-return-fail	2006-06-19 13:26:27.000000000 +0800
+++ linux-2.6.17/fs/autofs4/root.c	2006-06-19 13:26:51.000000000 +0800
@@ -421,8 +421,15 @@ static int autofs4_revalidate(struct den
 		DPRINTK("dentry=%p %.*s, emptydir",
 			 dentry, dentry->d_name.len, dentry->d_name.name);
 		spin_unlock(&dcache_lock);
-		if (!oz_mode)
+		if (!oz_mode) {
 			status = try_to_fill_dentry(dentry, flags);
+			if (status) {
+				spin_lock(&dentry->d_lock);
+				dentry->d_flags |= DCACHE_REVAL_FORCE_FAIL;
+				spin_unlock(&dentry->d_lock);
+				return 0;
+			}
+		}
 		return !status;
 	}
 	spin_unlock(&dcache_lock);
--- linux-2.6.17/fs/namei.c.dcache-revalidate-return-fail	2006-06-19 13:26:27.000000000 +0800
+++ linux-2.6.17/fs/namei.c	2006-06-19 13:31:31.000000000 +0800
@@ -380,9 +380,24 @@ static struct dentry * cached_lookup(str
 		dentry = d_lookup(parent, name);
 
 	if (dentry && dentry->d_op && dentry->d_op->d_revalidate) {
-		if (!dentry->d_op->d_revalidate(dentry, nd) && !d_invalidate(dentry)) {
-			dput(dentry);
-			dentry = NULL;
+		if (!dentry->d_op->d_revalidate(dentry, nd)) {
+			if (!d_invalidate(dentry)) {
+				dput(dentry);
+				return NULL;
+			}
+			/*
+			 * As well as the normal validation, check if we need
+			 * to force a fail on a valid dentry (autofs4 browsable
+			 * mounts).
+			 */
+			spin_lock(&dentry->d_lock);
+			if (dentry->d_flags & DCACHE_REVAL_FORCE_FAIL) {
+				dentry->d_flags &= ~DCACHE_REVAL_FORCE_FAIL;
+				spin_unlock(&dentry->d_lock);
+				dput(dentry);
+				return ERR_PTR(-ENOENT);
+			}
+			spin_unlock(&dentry->d_lock);
 		}
 	}
 	return dentry;
@@ -477,9 +492,24 @@ static struct dentry * real_lookup(struc
 	 */
 	mutex_unlock(&dir->i_mutex);
 	if (result->d_op && result->d_op->d_revalidate) {
-		if (!result->d_op->d_revalidate(result, nd) && !d_invalidate(result)) {
-			dput(result);
-			result = ERR_PTR(-ENOENT);
+		if (!result->d_op->d_revalidate(result, nd)) {
+			if (!d_invalidate(result)) {
+				dput(result);
+				return ERR_PTR(-ENOENT);
+			}
+			/*
+		 	* d_revalidate failed but the dentry is still valid so
+			* check if we need to force a fail on the dentry (autofs4
+			* browsable mounts).
+		 	*/
+			spin_lock(&result->d_lock);
+		    	if (result->d_flags & DCACHE_REVAL_FORCE_FAIL) {
+				result->d_flags &= ~DCACHE_REVAL_FORCE_FAIL;
+				spin_unlock(&result->d_lock);
+				dput(result);
+				return ERR_PTR(-ENOENT);
+			}
+			spin_unlock(&result->d_lock);
 		}
 	}
 	return result;
@@ -762,8 +792,21 @@ need_lookup:
 need_revalidate:
 	if (dentry->d_op->d_revalidate(dentry, nd))
 		goto done;
-	if (d_invalidate(dentry))
+	if (d_invalidate(dentry)) {
+		/*
+		 * d_revalidate failed but the dentry is still valid so check
+		 * if we need to return a fail (autofs4 browsable mounts).
+		 */
+		spin_lock(&dentry->d_lock);
+		if (dentry->d_flags & DCACHE_REVAL_FORCE_FAIL) {
+			dentry->d_flags &= ~DCACHE_REVAL_FORCE_FAIL;
+			spin_unlock(&dentry->d_lock);
+			dput(dentry);
+			return -ENOENT;
+		}
+		spin_unlock(&dentry->d_lock);
 		goto done;
+	}
 	dput(dentry);
 	goto need_lookup;
 


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] autofs4 needs to force fail return revalidate
  2006-06-21  6:18 [PATCH] autofs4 needs to force fail return revalidate Ian Kent
@ 2006-06-21  6:39 ` Andrew Morton
  2006-06-21 13:07   ` Ian Kent
                     ` (2 more replies)
  2006-06-21 12:25 ` Al Viro
  2006-06-21 13:37 ` [autofs] " Jeff Moyer
  2 siblings, 3 replies; 12+ messages in thread
From: Andrew Morton @ 2006-06-21  6:39 UTC (permalink / raw)
  To: Ian Kent; +Cc: autofs, linux-fsdevel, linux-kernel

On Wed, 21 Jun 2006 14:18:15 +0800
Ian Kent <raven@themaw.net> wrote:

> 
> Hi Andrew,
> 
> I didn't get any adverse (or other feedback) for this patch after posting
> an RFC to LKML so here it is.
> 
> For a long time now I have had a problem with not being able to return a 
> lookup failure on an existsing directory. In autofs this corresponds to a 
> mount failure on a autofs managed mount entry that is browsable (and so 
> the mount point directory exists).
> 
> While this problem has been present for a long time I've avoided resolving 
> it because it was not very visible. But now that autofs v5 has "mount and 
> expire on demand" of nested multiple mounts, such as is found when 
> mounting an export list from a server, solving the problem cannot be 
> avoided any longer.
> 
> I've tried very hard to find a way to do this entirely within the 
> autofs4 module but have not been able to find a satisfactory way to 
> achieve it.
> 
> So, I need to propose a change to the VFS.
> 
> --- linux-2.6.17/fs/namei.c.dcache-revalidate-return-fail	2006-06-19 13:26:27.000000000 +0800
> +++ linux-2.6.17/fs/namei.c	2006-06-19 13:31:31.000000000 +0800
> @@ -380,9 +380,24 @@ static struct dentry * cached_lookup(str
>  		dentry = d_lookup(parent, name);
>  
>  	if (dentry && dentry->d_op && dentry->d_op->d_revalidate) {
> -		if (!dentry->d_op->d_revalidate(dentry, nd) && !d_invalidate(dentry)) {
> -			dput(dentry);
> -			dentry = NULL;
> +		if (!dentry->d_op->d_revalidate(dentry, nd)) {
> +			if (!d_invalidate(dentry)) {
> +				dput(dentry);
> +				return NULL;
> +			}
> +			/*
> +			 * As well as the normal validation, check if we need
> +			 * to force a fail on a valid dentry (autofs4 browsable
> +			 * mounts).
> +			 */
> +			spin_lock(&dentry->d_lock);
> +			if (dentry->d_flags & DCACHE_REVAL_FORCE_FAIL) {
> +				dentry->d_flags &= ~DCACHE_REVAL_FORCE_FAIL;
> +				spin_unlock(&dentry->d_lock);
> +				dput(dentry);
> +				return ERR_PTR(-ENOENT);
> +			}
> +			spin_unlock(&dentry->d_lock);
>  		}
>  	}
>  	return dentry;
> @@ -477,9 +492,24 @@ static struct dentry * real_lookup(struc
>  	 */
>  	mutex_unlock(&dir->i_mutex);
>  	if (result->d_op && result->d_op->d_revalidate) {
> -		if (!result->d_op->d_revalidate(result, nd) && !d_invalidate(result)) {
> -			dput(result);
> -			result = ERR_PTR(-ENOENT);
> +		if (!result->d_op->d_revalidate(result, nd)) {
> +			if (!d_invalidate(result)) {
> +				dput(result);
> +				return ERR_PTR(-ENOENT);
> +			}
> +			/*
> +		 	* d_revalidate failed but the dentry is still valid so
> +			* check if we need to force a fail on the dentry (autofs4
> +			* browsable mounts).
> +		 	*/
> +			spin_lock(&result->d_lock);
> +		    	if (result->d_flags & DCACHE_REVAL_FORCE_FAIL) {
> +				result->d_flags &= ~DCACHE_REVAL_FORCE_FAIL;
> +				spin_unlock(&result->d_lock);
> +				dput(result);
> +				return ERR_PTR(-ENOENT);
> +			}
> +			spin_unlock(&result->d_lock);
>  		}
>  	}
>  	return result;
> @@ -762,8 +792,21 @@ need_lookup:
>  need_revalidate:
>  	if (dentry->d_op->d_revalidate(dentry, nd))
>  		goto done;
> -	if (d_invalidate(dentry))
> +	if (d_invalidate(dentry)) {
> +		/*
> +		 * d_revalidate failed but the dentry is still valid so check
> +		 * if we need to return a fail (autofs4 browsable mounts).
> +		 */
> +		spin_lock(&dentry->d_lock);
> +		if (dentry->d_flags & DCACHE_REVAL_FORCE_FAIL) {
> +			dentry->d_flags &= ~DCACHE_REVAL_FORCE_FAIL;
> +			spin_unlock(&dentry->d_lock);
> +			dput(dentry);
> +			return -ENOENT;
> +		}
> +		spin_unlock(&dentry->d_lock);
>  		goto done;
> +	}

All these are basically the same.  Could you look at creating a common
function, please?

Also, I don't know how frequently that code path is executed (presumably
infrequently) but would the semantics permit us to do:

	if (dentry->d_flags & DCACHE_REVAL_FORCE_FAIL) {
		spin_lock(&dentry->d_lock);
		if (dentry->d_flags & DCACHE_REVAL_FORCE_FAIL) {
			dentry->d_flags &= ~DCACHE_REVAL_FORCE_FAIL;
			spin_unlock(&dentry->d_lock);
			dput(dentry);
			return -ENOENT;
		}
		spin_unlock(&dentry->d_lock);
	}

to avoid taking the lock sometimes?

And we should have an unlikely() in there to tell the compiler to put the
code somewhere less likely to chew up CPU cache.


Also, did you consider broadening the ->d_revalidate() semantics?  It
appears that all implementations return 0 or 1.  You could teach the VFS to
also recognise and act upon a -ve return value, and do this trickery within
the autofs d_revalidate(), perhaps?


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] autofs4 needs to force fail return revalidate
  2006-06-21  6:18 [PATCH] autofs4 needs to force fail return revalidate Ian Kent
  2006-06-21  6:39 ` Andrew Morton
@ 2006-06-21 12:25 ` Al Viro
  2006-06-21 13:05   ` Ian Kent
  2006-06-21 13:37 ` [autofs] " Jeff Moyer
  2 siblings, 1 reply; 12+ messages in thread
From: Al Viro @ 2006-06-21 12:25 UTC (permalink / raw)
  To: Ian Kent; +Cc: akpm, autofs, linux-fsdevel, linux-kernel

On Wed, Jun 21, 2006 at 02:18:15PM +0800, Ian Kent wrote:
> While this problem has been present for a long time I've avoided resolving 
> it because it was not very visible. But now that autofs v5 has "mount and 
> expire on demand" of nested multiple mounts, such as is found when 
> mounting an export list from a server, solving the problem cannot be 
> avoided any longer.
> 
> I've tried very hard to find a way to do this entirely within the 
> autofs4 module but have not been able to find a satisfactory way to 
> achieve it.
> 
> So, I need to propose a change to the VFS.

NAK in that form.  Care to explain what should happen to mount tree
when you do that to mountpoint?

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] autofs4 needs to force fail return revalidate
  2006-06-21 12:25 ` Al Viro
@ 2006-06-21 13:05   ` Ian Kent
  0 siblings, 0 replies; 12+ messages in thread
From: Ian Kent @ 2006-06-21 13:05 UTC (permalink / raw)
  To: Al Viro; +Cc: akpm, autofs, linux-fsdevel, linux-kernel

On Wed, 21 Jun 2006, Al Viro wrote:

> On Wed, Jun 21, 2006 at 02:18:15PM +0800, Ian Kent wrote:
> > While this problem has been present for a long time I've avoided resolving 
> > it because it was not very visible. But now that autofs v5 has "mount and 
> > expire on demand" of nested multiple mounts, such as is found when 
> > mounting an export list from a server, solving the problem cannot be 
> > avoided any longer.
> > 
> > I've tried very hard to find a way to do this entirely within the 
> > autofs4 module but have not been able to find a satisfactory way to 
> > achieve it.
> > 
> > So, I need to propose a change to the VFS.
> 
> NAK in that form.  Care to explain what should happen to mount tree
> when you do that to mountpoint?
> 

The flag is never set if it a mount succeeds so there's never a tree under 
it. But that's only my usage, so point taken.

Thinking about it that's not the only potential problem either.

Ian


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] autofs4 needs to force fail return revalidate
  2006-06-21  6:39 ` Andrew Morton
@ 2006-06-21 13:07   ` Ian Kent
  2006-06-21 13:39   ` Ian Kent
  2006-06-23  4:14   ` Ian Kent
  2 siblings, 0 replies; 12+ messages in thread
From: Ian Kent @ 2006-06-21 13:07 UTC (permalink / raw)
  To: Andrew Morton; +Cc: autofs, linux-fsdevel, linux-kernel

On Tue, 20 Jun 2006, Andrew Morton wrote:

> On Wed, 21 Jun 2006 14:18:15 +0800
> Ian Kent <raven@themaw.net> wrote:
> 
> > 
> > Hi Andrew,
> > 
> > I didn't get any adverse (or other feedback) for this patch after posting
> > an RFC to LKML so here it is.
> > 
> > For a long time now I have had a problem with not being able to return a 
> > lookup failure on an existsing directory. In autofs this corresponds to a 
> > mount failure on a autofs managed mount entry that is browsable (and so 
> > the mount point directory exists).
> > 
> > While this problem has been present for a long time I've avoided resolving 
> > it because it was not very visible. But now that autofs v5 has "mount and 
> > expire on demand" of nested multiple mounts, such as is found when 
> > mounting an export list from a server, solving the problem cannot be 
> > avoided any longer.
> > 
> > I've tried very hard to find a way to do this entirely within the 
> > autofs4 module but have not been able to find a satisfactory way to 
> > achieve it.
> > 
> > So, I need to propose a change to the VFS.
> > 
> > --- linux-2.6.17/fs/namei.c.dcache-revalidate-return-fail	2006-06-19 13:26:27.000000000 +0800
> > +++ linux-2.6.17/fs/namei.c	2006-06-19 13:31:31.000000000 +0800
> > @@ -380,9 +380,24 @@ static struct dentry * cached_lookup(str
> >  		dentry = d_lookup(parent, name);
> >  
> >  	if (dentry && dentry->d_op && dentry->d_op->d_revalidate) {
> > -		if (!dentry->d_op->d_revalidate(dentry, nd) && !d_invalidate(dentry)) {
> > -			dput(dentry);
> > -			dentry = NULL;
> > +		if (!dentry->d_op->d_revalidate(dentry, nd)) {
> > +			if (!d_invalidate(dentry)) {
> > +				dput(dentry);
> > +				return NULL;
> > +			}
> > +			/*
> > +			 * As well as the normal validation, check if we need
> > +			 * to force a fail on a valid dentry (autofs4 browsable
> > +			 * mounts).
> > +			 */
> > +			spin_lock(&dentry->d_lock);
> > +			if (dentry->d_flags & DCACHE_REVAL_FORCE_FAIL) {
> > +				dentry->d_flags &= ~DCACHE_REVAL_FORCE_FAIL;
> > +				spin_unlock(&dentry->d_lock);
> > +				dput(dentry);
> > +				return ERR_PTR(-ENOENT);
> > +			}
> > +			spin_unlock(&dentry->d_lock);
> >  		}
> >  	}
> >  	return dentry;
> > @@ -477,9 +492,24 @@ static struct dentry * real_lookup(struc
> >  	 */
> >  	mutex_unlock(&dir->i_mutex);
> >  	if (result->d_op && result->d_op->d_revalidate) {
> > -		if (!result->d_op->d_revalidate(result, nd) && !d_invalidate(result)) {
> > -			dput(result);
> > -			result = ERR_PTR(-ENOENT);
> > +		if (!result->d_op->d_revalidate(result, nd)) {
> > +			if (!d_invalidate(result)) {
> > +				dput(result);
> > +				return ERR_PTR(-ENOENT);
> > +			}
> > +			/*
> > +		 	* d_revalidate failed but the dentry is still valid so
> > +			* check if we need to force a fail on the dentry (autofs4
> > +			* browsable mounts).
> > +		 	*/
> > +			spin_lock(&result->d_lock);
> > +		    	if (result->d_flags & DCACHE_REVAL_FORCE_FAIL) {
> > +				result->d_flags &= ~DCACHE_REVAL_FORCE_FAIL;
> > +				spin_unlock(&result->d_lock);
> > +				dput(result);
> > +				return ERR_PTR(-ENOENT);
> > +			}
> > +			spin_unlock(&result->d_lock);
> >  		}
> >  	}
> >  	return result;
> > @@ -762,8 +792,21 @@ need_lookup:
> >  need_revalidate:
> >  	if (dentry->d_op->d_revalidate(dentry, nd))
> >  		goto done;
> > -	if (d_invalidate(dentry))
> > +	if (d_invalidate(dentry)) {
> > +		/*
> > +		 * d_revalidate failed but the dentry is still valid so check
> > +		 * if we need to return a fail (autofs4 browsable mounts).
> > +		 */
> > +		spin_lock(&dentry->d_lock);
> > +		if (dentry->d_flags & DCACHE_REVAL_FORCE_FAIL) {
> > +			dentry->d_flags &= ~DCACHE_REVAL_FORCE_FAIL;
> > +			spin_unlock(&dentry->d_lock);
> > +			dput(dentry);
> > +			return -ENOENT;
> > +		}
> > +		spin_unlock(&dentry->d_lock);
> >  		goto done;
> > +	}
> 
> All these are basically the same.  Could you look at creating a common
> function, please?

Yep, I'll do that.
Jeff suggested that when he saw it.

> 
> Also, I don't know how frequently that code path is executed (presumably
> infrequently) but would the semantics permit us to do:
> 
> 	if (dentry->d_flags & DCACHE_REVAL_FORCE_FAIL) {
> 		spin_lock(&dentry->d_lock);
> 		if (dentry->d_flags & DCACHE_REVAL_FORCE_FAIL) {
> 			dentry->d_flags &= ~DCACHE_REVAL_FORCE_FAIL;
> 			spin_unlock(&dentry->d_lock);
> 			dput(dentry);
> 			return -ENOENT;
> 		}
> 		spin_unlock(&dentry->d_lock);
> 	}
> 
> to avoid taking the lock sometimes?

Should be infrequently.
I always balk at leaving things outside a lock but we test again so that 
should be fine. Will do.

> 
> And we should have an unlikely() in there to tell the compiler to put the
> code somewhere less likely to chew up CPU cache.

Yep.

> 
> 
> Also, did you consider broadening the ->d_revalidate() semantics?  It
> appears that all implementations return 0 or 1.  You could teach the VFS to
> also recognise and act upon a -ve return value, and do this trickery within
> the autofs d_revalidate(), perhaps?

I did but decided against it because that would change a long standing 
behaviour. Someone would surely get caught.

I could scan the tree and make the approiate changes.

Anyone else care to offer an opinion?

Ian


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [autofs] [PATCH] autofs4 needs to force fail return revalidate
  2006-06-21  6:18 [PATCH] autofs4 needs to force fail return revalidate Ian Kent
  2006-06-21  6:39 ` Andrew Morton
  2006-06-21 12:25 ` Al Viro
@ 2006-06-21 13:37 ` Jeff Moyer
  2 siblings, 0 replies; 12+ messages in thread
From: Jeff Moyer @ 2006-06-21 13:37 UTC (permalink / raw)
  To: Ian Kent; +Cc: akpm, autofs, linux-fsdevel, linux-kernel

==> Regarding [autofs] [PATCH] autofs4 needs to force fail return revalidate; Ian Kent <raven@themaw.net> adds:

raven> Hi Andrew,

raven> I didn't get any adverse (or other feedback) for this patch after
raven> posting an RFC to LKML so here it is.

raven> For a long time now I have had a problem with not being able to
raven> return a lookup failure on an existsing directory. In autofs this
raven> corresponds to a mount failure on a autofs managed mount entry that
raven> is browsable (and so the mount point directory exists).

raven> While this problem has been present for a long time I've avoided
raven> resolving it because it was not very visible. But now that autofs v5
raven> has "mount and expire on demand" of nested multiple mounts, such as
raven> is found when mounting an export list from a server, solving the
raven> problem cannot be avoided any longer.

raven> I've tried very hard to find a way to do this entirely within the
raven> autofs4 module but have not been able to find a satisfactory way to
raven> achieve it.

raven> So, I need to propose a change to the VFS.

raven> Signed-off-by: Ian Kent <raven@themaw.net>

Acked-by: Jeff Moyer <jmoyer@redhat.com>

-Jeff

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] autofs4 needs to force fail return revalidate
  2006-06-21  6:39 ` Andrew Morton
  2006-06-21 13:07   ` Ian Kent
@ 2006-06-21 13:39   ` Ian Kent
  2006-06-23  4:14   ` Ian Kent
  2 siblings, 0 replies; 12+ messages in thread
From: Ian Kent @ 2006-06-21 13:39 UTC (permalink / raw)
  To: Al Viro, Andrew Morton
  Cc: autofs mailing list, linux-fsdevel, Kernel Mailing List

On Tue, 20 Jun 2006, Andrew Morton wrote:

> 
> Also, did you consider broadening the ->d_revalidate() semantics?  It
> appears that all implementations return 0 or 1.  You could teach the VFS to
> also recognise and act upon a -ve return value, and do this trickery within
> the autofs d_revalidate(), perhaps?
> 

Now it occurs to me this is the only safe way to do this.
And a lot simpler.

Al, given this is such a heavily traveled piece of code, do you think 
it would be acceptable to change the semantics of revalidate in this way.

Ian


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] autofs4 needs to force fail return revalidate
  2006-06-21  6:39 ` Andrew Morton
  2006-06-21 13:07   ` Ian Kent
  2006-06-21 13:39   ` Ian Kent
@ 2006-06-23  4:14   ` Ian Kent
  2006-06-23  4:30     ` Andrew Morton
  2006-07-10 10:24     ` Andrew Morton
  2 siblings, 2 replies; 12+ messages in thread
From: Ian Kent @ 2006-06-23  4:14 UTC (permalink / raw)
  To: Andrew Morton; +Cc: autofs mailing list, linux-fsdevel, Kernel Mailing List


Here's another try at this.

On Tue, 20 Jun 2006, Andrew Morton wrote:

> On Wed, 21 Jun 2006 14:18:15 +0800
> Ian Kent <raven@themaw.net> wrote:
> 
> > 
> > Hi Andrew,
> > 
> > I didn't get any adverse (or other feedback) for this patch after posting
> > an RFC to LKML so here it is.
> > 
> > For a long time now I have had a problem with not being able to return a 
> > lookup failure on an existsing directory. In autofs this corresponds to a 
> > mount failure on a autofs managed mount entry that is browsable (and so 
> > the mount point directory exists).
> > 
> > While this problem has been present for a long time I've avoided resolving 
> > it because it was not very visible. But now that autofs v5 has "mount and 
> > expire on demand" of nested multiple mounts, such as is found when 
> > mounting an export list from a server, solving the problem cannot be 
> > avoided any longer.
> > 
> > I've tried very hard to find a way to do this entirely within the 
> > autofs4 module but have not been able to find a satisfactory way to 
> > achieve it.
> > 
> > So, I need to propose a change to the VFS.
> > 

snip ...

> 
> Also, did you consider broadening the ->d_revalidate() semantics?  It
> appears that all implementations return 0 or 1.  You could teach the VFS to
> also recognise and act upon a -ve return value, and do this trickery within
> the autofs d_revalidate(), perhaps?
> 

Yep and I've now done this.
I think this is in fact the only way to do it.

Signed-off-by: Ian Kent <raven@themaw.net>

--

--- linux-2.6.17-mm1/include/linux/dcache.h.dcache-revalidate-return-fail	2006-06-22 11:51:18.000000000 +0800
+++ linux-2.6.17-mm1/include/linux/dcache.h	2006-06-22 11:51:38.000000000 +0800
@@ -175,7 +175,6 @@ d_iput:		no		no		no       yes
 #define DCACHE_UNHASHED		0x0010	
 
 #define DCACHE_INOTIFY_PARENT_WATCHED	0x0020 /* Parent inode is watched */
-#define DCACHE_REVAL_FORCE_FAIL 0x0040	/* Force revalidate fail on valid dentry */
 
 extern spinlock_t dcache_lock;
 
--- linux-2.6.17-mm1/fs/autofs4/root.c.dcache-revalidate-return-fail	2006-06-22 11:54:24.000000000 +0800
+++ linux-2.6.17-mm1/fs/autofs4/root.c	2006-06-22 22:50:32.000000000 +0800
@@ -137,7 +137,9 @@ static int autofs4_dir_open(struct inode
 		nd.flags = LOOKUP_DIRECTORY;
 		ret = (dentry->d_op->d_revalidate)(dentry, &nd);
 
-		if (!ret) {
+		if (ret <= 0) {
+			if (ret < 0)
+				status = ret;
 			dcache_dir_close(inode, file);
 			goto out;
 		}
@@ -400,13 +402,23 @@ static int autofs4_revalidate(struct den
 	struct autofs_sb_info *sbi = autofs4_sbi(dir->i_sb);
 	int oz_mode = autofs4_oz_mode(sbi);
 	int flags = nd ? nd->flags : 0;
-	int status = 0;
+	int status = 1;
 
 	/* Pending dentry */
 	if (autofs4_ispending(dentry)) {
-		if (!oz_mode)
-			status = try_to_fill_dentry(dentry, flags);
-		return !status;
+		/* The daemon never causes a mount to trigger */
+		if (oz_mode)
+			return 1;
+
+		/*
+		 * A zero status is success otherwise we have a
+		 * negative error code.
+		 */
+		status = try_to_fill_dentry(dentry, flags);
+		if (status == 0)
+				return 1;
+
+		return status;
 	}
 
 	/* Negative dentry.. invalidate if "old" */
@@ -421,16 +433,19 @@ static int autofs4_revalidate(struct den
 		DPRINTK("dentry=%p %.*s, emptydir",
 			 dentry, dentry->d_name.len, dentry->d_name.name);
 		spin_unlock(&dcache_lock);
-		if (!oz_mode) {
-			status = try_to_fill_dentry(dentry, flags);
-			if (status) {
-				spin_lock(&dentry->d_lock);
-				dentry->d_flags |= DCACHE_REVAL_FORCE_FAIL;
-				spin_unlock(&dentry->d_lock);
-				return 0;
-			}
-		}
-		return !status;
+		/* The daemon never causes a mount to trigger */
+		if (oz_mode)
+			return 1;
+
+		/*
+		 * A zero status is success otherwise we have a
+		 * negative error code.
+		 */
+		status = try_to_fill_dentry(dentry, flags);
+		if (status == 0)
+			return 1;
+
+		return status;
 	}
 	spin_unlock(&dcache_lock);
 
--- linux-2.6.17-mm1/fs/namei.c.dcache-revalidate-return-fail	2006-06-22 11:50:13.000000000 +0800
+++ linux-2.6.17-mm1/fs/namei.c	2006-06-22 13:01:02.000000000 +0800
@@ -365,6 +365,29 @@ void release_open_intent(struct nameidat
 		fput(nd->intent.open.file);
 }
 
+static __always_inline struct dentry *do_revalidate(struct dentry *dentry, struct nameidata *nd)
+{
+	int status = dentry->d_op->d_revalidate(dentry, nd);
+	if (unlikely(status <= 0)) {
+		/*
+		 * The dentry failed validation.
+		 * If d_revalidate returned 0 attempt to invalidate
+		 * the dentry otherwise d_revalidate is asking us
+		 * to return a fail status.
+		 */
+		if (!status) {
+			if (!d_invalidate(dentry)) {
+				dput(dentry);
+				dentry = NULL;
+			}
+		} else {
+			dput(dentry);
+			dentry = ERR_PTR(status);
+		}
+	}
+	return dentry;
+}
+
 /*
  * Internal lookup() using the new generic dcache.
  * SMP-safe
@@ -379,27 +402,9 @@ static struct dentry * cached_lookup(str
 	if (!dentry)
 		dentry = d_lookup(parent, name);
 
-	if (dentry && dentry->d_op && dentry->d_op->d_revalidate) {
-		if (!dentry->d_op->d_revalidate(dentry, nd)) {
-			if (!d_invalidate(dentry)) {
-				dput(dentry);
-				return NULL;
-			}
-			/*
-			 * As well as the normal validation, check if we need
-			 * to force a fail on a valid dentry (autofs4 browsable
-			 * mounts).
-			 */
-			spin_lock(&dentry->d_lock);
-			if (dentry->d_flags & DCACHE_REVAL_FORCE_FAIL) {
-				dentry->d_flags &= ~DCACHE_REVAL_FORCE_FAIL;
-				spin_unlock(&dentry->d_lock);
-				dput(dentry);
-				return ERR_PTR(-ENOENT);
-			}
-			spin_unlock(&dentry->d_lock);
-		}
-	}
+	if (dentry && dentry->d_op && dentry->d_op->d_revalidate)
+		dentry = do_revalidate(dentry, nd);
+
 	return dentry;
 }
 
@@ -492,25 +497,9 @@ static struct dentry * real_lookup(struc
 	 */
 	mutex_unlock(&dir->i_mutex);
 	if (result->d_op && result->d_op->d_revalidate) {
-		if (!result->d_op->d_revalidate(result, nd)) {
-			if (!d_invalidate(result)) {
-				dput(result);
-				return ERR_PTR(-ENOENT);
-			}
-			/*
-		 	* d_revalidate failed but the dentry is still valid so
-			* check if we need to force a fail on the dentry (autofs4
-			* browsable mounts).
-		 	*/
-			spin_lock(&result->d_lock);
-		    	if (result->d_flags & DCACHE_REVAL_FORCE_FAIL) {
-				result->d_flags &= ~DCACHE_REVAL_FORCE_FAIL;
-				spin_unlock(&result->d_lock);
-				dput(result);
-				return ERR_PTR(-ENOENT);
-			}
-			spin_unlock(&result->d_lock);
-		}
+		result = do_revalidate(result, nd);
+		if (!result)
+			result = ERR_PTR(-ENOENT);
 	}
 	return result;
 }
@@ -790,25 +779,12 @@ need_lookup:
 	goto done;
 
 need_revalidate:
-	if (dentry->d_op->d_revalidate(dentry, nd))
-		goto done;
-	if (d_invalidate(dentry)) {
-		/*
-		 * d_revalidate failed but the dentry is still valid so check
-		 * if we need to return a fail (autofs4 browsable mounts).
-		 */
-		spin_lock(&dentry->d_lock);
-		if (dentry->d_flags & DCACHE_REVAL_FORCE_FAIL) {
-			dentry->d_flags &= ~DCACHE_REVAL_FORCE_FAIL;
-			spin_unlock(&dentry->d_lock);
-			dput(dentry);
-			return -ENOENT;
-		}
-		spin_unlock(&dentry->d_lock);
-		goto done;
-	}
-	dput(dentry);
-	goto need_lookup;
+	dentry = do_revalidate(dentry, nd);
+	if (!dentry)
+		goto need_lookup;
+	if (IS_ERR(dentry))
+		goto fail;
+	goto done;
 
 fail:
 	return PTR_ERR(dentry);

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] autofs4 needs to force fail return revalidate
  2006-06-23  4:14   ` Ian Kent
@ 2006-06-23  4:30     ` Andrew Morton
  2006-06-23  6:43       ` Ian Kent
  2006-07-10 10:24     ` Andrew Morton
  1 sibling, 1 reply; 12+ messages in thread
From: Andrew Morton @ 2006-06-23  4:30 UTC (permalink / raw)
  To: Ian Kent; +Cc: autofs, linux-fsdevel, linux-kernel

On Fri, 23 Jun 2006 12:14:09 +0800 (WST)
Ian Kent <raven@themaw.net> wrote:

> > 
> > Also, did you consider broadening the ->d_revalidate() semantics?  It
> > appears that all implementations return 0 or 1.  You could teach the VFS to
> > also recognise and act upon a -ve return value, and do this trickery within
> > the autofs d_revalidate(), perhaps?
> > 
> 
> Yep and I've now done this.
> I think this is in fact the only way to do it.

Below is the combined patch for reviewing purposes.

Removing that __always_inline saves 30-odd bytes.  I suspect that removing
it would be a performance loss in this case.  But I'll make it just
`inline' because __always_inline is peculiar, and people will wonder what's
special about this function.



 fs/autofs4/root.c |   38 ++++++++++++++++++++++++++--------
 fs/namei.c        |   49 ++++++++++++++++++++++++++++++--------------
 linux/dcache.h    |    0 
 3 files changed, 64 insertions(+), 23 deletions(-)

diff -puN fs/autofs4/root.c~autofs4-needs-to-force-fail-return-revalidate fs/autofs4/root.c
--- a/fs/autofs4/root.c~autofs4-needs-to-force-fail-return-revalidate
+++ a/fs/autofs4/root.c
@@ -137,7 +137,9 @@ static int autofs4_dir_open(struct inode
 		nd.flags = LOOKUP_DIRECTORY;
 		ret = (dentry->d_op->d_revalidate)(dentry, &nd);
 
-		if (!ret) {
+		if (ret <= 0) {
+			if (ret < 0)
+				status = ret;
 			dcache_dir_close(inode, file);
 			goto out;
 		}
@@ -400,13 +402,23 @@ static int autofs4_revalidate(struct den
 	struct autofs_sb_info *sbi = autofs4_sbi(dir->i_sb);
 	int oz_mode = autofs4_oz_mode(sbi);
 	int flags = nd ? nd->flags : 0;
-	int status = 0;
+	int status = 1;
 
 	/* Pending dentry */
 	if (autofs4_ispending(dentry)) {
-		if (!oz_mode)
-			status = try_to_fill_dentry(dentry, flags);
-		return !status;
+		/* The daemon never causes a mount to trigger */
+		if (oz_mode)
+			return 1;
+
+		/*
+		 * A zero status is success otherwise we have a
+		 * negative error code.
+		 */
+		status = try_to_fill_dentry(dentry, flags);
+		if (status == 0)
+				return 1;
+
+		return status;
 	}
 
 	/* Negative dentry.. invalidate if "old" */
@@ -421,9 +433,19 @@ static int autofs4_revalidate(struct den
 		DPRINTK("dentry=%p %.*s, emptydir",
 			 dentry, dentry->d_name.len, dentry->d_name.name);
 		spin_unlock(&dcache_lock);
-		if (!oz_mode)
-			status = try_to_fill_dentry(dentry, flags);
-		return !status;
+		/* The daemon never causes a mount to trigger */
+		if (oz_mode)
+			return 1;
+
+		/*
+		 * A zero status is success otherwise we have a
+		 * negative error code.
+		 */
+		status = try_to_fill_dentry(dentry, flags);
+		if (status == 0)
+			return 1;
+
+		return status;
 	}
 	spin_unlock(&dcache_lock);
 
diff -puN fs/namei.c~autofs4-needs-to-force-fail-return-revalidate fs/namei.c
--- a/fs/namei.c~autofs4-needs-to-force-fail-return-revalidate
+++ a/fs/namei.c
@@ -365,6 +365,29 @@ void release_open_intent(struct nameidat
 		fput(nd->intent.open.file);
 }
 
+static __always_inline struct dentry *do_revalidate(struct dentry *dentry, struct nameidata *nd)
+{
+	int status = dentry->d_op->d_revalidate(dentry, nd);
+	if (unlikely(status <= 0)) {
+		/*
+		 * The dentry failed validation.
+		 * If d_revalidate returned 0 attempt to invalidate
+		 * the dentry otherwise d_revalidate is asking us
+		 * to return a fail status.
+		 */
+		if (!status) {
+			if (!d_invalidate(dentry)) {
+				dput(dentry);
+				dentry = NULL;
+			}
+		} else {
+			dput(dentry);
+			dentry = ERR_PTR(status);
+		}
+	}
+	return dentry;
+}
+
 /*
  * Internal lookup() using the new generic dcache.
  * SMP-safe
@@ -379,12 +402,9 @@ static struct dentry * cached_lookup(str
 	if (!dentry)
 		dentry = d_lookup(parent, name);
 
-	if (dentry && dentry->d_op && dentry->d_op->d_revalidate) {
-		if (!dentry->d_op->d_revalidate(dentry, nd) && !d_invalidate(dentry)) {
-			dput(dentry);
-			dentry = NULL;
-		}
-	}
+	if (dentry && dentry->d_op && dentry->d_op->d_revalidate)
+		dentry = do_revalidate(dentry, nd);
+
 	return dentry;
 }
 
@@ -477,10 +497,9 @@ static struct dentry * real_lookup(struc
 	 */
 	mutex_unlock(&dir->i_mutex);
 	if (result->d_op && result->d_op->d_revalidate) {
-		if (!result->d_op->d_revalidate(result, nd) && !d_invalidate(result)) {
-			dput(result);
+		result = do_revalidate(result, nd);
+		if (!result)
 			result = ERR_PTR(-ENOENT);
-		}
 	}
 	return result;
 }
@@ -760,12 +779,12 @@ need_lookup:
 	goto done;
 
 need_revalidate:
-	if (dentry->d_op->d_revalidate(dentry, nd))
-		goto done;
-	if (d_invalidate(dentry))
-		goto done;
-	dput(dentry);
-	goto need_lookup;
+	dentry = do_revalidate(dentry, nd);
+	if (!dentry)
+		goto need_lookup;
+	if (IS_ERR(dentry))
+		goto fail;
+	goto done;
 
 fail:
 	return PTR_ERR(dentry);
diff -puN include/linux/dcache.h~autofs4-needs-to-force-fail-return-revalidate include/linux/dcache.h
_


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] autofs4 needs to force fail return revalidate
  2006-06-23  4:30     ` Andrew Morton
@ 2006-06-23  6:43       ` Ian Kent
  0 siblings, 0 replies; 12+ messages in thread
From: Ian Kent @ 2006-06-23  6:43 UTC (permalink / raw)
  To: Andrew Morton; +Cc: autofs, linux-fsdevel, linux-kernel

On Thu, 22 Jun 2006, Andrew Morton wrote:

> On Fri, 23 Jun 2006 12:14:09 +0800 (WST)
> Ian Kent <raven@themaw.net> wrote:
> 
> > > 
> > > Also, did you consider broadening the ->d_revalidate() semantics?  It
> > > appears that all implementations return 0 or 1.  You could teach the VFS to
> > > also recognise and act upon a -ve return value, and do this trickery within
> > > the autofs d_revalidate(), perhaps?
> > > 
> > 
> > Yep and I've now done this.
> > I think this is in fact the only way to do it.
> 
> Below is the combined patch for reviewing purposes.
> 
> Removing that __always_inline saves 30-odd bytes.  I suspect that removing
> it would be a performance loss in this case.  But I'll make it just
> `inline' because __always_inline is peculiar, and people will wonder what's
> special about this function.
> 

Thanks Andrew.
The combined diff is exactly what I'm trying to achieve.

I marked the function __always_inline because several other functions 
that appear to be frequently called from this code path are also declared 
this way. Hopefully someone with more experience of this area of the VFS 
will recommend the best choice.

Ian


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] autofs4 needs to force fail return revalidate
  2006-06-23  4:14   ` Ian Kent
  2006-06-23  4:30     ` Andrew Morton
@ 2006-07-10 10:24     ` Andrew Morton
  2006-07-10 16:23       ` [autofs] " Ian Kent
  1 sibling, 1 reply; 12+ messages in thread
From: Andrew Morton @ 2006-07-10 10:24 UTC (permalink / raw)
  To: Ian Kent, Al Viro; +Cc: autofs, linux-fsdevel, linux-kernel


btw, this patch is presently in a not-going-anywhere state because Al
expressed some reservations.  But then it all went quiet?


From: Ian Kent <raven@themaw.net>

For a long time now I have had a problem with not being able to return a
lookup failure on an existsing directory.  In autofs this corresponds to a
mount failure on a autofs managed mount entry that is browsable (and so the
mount point directory exists).

While this problem has been present for a long time I've avoided resolving
it because it was not very visible.  But now that autofs v5 has "mount and
expire on demand" of nested multiple mounts, such as is found when mounting
an export list from a server, solving the problem cannot be avoided any
longer.

I've tried very hard to find a way to do this entirely within the autofs4
module but have not been able to find a satisfactory way to achieve it.

So, I need to propose a change to the VFS.

Signed-off-by: Ian Kent <raven@themaw.net>
Signed-off-by: Andrew Morton <akpm@osdl.org>
---

 fs/autofs4/root.c |   38 ++++++++++++++++++++++++++-------
 fs/namei.c        |   50 ++++++++++++++++++++++++++++++--------------
 linux/dcache.h    |    0 
 3 files changed, 65 insertions(+), 23 deletions(-)

diff -puN fs/autofs4/root.c~autofs4-needs-to-force-fail-return-revalidate fs/autofs4/root.c
--- a/fs/autofs4/root.c~autofs4-needs-to-force-fail-return-revalidate
+++ a/fs/autofs4/root.c
@@ -137,7 +137,9 @@ static int autofs4_dir_open(struct inode
 		nd.flags = LOOKUP_DIRECTORY;
 		ret = (dentry->d_op->d_revalidate)(dentry, &nd);
 
-		if (!ret) {
+		if (ret <= 0) {
+			if (ret < 0)
+				status = ret;
 			dcache_dir_close(inode, file);
 			goto out;
 		}
@@ -400,13 +402,23 @@ static int autofs4_revalidate(struct den
 	struct autofs_sb_info *sbi = autofs4_sbi(dir->i_sb);
 	int oz_mode = autofs4_oz_mode(sbi);
 	int flags = nd ? nd->flags : 0;
-	int status = 0;
+	int status = 1;
 
 	/* Pending dentry */
 	if (autofs4_ispending(dentry)) {
-		if (!oz_mode)
-			status = try_to_fill_dentry(dentry, flags);
-		return !status;
+		/* The daemon never causes a mount to trigger */
+		if (oz_mode)
+			return 1;
+
+		/*
+		 * A zero status is success otherwise we have a
+		 * negative error code.
+		 */
+		status = try_to_fill_dentry(dentry, flags);
+		if (status == 0)
+				return 1;
+
+		return status;
 	}
 
 	/* Negative dentry.. invalidate if "old" */
@@ -421,9 +433,19 @@ static int autofs4_revalidate(struct den
 		DPRINTK("dentry=%p %.*s, emptydir",
 			 dentry, dentry->d_name.len, dentry->d_name.name);
 		spin_unlock(&dcache_lock);
-		if (!oz_mode)
-			status = try_to_fill_dentry(dentry, flags);
-		return !status;
+		/* The daemon never causes a mount to trigger */
+		if (oz_mode)
+			return 1;
+
+		/*
+		 * A zero status is success otherwise we have a
+		 * negative error code.
+		 */
+		status = try_to_fill_dentry(dentry, flags);
+		if (status == 0)
+			return 1;
+
+		return status;
 	}
 	spin_unlock(&dcache_lock);
 
diff -puN fs/namei.c~autofs4-needs-to-force-fail-return-revalidate fs/namei.c
--- a/fs/namei.c~autofs4-needs-to-force-fail-return-revalidate
+++ a/fs/namei.c
@@ -365,6 +365,30 @@ void release_open_intent(struct nameidat
 		fput(nd->intent.open.file);
 }
 
+static inline struct dentry *
+do_revalidate(struct dentry *dentry, struct nameidata *nd)
+{
+	int status = dentry->d_op->d_revalidate(dentry, nd);
+	if (unlikely(status <= 0)) {
+		/*
+		 * The dentry failed validation.
+		 * If d_revalidate returned 0 attempt to invalidate
+		 * the dentry otherwise d_revalidate is asking us
+		 * to return a fail status.
+		 */
+		if (!status) {
+			if (!d_invalidate(dentry)) {
+				dput(dentry);
+				dentry = NULL;
+			}
+		} else {
+			dput(dentry);
+			dentry = ERR_PTR(status);
+		}
+	}
+	return dentry;
+}
+
 /*
  * Internal lookup() using the new generic dcache.
  * SMP-safe
@@ -379,12 +403,9 @@ static struct dentry * cached_lookup(str
 	if (!dentry)
 		dentry = d_lookup(parent, name);
 
-	if (dentry && dentry->d_op && dentry->d_op->d_revalidate) {
-		if (!dentry->d_op->d_revalidate(dentry, nd) && !d_invalidate(dentry)) {
-			dput(dentry);
-			dentry = NULL;
-		}
-	}
+	if (dentry && dentry->d_op && dentry->d_op->d_revalidate)
+		dentry = do_revalidate(dentry, nd);
+
 	return dentry;
 }
 
@@ -477,10 +498,9 @@ static struct dentry * real_lookup(struc
 	 */
 	mutex_unlock(&dir->i_mutex);
 	if (result->d_op && result->d_op->d_revalidate) {
-		if (!result->d_op->d_revalidate(result, nd) && !d_invalidate(result)) {
-			dput(result);
+		result = do_revalidate(result, nd);
+		if (!result)
 			result = ERR_PTR(-ENOENT);
-		}
 	}
 	return result;
 }
@@ -760,12 +780,12 @@ need_lookup:
 	goto done;
 
 need_revalidate:
-	if (dentry->d_op->d_revalidate(dentry, nd))
-		goto done;
-	if (d_invalidate(dentry))
-		goto done;
-	dput(dentry);
-	goto need_lookup;
+	dentry = do_revalidate(dentry, nd);
+	if (!dentry)
+		goto need_lookup;
+	if (IS_ERR(dentry))
+		goto fail;
+	goto done;
 
 fail:
 	return PTR_ERR(dentry);
diff -puN include/linux/dcache.h~autofs4-needs-to-force-fail-return-revalidate include/linux/dcache.h
_


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [autofs] Re: [PATCH] autofs4 needs to force fail return revalidate
  2006-07-10 10:24     ` Andrew Morton
@ 2006-07-10 16:23       ` Ian Kent
  0 siblings, 0 replies; 12+ messages in thread
From: Ian Kent @ 2006-07-10 16:23 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Al Viro, autofs, linux-fsdevel, linux-kernel

On Mon, 2006-07-10 at 03:24 -0700, Andrew Morton wrote:
> btw, this patch is presently in a not-going-anywhere state because Al
> expressed some reservations.  But then it all went quiet?

Ya. Thought that might be the case.
This is in a sensitive place in the VFS.

Al, please your swift and sure guidance would be appreciated.

> 
> 
> From: Ian Kent <raven@themaw.net>
> 
> For a long time now I have had a problem with not being able to return a
> lookup failure on an existsing directory.  In autofs this corresponds to a
> mount failure on a autofs managed mount entry that is browsable (and so the
> mount point directory exists).
> 
> While this problem has been present for a long time I've avoided resolving
> it because it was not very visible.  But now that autofs v5 has "mount and
> expire on demand" of nested multiple mounts, such as is found when mounting
> an export list from a server, solving the problem cannot be avoided any
> longer.
> 
> I've tried very hard to find a way to do this entirely within the autofs4
> module but have not been able to find a satisfactory way to achieve it.
> 
> So, I need to propose a change to the VFS.
> 
> Signed-off-by: Ian Kent <raven@themaw.net>
> Signed-off-by: Andrew Morton <akpm@osdl.org>
> ---
> 
>  fs/autofs4/root.c |   38 ++++++++++++++++++++++++++-------
>  fs/namei.c        |   50 ++++++++++++++++++++++++++++++--------------
>  linux/dcache.h    |    0 
>  3 files changed, 65 insertions(+), 23 deletions(-)
> 
> diff -puN fs/autofs4/root.c~autofs4-needs-to-force-fail-return-revalidate fs/autofs4/root.c
> --- a/fs/autofs4/root.c~autofs4-needs-to-force-fail-return-revalidate
> +++ a/fs/autofs4/root.c
> @@ -137,7 +137,9 @@ static int autofs4_dir_open(struct inode
>  		nd.flags = LOOKUP_DIRECTORY;
>  		ret = (dentry->d_op->d_revalidate)(dentry, &nd);
>  
> -		if (!ret) {
> +		if (ret <= 0) {
> +			if (ret < 0)
> +				status = ret;
>  			dcache_dir_close(inode, file);
>  			goto out;
>  		}
> @@ -400,13 +402,23 @@ static int autofs4_revalidate(struct den
>  	struct autofs_sb_info *sbi = autofs4_sbi(dir->i_sb);
>  	int oz_mode = autofs4_oz_mode(sbi);
>  	int flags = nd ? nd->flags : 0;
> -	int status = 0;
> +	int status = 1;
>  
>  	/* Pending dentry */
>  	if (autofs4_ispending(dentry)) {
> -		if (!oz_mode)
> -			status = try_to_fill_dentry(dentry, flags);
> -		return !status;
> +		/* The daemon never causes a mount to trigger */
> +		if (oz_mode)
> +			return 1;
> +
> +		/*
> +		 * A zero status is success otherwise we have a
> +		 * negative error code.
> +		 */
> +		status = try_to_fill_dentry(dentry, flags);
> +		if (status == 0)
> +				return 1;
> +
> +		return status;
>  	}
>  
>  	/* Negative dentry.. invalidate if "old" */
> @@ -421,9 +433,19 @@ static int autofs4_revalidate(struct den
>  		DPRINTK("dentry=%p %.*s, emptydir",
>  			 dentry, dentry->d_name.len, dentry->d_name.name);
>  		spin_unlock(&dcache_lock);
> -		if (!oz_mode)
> -			status = try_to_fill_dentry(dentry, flags);
> -		return !status;
> +		/* The daemon never causes a mount to trigger */
> +		if (oz_mode)
> +			return 1;
> +
> +		/*
> +		 * A zero status is success otherwise we have a
> +		 * negative error code.
> +		 */
> +		status = try_to_fill_dentry(dentry, flags);
> +		if (status == 0)
> +			return 1;
> +
> +		return status;
>  	}
>  	spin_unlock(&dcache_lock);
>  
> diff -puN fs/namei.c~autofs4-needs-to-force-fail-return-revalidate fs/namei.c
> --- a/fs/namei.c~autofs4-needs-to-force-fail-return-revalidate
> +++ a/fs/namei.c
> @@ -365,6 +365,30 @@ void release_open_intent(struct nameidat
>  		fput(nd->intent.open.file);
>  }
>  
> +static inline struct dentry *
> +do_revalidate(struct dentry *dentry, struct nameidata *nd)
> +{
> +	int status = dentry->d_op->d_revalidate(dentry, nd);
> +	if (unlikely(status <= 0)) {
> +		/*
> +		 * The dentry failed validation.
> +		 * If d_revalidate returned 0 attempt to invalidate
> +		 * the dentry otherwise d_revalidate is asking us
> +		 * to return a fail status.
> +		 */
> +		if (!status) {
> +			if (!d_invalidate(dentry)) {
> +				dput(dentry);
> +				dentry = NULL;
> +			}
> +		} else {
> +			dput(dentry);
> +			dentry = ERR_PTR(status);
> +		}
> +	}
> +	return dentry;
> +}
> +
>  /*
>   * Internal lookup() using the new generic dcache.
>   * SMP-safe
> @@ -379,12 +403,9 @@ static struct dentry * cached_lookup(str
>  	if (!dentry)
>  		dentry = d_lookup(parent, name);
>  
> -	if (dentry && dentry->d_op && dentry->d_op->d_revalidate) {
> -		if (!dentry->d_op->d_revalidate(dentry, nd) && !d_invalidate(dentry)) {
> -			dput(dentry);
> -			dentry = NULL;
> -		}
> -	}
> +	if (dentry && dentry->d_op && dentry->d_op->d_revalidate)
> +		dentry = do_revalidate(dentry, nd);
> +
>  	return dentry;
>  }
>  
> @@ -477,10 +498,9 @@ static struct dentry * real_lookup(struc
>  	 */
>  	mutex_unlock(&dir->i_mutex);
>  	if (result->d_op && result->d_op->d_revalidate) {
> -		if (!result->d_op->d_revalidate(result, nd) && !d_invalidate(result)) {
> -			dput(result);
> +		result = do_revalidate(result, nd);
> +		if (!result)
>  			result = ERR_PTR(-ENOENT);
> -		}
>  	}
>  	return result;
>  }
> @@ -760,12 +780,12 @@ need_lookup:
>  	goto done;
>  
>  need_revalidate:
> -	if (dentry->d_op->d_revalidate(dentry, nd))
> -		goto done;
> -	if (d_invalidate(dentry))
> -		goto done;
> -	dput(dentry);
> -	goto need_lookup;
> +	dentry = do_revalidate(dentry, nd);
> +	if (!dentry)
> +		goto need_lookup;
> +	if (IS_ERR(dentry))
> +		goto fail;
> +	goto done;
>  
>  fail:
>  	return PTR_ERR(dentry);
> diff -puN include/linux/dcache.h~autofs4-needs-to-force-fail-return-revalidate include/linux/dcache.h
> _
> 
> _______________________________________________
> autofs mailing list
> autofs@linux.kernel.org
> http://linux.kernel.org/mailman/listinfo/autofs

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2006-07-10 16:23 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2006-06-21  6:18 [PATCH] autofs4 needs to force fail return revalidate Ian Kent
2006-06-21  6:39 ` Andrew Morton
2006-06-21 13:07   ` Ian Kent
2006-06-21 13:39   ` Ian Kent
2006-06-23  4:14   ` Ian Kent
2006-06-23  4:30     ` Andrew Morton
2006-06-23  6:43       ` Ian Kent
2006-07-10 10:24     ` Andrew Morton
2006-07-10 16:23       ` [autofs] " Ian Kent
2006-06-21 12:25 ` Al Viro
2006-06-21 13:05   ` Ian Kent
2006-06-21 13:37 ` [autofs] " Jeff Moyer

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