[PATCH] Btrfs: unset DCACHE_DISCONNECTED when mounting default subvol

[PATCH] Btrfs: unset DCACHE_DISCONNECTED when mounting default subvol

[Josef Bacik]

A user was running into errors from an NFS export of a subvolume that had a
default subvol set.  When we mount a default subvol we will use d_obtain_alias()
to find an existing dentry for the subvolume in the case that the root subvol
has already been mounted, or a dummy one is allocated in the case that the root
subvol has not already been mounted.  This allows us to connect the dentry later
on if we wander into the path.  However if we don't ever wander into the path we
will keep DCACHE_DISCONNECTED set for a long time, which angers NFS.  It doesn't
appear to cause any problems but it is annoying nonetheless, so simply unset
DCACHE_DISCONNECTED in the get_default_root case and switch btrfs_lookup() to
use d_materialise_unique() instead which will make everything play nicely
together and reconnect stuff if we wander into the defaul subvol path from a
different way.  With this patch I'm no longer getting the NFS errors when
exporting a volume that has been mounted with a default subvol set.  Thanks,

cc: bfields@fieldses.org
cc: ebiederm@xmission.com
Signed-off-by: Josef Bacik <jbacik@fb.com>
---
 fs/btrfs/inode.c | 2 +-
 fs/btrfs/super.c | 9 ++++++++-
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 197edee..8dba152 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -5157,7 +5157,7 @@ static struct dentry *btrfs_lookup(struct inode *dir, struct dentry *dentry,
 			return ERR_CAST(inode);
 	}
 
-	return d_splice_alias(inode, dentry);
+	return d_materialise_unique(dentry, inode);
 }
 
 unsigned char btrfs_filetype_table[] = {
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 147ca1d..dc0a315 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -855,6 +855,7 @@ 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;
 
@@ -925,7 +926,13 @@ setup_root:
 		return dget(sb->s_root);
 	}
 
