[PATCH 0/9] vfs: path lookup fixes and cleanups

[PATCH 0/9] vfs: path lookup fixes and cleanups

[Miklos Szeredi]

This series is a prerequisite for the atomic-open patches I will post shortly.
It contains fixes and cleanups that stand on their own.

Please apply.

Thanks,
Miklos

---
Miklos Szeredi (9):
      vfs: fix double put after complete_walk()
      vfs: fix return value from do_last()
      vfs: fix d_need_lookup/d_revalidate order in do_lookup
      vfs: don't revalidate just looked up dentry
      vfs: move MAY_EXEC check from __lookup_hash()
      vfs: set LOOKUP_JUMPED in follow_managed
      vfs: reorganize do_lookup
      vfs: split __lookup_hash
      nfs: don't open in ->d_revalidate

---
 fs/namei.c    |  228 ++++++++++++++++++++++++---------------------------------
 fs/nfs/dir.c  |   47 +-----------
 fs/nfs/file.c |   69 ++++++++++++++++-
 3 files changed, 167 insertions(+), 177 deletions(-)

[PATCH 1/9] vfs: fix double put after complete_walk()

[Miklos Szeredi]

From: Miklos Szeredi <mszeredi@suse.cz>

complete_walk() already puts nd->path, no need to do it again at cleanup time.

