[PATCH 00/11] mainly d_splice_alias fixes

[PATCH 00/11] mainly d_splice_alias fixes

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

The following are fixes mainly to d_splice_alias and related code.  I
think they'd be appropriate for 3.16, though some of them could probably
use another pair of eyes.

For future work:

	- d_splice_alias and d_materialise_unique are now doing nearly
	  the same thing and should probably be combined.
	- __d_unalias's dependence on trylock's can cause spurious
	  lookup failures on NFS.  I've been meaning to look into
	  whether it's reasonable (and good enough) to just retry in
	  those cases.

--b.

J. Bruce Fields (11):
  namei: trivial fix to vfs_rename_dir comment
  dcache: move d_splice_alias
  dcache: close d_move race in d_splice_alias
  dcache: d_splice_alias mustn't create directory aliases
  dcache: d_splice_alias should ignore DCACHE_DISCONNECTED
  dcache: d_obtain_alias callers don't all want DISCONNECTED
  dcache: remove unused d_find_alias parameter
  dcache: d_find_alias needn't recheck IS_ROOT && DCACHE_DISCONNECTED
  exportfs: update Exporting documentation
  dcache: d_materialise_unique isn't GPL-only
  dcache: d_splice_alias should detect loops

 Documentation/filesystems/nfs/Exporting |   38 +++---
 fs/btrfs/super.c                        |    9 +-
 fs/ceph/super.c                         |    2 +-
 fs/dcache.c                             |  198 +++++++++++++++++++------------
 fs/namei.c                              |    2 +-
 fs/nfs/getroot.c                        |    2 +-
 fs/nilfs2/super.c                       |    2 +-
 include/linux/dcache.h                  |    1 +
 8 files changed, 148 insertions(+), 106 deletions(-)

-- 
1.7.9.5

[PATCH 01/11] namei: trivial fix to vfs_rename_dir comment

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

Looks like the directory loop check is actually done in renameat?
Whatever, leave this out rather than trying to keep it up to date with
the code.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/namei.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/namei.c b/fs/namei.c
index 8016827..b837754 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -4017,7 +4017,7 @@ SYSCALL_DEFINE2(link, const char __user *, oldname, const char __user *, newname
  * The worst of all namespace operations - renaming directory. "Perverted"
  * doesn't even start to describe it. Somebody in UCB had a heck of a trip...
  * Problems:
- *	a) we can get into loop creation. Check is done in is_subdir().
+ *	a) we can get into loop creation.
  *	b) race potential - two innocent renames can create a loop together.
  *	   That's where 4.4 screws up. Current fix: serialization on
  *	   sb->s_vfs_rename_mutex. We might be more accurate, but that's another
-- 
1.7.9.5

[PATCH 02/11] dcache: move d_splice_alias

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

Just a trivial move to locate it near (similar) d_materialise_unique
code and save some forward references in a following patch.

Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/dcache.c |  104 +++++++++++++++++++++++++++++------------------------------
 1 file changed, 52 insertions(+), 52 deletions(-)