-	return d_obtain_alias(inode);
+	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;
 }
 
 static int btrfs_fill_super(struct super_block *sb,
-- 
1.8.3.1

Re: [PATCH] Btrfs: unset DCACHE_DISCONNECTED when mounting default subvol

[Eric W. Biederman]

Josef Bacik <jbacik@fb.com> writes:

> A user was running into errors from an NFS export of a subvolume that had a
> default subvol set.  When we mount a default subvol we will use d_obtain_alias()
> to find an existing dentry for the subvolume in the case that the root subvol
> has already been mounted, or a dummy one is allocated in the case that the root
> subvol has not already been mounted.  This allows us to connect the dentry later
> on if we wander into the path.  However if we don't ever wander into the path we
> will keep DCACHE_DISCONNECTED set for a long time, which angers NFS.  It doesn't
> appear to cause any problems but it is annoying nonetheless, so simply unset
> DCACHE_DISCONNECTED in the get_default_root case and switch btrfs_lookup() to
> use d_materialise_unique() instead which will make everything play nicely
> together and reconnect stuff if we wander into the defaul subvol path from a
> different way.  With this patch I'm no longer getting the NFS errors when
> exporting a volume that has been mounted with a default subvol set.  Thanks,
>
> cc: bfields@fieldses.org
> cc: ebiederm@xmission.com
Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>

> Signed-off-by: Josef Bacik <jbacik@fb.com>
> ---
>  fs/btrfs/inode.c | 2 +-
>  fs/btrfs/super.c | 9 ++++++++-
>  2 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 197edee..8dba152 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -5157,7 +5157,7 @@ static struct dentry *btrfs_lookup(struct inode *dir, struct dentry *dentry,
>  			return ERR_CAST(inode);
>  	}
>  
> -	return d_splice_alias(inode, dentry);
> +	return d_materialise_unique(dentry, inode);
>  }
>  
>  unsigned char btrfs_filetype_table[] = {
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index 147ca1d..dc0a315 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -855,6 +855,7 @@ 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;
>  
> @@ -925,7 +926,13 @@ setup_root:
>  		return dget(sb->s_root);
>  	}
>  
> -	return d_obtain_alias(inode);
> +	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;
>  }
>  
>  static int btrfs_fill_super(struct super_block *sb,

Re: [PATCH] Btrfs: unset DCACHE_DISCONNECTED when mounting default subvol

[J. Bruce Fields]

On Fri, Feb 14, 2014 at 01:43:48PM -0500, Josef Bacik wrote:
> A user was running into errors from an NFS export of a subvolume that had a
> default subvol set.  When we mount a default subvol we will use d_obtain_alias()
> to find an existing dentry for the subvolume in the case that the root subvol
> has already been mounted, or a dummy one is allocated in the case that the root
> subvol has not already been mounted.  This allows us to connect the dentry later
> on if we wander into the path.  However if we don't ever wander into the path we
> will keep DCACHE_DISCONNECTED set for a long time, which angers NFS.  It doesn't
> appear to cause any problems but it is annoying nonetheless, so simply unset
> DCACHE_DISCONNECTED in the get_default_root case and switch btrfs_lookup() to
> use d_materialise_unique() instead which will make everything play nicely
> together and reconnect stuff if we wander into the defaul subvol path from a
> different way.  With this patch I'm no longer getting the NFS errors when
> exporting a volume that has been mounted with a default subvol set.  Thanks,

Looks obviously correct, but based on a quick grep, there are four
d_obtain_alias callers outside export methods:

	- btrfs/super.c:get_default_root()
	- fs/ceph/super.c:open_root_dentry()
	- fs/nfs/getroot.c:nfs_get_root()
	- fs/nilfs2/super.c:nilfs_get_root_dentry()

It'd be nice to give them a common d_obtain_alias variant instead of
making them all clear this by hand.

Of those nilfs2 also uses d_splice_alias.  I think that problem would
best be solved by fixing d_splice_alias not to require a
DCACHE_DISCONNECTED dentry; IS_ROOT() on its own should be fine.

--b.

> 
> cc: bfields@fieldses.org
> cc: ebiederm@xmission.com
> Signed-off-by: Josef Bacik <jbacik@fb.com>
> ---
>  fs/btrfs/inode.c | 2 +-
>  fs/btrfs/super.c | 9 ++++++++-
>  2 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 197edee..8dba152 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -5157,7 +5157,7 @@ static struct dentry *btrfs_lookup(struct inode *dir, struct dentry *dentry,
>  			return ERR_CAST(inode);
>  	}
>  
> -	return d_splice_alias(inode, dentry);
> +	return d_materialise_unique(dentry, inode);
>  }
>  
>  unsigned char btrfs_filetype_table[] = {
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index 147ca1d..dc0a315 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -855,6 +855,7 @@ 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;
>  
> @@ -925,7 +926,13 @@ setup_root:
>  		return dget(sb->s_root);
>  	}
>  
> -	return d_obtain_alias(inode);
> +	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;
>  }
>  
>  static int btrfs_fill_super(struct super_block *sb,
> -- 
> 1.8.3.1
> 

Re: [PATCH] Btrfs: unset DCACHE_DISCONNECTED when mounting default subvol

[Eric W. Biederman]

"J. Bruce Fields" <bfields@fieldses.org> writes:

> On Fri, Feb 14, 2014 at 01:43:48PM -0500, Josef Bacik wrote:
>> A user was running into errors from an NFS export of a subvolume that had a
>> default subvol set.  When we mount a default subvol we will use d_obtain_alias()
>> to find an existing dentry for the subvolume in the case that the root subvol
>> has already been mounted, or a dummy one is allocated in the case that the root
>> subvol has not already been mounted.  This allows us to connect the dentry later
>> on if we wander into the path.  However if we don't ever wander into the path we
>> will keep DCACHE_DISCONNECTED set for a long time, which angers NFS.  It doesn't
>> appear to cause any problems but it is annoying nonetheless, so simply unset
>> DCACHE_DISCONNECTED in the get_default_root case and switch btrfs_lookup() to
>> use d_materialise_unique() instead which will make everything play nicely
>> together and reconnect stuff if we wander into the defaul subvol path from a
>> different way.  With this patch I'm no longer getting the NFS errors when
>> exporting a volume that has been mounted with a default subvol set.  Thanks,
>
> Looks obviously correct, but based on a quick grep, there are four
> d_obtain_alias callers outside export methods:
>
> 	- btrfs/super.c:get_default_root()
> 	- fs/ceph/super.c:open_root_dentry()
> 	- fs/nfs/getroot.c:nfs_get_root()
> 	- fs/nilfs2/super.c:nilfs_get_root_dentry()
>
> It'd be nice to give them a common d_obtain_alias variant instead of
> making them all clear this by hand.

I am in favor of one small fix at a time, so that progress is made and
fixing something just for btrfs seems reasonable for the short term.

> Of those nilfs2 also uses d_splice_alias.  I think that problem would
> best be solved by fixing d_splice_alias not to require a
> DCACHE_DISCONNECTED dentry; IS_ROOT() on its own should be fine.

You mean by renaming d_splice_alias d_materialise_unique?

Or is there a useful distinction you see that should be preserved
between the two methods?

Right now my inclination is that everyone should just use
d_materialise_unique and we should kill d_splice_alias.

And by everyone I mean all file systems that are either distributed
(implementing d_revalidate) or exportable by knfsd.

One of the interesting things that d_materialise_unique does is get the
lazy rename case correct for a distributed filesystem.
check_submounts_and_drop can drop a directory when it is found not to be
accessible by that name, but later when we look it up
d_materialise_uniuqe will resuscciate the existing dentry.

Eric

Re: [PATCH] Btrfs: unset DCACHE_DISCONNECTED when mounting default subvol

[J. Bruce Fields]

On Fri, Feb 14, 2014 at 05:40:55PM -0800, Eric W. Biederman wrote:
> "J. Bruce Fields" <bfields@fieldses.org> writes:
> 
> > On Fri, Feb 14, 2014 at 01:43:48PM -0500, Josef Bacik wrote:
> >> A user was running into errors from an NFS export of a subvolume that had a
> >> default subvol set.  When we mount a default subvol we will use d_obtain_alias()
> >> to find an existing dentry for the subvolume in the case that the root subvol
> >> has already been mounted, or a dummy one is allocated in the case that the root
> >> subvol has not already been mounted.  This allows us to connect the dentry later
> >> on if we wander into the path.  However if we don't ever wander into the path we
> >> will keep DCACHE_DISCONNECTED set for a long time, which angers NFS.  It doesn't
> >> appear to cause any problems but it is annoying nonetheless, so simply unset
> >> DCACHE_DISCONNECTED in the get_default_root case and switch btrfs_lookup() to
> >> use d_materialise_unique() instead which will make everything play nicely
> >> together and reconnect stuff if we wander into the defaul subvol path from a
> >> different way.  With this patch I'm no longer getting the NFS errors when
> >> exporting a volume that has been mounted with a default subvol set.  Thanks,
> >
> > Looks obviously correct, but based on a quick grep, there are four
> > d_obtain_alias callers outside export methods:
> >
> > 	- btrfs/super.c:get_default_root()
> > 	- fs/ceph/super.c:open_root_dentry()
> > 	- fs/nfs/getroot.c:nfs_get_root()
> > 	- fs/nilfs2/super.c:nilfs_get_root_dentry()
> >
> > It'd be nice to give them a common d_obtain_alias variant instead of
> > making them all clear this by hand.
> 
> I am in favor of one small fix at a time, so that progress is made and
> fixing something just for btrfs seems reasonable for the short term.
> 
> > Of those nilfs2 also uses d_splice_alias.  I think that problem would
> > best be solved by fixing d_splice_alias not to require a
> > DCACHE_DISCONNECTED dentry; IS_ROOT() on its own should be fine.
> 
> You mean by renaming d_splice_alias d_materialise_unique?
> 
> Or is there a useful distinction you see that should be preserved
> between the two methods?
> 
> Right now my inclination is that everyone should just use
> d_materialise_unique and we should kill d_splice_alias.

Probably.  One remaining distinction:

	- In the local filesystem case if you discover a directory is
	  already aliased elsewhere, you have a corrupted filesystem and
	  want to error out the lookup.  (Didn't you propose a patch to
	  do something like that before?)
	- In the distributed filesystem this is perfectly normal and we
	  want to do our best to fix up our local cache to represent
	  remote reality.

> And by everyone I mean all file systems that are either distributed
> (implementing d_revalidate) or exportable by knfsd.
> 
> One of the interesting things that d_materialise_unique does is get the
> lazy rename case correct for a distributed filesystem.
> check_submounts_and_drop can drop a directory when it is found not to be
> accessible by that name, but later when we look it up
> d_materialise_uniuqe will resuscciate the existing dentry.

OK.  I'm not sure I understand how that helps.

Ugly untested draft follows.

--b.

diff --git a/fs/dcache.c b/fs/dcache.c
index 265e0ce..b4572fa 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -1905,58 +1905,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
@@ -2716,19 +2664,42 @@ static void __d_materialise_dentry(struct dentry *dentry, struct dentry *anon)
 }
 
 /**
- * d_materialise_unique - introduce an inode into the tree
- * @dentry: candidate dentry
+ * d_splice_alias - introduce an inode into the tree
  * @inode: inode to bind to the dentry, to which aliases may be attached
+ * @dentry: candidate dentry
+ * @force: move an existing non-root alias if necessary
+ *
+ * Introduces a dentry into the tree, using either the provided dentry
+ * or, if appropriate, a preexisting alias for the same inode.  Return
+ * NULL in the former case, the found alias in the latter.  Caller must
+ * hold the i_mutex of the parent directory.
  *
- * Introduces an dentry into the tree, substituting an extant disconnected
- * root directory alias in its place if there is one. Caller must hold the
- * i_mutex of the parent directory.
+ * The inode may also be an error or NULL; in the former case the error is
+ * just passed through, in the latter we hash and instantiate the negative
+ * dentry.  This allows filesystems to use d_splice_alias as the
+ * unconditional last step of their lookup method.
+ *
+ * Directories must have unique aliases.  In the case a directory is
+ * found to already have an alias, if that alias is IS_ROOT, we move
+ * it into place.  Otherwise, if @force is false, we fail with -EIO.  If
+ * @force is true, we try to move the alias anyway, returning -ELOOP if
+ * that would create a directory alias or -EBUSY if we fail to acquire
+ * the locks required for a rename.
+ *
+ * d_splice_alias makes no such guarantee for files, but may still
+ * use a preexisting alias when convenient.
+ *
+ * Note that d_splice_alias normally expects an unhashed dentry, the single
+ * exception being that cluster filesystems may call this function
+ * during atomic_open with a negative hashed dentry, and with inode a
+ * regular file.
  */
