mainly d_splice_alias fixes

PATCH In review J. Bruce Fields 3 participants 16 messages Jun 4, 2014 – Jun 6, 2014

Subsystems (7)

Documentation VFS Btrfs file system NFS client +3 more subsystems

lore

Share & tools

lore

J. Bruce Fields

author

2014-06-04 21:21 UTC #0 lore raw

cover mainly d_splice_alias fixes

From: "J. Bruce Fields" <[email protected]>

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

Signature · 1 line

1.7.9.5

J. Bruce Fields

author

2014-06-04 21:21 UTC #1 lore raw

PATCH 1/11 namei: trivial fix to vfs_rename_dir comment

From: "J. Bruce Fields" <[email protected]>

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 <[email protected]>

---
 fs/namei.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

fs/namei.c +1 -1

index 8016827..b837754 100644
@@ -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

Signature · 1 line

1.7.9.5

J. Bruce Fields

author

2014-06-04 21:21 UTC #2 lore raw

PATCH 2/11 dcache: move d_splice_alias

In reply to J. Bruce Fields

From: "J. Bruce Fields" <[email protected]>

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 <[email protected]>

Signed-off-by J. Bruce Fields <[email protected]>

---
 fs/dcache.c |  104 +++++++++++++++++++++++++++++------------------------------
 1 file changed, 52 insertions(+), 52 deletions(-)

fs/dcache.c +52 ~~-52~~

index 42ae01e..467389f 100644
@@ -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

Signature · 1 line

1.7.9.5

J. Bruce Fields

author

2014-06-04 21:21 UTC #3 lore raw

PATCH 3/11 dcache: close d_move race in d_splice_alias

In reply to J. Bruce Fields

From: "J. Bruce Fields" <[email protected]>

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 <[email protected]>

Signed-off-by J. Bruce Fields <[email protected]>

---
 fs/dcache.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

fs/dcache.c +6 -1

index 467389f..a833e97 100644
@@ -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 */

Signature · 1 line

1.7.9.5

J. Bruce Fields

author

2014-06-04 21:21 UTC #4 lore raw

PATCH 4/11 dcache: d_splice_alias mustn't create directory aliases

In reply to J. Bruce Fields

From: "J. Bruce Fields" <[email protected]>

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 <[email protected]>

---
 fs/dcache.c |   11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

fs/dcache.c +9 -2

index a833e97..54b602e 100644
@@ -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);

Signature · 1 line

1.7.9.5

J. Bruce Fields

author

2014-06-04 21:21 UTC #5 lore raw

PATCH 5/11 dcache: d_splice_alias should ignore DCACHE_DISCONNECTED

In reply to J. Bruce Fields

From: "J. Bruce Fields" <[email protected]>

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 <[email protected]>

---
 fs/dcache.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

fs/dcache.c +4 -4

index 54b602e..c42009f 100644
@@ -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);

Signature · 1 line

1.7.9.5

J. Bruce Fields

author

2014-06-04 21:21 UTC #6 lore raw

PATCH 6/11 dcache: d_obtain_alias callers don't all want DISCONNECTED

In reply to J. Bruce Fields

From: "J. Bruce Fields" <[email protected]>

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 <[email protected]>

Signed-off-by J. Bruce Fields <[email protected]>

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

fs/btrfs/super.c +1 -8

index 9601d25..b05588b 100644
@@ -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,

fs/ceph/super.c +1 -1

index 06150fd..f6e1237 100644
@@ -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);

fs/dcache.c +49 ~~-20~~

90 lines not on this page · raw message

fs/nfs/getroot.c +1 -1

index 66984a9..e851f53 100644
@@ -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;

fs/nilfs2/super.c +1 -1

index 8c532b2..ac91499 100644
@@ -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;

include/linux/dcache.h +1 -0

index 3c7ec32..e4ae2ad 100644
@@ -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 *);

Signature · 1 line

1.7.9.5

J. Bruce Fields

author

2014-06-04 21:21 UTC #7 lore raw

PATCH 7/11 dcache: remove unused d_find_alias parameter

In reply to J. Bruce Fields

From: "J. Bruce Fields" <[email protected]>

Signed-off-by J. Bruce Fields <[email protected]>

---
 fs/dcache.c |   13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

fs/dcache.c +5 -8

index 5a8517f..9209479 100644
@@ -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);

Signature · 1 line

1.7.9.5

J. Bruce Fields

author

2014-06-04 21:21 UTC #8 lore raw

PATCH 8/11 dcache: d_find_alias needn't recheck IS_ROOT && DCACHE_DISCONNECTED

In reply to J. Bruce Fields

From: "J. Bruce Fields" <[email protected]>

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 <[email protected]>

---
 fs/dcache.c |    9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

fs/dcache.c +3 -6