This would result in Oopses if triggered, apparently the codepath is not too
well exercised.

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
CC: stable@vger.kernel.org
---
 fs/namei.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index e2ba628..f79aef1 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2261,7 +2261,7 @@ static struct file *do_last(struct nameidata *nd, struct path *path,
 	/* Why this, you ask?  _Now_ we might have grown LOOKUP_JUMPED... */
 	error = complete_walk(nd);
 	if (error)
-		goto exit;
+		return ERR_PTR(error);
 	error = -EISDIR;
 	if (S_ISDIR(nd->inode->i_mode))
 		goto exit;
-- 
1.7.7

[PATCH 2/9] vfs: fix return value from do_last()

[Miklos Szeredi]

From: Miklos Szeredi <mszeredi@suse.cz>

complete_walk() returns either ECHILD or ESTALE.  do_last() turns this into
ECHILD unconditionally.  If not in RCU mode, this error will reach userspace
which is complete nonsense.

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
CC: stable@vger.kernel.org
---
 fs/namei.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index f79aef1..46ea9cc 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2162,7 +2162,7 @@ static struct file *do_last(struct nameidata *nd, struct path *path,
 		/* sayonara */
 		error = complete_walk(nd);
 		if (error)
-			return ERR_PTR(-ECHILD);
+			return ERR_PTR(error);
 
 		error = -ENOTDIR;
 		if (nd->flags & LOOKUP_DIRECTORY) {
-- 
1.7.7

[PATCH 3/9] vfs: fix d_need_lookup/d_revalidate order in do_lookup

[Miklos Szeredi]

From: Miklos Szeredi <mszeredi@suse.cz>

Doing revalidate on a dentry which has not yet been looked up makes no sense.

Move the d_need_lookup() check before d_revalidate().

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
---
 fs/namei.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index 46ea9cc..599e4a0 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -1139,6 +1139,8 @@ static int do_lookup(struct nameidata *nd, struct qstr *name,
 			return -ECHILD;
 		nd->seq = seq;
 
+		if (unlikely(d_need_lookup(dentry)))
+			goto unlazy;
 		if (unlikely(dentry->d_flags & DCACHE_OP_REVALIDATE)) {
 			status = d_revalidate(dentry, nd);
 			if (unlikely(status <= 0)) {
@@ -1147,8 +1149,6 @@ static int do_lookup(struct nameidata *nd, struct qstr *name,
 				goto unlazy;
 			}
 		}
-		if (unlikely(d_need_lookup(dentry)))
-			goto unlazy;
 		path->mnt = mnt;
 		path->dentry = dentry;
 		if (unlikely(!__follow_mount_rcu(nd, path, inode)))
-- 
1.7.7

[PATCH 4/9] vfs: don't revalidate just looked up dentry

[Miklos Szeredi]

From: Miklos Szeredi <mszeredi@suse.cz>

__lookup_hash() calls ->lookup() if the dentry needs lookup and on success
revalidates the dentry (all under dir->i_mutex).

While this is harmless it doesn't make a lot of sense.

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
---
 fs/namei.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index 599e4a0..c93ea6d 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -1746,9 +1746,7 @@ static struct dentry *__lookup_hash(struct qstr *name,
 		 * __lookup_hash is called with the parent dir's i_mutex already
 		 * held, so we are good to go here.
 		 */
-		dentry = d_inode_lookup(base, dentry, nd);
-		if (IS_ERR(dentry))
-			return dentry;
+		return d_inode_lookup(base, dentry, nd);
 	}
 
 	if (dentry && (dentry->d_flags & DCACHE_OP_REVALIDATE)) {
-- 
1.7.7

[PATCH 5/9] vfs: move MAY_EXEC check from __lookup_hash()

[Miklos Szeredi]

From: Miklos Szeredi <mszeredi@suse.cz>

The only caller of __lookup_hash() that needs the exec permission check on
parent is lookup_one_len().

All lookup_hash() callers already checked permission in LOOKUP_PARENT walk.

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
---
 fs/namei.c |   11 +++++------
 1 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index c93ea6d..3e13412 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -1726,13 +1726,7 @@ int vfs_path_lookup(struct dentry *dentry, struct vfsmount *mnt,
 static struct dentry *__lookup_hash(struct qstr *name,
 		struct dentry *base, struct nameidata *nd)
 {
-	struct inode *inode = base->d_inode;
 	struct dentry *dentry;
-	int err;
-
-	err = inode_permission(inode, MAY_EXEC);
-	if (err)
-		return ERR_PTR(err);
 
 	/*
 	 * Don't bother with __d_lookup: callers are for creat as
@@ -1799,6 +1793,7 @@ struct dentry *lookup_one_len(const char *name, struct dentry *base, int len)
 {
 	struct qstr this;
 	unsigned int c;
+	int err;
 
 	WARN_ON_ONCE(!mutex_is_locked(&base->d_inode->i_mutex));
 
@@ -1823,6 +1818,10 @@ struct dentry *lookup_one_len(const char *name, struct dentry *base, int len)
 			return ERR_PTR(err);
 	}
 
+	err = inode_permission(base->d_inode, MAY_EXEC);
+	if (err)
+		return ERR_PTR(err);
+
 	return __lookup_hash(&this, base, NULL);
 }
 
-- 
1.7.7

[PATCH 6/9] vfs: set LOOKUP_JUMPED in follow_managed

[Miklos Szeredi]

From: Miklos Szeredi <mszeredi@suse.cz>

This is cleaner than if callers need to do it.

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
---
 fs/namei.c |   27 +++++++++++++--------------
 1 files changed, 13 insertions(+), 14 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index 3e13412..482992e 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -794,8 +794,10 @@ static int follow_automount(struct path *path, unsigned flags,
  * This may only be called in refwalk mode.
  *
  * Serialization is taken care of in namespace.c
+ *
+ * If a mount was followed, set LOOKUP_JUMPED in nd->flags
  */
-static int follow_managed(struct path *path, unsigned flags)
+static int follow_managed(struct path *path, struct nameidata *nd)
 {
 	struct vfsmount *mnt = path->mnt; /* held by caller, must be left alone */
 	unsigned managed;
@@ -839,7 +841,7 @@ static int follow_managed(struct path *path, unsigned flags)
 
 		/* Handle an automount point */
 		if (managed & DCACHE_NEED_AUTOMOUNT) {
-			ret = follow_automount(path, flags, &need_mntput);
+			ret = follow_automount(path, nd->flags, &need_mntput);
 			if (ret < 0)
 				break;
 			continue;
@@ -851,9 +853,11 @@ static int follow_managed(struct path *path, unsigned flags)
 
 	if (need_mntput && path->mnt == mnt)
 		mntput(path->mnt);
-	if (ret == -EISDIR)
-		ret = 0;
-	return ret < 0 ? ret : need_mntput;
+	if (ret < 0 && ret != -EISDIR)
+		return ret;
+	if (need_mntput)
+		nd->flags |= LOOKUP_JUMPED;
+	return 0;
 }
 
 int follow_down_one(struct path *path)
@@ -1212,13 +1216,11 @@ retry:
 
 	path->mnt = mnt;
 	path->dentry = dentry;
-	err = follow_managed(path, nd->flags);
-	if (unlikely(err < 0)) {
+	err = follow_managed(path, nd);
+	if (unlikely(err)) {
 		path_put_conditional(path, nd);
 		return err;
 	}
-	if (err)
-		nd->flags |= LOOKUP_JUMPED;
 	*inode = path->dentry->d_inode;
 	return 0;
 }
@@ -2239,12 +2241,9 @@ static struct file *do_last(struct nameidata *nd, struct path *path,
 	if (open_flag & O_EXCL)
 		goto exit_dput;
 
-	error = follow_managed(path, nd->flags);
-	if (error < 0)
-		goto exit_dput;
-
+	error = follow_managed(path, nd);
 	if (error)
-		nd->flags |= LOOKUP_JUMPED;
+		goto exit_dput;
 
 	error = -ENOENT;
 	if (!path->dentry->d_inode)
-- 
1.7.7

[PATCH 7/9] vfs: reorganize do_lookup

[Miklos Szeredi]

From: Miklos Szeredi <mszeredi@suse.cz>

do_lookup's uncached lookup part basically open codes __lookup_hash so use that
instead.

This also eliminates the weird retry loop, that could, in theory, retry the
cached lookup any number of times (very unlikely scenario: needs two parallel
do_lookups and d_revalidate always returning zero).

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
---
 fs/namei.c |  141 +++++++++++++++++++++++++++---------------------------------
 1 files changed, 63 insertions(+), 78 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index 482992e..b1b8f1a 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -1112,6 +1112,51 @@ static struct dentry *d_inode_lookup(struct dentry *parent, struct dentry *dentr
 	return dentry;
 }
 
+static struct dentry *__lookup_hash(struct qstr *name, struct dentry *base,
+				    struct nameidata *nd)
+{
+	struct dentry *dentry;
+
+	/*
+	 * Don't bother with __d_lookup: callers are for creat as
+	 * well as unlink, so a lot of the time it would cost
+	 * a double lookup.
+	 */
+	dentry = d_lookup(base, name);
+
+	if (dentry && d_need_lookup(dentry)) {
+		/*
+		 * __lookup_hash is called with the parent dir's i_mutex already
+		 * held, so we are good to go here.
+		 */
+		return d_inode_lookup(base, dentry, nd);
+	}
+
+	if (dentry && (dentry->d_flags & DCACHE_OP_REVALIDATE)) {
+		int status = 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 < 0) {
+				dput(dentry);
+				return ERR_PTR(status);
+			} else if (!d_invalidate(dentry)) {
+				dput(dentry);
+				dentry = NULL;
+			}
+		}
+	}
+
+	if (!dentry)
+		dentry = d_alloc_and_lookup(base, name, nd);
+
+	return dentry;
+}
+
 /*
  *  It's more convoluted than I'd like it to be, but... it's still fairly
  *  small and for now I'd prefer to have fast path as straight as possible.
@@ -1167,37 +1212,12 @@ unlazy:
 		dentry = __d_lookup(parent, name);
 	}
 
-	if (dentry && unlikely(d_need_lookup(dentry))) {
-		dput(dentry);
-		dentry = NULL;
-	}
-retry:
-	if (unlikely(!dentry)) {
-		struct inode *dir = parent->d_inode;
-		BUG_ON(nd->inode != dir);
+	if (unlikely(!dentry))
+		goto need_lookup;
 
-		mutex_lock(&dir->i_mutex);
-		dentry = d_lookup(parent, name);
-		if (likely(!dentry)) {
-			dentry = d_alloc_and_lookup(parent, name, nd);
-			if (IS_ERR(dentry)) {
-				mutex_unlock(&dir->i_mutex);
-				return PTR_ERR(dentry);
-			}
-			/* known good */
-			need_reval = 0;
-			status = 1;
-		} else if (unlikely(d_need_lookup(dentry))) {
-			dentry = d_inode_lookup(parent, dentry, nd);
-			if (IS_ERR(dentry)) {
-				mutex_unlock(&dir->i_mutex);
-				return PTR_ERR(dentry);
-			}
-			/* known good */
-			need_reval = 0;
-			status = 1;
-		}
-		mutex_unlock(&dir->i_mutex);
+	if (unlikely(d_need_lookup(dentry))) {
+		dput(dentry);
+		goto need_lookup;
 	}
 	if (unlikely(dentry->d_flags & DCACHE_OP_REVALIDATE) && need_reval)
 		status = d_revalidate(dentry, nd);
@@ -1208,12 +1228,11 @@ retry:
 		}
 		if (!d_invalidate(dentry)) {
 			dput(dentry);
-			dentry = NULL;
-			need_reval = 1;
-			goto retry;
+			goto need_lookup;
 		}
 	}
 
+success:
 	path->mnt = mnt;
 	path->dentry = dentry;
 	err = follow_managed(path, nd);
@@ -1223,6 +1242,17 @@ retry:
 	}
 	*inode = path->dentry->d_inode;
 	return 0;
+
+need_lookup:
+	BUG_ON(nd->inode != parent->d_inode);
+
+	mutex_lock(&parent->d_inode->i_mutex);
+	dentry = __lookup_hash(name, parent, nd);
+	mutex_unlock(&parent->d_inode->i_mutex);
+	if (IS_ERR(dentry))
+		return PTR_ERR(dentry);
+
+	goto success;
 }
 
 static inline int may_lookup(struct nameidata *nd)
@@ -1725,51 +1755,6 @@ int vfs_path_lookup(struct dentry *dentry, struct vfsmount *mnt,
 	return err;
 }
 
-static struct dentry *__lookup_hash(struct qstr *name,
-		struct dentry *base, struct nameidata *nd)
-{
-	struct dentry *dentry;
-
-	/*
-	 * Don't bother with __d_lookup: callers are for creat as
-	 * well as unlink, so a lot of the time it would cost
-	 * a double lookup.
-	 */
-	dentry = d_lookup(base, name);
-
-	if (dentry && d_need_lookup(dentry)) {
-		/*
-		 * __lookup_hash is called with the parent dir's i_mutex already
-		 * held, so we are good to go here.
-		 */
-		return d_inode_lookup(base, dentry, nd);
-	}
-
-	if (dentry && (dentry->d_flags & DCACHE_OP_REVALIDATE)) {
-		int status = 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 < 0) {
-				dput(dentry);
-				return ERR_PTR(status);
-			} else if (!d_invalidate(dentry)) {
-				dput(dentry);
-				dentry = NULL;
-			}
-		}
-	}
-
-	if (!dentry)
-		dentry = d_alloc_and_lookup(base, name, nd);
-
-	return dentry;
-}
-
 /*
  * Restricted form of lookup. Doesn't follow links, single-component only,
  * needs parent already locked. Doesn't follow mounts.
-- 
1.7.7

Re: [PATCH 7/9] vfs: reorganize do_lookup

[Christoph Hellwig]

> +	/*
> +	 * Don't bother with __d_lookup: callers are for creat as
> +	 * well as unlink, so a lot of the time it would cost
> +	 * a double lookup.
> +	 */
> +	dentry = d_lookup(base, name);

This comment isn't true anymore now, although the actual code should
still be fine given that we already did the __d_lookup before for
regular lookups.

Re: [PATCH 7/9] vfs: reorganize do_lookup

[Miklos Szeredi]

Christoph Hellwig <hch@infradead.org> writes:

>> +	/*
>> +	 * Don't bother with __d_lookup: callers are for creat as
>> +	 * well as unlink, so a lot of the time it would cost
>> +	 * a double lookup.
>> +	 */
>> +	dentry = d_lookup(base, name);
>
> This comment isn't true anymore now, although the actual code should
> still be fine given that we already did the __d_lookup before for
> regular lookups.

Okay.

Thanks,
Miklos

[PATCH 8/9] vfs: split __lookup_hash

[Miklos Szeredi]

From: Miklos Szeredi <mszeredi@suse.cz>

Split __lookup_hash into two component functions:

 lookup_dcache - tries cached lookup, returns whether real lookup is needed
 lookup_real - calls i_op->lookup

This eliminates code duplication between d_alloc_and_lookup() and
d_inode_lookup().

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
---
 fs/namei.c |  113 ++++++++++++++++++++++++++----------------------------------
 1 files changed, 49 insertions(+), 64 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index b1b8f1a..29a00e3 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -1058,53 +1058,70 @@ static void follow_dotdot(struct nameidata *nd)
 }
 
 /*
- * Allocate a dentry with name and parent, and perform a parent
- * directory ->lookup on it. Returns the new dentry, or ERR_PTR
- * on error. parent->d_inode->i_mutex must be held. d_lookup must
- * have verified that no child exists while under i_mutex.
+ * This looks up the name in dcache, possibly revalidates the old dentry and
+ * allocates a new one if not found or not valid.  In the need_lookup argument
+ * returns whether i_op->lookup is necessary.
+ *
+ * dir->d_inode->i_mutex must be held
  */