-struct dentry *d_materialise_unique(struct dentry *dentry, struct inode *inode)
+struct dentry *__d_splice_alias(struct inode *inode, struct dentry *dentry, bool force)
 {
 	struct dentry *actual;
 
-	BUG_ON(!d_unhashed(dentry));
+	if (IS_ERR(inode))
+		return ERR_CAST(inode);
 
 	if (!inode) {
 		actual = dentry;
@@ -2760,9 +2731,27 @@ struct dentry *d_materialise_unique(struct dentry *dentry, struct inode *inode)
 				__d_drop(alias);
 				goto found;
 			} else {
-				/* Nope, but we must(!) avoid directory
-				 * aliasing. This drops inode->i_lock */
-				actual = __d_unalias(inode, dentry, alias);
+				/*
+				 * Nope, but we must(!) avoid directory
+				 * aliasing.
+				 */
+				if (force) {
+					/*
+					 * Distributed filesystem case:
+					 * somebody else moved things
+					 * out from under us.  Let's do
+					 * our best to fix up things
+					 * locally:
+					 */
+					actual = __d_unalias(inode, dentry, alias);
+				} else {
+					/*
+					 * local filesystem case:
+					 * filesystem is just corrupted:
+					 */
+					actual = ERR_PTR(-EIO);
+					spin_unlock(&inode->i_lock);
+				}
 			}
 			write_sequnlock(&rename_lock);
 			if (IS_ERR(actual)) {
@@ -2788,7 +2777,8 @@ struct dentry *d_materialise_unique(struct dentry *dentry, struct inode *inode)
 
 	spin_lock(&actual->d_lock);
 found:
-	_d_rehash(actual);
+	if (d_unhashed(actual))
+		_d_rehash(actual);
 	spin_unlock(&actual->d_lock);
 	spin_unlock(&inode->i_lock);
 out_nolock:
@@ -2800,6 +2790,17 @@ out_nolock:
 	iput(inode);
 	return actual;
 }
+EXPORT_SYMBOL(d_splice_alias);
+
+struct dentry *d_splice_alias(struct inode *inode, struct dentry *dentry)
+{
+	return __d_splice_alias(inode, dentry, 0);
+}
+
+struct dentry *d_materialise_unique(struct dentry *dentry, struct inode *inode)
+{
+	return __d_splice_alias(inode, dentry, 1);
+}
 EXPORT_SYMBOL_GPL(d_materialise_unique);
 
 static int prepend(char **buffer, int *buflen, const char *str, int namelen)

Re: [PATCH] Btrfs: unset DCACHE_DISCONNECTED when mounting default subvol

[J. Bruce Fields]

On Fri, Feb 14, 2014 at 09:45:24PM -0500, J. Bruce Fields wrote:
> On Fri, Feb 14, 2014 at 05:40:55PM -0800, Eric W. Biederman wrote:
> > "J. Bruce Fields" <bfields@fieldses.org> writes:
> > 
> > > On Fri, Feb 14, 2014 at 01:43:48PM -0500, Josef Bacik wrote:
> > >> A user was running into errors from an NFS export of a subvolume that had a
> > >> default subvol set.  When we mount a default subvol we will use d_obtain_alias()
> > >> to find an existing dentry for the subvolume in the case that the root subvol
> > >> has already been mounted, or a dummy one is allocated in the case that the root
> > >> subvol has not already been mounted.  This allows us to connect the dentry later
> > >> on if we wander into the path.  However if we don't ever wander into the path we
> > >> will keep DCACHE_DISCONNECTED set for a long time, which angers NFS.  It doesn't
> > >> appear to cause any problems but it is annoying nonetheless, so simply unset
> > >> DCACHE_DISCONNECTED in the get_default_root case and switch btrfs_lookup() to
> > >> use d_materialise_unique() instead which will make everything play nicely
> > >> together and reconnect stuff if we wander into the defaul subvol path from a
> > >> different way.  With this patch I'm no longer getting the NFS errors when
> > >> exporting a volume that has been mounted with a default subvol set.  Thanks,
> > >
> > > Looks obviously correct, but based on a quick grep, there are four
> > > d_obtain_alias callers outside export methods:
> > >
> > > 	- btrfs/super.c:get_default_root()
> > > 	- fs/ceph/super.c:open_root_dentry()
> > > 	- fs/nfs/getroot.c:nfs_get_root()
> > > 	- fs/nilfs2/super.c:nilfs_get_root_dentry()
> > >
> > > It'd be nice to give them a common d_obtain_alias variant instead of
> > > making them all clear this by hand.
> > 
> > I am in favor of one small fix at a time, so that progress is made and
> > fixing something just for btrfs seems reasonable for the short term.
> > 
> > > Of those nilfs2 also uses d_splice_alias.  I think that problem would
> > > best be solved by fixing d_splice_alias not to require a
> > > DCACHE_DISCONNECTED dentry; IS_ROOT() on its own should be fine.
> > 
> > You mean by renaming d_splice_alias d_materialise_unique?
> > 
> > Or is there a useful distinction you see that should be preserved
> > between the two methods?
> > 
> > Right now my inclination is that everyone should just use
> > d_materialise_unique and we should kill d_splice_alias.
> 
> Probably.  One remaining distinction:
> 
> 	- In the local filesystem case if you discover a directory is
> 	  already aliased elsewhere, you have a corrupted filesystem and
> 	  want to error out the lookup.  (Didn't you propose a patch to
> 	  do something like that before?)
> 	- In the distributed filesystem this is perfectly normal and we
> 	  want to do our best to fix up our local cache to represent
> 	  remote reality.

The following keeps the d_splice_alias/d_materialise_unique distinction
and (hopefully) fixes Josef's bug, and does a little cleanup (including
your suggested DISCONNECTED->CONNECTING rename).