index 9209479..671a04a 100644
@@ -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;

Signature · 1 line

1.7.9.5

J. Bruce Fields

author

2014-06-04 21:21 UTC #9 lore raw

PATCH 9/11 exportfs: update Exporting documentation

In reply to J. Bruce Fields

From: "J. Bruce Fields" <[email protected]>

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 <[email protected]>

---
 Documentation/filesystems/nfs/Exporting |   38 +++++++++++++++++++------------
 1 file changed, 23 insertions(+), 15 deletions(-)

Documentation/filesystems/nfs/Exporting +23 ~~-15~~

index e543b1a..9b7de5b 100644
@@ -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

Signature · 1 line

1.7.9.5

Bernd Schubert

2014-06-04 21:54 UTC #10 lore raw

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

Quoted 49 lines · J. Bruce Fields

From: "J. Bruce Fields" <[email protected]>

Signed-off-by J. Bruce Fields <[email protected]>

---
  Documentation/filesystems/nfs/Exporting |   38 +++++++++++++++++++------------
  1 file changed, 23 insertions(+), 15 deletions(-)

Documentation/filesystems/nfs/Exporting +10 ~~-12~~

index e543b1a..9b7de5b 100644
@@ -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

Documentation/filesystems/nfs/Exporting b/ A per-superblock list "s_anon" of dentries which are the roots of · line 93

Quoted 10 lines

Documentation/filesystems/nfs/Exporting +10 -0

+	  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

J. Bruce Fields

author

2014-06-06 21:23 UTC #11 lore raw

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

Quoted 57 lines · Bernd Schubert

Quoted 56 lines · J. Bruce Fields

From: "J. Bruce Fields" <[email protected]>

Signed-off-by J. Bruce Fields <[email protected]>

---
 Documentation/filesystems/nfs/Exporting |   38 +++++++++++++++++++------------
 1 file changed, 23 insertions(+), 15 deletions(-)

Documentation/filesystems/nfs/Exporting +17 ~~-12~~

index e543b1a..9b7de5b 100644
@@ -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.

Quoted 8 lines

Documentation/filesystems/nfs/Exporting b/ A per-superblock list "s_anon" of dentries which are the roots of · line 93
Quoted 3 lines
Documentation/filesystems/nfs/Exporting +3 -0
+      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.

Marc Dionne

2014-06-05 12:51 UTC #12 lore raw

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

In reply to J. Bruce Fields

Quoted 43 lines · J. Bruce Fields

From: "J. Bruce Fields" <[email protected]>

Signed-off-by J. Bruce Fields <[email protected]>

---
 Documentation/filesystems/nfs/Exporting |   38 +++++++++++++++++++------------
 1 file changed, 23 insertions(+), 15 deletions(-)

Documentation/filesystems/nfs/Exporting +4 ~~-12~~

index e543b1a..9b7de5b 100644
@@ -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

Documentation/filesystems/nfs/Exporting b/ A per-superblock list "s_anon" of dentries which are the roots of · line 87

Quoted 10 lines

Documentation/filesystems/nfs/Exporting +10 -0

+      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

J. Bruce Fields

author

2014-06-06 21:20 UTC #13 lore raw

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

Quoted 48 lines · Marc Dionne

Quoted 47 lines · J. Bruce Fields

From: "J. Bruce Fields" <[email protected]>

Signed-off-by J. Bruce Fields <[email protected]>

---
 Documentation/filesystems/nfs/Exporting |   38 +++++++++++++++++++------------
 1 file changed, 23 insertions(+), 15 deletions(-)

Documentation/filesystems/nfs/Exporting +8 ~~-12~~

index e543b1a..9b7de5b 100644
@@ -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

Quoted 8 lines

Documentation/filesystems/nfs/Exporting b/ A per-superblock list "s_anon" of dentries which are the roots of · line 87
Quoted 6 lines
Documentation/filesystems/nfs/Exporting +6 -0
+         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.

J. Bruce Fields

author

2014-06-04 21:21 UTC #14 lore raw

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

In reply to J. Bruce Fields

From: "J. Bruce Fields" <[email protected]>

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 <[email protected]>

---
 fs/dcache.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

fs/dcache.c +1 -1

index 671a04a..960a2e7 100644
@@ -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)
 {

Signature · 1 line

1.7.9.5

J. Bruce Fields

author

2014-06-04 21:21 UTC #15 lore raw

PATCH 11/11 dcache: d_splice_alias should detect loops

In reply to J. Bruce Fields

From: "J. Bruce Fields" <[email protected]>

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 <[email protected]>

---
 fs/dcache.c |    5 +++++
 1 file changed, 5 insertions(+)

fs/dcache.c +5 -0

index 960a2e7..f6ab71b 100644
@@ -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);

Signature · 1 line

1.7.9.5