diff --git a/fs/dcache.c b/fs/dcache.c
index 42ae01e..467389f 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -1791,58 +1791,6 @@ struct dentry *d_obtain_alias(struct inode *inode)
 EXPORT_SYMBOL(d_obtain_alias);
 
 /**
- * d_splice_alias - splice a disconnected dentry into the tree if one exists
- * @inode:  the inode which may have a disconnected dentry
- * @dentry: a negative dentry which we want to point to the inode.
- *
- * If inode is a directory and has a 'disconnected' dentry (i.e. IS_ROOT and
- * DCACHE_DISCONNECTED), then d_move that in place of the given dentry
- * and return it, else simply d_add the inode to the dentry and return NULL.
- *
- * This is needed in the lookup routine of any filesystem that is exportable
- * (via knfsd) so that we can build dcache paths to directories effectively.
- *
- * If a dentry was found and moved, then it is returned.  Otherwise NULL
- * is returned.  This matches the expected return value of ->lookup.
- *
- * Cluster filesystems may call this function with a negative, hashed dentry.
- * In that case, we know that the inode will be a regular file, and also this
- * will only occur during atomic_open. So we need to check for the dentry
- * being already hashed only in the final case.
- */
-struct dentry *d_splice_alias(struct inode *inode, struct dentry *dentry)
-{
-	struct dentry *new = NULL;
-
-	if (IS_ERR(inode))
-		return ERR_CAST(inode);
-
-	if (inode && S_ISDIR(inode->i_mode)) {
-		spin_lock(&inode->i_lock);
-		new = __d_find_alias(inode, 1);
-		if (new) {
-			BUG_ON(!(new->d_flags & DCACHE_DISCONNECTED));
-			spin_unlock(&inode->i_lock);
-			security_d_instantiate(new, inode);
-			d_move(new, dentry);
-			iput(inode);
-		} else {
-			/* already taking inode->i_lock, so d_add() by hand */
-			__d_instantiate(dentry, inode);
-			spin_unlock(&inode->i_lock);
-			security_d_instantiate(dentry, inode);
-			d_rehash(dentry);
-		}
-	} else {
-		d_instantiate(dentry, inode);
-		if (d_unhashed(dentry))
-			d_rehash(dentry);
-	}
-	return new;
-}
-EXPORT_SYMBOL(d_splice_alias);
-
-/**
  * d_add_ci - lookup or allocate new dentry with case-exact name
  * @inode:  the inode case-insensitive lookup has found
  * @dentry: the negative dentry that was passed to the parent's lookup func
@@ -2634,6 +2582,58 @@ static void __d_materialise_dentry(struct dentry *dentry, struct dentry *anon)
 }
 
 /**
+ * d_splice_alias - splice a disconnected dentry into the tree if one exists
+ * @inode:  the inode which may have a disconnected dentry
+ * @dentry: a negative dentry which we want to point to the inode.
+ *
+ * If inode is a directory and has a 'disconnected' dentry (i.e. IS_ROOT and
+ * DCACHE_DISCONNECTED), then d_move that in place of the given dentry
+ * and return it, else simply d_add the inode to the dentry and return NULL.
+ *
+ * This is needed in the lookup routine of any filesystem that is exportable
+ * (via knfsd) so that we can build dcache paths to directories effectively.
+ *
+ * If a dentry was found and moved, then it is returned.  Otherwise NULL
+ * is returned.  This matches the expected return value of ->lookup.
+ *
+ * Cluster filesystems may call this function with a negative, hashed dentry.
+ * In that case, we know that the inode will be a regular file, and also this
+ * will only occur during atomic_open. So we need to check for the dentry
+ * being already hashed only in the final case.
+ */
+struct dentry *d_splice_alias(struct inode *inode, struct dentry *dentry)
+{
+	struct dentry *new = NULL;
+
+	if (IS_ERR(inode))
+		return ERR_CAST(inode);
+
+	if (inode && S_ISDIR(inode->i_mode)) {
+		spin_lock(&inode->i_lock);
+		new = __d_find_alias(inode, 1);
+		if (new) {
+			BUG_ON(!(new->d_flags & DCACHE_DISCONNECTED));
+			spin_unlock(&inode->i_lock);
+			security_d_instantiate(new, inode);
+			d_move(new, dentry);
+			iput(inode);
+		} else {
+			/* already taking inode->i_lock, so d_add() by hand */
+			__d_instantiate(dentry, inode);
+			spin_unlock(&inode->i_lock);
+			security_d_instantiate(dentry, inode);
+			d_rehash(dentry);
+		}
+	} else {
+		d_instantiate(dentry, inode);
+		if (d_unhashed(dentry))
+			d_rehash(dentry);
+	}
+	return new;
+}
+EXPORT_SYMBOL(d_splice_alias);
+
+/**
  * d_materialise_unique - introduce an inode into the tree
  * @dentry: candidate dentry
  * @inode: inode to bind to the dentry, to which aliases may be attached
-- 
1.7.9.5

[PATCH 03/11] dcache: close d_move race in d_splice_alias

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

d_splice_alias will d_move an IS_ROOT() directory dentry into place if
one exists.  This should be safe as long as the dentry remains IS_ROOT,
but I can't see what guarantees that: once we drop the i_lock all we
hold here is the i_mutex on an unrelated parent directory.

Instead copy the logic of d_materialise_unique.

Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/dcache.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/fs/dcache.c b/fs/dcache.c
index 467389f..a833e97 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -2613,9 +2613,14 @@ struct dentry *d_splice_alias(struct inode *inode, struct dentry *dentry)
 		new = __d_find_alias(inode, 1);
 		if (new) {
 			BUG_ON(!(new->d_flags & DCACHE_DISCONNECTED));
+			write_seqlock(&rename_lock);
+			__d_materialise_dentry(dentry, new);
+			write_sequnlock(&rename_lock);
+			__d_drop(new);
+			_d_rehash(new);
+			spin_unlock(&new->d_lock);
 			spin_unlock(&inode->i_lock);
 			security_d_instantiate(new, inode);
-			d_move(new, dentry);
 			iput(inode);
 		} else {
 			/* already taking inode->i_lock, so d_add() by hand */
-- 
1.7.9.5

[PATCH 04/11] dcache: d_splice_alias mustn't create directory aliases

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

Currently if d_splice_alias finds a directory with an alias that is not
IS_ROOT or not DCACHE_DISCONNECTED, it creates a duplicate directory.

Duplicate directory dentries are unacceptable; it is better just to
error out.

(In the case of a local filesystem the most likely case is filesystem
corruption: for example, perhaps two directories point to the same child
directory, and the other parent has already been found and cached.)

Note that distributed filesystems may encounter this case in normal
operation if a remote host moves a directory to a location different
from the one we last cached in the dcache.  For that reason, such
filesystems should instead use d_materialise_unique, which tries to move
the old directory alias to the right place instead of erroring out.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/dcache.c |   11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/fs/dcache.c b/fs/dcache.c
index a833e97..54b602e 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -2590,6 +2590,9 @@ static void __d_materialise_dentry(struct dentry *dentry, struct dentry *anon)
  * DCACHE_DISCONNECTED), then d_move that in place of the given dentry
  * and return it, else simply d_add the inode to the dentry and return NULL.
  *
+ * If a non-IS_ROOT directory is found, the filesystem is corrupt, and
+ * we should error out: directories can't have multiple aliases.
+ *
  * This is needed in the lookup routine of any filesystem that is exportable
  * (via knfsd) so that we can build dcache paths to directories effectively.
  *