Any better idea for the naming of d_materialise_unique?

I also didn't try to merge the implementations--the merged
d_splice_alias/d_materialise_unique was a little uglier than I expected.
I'll keep messing around with it though.

--b.

[PATCH 1/9] 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.

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 265e0ce..332b58c 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -1905,58 +1905,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
@@ -2716,6 +2664,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.8.5.3

[PATCH 2/9] 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.

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 332b58c..fd50e52 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -2695,9 +2695,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.8.5.3

[PATCH 3/9] 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.)

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 fd50e52..4550227 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -2672,6 +2672,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.
  *
@@ -2692,9 +2695,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.8.5.3

[PATCH 4/9] 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.

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 4550227..e3e4618 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -2668,9 +2668,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.
@@ -2697,7 +2697,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.8.5.3

[PATCH 5/9] 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.

In the btrfs case this was causing a spurious printk from
nfsd/nfsfh.c:fh_verify when it found an unexpected DCACHE_DISCONNECTED
dentry.

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

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/btrfs/super.c       |  2 +-
 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(+), 24 deletions(-)

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 97cc241..07ce96b 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -925,7 +925,7 @@ setup_root:
 		return dget(sb->s_root);
 	}
 
-	return d_obtain_alias(inode);
+	return d_obtain_alias_root(inode);
 }
 
 static int btrfs_fill_super(struct super_block *sb,
diff --git a/fs/ceph/super.c b/fs/ceph/super.c
index 2df963f..9666950 100644
--- a/fs/ceph/super.c
+++ b/fs/ceph/super.c
@@ -732,7 +732,7 @@ static struct dentry *open_root_dentry(struct ceph_fs_client *fsc,
 				goto out;
 			}
 		} else {
-			root = d_obtain_alias(inode);
+			root = d_obtain_alias_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 e3e4618..3a1057a 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -1832,25 +1832,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;
@@ -1881,7 +1863,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;
@@ -1902,9 +1887,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_alias_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_alias_root(struct inode *inode)
+{
+	return __d_obtain_alias(inode, 0);
+}
+EXPORT_SYMBOL(d_obtain_alias_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..7b32522 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_alias_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 7ac2a12..a518a73 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_alias_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 bf72e9a..2973aab 100644
--- a/include/linux/dcache.h
+++ b/include/linux/dcache.h
@@ -247,6 +247,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_alias_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.8.5.3

[PATCH 6/9] 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 3a1057a..efe3d3b 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -719,8 +719,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.
@@ -729,10 +727,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;
 
@@ -744,7 +741,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;
@@ -775,7 +772,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;
@@ -2784,7 +2781,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.8.5.3

[PATCH 7/9] 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 efe3d3b..448ef98 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -753,12 +753,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.8.5.3

[PATCH 8/9] 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.8.5.3

[PATCH 9/9] dcache: rename DCACHE_DISCONNECTED -> DCACHE_CONNECTING

[J. Bruce Fields]

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

The DCACHE_DISCONNECTED flag was intended *only* to mark dentries which
were looked up by filehandle and are currently in the process of being
hooked up to the rest of dcache.

Over time people have also confused it with IS_ROOT, using it to mark
directories that are permanently "disconnected" just because for example
they're the root of some filesystem.

Perhaps a different name would have helped. (Thanks to Eric Biederman
for the suggestion of DCACHE_CONNECTING.)

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 Documentation/filesystems/nfs/Exporting     |  4 ++--
 drivers/staging/lustre/lustre/llite/namei.c |  4 ++--
 fs/dcache.c                                 |  8 ++++----
 fs/exportfs/expfs.c                         | 16 ++++++++--------
 fs/fat/namei_vfat.c                         |  4 ++--
 fs/nfsd/nfsfh.c                             |  4 ++--
 fs/ocfs2/dcache.c                           |  2 +-
 fs/ocfs2/namei.c                            |  2 +-
 include/linux/dcache.h                      |  4 ++--
 9 files changed, 24 insertions(+), 24 deletions(-)

diff --git a/Documentation/filesystems/nfs/Exporting b/Documentation/filesystems/nfs/Exporting
index 9b7de5b..6d3b272 100644
--- a/Documentation/filesystems/nfs/Exporting
+++ b/Documentation/filesystems/nfs/Exporting
@@ -52,7 +52,7 @@ the dcache that are not needed for normal filesystem access.
 
 To implement these features, the dcache has:
 
-a/ A dentry flag DCACHE_DISCONNECTED which is set on
+a/ A dentry flag DCACHE_CONNECTING which is set on
    any dentry that might not be part of the proper prefix.
    This is set when anonymous dentries are created, and cleared when a
    dentry is noticed to be a child of a dentry which is in the proper
@@ -69,7 +69,7 @@ c/ Helper routines to allocate anonymous dentries, and to help attach
     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.
+        DCACHE_CONNECTING) 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) or d_materialise_unique(dentry, inode)
diff --git a/drivers/staging/lustre/lustre/llite/namei.c b/drivers/staging/lustre/lustre/llite/namei.c
index fc8d264..f271111 100644
--- a/drivers/staging/lustre/lustre/llite/namei.c
+++ b/drivers/staging/lustre/lustre/llite/namei.c
@@ -368,7 +368,7 @@ static struct dentry *ll_find_alias(struct inode *inode, struct dentry *dentry)
 		LASSERT(alias != dentry);
 
 		spin_lock(&alias->d_lock);
