[PATCH] VFS: allow ->d_manage() to declare -EISDIR in rcu_walk mode.

[PATCH] VFS: allow ->d_manage() to declare -EISDIR in rcu_walk mode.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 3471 bytes --]


In REF-walk mode, ->d_manage can return -EISDIR to indicate
that the dentry is not really a mount trap (or even a mount point)
and that any mounts or any DCACHE_NEED_AUTOMOUNT flag should be
ignored.

RCU-walk mode doesn't currently support this, so if there is a dentry
with DCACHE_NEED_AUTOMOUNT set but which shouldn't be a mount-trap,
lookup_fast() will always drop in REF-walk mode.

With this patch, an -EISDIR from ->d_manage will always cause mounts
and automounts to be ignored, both in REF-walk and RCU-walk.

Cc: Ian Kent <raven@themaw.net>
Signed-off-by: NeilBrown <neilb@suse.de>

---
Hi Al,
 this patch is needed before I can make autofs4 fully support RCU-walk.
There are cases currently were directories have DCACHE_NEED_AUTOMOUNT but for which
no automount is required.

Thanks,
NeilBrown


diff --git a/Documentation/filesystems/vfs.txt b/Documentation/filesystems/vfs.txt
index a1d0d7a30165..61d65cc65c54 100644
--- a/Documentation/filesystems/vfs.txt
+++ b/Documentation/filesystems/vfs.txt
@@ -1053,7 +1053,8 @@ struct dentry_operations {
 	If the 'rcu_walk' parameter is true, then the caller is doing a
 	pathwalk in RCU-walk mode.  Sleeping is not permitted in this mode,
 	and the caller can be asked to leave it and call again by returning
-	-ECHILD.
+	-ECHILD.  -EISDIR may also be returned to tell pathwalk to
+	ignore d_automount or any mounts.
 
 	This function is only used if DCACHE_MANAGE_TRANSIT is set on the
 	dentry being transited from.
diff --git a/fs/namei.c b/fs/namei.c
index 985c6f368485..0abfea5697e0 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -1091,10 +1091,10 @@ int follow_down_one(struct path *path)
 }
 EXPORT_SYMBOL(follow_down_one);
 
-static inline bool managed_dentry_might_block(struct dentry *dentry)
+static inline int managed_dentry_rcu(struct dentry *dentry)
 {
-	return (dentry->d_flags & DCACHE_MANAGE_TRANSIT &&
-		dentry->d_op->d_manage(dentry, true) < 0);
+	return (dentry->d_flags & DCACHE_MANAGE_TRANSIT) ?
+		dentry->d_op->d_manage(dentry, true) : 0;
 }
 
 /*
@@ -1110,11 +1110,16 @@ static bool __follow_mount_rcu(struct nameidata *nd, struct path *path,
 		 * Don't forget we might have a non-mountpoint managed dentry
 		 * that wants to block transit.
 		 */
-		if (unlikely(managed_dentry_might_block(path->dentry)))
+		switch (managed_dentry_rcu(path->dentry)) {
+		case -ECHILD:
+		default:
 			return false;
+		case -EISDIR:
+			return true;
+		}
 
 		if (!d_mountpoint(path->dentry))
-			return true;
+			return !(path->dentry->d_flags & DCACHE_NEED_AUTOMOUNT);
 
 		mounted = __lookup_mnt(path->mnt, path->dentry);
 		if (!mounted)
@@ -1130,7 +1135,8 @@ static bool __follow_mount_rcu(struct nameidata *nd, struct path *path,
 		 */
 		*inode = path->dentry->d_inode;
 	}
-	return read_seqretry(&mount_lock, nd->m_seq);
+	return read_seqretry(&mount_lock, nd->m_seq) &&
+		!(path->dentry->d_flags & DCACHE_NEED_AUTOMOUNT);
 }
 
 static int follow_dotdot_rcu(struct nameidata *nd)
@@ -1402,11 +1408,8 @@ static int lookup_fast(struct nameidata *nd,
 		}
 		path->mnt = mnt;
 		path->dentry = dentry;
-		if (unlikely(!__follow_mount_rcu(nd, path, inode)))
-			goto unlazy;
-		if (unlikely(path->dentry->d_flags & DCACHE_NEED_AUTOMOUNT))
-			goto unlazy;
-		return 0;
+		if (likely(__follow_mount_rcu(nd, path, inode)))
+			return 0;
 unlazy:
 		if (unlazy_walk(nd, dentry))
 			return -ECHILD;

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH] VFS: allow ->d_manage() to declare -EISDIR in rcu_walk mode.