@@ -2610,9 +2613,13 @@ struct dentry *d_splice_alias(struct inode *inode, struct dentry *dentry)
 
 	if (inode && S_ISDIR(inode->i_mode)) {
 		spin_lock(&inode->i_lock);
-		new = __d_find_alias(inode, 1);
+		new = __d_find_any_alias(inode);
 		if (new) {
-			BUG_ON(!(new->d_flags & DCACHE_DISCONNECTED));
+			if (!IS_ROOT(new) || !(new->d_flags & DCACHE_DISCONNECTED)) {
+				spin_unlock(&inode->i_lock);
+				dput(new);
+				return ERR_PTR(-EIO);
+			}
 			write_seqlock(&rename_lock);
 			__d_materialise_dentry(dentry, new);
 			write_sequnlock(&rename_lock);
-- 
1.7.9.5

[PATCH 05/11] dcache: d_splice_alias should ignore DCACHE_DISCONNECTED

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

Any IS_ROOT() alias should be safe to use; there's nothing special about
DCACHE_DISCONNECTED dentries.

Note that this is in fact useful for filesystems such as btrfs which can
legimately encounter a directory with a preexisting IS_ROOT alias on a
lookup that crosses into a subvolume.  (Those aliases are currently
marked DCACHE_DISCONNECTED--but not really for any good reason, and
we'll change that soon.)

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/dcache.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/dcache.c b/fs/dcache.c
index 54b602e..c42009f 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -2586,9 +2586,9 @@ static void __d_materialise_dentry(struct dentry *dentry, struct dentry *anon)
  * @inode:  the inode which may have a disconnected dentry
  * @dentry: a negative dentry which we want to point to the inode.
  *
- * If inode is a directory and has a 'disconnected' dentry (i.e. IS_ROOT and
- * DCACHE_DISCONNECTED), then d_move that in place of the given dentry
- * and return it, else simply d_add the inode to the dentry and return NULL.
+ * If inode is a directory and has an IS_ROOT alias, then d_move that in
+ * place of the given dentry and return it, else simply d_add the inode
+ * to the dentry and return NULL.
  *
  * If a non-IS_ROOT directory is found, the filesystem is corrupt, and
  * we should error out: directories can't have multiple aliases.
@@ -2615,7 +2615,7 @@ struct dentry *d_splice_alias(struct inode *inode, struct dentry *dentry)
 		spin_lock(&inode->i_lock);
 		new = __d_find_any_alias(inode);
 		if (new) {
-			if (!IS_ROOT(new) || !(new->d_flags & DCACHE_DISCONNECTED)) {
+			if (!IS_ROOT(new)) {
 				spin_unlock(&inode->i_lock);
 				dput(new);
 				return ERR_PTR(-EIO);
-- 
1.7.9.5

[PATCH 06/11] dcache: d_obtain_alias callers don't all want DISCONNECTED

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

There are a few d_obtain_alias callers that are using it to get the
root of a filesystem which may already have an alias somewhere else.

This is not the same as the filehandle-lookup case, and none of them
actually need DCACHE_DISCONNECTED set.

It isn't really a serious problem, but it would really be clearer if we
reserved DCACHE_DISCONNECTED for those cases where it's actually needed.

In the btrfs case this was causing a spurious printk from
nfsd/nfsfh.c:fh_verify when it found an unexpected DCACHE_DISCONNECTED
dentry.  Josef worked around this by unsetting DCACHE_DISCONNECTED
manually in 3a0dfa6a12e "Btrfs: unset DCACHE_DISCONNECTED when mounting
default subvol", and this replaces that workaround.

Cc: Josef Bacik <jbacik@fb.com>
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/btrfs/super.c       |    9 +------
 fs/ceph/super.c        |    2 +-
 fs/dcache.c            |   69 ++++++++++++++++++++++++++++++++++--------------
 fs/nfs/getroot.c       |    2 +-
 fs/nilfs2/super.c      |    2 +-
 include/linux/dcache.h |    1 +
 6 files changed, 54 insertions(+), 31 deletions(-)

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 9601d25..b05588b 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -843,7 +843,6 @@ static struct dentry *get_default_root(struct super_block *sb,
 	struct btrfs_path *path;
 	struct btrfs_key location;
 	struct inode *inode;
-	struct dentry *dentry;
 	u64 dir_id;
 	int new = 0;
 
@@ -914,13 +913,7 @@ setup_root:
 		return dget(sb->s_root);
 	}
 
-	dentry = d_obtain_alias(inode);
-	if (!IS_ERR(dentry)) {
-		spin_lock(&dentry->d_lock);
-		dentry->d_flags &= ~DCACHE_DISCONNECTED;
-		spin_unlock(&dentry->d_lock);
-	}
-	return dentry;
+	return d_obtain_root(inode);
 }
 
 static int btrfs_fill_super(struct super_block *sb,
diff --git a/fs/ceph/super.c b/fs/ceph/super.c
index 06150fd..f6e1237 100644
--- a/fs/ceph/super.c
+++ b/fs/ceph/super.c
@@ -755,7 +755,7 @@ static struct dentry *open_root_dentry(struct ceph_fs_client *fsc,
 				goto out;
 			}
 		} else {
-			root = d_obtain_alias(inode);
+			root = d_obtain_root(inode);
 		}
 		ceph_init_dentry(root);
 		dout("open_root_inode success, root dentry is %p\n", root);
diff --git a/fs/dcache.c b/fs/dcache.c
index c42009f..5a8517f 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -1718,25 +1718,7 @@ struct dentry *d_find_any_alias(struct inode *inode)
 }
 EXPORT_SYMBOL(d_find_any_alias);
 
-/**
- * d_obtain_alias - find or allocate a dentry for a given inode
- * @inode: inode to allocate the dentry for
- *
- * Obtain a dentry for an inode resulting from NFS filehandle conversion or
- * similar open by handle operations.  The returned dentry may be anonymous,
- * or may have a full name (if the inode was already in the cache).
- *
- * When called on a directory inode, we must ensure that the inode only ever
- * has one dentry.  If a dentry is found, that is returned instead of
- * allocating a new one.
- *
- * On successful return, the reference to the inode has been transferred
- * to the dentry.  In case of an error the reference on the inode is released.
- * To make it easier to use in export operations a %NULL or IS_ERR inode may
- * be passed in and will be the error will be propagate to the return value,
- * with a %NULL @inode replaced by ERR_PTR(-ESTALE).
- */
-struct dentry *d_obtain_alias(struct inode *inode)
+struct dentry *__d_obtain_alias(struct inode *inode, int disconnected)
 {
 	static const struct qstr anonstring = QSTR_INIT("/", 1);
 	struct dentry *tmp;
@@ -1767,7 +1749,10 @@ struct dentry *d_obtain_alias(struct inode *inode)
 	}
 
 	/* attach a disconnected dentry */
-	add_flags = d_flags_for_inode(inode) | DCACHE_DISCONNECTED;
+	add_flags = d_flags_for_inode(inode);
+
+	if (disconnected)
+		add_flags |= DCACHE_DISCONNECTED;
 
 	spin_lock(&tmp->d_lock);
 	tmp->d_inode = inode;
@@ -1788,9 +1773,53 @@ struct dentry *d_obtain_alias(struct inode *inode)
 	iput(inode);
 	return res;
 }
+
+/**
+ * d_obtain_alias - find or allocate a DISCONNECTED dentry for a given inode
+ * @inode: inode to allocate the dentry for
+ *
+ * Obtain a dentry for an inode resulting from NFS filehandle conversion or
+ * similar open by handle operations.  The returned dentry may be anonymous,
+ * or may have a full name (if the inode was already in the cache).
+ *
+ * When called on a directory inode, we must ensure that the inode only ever
+ * has one dentry.  If a dentry is found, that is returned instead of
+ * allocating a new one.
+ *
+ * On successful return, the reference to the inode has been transferred
+ * to the dentry.  In case of an error the reference on the inode is released.
+ * To make it easier to use in export operations a %NULL or IS_ERR inode may
+ * be passed in and the error will be propagated to the return value,
+ * with a %NULL @inode replaced by ERR_PTR(-ESTALE).
+ */
+struct dentry *d_obtain_alias(struct inode *inode)
+{
+	return __d_obtain_alias(inode, 1);
+}
 EXPORT_SYMBOL(d_obtain_alias);
 
 /**
+ * d_obtain_root - find or allocate a dentry for a given inode
+ * @inode: inode to allocate the dentry for
+ *
+ * Obtain an IS_ROOT dentry for the root of a filesystem.
+ *
+ * We must ensure that directory inodes only ever have one dentry.  If a
+ * dentry is found, that is returned instead of allocating a new one.
+ *
+ * On successful return, the reference to the inode has been transferred
+ * to the dentry.  In case of an error the reference on the inode is
+ * released.  A %NULL or IS_ERR inode may be passed in and will be the
+ * error will be propagate to the return value, with a %NULL @inode
+ * replaced by ERR_PTR(-ESTALE).
+ */
+struct dentry *d_obtain_root(struct inode *inode)
+{
+	return __d_obtain_alias(inode, 0);
+}
+EXPORT_SYMBOL(d_obtain_root);
+
+/**
  * d_add_ci - lookup or allocate new dentry with case-exact name
  * @inode:  the inode case-insensitive lookup has found
  * @dentry: the negative dentry that was passed to the parent's lookup func
diff --git a/fs/nfs/getroot.c b/fs/nfs/getroot.c
index 66984a9..e851f53 100644
--- a/fs/nfs/getroot.c
+++ b/fs/nfs/getroot.c
@@ -112,7 +112,7 @@ struct dentry *nfs_get_root(struct super_block *sb, struct nfs_fh *mntfh,
 	 * if the dentry tree reaches them; however if the dentry already
 	 * exists, we'll pick it up at this point and use it as the root
 	 */
-	ret = d_obtain_alias(inode);
+	ret = d_obtain_root(inode);
 	if (IS_ERR(ret)) {
 		dprintk("nfs_get_root: get root dentry failed\n");
 		goto out;
diff --git a/fs/nilfs2/super.c b/fs/nilfs2/super.c
index 8c532b2..ac91499 100644
--- a/fs/nilfs2/super.c
+++ b/fs/nilfs2/super.c
@@ -942,7 +942,7 @@ static int nilfs_get_root_dentry(struct super_block *sb,
 			iput(inode);
 		}
 	} else {
-		dentry = d_obtain_alias(inode);
+		dentry = d_obtain_root(inode);
 		if (IS_ERR(dentry)) {
 			ret = PTR_ERR(dentry);
 			goto failed_dentry;
diff --git a/include/linux/dcache.h b/include/linux/dcache.h
index 3c7ec32..e4ae2ad 100644
--- a/include/linux/dcache.h
+++ b/include/linux/dcache.h
@@ -249,6 +249,7 @@ extern struct dentry * d_splice_alias(struct inode *, struct dentry *);
 extern struct dentry * d_add_ci(struct dentry *, struct inode *, struct qstr *);
 extern struct dentry *d_find_any_alias(struct inode *inode);
 extern struct dentry * d_obtain_alias(struct inode *);
+extern struct dentry * d_obtain_root(struct inode *);
 extern void shrink_dcache_sb(struct super_block *);
 extern void shrink_dcache_parent(struct dentry *);
 extern void shrink_dcache_for_umount(struct super_block *);
-- 
1.7.9.5

[PATCH 07/11] dcache: remove unused d_find_alias parameter

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/dcache.c |   13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/fs/dcache.c b/fs/dcache.c
index 5a8517f..9209479 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -696,8 +696,6 @@ EXPORT_SYMBOL(dget_parent);
 /**
  * d_find_alias - grab a hashed alias of inode
  * @inode: inode in question
- * @want_discon:  flag, used by d_splice_alias, to request
- *          that only a DISCONNECTED alias be returned.
  *
  * If inode has a hashed alias, or is a directory and has any alias,
  * acquire the reference to alias and return it. Otherwise return NULL.
@@ -706,10 +704,9 @@ EXPORT_SYMBOL(dget_parent);
  * of a filesystem.
  *
  * If the inode has an IS_ROOT, DCACHE_DISCONNECTED alias, then prefer
- * any other hashed alias over that one unless @want_discon is set,
- * in which case only return an IS_ROOT, DCACHE_DISCONNECTED alias.
+ * any other hashed alias over that one.
  */
-static struct dentry *__d_find_alias(struct inode *inode, int want_discon)
+static struct dentry *__d_find_alias(struct inode *inode)
 {
 	struct dentry *alias, *discon_alias;
 
@@ -721,7 +718,7 @@ again:
 			if (IS_ROOT(alias) &&
 			    (alias->d_flags & DCACHE_DISCONNECTED)) {
 				discon_alias = alias;
-			} else if (!want_discon) {
+			} else {
 				__dget_dlock(alias);
 				spin_unlock(&alias->d_lock);
 				return alias;
@@ -752,7 +749,7 @@ struct dentry *d_find_alias(struct inode *inode)
 
 	if (!hlist_empty(&inode->i_dentry)) {
 		spin_lock(&inode->i_lock);
-		de = __d_find_alias(inode, 0);
+		de = __d_find_alias(inode);
 		spin_unlock(&inode->i_lock);
 	}
 	return de;
@@ -2702,7 +2699,7 @@ struct dentry *d_materialise_unique(struct dentry *dentry, struct inode *inode)
 		struct dentry *alias;
 
 		/* Does an aliased dentry already exist? */
-		alias = __d_find_alias(inode, 0);
+		alias = __d_find_alias(inode);
 		if (alias) {
 			actual = alias;
 			write_seqlock(&rename_lock);
-- 
1.7.9.5

[PATCH 08/11] dcache: d_find_alias needn't recheck IS_ROOT && DCACHE_DISCONNECTED

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

If we get to this point and discover the dentry is not a root dentry, or
not DCACHE_DISCONNECTED--great, we always prefer that anyway.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/dcache.c |    9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/fs/dcache.c b/fs/dcache.c
index 9209479..671a04a 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -730,12 +730,9 @@ again:
 		alias = discon_alias;
 		spin_lock(&alias->d_lock);
 		if (S_ISDIR(inode->i_mode) || !d_unhashed(alias)) {
-			if (IS_ROOT(alias) &&
-			    (alias->d_flags & DCACHE_DISCONNECTED)) {
-				__dget_dlock(alias);
-				spin_unlock(&alias->d_lock);
-				return alias;
-			}
+			__dget_dlock(alias);
+			spin_unlock(&alias->d_lock);
+			return alias;
 		}
 		spin_unlock(&alias->d_lock);
 		goto again;
-- 
1.7.9.5

[PATCH 09/11] exportfs: update Exporting documentation

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

Minor documentation updates:
	- refer to d_obtain_alias rather than d_alloc_anon
	- explain when to use d_splice_alias and when
	  d_materialise_unique.
	- cut some details of d_splice_alias/d_materialise_unique
	  implementation.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 Documentation/filesystems/nfs/Exporting |   38 +++++++++++++++++++------------
 1 file changed, 23 insertions(+), 15 deletions(-)

diff --git a/Documentation/filesystems/nfs/Exporting b/Documentation/filesystems/nfs/Exporting
index e543b1a..9b7de5b 100644
--- a/Documentation/filesystems/nfs/Exporting
+++ b/Documentation/filesystems/nfs/Exporting
@@ -66,23 +66,31 @@ b/ A per-superblock list "s_anon" of dentries which are the roots of
 
 c/ Helper routines to allocate anonymous dentries, and to help attach
    loose directory dentries at lookup time. They are:
-    d_alloc_anon(inode) will return a dentry for the given inode.
+    d_obtain_alias(inode) will return a dentry for the given inode.
       If the inode already has a dentry, one of those is returned.
       If it doesn't, a new anonymous (IS_ROOT and
         DCACHE_DISCONNECTED) dentry is allocated and attached.
       In the case of a directory, care is taken that only one dentry
       can ever be attached.
-    d_splice_alias(inode, dentry) will make sure that there is a
-      dentry with the same name and parent as the given dentry, and
-      which refers to the given inode.
-      If the inode is a directory and already has a dentry, then that
-      dentry is d_moved over the given dentry.
-      If the passed dentry gets attached, care is taken that this is
-      mutually exclusive to a d_alloc_anon operation.
-      If the passed dentry is used, NULL is returned, else the used
-      dentry is returned.  This corresponds to the calling pattern of
-      ->lookup.
-  
+    d_splice_alias(inode, dentry) or d_materialise_unique(dentry, inode)
+      will introduce a new dentry into the tree; either the passed-in
+      dentry or a preexisting alias for the given inode (such as an
+      anonymous one created by d_obtain_alias), if appropriate.  The two
+      functions differ in their handling of directories with preexisting
+      aliases:
+        d_splice_alias will use any existing IS_ROOT dentry, but it will
+	  return -EIO rather than try to move a dentry with a different
+	  parent.  This is appropriate for local filesystems, which
+	  should never see such an alias unless the filesystem is
+	  corrupted somehow (for example, if two on-disk directory
+	  entries refer to the same directory.)
+	d_obtain_alias will attempt to move any dentry.  This is
+	  appropriate for distributed filesystems, where finding a
+	  directory other than where we last cached it may be a normal
+	  consequence of concurrent operations on other hosts.
+      Both functions return NULL when the passed-in dentry is used,
+      following the calling convention of ->lookup.
+
  
 Filesystem Issues
 -----------------
@@ -120,12 +128,12 @@ struct which has the following members:
 
   fh_to_dentry (mandatory)
     Given a filehandle fragment, this should find the implied object and
-    create a dentry for it (possibly with d_alloc_anon).
+    create a dentry for it (possibly with d_obtain_alias).
 
   fh_to_parent (optional but strongly recommended)
     Given a filehandle fragment, this should find the parent of the
-    implied object and create a dentry for it (possibly with d_alloc_anon).
-    May fail if the filehandle fragment is too small.
+    implied object and create a dentry for it (possibly with
+    d_obtain_alias).  May fail if the filehandle fragment is too small.
 
   get_parent (optional but strongly recommended)
     When given a dentry for a directory, this should return  a dentry for
-- 
1.7.9.5

[PATCH 10/11] dcache: d_materialise_unique isn't GPL-only

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

d_materialise_unique and d_splice_alias are really just minor variants
on the same API--I can't see a reason to export them differently.  Any
distributed filesystem should be using d_materialise_unique.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/dcache.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/dcache.c b/fs/dcache.c
index 671a04a..960a2e7 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -2753,7 +2753,7 @@ out_nolock:
 	iput(inode);
 	return actual;
 }
-EXPORT_SYMBOL_GPL(d_materialise_unique);
+EXPORT_SYMBOL(d_materialise_unique);
 
 static int prepend(char **buffer, int *buflen, const char *str, int namelen)
 {
-- 
1.7.9.5

[PATCH 11/11] dcache: d_splice_alias should detect loops

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

I believe this can only happen in the case of a corrupted filesystem.
So -EIO looks like the appropriate error.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/dcache.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/fs/dcache.c b/fs/dcache.c
index 960a2e7..f6ab71b 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -2643,6 +2643,11 @@ struct dentry *d_splice_alias(struct inode *inode, struct dentry *dentry)
 				dput(new);
 				return ERR_PTR(-EIO);
 			}
+			if (d_ancestor(new, dentry)) {
+				spin_unlock(&inode->i_lock);
+				dput(new);
+				return ERR_PTR(-EIO);
+			}
 			write_seqlock(&rename_lock);
 			__d_materialise_dentry(dentry, new);
 			write_sequnlock(&rename_lock);
-- 
1.7.9.5

Re: [PATCH 09/11] exportfs: update Exporting documentation

[Bernd Schubert]

On 06/04/2014 11:21 PM, J. Bruce Fields wrote:
> From: "J. Bruce Fields" <bfields@redhat.com>
>
> Minor documentation updates:
> 	- refer to d_obtain_alias rather than d_alloc_anon
> 	- explain when to use d_splice_alias and when
> 	  d_materialise_unique.
> 	- cut some details of d_splice_alias/d_materialise_unique
> 	  implementation.
>
> Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> ---
>   Documentation/filesystems/nfs/Exporting |   38 +++++++++++++++++++------------
>   1 file changed, 23 insertions(+), 15 deletions(-)
>
> diff --git a/Documentation/filesystems/nfs/Exporting b/Documentation/filesystems/nfs/Exporting
> index e543b1a..9b7de5b 100644
> --- a/Documentation/filesystems/nfs/Exporting
> +++ b/Documentation/filesystems/nfs/Exporting
> @@ -66,23 +66,31 @@ b/ A per-superblock list "s_anon" of dentries which are the roots of
>
>   c/ Helper routines to allocate anonymous dentries, and to help attach
>      loose directory dentries at lookup time. They are:
> -    d_alloc_anon(inode) will return a dentry for the given inode.
> +    d_obtain_alias(inode) will return a dentry for the given inode.
>         If the inode already has a dentry, one of those is returned.
>         If it doesn't, a new anonymous (IS_ROOT and
>           DCACHE_DISCONNECTED) dentry is allocated and attached.
>         In the case of a directory, care is taken that only one dentry
>         can ever be attached.
> -    d_splice_alias(inode, dentry) will make sure that there is a
> -      dentry with the same name and parent as the given dentry, and
> -      which refers to the given inode.
> -      If the inode is a directory and already has a dentry, then that
> -      dentry is d_moved over the given dentry.
> -      If the passed dentry gets attached, care is taken that this is
> -      mutually exclusive to a d_alloc_anon operation.
> -      If the passed dentry is used, NULL is returned, else the used
> -      dentry is returned.  This corresponds to the calling pattern of
> -      ->lookup.
> -
> +    d_splice_alias(inode, dentry) or d_materialise_unique(dentry, inode)
> +      will introduce a new dentry into the tree; either the passed-in
> +      dentry or a preexisting alias for the given inode (such as an
> +      anonymous one created by d_obtain_alias), if appropriate.  The two
> +      functions differ in their handling of directories with preexisting
> +      aliases:
> +        d_splice_alias will use any existing IS_ROOT dentry, but it will
> +	  return -EIO rather than try to move a dentry with a different
> +	  parent.  This is appropriate for local filesystems, which
> +	  should never see such an alias unless the filesystem is
> +	  corrupted somehow (for example, if two on-disk directory
> +	  entries refer to the same directory.)
> +	d_obtain_alias will attempt to move any dentry.  This is
> +	  appropriate for distributed filesystems, where finding a
> +	  directory other than where we last cached it may be a normal
> +	  consequence of concurrent operations on other hosts.
> +      Both functions return NULL when the passed-in dentry is used,
> +      following the calling convention of ->lookup.
> +

Hmm, is this really supposed to be in nfs/Exporting? Wouldn't be a new 
chapter in vfs.txt or a new file dache.c more appropriate? I was looking 
in the past for their documentation, but then thought it does not exist 
- one needs to use these even without nfs exports.


Thanks,
Bernd

Re: [PATCH 09/11] exportfs: update Exporting documentation

[Marc Dionne]

On Wed, Jun 4, 2014 at 5:21 PM, J. Bruce Fields <bfields@redhat.com> wrote:
> From: "J. Bruce Fields" <bfields@redhat.com>
>
> Minor documentation updates:
>         - refer to d_obtain_alias rather than d_alloc_anon
>         - explain when to use d_splice_alias and when
>           d_materialise_unique.
>         - cut some details of d_splice_alias/d_materialise_unique
>           implementation.
>
> Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> ---
>  Documentation/filesystems/nfs/Exporting |   38 +++++++++++++++++++------------
>  1 file changed, 23 insertions(+), 15 deletions(-)
>
> diff --git a/Documentation/filesystems/nfs/Exporting b/Documentation/filesystems/nfs/Exporting
> index e543b1a..9b7de5b 100644
> --- a/Documentation/filesystems/nfs/Exporting
> +++ b/Documentation/filesystems/nfs/Exporting
> @@ -66,23 +66,31 @@ b/ A per-superblock list "s_anon" of dentries which are the roots of
>
>  c/ Helper routines to allocate anonymous dentries, and to help attach
>     loose directory dentries at lookup time. They are:
> -    d_alloc_anon(inode) will return a dentry for the given inode.
> +    d_obtain_alias(inode) will return a dentry for the given inode.
>        If the inode already has a dentry, one of those is returned.
>        If it doesn't, a new anonymous (IS_ROOT and
>          DCACHE_DISCONNECTED) dentry is allocated and attached.
>        In the case of a directory, care is taken that only one dentry
>        can ever be attached.
> -    d_splice_alias(inode, dentry) will make sure that there is a
> -      dentry with the same name and parent as the given dentry, and
> -      which refers to the given inode.
> -      If the inode is a directory and already has a dentry, then that
> -      dentry is d_moved over the given dentry.
> -      If the passed dentry gets attached, care is taken that this is
> -      mutually exclusive to a d_alloc_anon operation.
> -      If the passed dentry is used, NULL is returned, else the used
> -      dentry is returned.  This corresponds to the calling pattern of
> -      ->lookup.
> -
> +    d_splice_alias(inode, dentry) or d_materialise_unique(dentry, inode)
> +      will introduce a new dentry into the tree; either the passed-in
> +      dentry or a preexisting alias for the given inode (such as an
> +      anonymous one created by d_obtain_alias), if appropriate.  The two
> +      functions differ in their handling of directories with preexisting
> +      aliases:
> +        d_splice_alias will use any existing IS_ROOT dentry, but it will
> +         return -EIO rather than try to move a dentry with a different
> +         parent.  This is appropriate for local filesystems, which
> +         should never see such an alias unless the filesystem is
> +         corrupted somehow (for example, if two on-disk directory
> +         entries refer to the same directory.)
> +       d_obtain_alias will attempt to move any dentry.  This is

Should this be d_materialise_unique instead of d_obtain_alias ?

Marc

Re: [PATCH 09/11] exportfs: update Exporting documentation

[J. Bruce Fields]

On Thu, Jun 05, 2014 at 08:51:21AM -0400, Marc Dionne wrote:
> On Wed, Jun 4, 2014 at 5:21 PM, J. Bruce Fields <bfields@redhat.com> wrote:
> > From: "J. Bruce Fields" <bfields@redhat.com>
> >
> > Minor documentation updates:
> >         - refer to d_obtain_alias rather than d_alloc_anon
> >         - explain when to use d_splice_alias and when
> >           d_materialise_unique.
> >         - cut some details of d_splice_alias/d_materialise_unique
> >           implementation.
> >
> > Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> > ---
> >  Documentation/filesystems/nfs/Exporting |   38 +++++++++++++++++++------------
> >  1 file changed, 23 insertions(+), 15 deletions(-)
> >
> > diff --git a/Documentation/filesystems/nfs/Exporting b/Documentation/filesystems/nfs/Exporting
> > index e543b1a..9b7de5b 100644
> > --- a/Documentation/filesystems/nfs/Exporting
> > +++ b/Documentation/filesystems/nfs/Exporting
> > @@ -66,23 +66,31 @@ b/ A per-superblock list "s_anon" of dentries which are the roots of
> >
> >  c/ Helper routines to allocate anonymous dentries, and to help attach
> >     loose directory dentries at lookup time. They are:
> > -    d_alloc_anon(inode) will return a dentry for the given inode.
> > +    d_obtain_alias(inode) will return a dentry for the given inode.
> >        If the inode already has a dentry, one of those is returned.
> >        If it doesn't, a new anonymous (IS_ROOT and
> >          DCACHE_DISCONNECTED) dentry is allocated and attached.
> >        In the case of a directory, care is taken that only one dentry
> >        can ever be attached.
> > -    d_splice_alias(inode, dentry) will make sure that there is a
> > -      dentry with the same name and parent as the given dentry, and
> > -      which refers to the given inode.
> > -      If the inode is a directory and already has a dentry, then that
> > -      dentry is d_moved over the given dentry.
> > -      If the passed dentry gets attached, care is taken that this is
> > -      mutually exclusive to a d_alloc_anon operation.
> > -      If the passed dentry is used, NULL is returned, else the used
> > -      dentry is returned.  This corresponds to the calling pattern of
> > -      ->lookup.
> > -
> > +    d_splice_alias(inode, dentry) or d_materialise_unique(dentry, inode)
> > +      will introduce a new dentry into the tree; either the passed-in
> > +      dentry or a preexisting alias for the given inode (such as an
> > +      anonymous one created by d_obtain_alias), if appropriate.  The two
> > +      functions differ in their handling of directories with preexisting
> > +      aliases:
> > +        d_splice_alias will use any existing IS_ROOT dentry, but it will
> > +         return -EIO rather than try to move a dentry with a different
> > +         parent.  This is appropriate for local filesystems, which
> > +         should never see such an alias unless the filesystem is
> > +         corrupted somehow (for example, if two on-disk directory
> > +         entries refer to the same directory.)
> > +       d_obtain_alias will attempt to move any dentry.  This is
> 
> Should this be d_materialise_unique instead of d_obtain_alias ?

Whoops, thanks for catching that.

Fixed in the version at

	git://linux-nfs.org/~bfields/linux-topics.git for-viro

--b.

Re: [PATCH 09/11] exportfs: update Exporting documentation

[J. Bruce Fields]

On Wed, Jun 04, 2014 at 11:54:05PM +0200, Bernd Schubert wrote:
> On 06/04/2014 11:21 PM, J. Bruce Fields wrote:
> >From: "J. Bruce Fields" <bfields@redhat.com>
> >
> >Minor documentation updates:
> >	- refer to d_obtain_alias rather than d_alloc_anon
> >	- explain when to use d_splice_alias and when
> >	  d_materialise_unique.
> >	- cut some details of d_splice_alias/d_materialise_unique
> >	  implementation.
> >
> >Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> >---
> >  Documentation/filesystems/nfs/Exporting |   38 +++++++++++++++++++------------
> >  1 file changed, 23 insertions(+), 15 deletions(-)
> >
> >diff --git a/Documentation/filesystems/nfs/Exporting b/Documentation/filesystems/nfs/Exporting
> >index e543b1a..9b7de5b 100644
> >--- a/Documentation/filesystems/nfs/Exporting
> >+++ b/Documentation/filesystems/nfs/Exporting
> >@@ -66,23 +66,31 @@ b/ A per-superblock list "s_anon" of dentries which are the roots of
> >
> >  c/ Helper routines to allocate anonymous dentries, and to help attach
> >     loose directory dentries at lookup time. They are:
> >-    d_alloc_anon(inode) will return a dentry for the given inode.
> >+    d_obtain_alias(inode) will return a dentry for the given inode.
> >        If the inode already has a dentry, one of those is returned.
> >        If it doesn't, a new anonymous (IS_ROOT and
> >          DCACHE_DISCONNECTED) dentry is allocated and attached.
> >        In the case of a directory, care is taken that only one dentry
> >        can ever be attached.
> >-    d_splice_alias(inode, dentry) will make sure that there is a
> >-      dentry with the same name and parent as the given dentry, and
> >-      which refers to the given inode.
> >-      If the inode is a directory and already has a dentry, then that
> >-      dentry is d_moved over the given dentry.
> >-      If the passed dentry gets attached, care is taken that this is
> >-      mutually exclusive to a d_alloc_anon operation.
> >-      If the passed dentry is used, NULL is returned, else the used
> >-      dentry is returned.  This corresponds to the calling pattern of
> >-      ->lookup.
> >-
> >+    d_splice_alias(inode, dentry) or d_materialise_unique(dentry, inode)
> >+      will introduce a new dentry into the tree; either the passed-in
> >+      dentry or a preexisting alias for the given inode (such as an
> >+      anonymous one created by d_obtain_alias), if appropriate.  The two
> >+      functions differ in their handling of directories with preexisting
> >+      aliases:
> >+        d_splice_alias will use any existing IS_ROOT dentry, but it will
> >+	  return -EIO rather than try to move a dentry with a different
> >+	  parent.  This is appropriate for local filesystems, which
> >+	  should never see such an alias unless the filesystem is
> >+	  corrupted somehow (for example, if two on-disk directory
> >+	  entries refer to the same directory.)
> >+	d_obtain_alias will attempt to move any dentry.  This is
> >+	  appropriate for distributed filesystems, where finding a
> >+	  directory other than where we last cached it may be a normal
> >+	  consequence of concurrent operations on other hosts.
> >+      Both functions return NULL when the passed-in dentry is used,
> >+      following the calling convention of ->lookup.
> >+
> 
> Hmm, is this really supposed to be in nfs/Exporting? Wouldn't be a new
> chapter in vfs.txt or a new file dache.c more appropriate? I was looking in
> the past for their documentation, but then thought it does not exist - one
> needs to use these even without nfs exports.

Yes, nfs for example uses d_materialise_unique despite not itself being
exportable.

I'm not opposed to finding a better home for this.

--b.