-		if (alias->d_flags & DCACHE_DISCONNECTED)
+		if (alias->d_flags & DCACHE_CONNECTING)
 			/* LASSERT(last_discon == NULL); LU-405, bz 20055 */
 			discon_alias = alias;
 		else if (alias->d_parent == dentry->d_parent	     &&
@@ -395,7 +395,7 @@ static struct dentry *ll_find_alias(struct inode *inode, struct dentry *dentry)
 
 /*
  * Similar to d_splice_alias(), but lustre treats invalid alias
- * similar to DCACHE_DISCONNECTED, and tries to use it anyway.
+ * similar to DCACHE_CONNECTING, and tries to use it anyway.
  */
 struct dentry *ll_splice_alias(struct inode *inode, struct dentry *de)
 {
diff --git a/fs/dcache.c b/fs/dcache.c
index 448ef98..0d5e1a5 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -726,7 +726,7 @@ EXPORT_SYMBOL(dget_parent);
  * it can be unhashed only if it has no children, or if it is the root
  * of a filesystem.
  *
- * If the inode has an IS_ROOT, DCACHE_DISCONNECTED alias, then prefer
+ * If the inode has an IS_ROOT, DCACHE_CONNECTING alias, then prefer
  * any other hashed alias over that one.
  */
 static struct dentry *__d_find_alias(struct inode *inode)
@@ -739,7 +739,7 @@ again:
 		spin_lock(&alias->d_lock);
  		if (S_ISDIR(inode->i_mode) || !d_unhashed(alias)) {
 			if (IS_ROOT(alias) &&
-			    (alias->d_flags & DCACHE_DISCONNECTED)) {
+			    (alias->d_flags & DCACHE_CONNECTING)) {
 				discon_alias = alias;
 			} else {
 				__dget_dlock(alias);
@@ -1860,7 +1860,7 @@ struct dentry *__d_obtain_alias(struct inode *inode, int disconnected)
 	add_flags = d_flags_for_inode(inode);
 
 	if (disconnected)
-		add_flags |= DCACHE_DISCONNECTED;
+		add_flags |= DCACHE_CONNECTING;
 
 	spin_lock(&tmp->d_lock);
 	tmp->d_inode = inode;
@@ -1883,7 +1883,7 @@ struct dentry *__d_obtain_alias(struct inode *inode, int disconnected)
 }
 
 /**
- * d_obtain_alias - find or allocate a DISCONNECTED dentry for a given inode
+ * d_obtain_alias - find or allocate a CONNECTING dentry for a given inode
  * @inode: inode to allocate the dentry for
  *
  * Obtain a dentry for an inode resulting from NFS filehandle conversion or
diff --git a/fs/exportfs/expfs.c b/fs/exportfs/expfs.c
index 48a359d..50c6973 100644
--- a/fs/exportfs/expfs.c
+++ b/fs/exportfs/expfs.c
@@ -72,7 +72,7 @@ find_acceptable_alias(struct dentry *result,
 static bool dentry_connected(struct dentry *dentry)
 {
 	dget(dentry);
-	while (dentry->d_flags & DCACHE_DISCONNECTED) {
+	while (dentry->d_flags & DCACHE_CONNECTING) {
 		struct dentry *parent = dget_parent(dentry);
 
 		dput(dentry);
@@ -89,13 +89,13 @@ static bool dentry_connected(struct dentry *dentry)
 static void clear_disconnected(struct dentry *dentry)
 {
 	dget(dentry);
-	while (dentry->d_flags & DCACHE_DISCONNECTED) {
+	while (dentry->d_flags & DCACHE_CONNECTING) {
 		struct dentry *parent = dget_parent(dentry);
 
 		WARN_ON_ONCE(IS_ROOT(dentry));
 
 		spin_lock(&dentry->d_lock);
-		dentry->d_flags &= ~DCACHE_DISCONNECTED;
+		dentry->d_flags &= ~DCACHE_CONNECTING;
 		spin_unlock(&dentry->d_lock);
 
 		dput(dentry);
@@ -187,12 +187,12 @@ out_reconnected:
 /*
  * Make sure target_dir is fully connected to the dentry tree.
  *
- * On successful return, DCACHE_DISCONNECTED will be cleared on
+ * On successful return, DCACHE_CONNECTING will be cleared on
  * target_dir, and target_dir->d_parent->...->d_parent will reach the
  * root of the filesystem.
  *
- * Whenever DCACHE_DISCONNECTED is unset, target_dir is fully connected.
- * But the converse is not true: target_dir may have DCACHE_DISCONNECTED
+ * Whenever DCACHE_CONNECTING is unset, target_dir is fully connected.
+ * But the converse is not true: target_dir may have DCACHE_CONNECTING
  * set but already be connected.  In that case we'll verify the
  * connection to root and then clear the flag.
  *
@@ -208,7 +208,7 @@ reconnect_path(struct vfsmount *mnt, struct dentry *target_dir, char *nbuf)
 
 	dentry = dget(target_dir);
 
-	while (dentry->d_flags & DCACHE_DISCONNECTED) {
+	while (dentry->d_flags & DCACHE_CONNECTING) {
 		BUG_ON(dentry == mnt->mnt_sb->s_root);
 
 		if (IS_ROOT(dentry))
@@ -437,7 +437,7 @@ struct dentry *exportfs_decode_fh(struct vfsmount *mnt, struct fid *fid,
 		 * to ensure our dentry is connected all the way up to the
 		 * filesystem root.
 		 */
-		if (result->d_flags & DCACHE_DISCONNECTED) {
+		if (result->d_flags & DCACHE_CONNECTING) {
 			err = reconnect_path(mnt, result, nbuf);
 			if (err)
 				goto err_result;
diff --git a/fs/fat/namei_vfat.c b/fs/fat/namei_vfat.c
index 6df8d3d..3e450fc 100644
--- a/fs/fat/namei_vfat.c
+++ b/fs/fat/namei_vfat.c
@@ -705,7 +705,7 @@ static int vfat_find(struct inode *dir, struct qstr *qname,
  */
 static int vfat_d_anon_disconn(struct dentry *dentry)
 {
-	return IS_ROOT(dentry) && (dentry->d_flags & DCACHE_DISCONNECTED);
+	return IS_ROOT(dentry) && (dentry->d_flags & DCACHE_CONNECTING);
 }
 
 static struct dentry *vfat_lookup(struct inode *dir, struct dentry *dentry,
@@ -738,7 +738,7 @@ static struct dentry *vfat_lookup(struct inode *dir, struct dentry *dentry,
 	alias = d_find_alias(inode);
 	if (alias && !vfat_d_anon_disconn(alias)) {
 		/*
-		 * This inode has non anonymous-DCACHE_DISCONNECTED
+		 * This inode has non anonymous-DCACHE_CONNECTING
 		 * dentry. This means, the user did ->lookup() by an
 		 * another name (longname vs 8.3 alias of it) in past.
 		 *
diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
index 3c37b16..0bc18ae 100644
--- a/fs/nfsd/nfsfh.c
+++ b/fs/nfsd/nfsfh.c
@@ -252,8 +252,8 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct svc_fh *fhp)
 	}
 
 	if (S_ISDIR(dentry->d_inode->i_mode) &&
-			(dentry->d_flags & DCACHE_DISCONNECTED)) {
-		printk("nfsd: find_fh_dentry returned a DISCONNECTED directory: %pd2\n",
+			(dentry->d_flags & DCACHE_CONNECTING)) {
+		printk("nfsd: find_fh_dentry returned a CONNECTING directory: %pd2\n",
 				dentry);
 	}
 
diff --git a/fs/ocfs2/dcache.c b/fs/ocfs2/dcache.c
index 0d3a97d..3e954a1 100644
--- a/fs/ocfs2/dcache.c
+++ b/fs/ocfs2/dcache.c
@@ -455,7 +455,7 @@ static void ocfs2_dentry_iput(struct dentry *dentry, struct inode *inode)
 		 * No dentry lock is ok if we're disconnected or
 		 * unhashed.
 		 */
-		if (!(dentry->d_flags & DCACHE_DISCONNECTED) &&
+		if (!(dentry->d_flags & DCACHE_CONNECTING) &&
 		    !d_unhashed(dentry)) {
 			unsigned long long ino = 0ULL;
 			if (inode)
diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c
index f4d609b..a753ae5 100644
--- a/fs/ocfs2/namei.c
+++ b/fs/ocfs2/namei.c
@@ -149,7 +149,7 @@ bail_add:
 
 	if (inode) {
 		/*
-		 * If d_splice_alias() finds a DCACHE_DISCONNECTED
+		 * If d_splice_alias() finds a DCACHE_CONNECTING
 		 * dentry, it will d_move() it on top of ourse. The
 		 * return value will indicate this however, so in
 		 * those cases, we switch them around for the locking
diff --git a/include/linux/dcache.h b/include/linux/dcache.h
index 2973aab..9f22329 100644
--- a/include/linux/dcache.h
+++ b/include/linux/dcache.h
@@ -177,14 +177,14 @@ struct dentry_operations {
 #define DCACHE_OP_DELETE		0x00000008
 #define DCACHE_OP_PRUNE			0x00000010
 
-#define	DCACHE_DISCONNECTED		0x00000020
+#define	DCACHE_CONNECTING		0x00000020
      /* This dentry is possibly not currently connected to the dcache tree, in
       * which case its parent will either be itself, or will have this flag as
       * well.  nfsd will not use a dentry with this bit set, but will first
       * endeavour to clear the bit either by discovering that it is connected,
       * or by performing lookup operations.   Any filesystem which supports
       * nfsd_operations MUST have a lookup function which, if it finds a
-      * directory inode with a DCACHE_DISCONNECTED dentry, will d_move that
+      * directory inode with a DCACHE_CONNECTING dentry, will d_move that
       * dentry into place and return that dentry rather than the passed one,
       * typically using d_splice_alias. */
 
-- 
1.8.5.3

Re: [PATCH] Btrfs: unset DCACHE_DISCONNECTED when mounting default subvol

[Eric W. Biederman]

"J. Bruce Fields" <bfields@fieldses.org> writes:

> On Fri, Feb 14, 2014 at 09:45:24PM -0500, J. Bruce Fields wrote:
>> On Fri, Feb 14, 2014 at 05:40:55PM -0800, Eric W. Biederman wrote:
>> > "J. Bruce Fields" <bfields@fieldses.org> writes:
>> > 
>> > > On Fri, Feb 14, 2014 at 01:43:48PM -0500, Josef Bacik wrote:
>> > >> A user was running into errors from an NFS export of a subvolume that had a
>> > >> default subvol set.  When we mount a default subvol we will use d_obtain_alias()
>> > >> to find an existing dentry for the subvolume in the case that the root subvol
>> > >> has already been mounted, or a dummy one is allocated in the case that the root
>> > >> subvol has not already been mounted.  This allows us to connect the dentry later
>> > >> on if we wander into the path.  However if we don't ever wander into the path we
>> > >> will keep DCACHE_DISCONNECTED set for a long time, which angers NFS.  It doesn't
>> > >> appear to cause any problems but it is annoying nonetheless, so simply unset
>> > >> DCACHE_DISCONNECTED in the get_default_root case and switch btrfs_lookup() to
>> > >> use d_materialise_unique() instead which will make everything play nicely
>> > >> together and reconnect stuff if we wander into the defaul subvol path from a
>> > >> different way.  With this patch I'm no longer getting the NFS errors when
>> > >> exporting a volume that has been mounted with a default subvol set.  Thanks,
>> > >
>> > > Looks obviously correct, but based on a quick grep, there are four
>> > > d_obtain_alias callers outside export methods:
>> > >
>> > > 	- btrfs/super.c:get_default_root()
>> > > 	- fs/ceph/super.c:open_root_dentry()
>> > > 	- fs/nfs/getroot.c:nfs_get_root()
>> > > 	- fs/nilfs2/super.c:nilfs_get_root_dentry()
>> > >
>> > > It'd be nice to give them a common d_obtain_alias variant instead of
>> > > making them all clear this by hand.
>> > 
>> > I am in favor of one small fix at a time, so that progress is made and
>> > fixing something just for btrfs seems reasonable for the short term.
>> > 
>> > > Of those nilfs2 also uses d_splice_alias.  I think that problem would
>> > > best be solved by fixing d_splice_alias not to require a
>> > > DCACHE_DISCONNECTED dentry; IS_ROOT() on its own should be fine.
>> > 
>> > You mean by renaming d_splice_alias d_materialise_unique?
>> > 
>> > Or is there a useful distinction you see that should be preserved
>> > between the two methods?
>> > 
>> > Right now my inclination is that everyone should just use
>> > d_materialise_unique and we should kill d_splice_alias.
>> 
>> Probably.  One remaining distinction:
>> 
>> 	- In the local filesystem case if you discover a directory is
>> 	  already aliased elsewhere, you have a corrupted filesystem and
>> 	  want to error out the lookup.  (Didn't you propose a patch to
>> 	  do something like that before?)
>> 	- In the distributed filesystem this is perfectly normal and we
>> 	  want to do our best to fix up our local cache to represent
>> 	  remote reality.
>
> The following keeps the d_splice_alias/d_materialise_unique distinction
> and (hopefully) fixes Josef's bug, and does a little cleanup (including
> your suggested DISCONNECTED->CONNECTING rename).
>
> Any better idea for the naming of d_materialise_unique?
>
> I also didn't try to merge the implementations--the merged
> d_splice_alias/d_materialise_unique was a little uglier than I expected.
> I'll keep messing around with it though.

Your patchset sounds good I am going to aim for reading them through and
reviewing them soonish.

Eric

Re: [PATCH 1/9] dcache: move d_splice_alias

[Christoph Hellwig]

On Tue, Feb 18, 2014 at 03:28:57PM -0500, J. Bruce Fields wrote:
> 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.
> 
> Signed-off-by: J. Bruce Fields <bfields@redhat.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 2/9] dcache: close d_move race in d_splice_alias

[Christoph Hellwig]

On Tue, Feb 18, 2014 at 03:28:58PM -0500, J. Bruce Fields wrote:
> 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.

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 5/9] dcache: d_obtain_alias callers don't all want DISCONNECTED

[Christoph Hellwig]

>  
> -	return d_obtain_alias(inode);
> +	return d_obtain_alias_root(inode);

Can we call this d_obtain_root or similar, please?

Re: [PATCH 5/9] dcache: d_obtain_alias callers don't all want DISCONNECTED

[J. Bruce Fields]

On Thu, Feb 20, 2014 at 05:44:14PM -0800, Christoph Hellwig wrote:
> >  
> > -	return d_obtain_alias(inode);
> > +	return d_obtain_alias_root(inode);
> 
> Can we call this d_obtain_root or similar, please?

Yes, I like d_obtain_root better, done.

I'll send out the updated series sometime.

--b.