[Christoph Hellwig]

This looks good enough that I'll apply it to the interim vfs queue.

Re: [PATCH] VFS: allow ->d_manage() to declare -EISDIR in rcu_walk mode.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 179 bytes --]

On Wed, 30 Jul 2014 13:16:52 -0700 Christoph Hellwig <hch@infradead.org>
wrote:

> This looks good enough that I'll apply it to the interim vfs queue.

Thanks!

NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH] VFS: allow ->d_manage() to declare -EISDIR in rcu_walk mode.

[Dan Carpenter]

On Wed, Jul 30, 2014 at 04:08:33PM +1000, NeilBrown wrote:
> @@ -1110,11 +1110,16 @@ static bool __follow_mount_rcu(struct nameidata *nd, struct path *path,
>  		 * Don't forget we might have a non-mountpoint managed dentry
>  		 * that wants to block transit.
>  		 */
> -		if (unlikely(managed_dentry_might_block(path->dentry)))
> +		switch (managed_dentry_rcu(path->dentry)) {
> +		case -ECHILD:
> +		default:
>  			return false;
> +		case -EISDIR:
> +			return true;
> +		}
>  

Smatch says that any lines after that switch statement are unreachable.

Is the "default" intended?

>  		if (!d_mountpoint(path->dentry))
> -			return true;
> +			return !(path->dentry->d_flags & DCACHE_NEED_AUTOMOUNT);
>  
>  		mounted = __lookup_mnt(path->mnt, path->dentry);
>  		if (!mounted)

regards,
dan carpenter

Re: [PATCH] VFS: allow ->d_manage() to declare -EISDIR in rcu_walk mode.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1303 bytes --]

On Fri, 1 Aug 2014 11:33:18 +0300 Dan Carpenter <dan.carpenter@oracle.com>
wrote:

> On Wed, Jul 30, 2014 at 04:08:33PM +1000, NeilBrown wrote:
> > @@ -1110,11 +1110,16 @@ static bool __follow_mount_rcu(struct nameidata *nd, struct path *path,
> >  		 * Don't forget we might have a non-mountpoint managed dentry
> >  		 * that wants to block transit.
> >  		 */
> > -		if (unlikely(managed_dentry_might_block(path->dentry)))
> > +		switch (managed_dentry_rcu(path->dentry)) {
> > +		case -ECHILD:
> > +		default:
> >  			return false;
> > +		case -EISDIR:
> > +			return true;
> > +		}
> >  
> 
> Smatch says that any lines after that switch statement are unreachable.
> 
> Is the "default" intended?

Ugh.  It was an after thought.

The defined error codes are -ECHILD and -EISDIR.
But current code actually treats any other negative number as -ECHILD.
So I stuck in the default .... which of course was wrong.
I need to add
   case 0:
       break;

I'll send that out on Monday.

Thanks heaps.

NeilBrown


> 
> >  		if (!d_mountpoint(path->dentry))
> > -			return true;
> > +			return !(path->dentry->d_flags & DCACHE_NEED_AUTOMOUNT);
> >  
> >  		mounted = __lookup_mnt(path->mnt, path->dentry);
> >  		if (!mounted)
> 
> regards,
> dan carpenter


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

[PATCH - V2] VFS: allow ->d_manage() to declare -EISDIR in rcu_walk mode.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 3467 bytes --]


In REF-walk mode, ->d_manage can return -EISDIR to indicate
that the dentry is not really a mount trap (or even a mount point)
and that any mounts or any DCACHE_NEED_AUTOMOUNT flag should be
ignored.

RCU-walk mode doesn't currently support this, so if there is a dentry
with DCACHE_NEED_AUTOMOUNT set but which shouldn't be a mount-trap,
lookup_fast() will always drop in REF-walk mode.

With this patch, an -EISDIR from ->d_manage will always cause mounts
and automounts to be ignored, both in REF-walk and RCU-walk.

Bug-fixed-by: Dan Carpenter <dan.carpenter@oracle.com>
Cc: Ian Kent <raven@themaw.net>
Signed-off-by: NeilBrown <neilb@suse.de>

---
Previous version of this patch had a bug causing code to be unreachable.
Please replace with this version.

Thanks,
NeilBrown