-static struct dentry *d_alloc_and_lookup(struct dentry *parent,
-				struct qstr *name, struct nameidata *nd)
+static struct dentry *lookup_dcache(struct qstr *name, struct dentry *dir,
+				    struct nameidata *nd, bool *need_lookup)
 {
-	struct inode *inode = parent->d_inode;
 	struct dentry *dentry;
-	struct dentry *old;
+	int error;
 
-	/* Don't create child dentry for a dead directory. */
-	if (unlikely(IS_DEADDIR(inode)))
-		return ERR_PTR(-ENOENT);
+	*need_lookup = false;
+	/*
+	 * Don't bother with __d_lookup: callers are for creat as
+	 * well as unlink, so a lot of the time it would cost
+	 * a double lookup.
+	 */
+	dentry = d_lookup(dir, name);
+	if (dentry) {
+		if (d_need_lookup(dentry)) {
+			*need_lookup = true;
+		} else if (dentry->d_flags & DCACHE_OP_REVALIDATE) {
+			error = d_revalidate(dentry, nd);
+			if (unlikely(error <= 0)) {
+				if (error < 0) {
+					dput(dentry);
+					return ERR_PTR(error);
+				} else if (!d_invalidate(dentry)) {
+					dput(dentry);
+					dentry = NULL;
+				}
+			}
+		}
+	}
 
-	dentry = d_alloc(parent, name);
-	if (unlikely(!dentry))
-		return ERR_PTR(-ENOMEM);
+	if (!dentry) {
+		dentry = d_alloc(dir, name);
+		if (unlikely(!dentry))
+			return ERR_PTR(-ENOMEM);
 
-	old = inode->i_op->lookup(inode, dentry, nd);
-	if (unlikely(old)) {
-		dput(dentry);
-		dentry = old;
+		*need_lookup = true;
 	}
 	return dentry;
 }
 
 /*
- * We already have a dentry, but require a lookup to be performed on the parent
- * directory to fill in d_inode. Returns the new dentry, or ERR_PTR on error.
- * parent->d_inode->i_mutex must be held. d_lookup must have verified that no
- * child exists while under i_mutex.
+ * Call i_op->lookup on the dentry.  The dentry must be negative but may be
+ * hashed if it was pouplated with DCACHE_NEED_LOOKUP.
+ *
+ * dir->d_inode->i_mutex must be held
  */
-static struct dentry *d_inode_lookup(struct dentry *parent, struct dentry *dentry,
-				     struct nameidata *nd)
+static struct dentry *lookup_real(struct inode *dir, struct dentry *dentry,
+				  struct nameidata *nd)
 {
-	struct inode *inode = parent->d_inode;
 	struct dentry *old;
 
 	/* Don't create child dentry for a dead directory. */
-	if (unlikely(IS_DEADDIR(inode))) {
+	if (unlikely(IS_DEADDIR(dir))) {
 		dput(dentry);
 		return ERR_PTR(-ENOENT);
 	}
 
-	old = inode->i_op->lookup(inode, dentry, nd);
+	old = dir->i_op->lookup(dir, dentry, nd);
 	if (unlikely(old)) {
 		dput(dentry);
 		dentry = old;
@@ -1115,46 +1132,14 @@ static struct dentry *d_inode_lookup(struct dentry *parent, struct dentry *dentr
 static struct dentry *__lookup_hash(struct qstr *name, struct dentry *base,
 				    struct nameidata *nd)
 {
+	bool need_lookup;
 	struct dentry *dentry;
 
-	/*
-	 * Don't bother with __d_lookup: callers are for creat as
-	 * well as unlink, so a lot of the time it would cost
-	 * a double lookup.
-	 */
-	dentry = d_lookup(base, name);
-
-	if (dentry && d_need_lookup(dentry)) {
-		/*
-		 * __lookup_hash is called with the parent dir's i_mutex already
-		 * held, so we are good to go here.
-		 */
-		return d_inode_lookup(base, dentry, nd);
-	}
+	dentry = lookup_dcache(name, base, nd, &need_lookup);
+	if (IS_ERR(dentry) || !need_lookup)
+		return dentry;
 
-	if (dentry && (dentry->d_flags & DCACHE_OP_REVALIDATE)) {
-		int status = 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 < 0) {
-				dput(dentry);
-				return ERR_PTR(status);
-			} else if (!d_invalidate(dentry)) {
-				dput(dentry);
-				dentry = NULL;
-			}
-		}
-	}
-
-	if (!dentry)
-		dentry = d_alloc_and_lookup(base, name, nd);
-
-	return dentry;
+	return lookup_real(base->d_inode, dentry, nd);
 }
 
 /*
-- 
1.7.7

Re: [PATCH 8/9] vfs: split __lookup_hash

[Christoph Hellwig]

On Tue, Mar 06, 2012 at 01:56:40PM +0100, Miklos Szeredi wrote:
> From: Miklos Szeredi <mszeredi@suse.cz>
> 
> Split __lookup_hash into two component functions:
> 
>  lookup_dcache - tries cached lookup, returns whether real lookup is needed
>  lookup_real - calls i_op->lookup
> 
> This eliminates code duplication between d_alloc_and_lookup() and
> d_inode_lookup().

The return value from lookup_dcache is a bit confusing.  What about
returning the need_lookup flag, and passing the dentry as a parameter,
that way the callers would do the more obvious:

	if (lookup_dcache(name, base, nd, &dentry))
		return dentry;

instead of having to check two conditions.

Re: [PATCH 8/9] vfs: split __lookup_hash

[Miklos Szeredi]

On Tue, 2012-03-20 at 12:39 -0400, Christoph Hellwig wrote:
> On Tue, Mar 06, 2012 at 01:56:40PM +0100, Miklos Szeredi wrote:
> > From: Miklos Szeredi <mszeredi@suse.cz>
> > 
> > Split __lookup_hash into two component functions:
> > 
> >  lookup_dcache - tries cached lookup, returns whether real lookup is needed
> >  lookup_real - calls i_op->lookup
> > 
> > This eliminates code duplication between d_alloc_and_lookup() and
> > d_inode_lookup().
> 
> The return value from lookup_dcache is a bit confusing.  What about
> returning the need_lookup flag, and passing the dentry as a parameter,
> that way the callers would do the more obvious:
> 
> 	if (lookup_dcache(name, base, nd, &dentry))
> 		return dentry;
> 
> instead of having to check two conditions.
> 

The reason I dislike your version is that returning an error value in an
argument would be strange.

And this works too:

	dentry = lookup_dcache(name, base, nd, &need_lookup);
	if (!need_lookup)
		return dentry;


Thanks,
Miklos

[PATCH 9/9] nfs: don't open in ->d_revalidate

[Miklos Szeredi]

From: Miklos Szeredi <mszeredi@suse.cz>

NFSv4 can't do reliable opens in d_revalidate, since it cannot know whether a
mount needs to be followed or not.  It does check d_mountpoint() on the dentry,
which can result in a weird error if the VFS found that the mount does not in
fact need to be followed, e.g.:

  # mount --bind /mnt/nfs /mnt/nfs-clone
  # echo something > /mnt/nfs/tmp/bar
  # echo x > /tmp/file
  # mount --bind /tmp/file /mnt/nfs-clone/tmp/bar
  # cat  /mnt/nfs/tmp/bar
  cat: /mnt/nfs/tmp/bar: Not a directory

Which should, by any sane filesystem, result in "something" being printed.

So instead do the open in f_op->open() and in the unlikely case that the cached
dentry turned out to be invalid, drop the dentry and return ESTALE to let the
VFS retry.

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
CC: Trond Myklebust <Trond.Myklebust@netapp.com>
---
 fs/nfs/dir.c  |   47 ++++----------------------------------
 fs/nfs/file.c |   69 +++++++++++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 70 insertions(+), 46 deletions(-)

diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index fd9a872..715a8c1 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -1328,10 +1328,10 @@ out:
 }
 
 #ifdef CONFIG_NFS_V4
-static int nfs_open_revalidate(struct dentry *, struct nameidata *);
+static int nfs4_lookup_revalidate(struct dentry *, struct nameidata *);
 
 const struct dentry_operations nfs4_dentry_operations = {
-	.d_revalidate	= nfs_open_revalidate,
+	.d_revalidate	= nfs4_lookup_revalidate,
 	.d_delete	= nfs_dentry_delete,
 	.d_iput		= nfs_dentry_iput,
 	.d_automount	= nfs_d_automount,
@@ -1489,12 +1489,11 @@ no_open:
 	return nfs_lookup(dir, dentry, nd);
 }
 
-static int nfs_open_revalidate(struct dentry *dentry, struct nameidata *nd)
+static int nfs4_lookup_revalidate(struct dentry *dentry, struct nameidata *nd)
 {
 	struct dentry *parent = NULL;
 	struct inode *inode;
 	struct inode *dir;
-	struct nfs_open_context *ctx;
 	int openflags, ret = 0;
 
 	if (nd->flags & LOOKUP_RCU)
@@ -1523,49 +1522,13 @@ static int nfs_open_revalidate(struct dentry *dentry, struct nameidata *nd)
 	/* We cannot do exclusive creation on a positive dentry */
 	if ((openflags & (O_CREAT|O_EXCL)) == (O_CREAT|O_EXCL))
 		goto no_open_dput;
-	/* We can't create new files, or truncate existing ones here */
-	openflags &= ~(O_CREAT|O_EXCL|O_TRUNC);
 
-	ctx = create_nfs_open_context(dentry, openflags);
-	ret = PTR_ERR(ctx);
-	if (IS_ERR(ctx))
-		goto out;
-	/*
-	 * Note: we're not holding inode->i_mutex and so may be racing with
-	 * operations that change the directory. We therefore save the
-	 * change attribute *before* we do the RPC call.
-	 */
-	inode = NFS_PROTO(dir)->open_context(dir, ctx, openflags, NULL);
-	if (IS_ERR(inode)) {
-		ret = PTR_ERR(inode);
-		switch (ret) {
-		case -EPERM:
-		case -EACCES:
-		case -EDQUOT:
-		case -ENOSPC:
-		case -EROFS:
-			goto out_put_ctx;
-		default:
-			goto out_drop;
-		}
-	}
-	iput(inode);
-	if (inode != dentry->d_inode)
-		goto out_drop;
+	/* Let f_op->open() actually open (and revalidate) the file */
+	ret = 1;
 
-	nfs_set_verifier(dentry, nfs_save_change_attribute(dir));
-	ret = nfs_intent_set_file(nd, ctx);
-	if (ret >= 0)
-		ret = 1;
 out:
 	dput(parent);
 	return ret;
-out_drop:
-	d_drop(dentry);
-	ret = 0;
-out_put_ctx:
-	put_nfs_open_context(ctx);
-	goto out;
 
 no_open_dput:
 	dput(parent);
diff --git a/fs/nfs/file.c b/fs/nfs/file.c
index c43a452..4e626ec 100644
--- a/fs/nfs/file.c
+++ b/fs/nfs/file.c
@@ -870,12 +870,73 @@ const struct file_operations nfs_file_operations = {
 static int
 nfs4_file_open(struct inode *inode, struct file *filp)
 {
+	struct nfs_open_context *ctx;
+	struct dentry *dentry = filp->f_path.dentry;
+	struct dentry *parent = NULL;
+	struct inode *dir;
+	unsigned openflags = filp->f_flags;
+	int err;
+
+	BUG_ON(inode != dentry->d_inode);
 	/*
-	 * NFSv4 opens are handled in d_lookup and d_revalidate. If we get to
-	 * this point, then something is very wrong
+	 * If no cached dentry exists or if it's negative, NFSv4 handled the
+	 * opens in ->lookup() or ->create().
+	 *
+	 * We only get this far for a cached positive dentry.  We skipped
+	 * revalidation, so handle it here by dropping the dentry and returning
+	 * -ESTALE.  The VFS will retry the lookup/create/open.
 	 */
-	dprintk("NFS: %s called! inode=%p filp=%p\n", __func__, inode, filp);
-	return -ENOTDIR;
+
+	dprintk("NFS: open file(%s/%s)\n",
+		dentry->d_parent->d_name.name,
+		dentry->d_name.name);
+
+	if ((openflags & O_ACCMODE) == 3)
+		openflags--;
+
+	/* We can't create new files, or truncate existing ones here */
+	openflags &= ~(O_CREAT|O_EXCL|O_TRUNC);
+
+	parent = dget_parent(dentry);
+	dir = parent->d_inode;
+
+	ctx = alloc_nfs_open_context(filp->f_path.dentry, filp->f_mode);
+	err = PTR_ERR(ctx);
+	if (IS_ERR(ctx))
+		goto out;
+
+	inode = NFS_PROTO(dir)->open_context(dir, ctx, openflags, NULL);
+	if (IS_ERR(inode)) {
+		err = PTR_ERR(inode);
+		switch (err) {
+		case -EPERM:
+		case -EACCES:
+		case -EDQUOT:
+		case -ENOSPC:
+		case -EROFS:
+			goto out_put_ctx;
+		default:
+			goto out_drop;
+		}
+	}
+	iput(inode);
+	if (inode != dentry->d_inode)
+		goto out_drop;
+
+	nfs_set_verifier(dentry, nfs_save_change_attribute(dir));
+	nfs_file_set_open_context(filp, ctx);
+	err = 0;
+
+out_put_ctx:
+	put_nfs_open_context(ctx);
+out:
+	dput(parent);
+	return err;
+
+out_drop:
+	d_drop(dentry);
+	err = -ESTALE;
+	goto out_put_ctx;
 }
 
 const struct file_operations nfs4_file_operations = {
-- 
1.7.7

Re: [PATCH 9/9] nfs: don't open in ->d_revalidate

[Myklebust, Trond]

On Tue, 2012-03-06 at 13:56 +0100, Miklos Szeredi wrote:
> From: Miklos Szeredi <mszeredi@suse.cz>
> 
> NFSv4 can't do reliable opens in d_revalidate, since it cannot know whether a
> mount needs to be followed or not.  It does check d_mountpoint() on the dentry,
> which can result in a weird error if the VFS found that the mount does not in
> fact need to be followed, e.g.:
> 
>   # mount --bind /mnt/nfs /mnt/nfs-clone
>   # echo something > /mnt/nfs/tmp/bar
>   # echo x > /tmp/file
>   # mount --bind /tmp/file /mnt/nfs-clone/tmp/bar
>   # cat  /mnt/nfs/tmp/bar
>   cat: /mnt/nfs/tmp/bar: Not a directory
> 
> Which should, by any sane filesystem, result in "something" being printed.
> 
> So instead do the open in f_op->open() and in the unlikely case that the cached
> dentry turned out to be invalid, drop the dentry and return ESTALE to let the
> VFS retry.

This patch would force a complete new walk of the path in cases where
today we just do a single lookup of the last component. It really
doesn't seem worth taking that penalty just in order to make some insane
bind mount corner cases work.

-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com

Re: [PATCH 9/9] nfs: don't open in ->d_revalidate

[Miklos Szeredi]

"Myklebust, Trond" <Trond.Myklebust@netapp.com> writes:

> On Tue, 2012-03-06 at 13:56 +0100, Miklos Szeredi wrote:
>> From: Miklos Szeredi <mszeredi@suse.cz>
>> 
>> NFSv4 can't do reliable opens in d_revalidate, since it cannot know whether a
>> mount needs to be followed or not.  It does check d_mountpoint() on the dentry,
>> which can result in a weird error if the VFS found that the mount does not in
>> fact need to be followed, e.g.:
>> 
>>   # mount --bind /mnt/nfs /mnt/nfs-clone
>>   # echo something > /mnt/nfs/tmp/bar
>>   # echo x > /tmp/file
>>   # mount --bind /tmp/file /mnt/nfs-clone/tmp/bar
>>   # cat  /mnt/nfs/tmp/bar
>>   cat: /mnt/nfs/tmp/bar: Not a directory
>> 
>> Which should, by any sane filesystem, result in "something" being printed.
>> 
>> So instead do the open in f_op->open() and in the unlikely case that the cached
>> dentry turned out to be invalid, drop the dentry and return ESTALE to let the
>> VFS retry.
>
> This patch would force a complete new walk of the path in cases where
> today we just do a single lookup of the last component.

I sort of anticipated that, but I really don't know how much of an issue
is this.

Of course it is possible to redo only the last component after
f_op->open() return ESTALE but that requires reshuffling do_last().

>  It really
> doesn't seem worth taking that penalty just in order to make some insane
> bind mount corner cases work.

It's not at all insane if for example we have multiple mount namespaces.
NFS is clearly broken and it needs to be fixed, one way or another.

If you think this is a serious performance regression then lets drop
this patch and add it to the atomic open series together with the
do_last() reshuffling.

Thanks,
Miklos

Re: [PATCH 9/9] nfs: don't open in ->d_revalidate

[Myklebust, Trond]

On Tue, 2012-03-06 at 17:26 +0100, Miklos Szeredi wrote:
> "Myklebust, Trond" <Trond.Myklebust@netapp.com> writes:
> 
> > On Tue, 2012-03-06 at 13:56 +0100, Miklos Szeredi wrote:
> >> From: Miklos Szeredi <mszeredi@suse.cz>
> >> 
> >> NFSv4 can't do reliable opens in d_revalidate, since it cannot know whether a
> >> mount needs to be followed or not.  It does check d_mountpoint() on the dentry,
> >> which can result in a weird error if the VFS found that the mount does not in
> >> fact need to be followed, e.g.:
> >> 
> >>   # mount --bind /mnt/nfs /mnt/nfs-clone
> >>   # echo something > /mnt/nfs/tmp/bar
> >>   # echo x > /tmp/file
> >>   # mount --bind /tmp/file /mnt/nfs-clone/tmp/bar
> >>   # cat  /mnt/nfs/tmp/bar
> >>   cat: /mnt/nfs/tmp/bar: Not a directory
> >> 
> >> Which should, by any sane filesystem, result in "something" being printed.
> >> 
> >> So instead do the open in f_op->open() and in the unlikely case that the cached
> >> dentry turned out to be invalid, drop the dentry and return ESTALE to let the
> >> VFS retry.
> >
> > This patch would force a complete new walk of the path in cases where
> > today we just do a single lookup of the last component.
> 
> I sort of anticipated that, but I really don't know how much of an issue
> is this.

It makes a big difference to the cache miss case, since not only is the
entire path looked up again, but the LOOKUP_REVAL flag gets set, which
forces a full lookup of each component (as opposed to just
revalidating).

> Of course it is possible to redo only the last component after
> f_op->open() return ESTALE but that requires reshuffling do_last().
> 
> >  It really
> > doesn't seem worth taking that penalty just in order to make some insane
> > bind mount corner cases work.
> 
> It's not at all insane if for example we have multiple mount namespaces.
> NFS is clearly broken and it needs to be fixed, one way or another.

Right, but this is still an extremely rare usage case: you are the first
person to report it as being broken (OK, Al may have mentioned it in the
past too). There have been no actual user reports AFAIK.

OTOH, cache misses are frequent.

> If you think this is a serious performance regression then lets drop
> this patch and add it to the atomic open series together with the
> do_last() reshuffling.

That sounds like a good suggestion to me.

-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com

Re: [PATCH 9/9] nfs: don't open in ->d_revalidate

[Miklos Szeredi]

"Myklebust, Trond" <Trond.Myklebust@netapp.com> writes:

> It makes a big difference to the cache miss case, since not only is the
> entire path looked up again, but the LOOKUP_REVAL flag gets set, which
> forces a full lookup of each component (as opposed to just
> revalidating).

This is not about a cache miss.  It's about a cache hit (positive cached
dentry) but changed inode on the server.

Is this a likely scenario?

Thanks,
Miklos

Re: [PATCH 9/9] nfs: don't open in ->d_revalidate

[Myklebust, Trond]

On Tue, 2012-03-06 at 18:10 +0100, Miklos Szeredi wrote:
> "Myklebust, Trond" <Trond.Myklebust@netapp.com> writes:
> 
> > It makes a big difference to the cache miss case, since not only is the
> > entire path looked up again, but the LOOKUP_REVAL flag gets set, which
> > forces a full lookup of each component (as opposed to just
> > revalidating).
> 
> This is not about a cache miss.  It's about a cache hit (positive cached
> dentry) but changed inode on the server.
> 
> Is this a likely scenario?

Consider something like a git pull on the server, or a distributed
compile across many NFS clients. In both those cases, you are typically
creating and re-creating files with the same name while bypassing the
dcache on your client.

-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com