diff --git a/Documentation/filesystems/vfs.txt b/Documentation/filesystems/vfs.txt
index a1d0d7a30165..61d65cc65c54 100644
--- a/Documentation/filesystems/vfs.txt
+++ b/Documentation/filesystems/vfs.txt
@@ -1053,7 +1053,8 @@ struct dentry_operations {
 	If the 'rcu_walk' parameter is true, then the caller is doing a
 	pathwalk in RCU-walk mode.  Sleeping is not permitted in this mode,
 	and the caller can be asked to leave it and call again by returning
-	-ECHILD.
+	-ECHILD.  -EISDIR may also be returned to tell pathwalk to
+	ignore d_automount or any mounts.
 
 	This function is only used if DCACHE_MANAGE_TRANSIT is set on the
 	dentry being transited from.
diff --git a/fs/namei.c b/fs/namei.c
index 9eb787e5c167..98ce4cffe51c 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -1091,10 +1091,10 @@ int follow_down_one(struct path *path)
 }
 EXPORT_SYMBOL(follow_down_one);
 
-static inline bool managed_dentry_might_block(struct dentry *dentry)
+static inline int managed_dentry_rcu(struct dentry *dentry)
 {
-	return (dentry->d_flags & DCACHE_MANAGE_TRANSIT &&
-		dentry->d_op->d_manage(dentry, true) < 0);
+	return (dentry->d_flags & DCACHE_MANAGE_TRANSIT) ?
+		dentry->d_op->d_manage(dentry, true) : 0;
 }
 
 /*
@@ -1110,11 +1110,18 @@ static bool __follow_mount_rcu(struct nameidata *nd, struct path *path,
 		 * Don't forget we might have a non-mountpoint managed dentry
 		 * that wants to block transit.
 		 */
-		if (unlikely(managed_dentry_might_block(path->dentry)))
+		switch (managed_dentry_rcu(path->dentry)) {
+		case -ECHILD:
+		default:
 			return false;
+		case -EISDIR:
+			return true;
+		case 0:
+			break;
+		}
 
 		if (!d_mountpoint(path->dentry))
-			return true;
+			return !(path->dentry->d_flags & DCACHE_NEED_AUTOMOUNT);
 
 		mounted = __lookup_mnt(path->mnt, path->dentry);
 		if (!mounted)
@@ -1130,7 +1137,8 @@ static bool __follow_mount_rcu(struct nameidata *nd, struct path *path,
 		 */
 		*inode = path->dentry->d_inode;
 	}
-	return read_seqretry(&mount_lock, nd->m_seq);
+	return read_seqretry(&mount_lock, nd->m_seq) &&
+		!(path->dentry->d_flags & DCACHE_NEED_AUTOMOUNT);
 }
 
 static int follow_dotdot_rcu(struct nameidata *nd)
@@ -1402,11 +1410,8 @@ static int lookup_fast(struct nameidata *nd,
 		}
 		path->mnt = mnt;
 		path->dentry = dentry;
-		if (unlikely(!__follow_mount_rcu(nd, path, inode)))
-			goto unlazy;
-		if (unlikely(path->dentry->d_flags & DCACHE_NEED_AUTOMOUNT))
-			goto unlazy;
-		return 0;
+		if (likely(__follow_mount_rcu(nd, path, inode)))
+			return 0;
 unlazy:
 		if (unlazy_walk(nd, dentry))
 			return -ECHILD;

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH - V2] VFS: allow ->d_manage() to declare -EISDIR in rcu_walk mode.

[Al Viro]

On Mon, Aug 04, 2014 at 05:06:29PM +1000, NeilBrown wrote:
> 
> In REF-walk mode, ->d_manage can return -EISDIR to indicate
> that the dentry is not really a mount trap (or even a mount point)
> and that any mounts or any DCACHE_NEED_AUTOMOUNT flag should be
> ignored.
> 
> RCU-walk mode doesn't currently support this, so if there is a dentry
> with DCACHE_NEED_AUTOMOUNT set but which shouldn't be a mount-trap,
> lookup_fast() will always drop in REF-walk mode.
> 
> With this patch, an -EISDIR from ->d_manage will always cause mounts
> and automounts to be ignored, both in REF-walk and RCU-walk.

In queue.  Right now I'm trying to linearize the damn acct fixes from late
April, needed to get Eric's umount-on-rmdir series into the mix safely ;-/
Hopefully I'll have something pushable by Tuesday; your patch definitely
will be in the pile.