[PATCH 1/3] dcache: use IS_ROOT to decide where dentry is hashed

[PATCH 1/3] dcache: use IS_ROOT to decide where dentry is hashed

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Every hashed dentry is either hashed in the dentry_hashtable, or a
superblock's s_anon list.

__d_shrink assumes it can determine which is the case by checking
DCACHE_DISCONNECTED; this is not true.

It is true that when DCACHE_DISCONNECTED is cleared, the dentry is not
only hashed on dentry_hashtable, but is fully connected to its parents
back to the root.

But the converse is *not* true: fs/exportfs/expfs.c:reconnect_path()
attempts to connect a directory (found by filehandle lookup) back to
root by ascending to parents and performing lookups one at a time.  It
does not clear DCACHE_DISCONNECTED until it's done, and that is not at
all an atomic process.

In particular, it is possible for DCACHE_DISCONNECTED to be set on a
dentry which is hashed on the dentry_hashtable.

Instead, use IS_ROOT() to check which hash chain a dentry is on.  This
*does* work:

Dentries are hashed only by:

	- d_obtain_alias, which adds an IS_ROOT() dentry to sb_anon.

	- __d_rehash, called by _d_rehash: hashes to the dentry's
	  parent, and all callers of _d_rehash appear to have d_parent
	  set to a "real" parent.
	- __d_rehash, called by __d_move: rehashes the moved dentry to
	  hash chain determined by target, and assigns target's d_parent
	  to its d_parent, before dropping the dentry's d_lock.

Therefore I believe it's safe for a holder of a dentry's d_lock to
assume that it is hashed on sb_anon if and only if IS_ROOT(dentry) is
true.

I believe the incorrect assumption about DCACHE_DISCONNECTED was
originally introduced by ceb5bdc2d246 "fs: dcache per-bucket dcache hash
locking".

Cc: Nick Piggin <npiggin-tSWWG44O7X1aa/9Udqfwiw@public.gmane.org>
Reviewed-by: NeilBrown <neilb-l3A5Bk7waGM@public.gmane.org>
Signed-off-by: J. Bruce Fields <bfields-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/dcache.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/dcache.c b/fs/dcache.c
index b949af8..934f02d 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -393,7 +393,7 @@ static void __d_shrink(struct dentry *dentry)
 {
 	if (!d_unhashed(dentry)) {
 		struct hlist_bl_head *b;
-		if (unlikely(dentry->d_flags & DCACHE_DISCONNECTED))
+		if (unlikely(IS_ROOT(dentry)))
 			b = &dentry->d_sb->s_anon;
 		else
 			b = d_hash(dentry->d_parent, dentry->d_name.hash);
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/3] dcache: Don't set DISCONNECTED on "pseudo filesystem" dentries

[J. Bruce Fields]

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

I can't for the life of me see any reason why anyone would care whether
a dentry that is never hooked into the dentry cache would need
DCACHE_DISCONNECTED set.

This originates from 4b936885ab04dc6e0bb0ef35e0e23c1a7364d9e5 "fs:
improve scalability of pseudo filesystems", which probably just made the
false assumption the DCACHE_DISCONNECTED was meant to be set on anything
not connected to a parent somehow.

So this is just confusing.  Ideally the only uses of DCACHE_DISCONNECTED
would be in the filehandle-lookup code, which needs it to ensure
dentries are connected into the dentry tree before use.

I left d_alloc_pseudo there even though it's now equivalent to
__d_alloc(), just on the theory the name is better documentation of its
intended use outside dcache.c.

Cc: Nick Piggin <npiggin@kernel.dk>
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/dcache.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/fs/dcache.c b/fs/dcache.c
index 934f02d..ec66780 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -1308,12 +1308,13 @@ struct dentry *d_alloc(struct dentry * parent, const struct qstr *name)
 }
 EXPORT_SYMBOL(d_alloc);
 
+/*
+ * For filesystems that do not actually use the dentry cache at all, and
+ * only ever deal in IS_ROOT() dentries:
+ */
 struct dentry *d_alloc_pseudo(struct super_block *sb, const struct qstr *name)
 {
-	struct dentry *dentry = __d_alloc(sb, name);
-	if (dentry)
-		dentry->d_flags |= DCACHE_DISCONNECTED;
-	return dentry;
+	return __d_alloc(sb, name);
 }
 EXPORT_SYMBOL(d_alloc_pseudo);
 
-- 
1.7.9.5

[PATCH 3/3] dcache: don't clear DCACHE_DISCONNECTED too early

[J. Bruce Fields]

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

DCACHE_DISCONNECTED should not be cleared until we're sure the dentry is
connected all the way up to the root of the filesystem.  It *shouldn't*
be cleared as soon as the dentry is connected to a parent.  That will
cause bugs at least on exportable filesystems.

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

diff --git a/fs/dcache.c b/fs/dcache.c
index ec66780..69f1ba0 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -2396,7 +2396,6 @@ static void __d_materialise_dentry(struct dentry *dentry, struct dentry *anon)
 	spin_unlock(&dentry->d_lock);
 
 	/* anon->d_lock still locked, returns locked */
-	anon->d_flags &= ~DCACHE_DISCONNECTED;
 }
 
 /**
-- 
1.7.9.5

Re: [PATCH 1/3] dcache: use IS_ROOT to decide where dentry is hashed

[Christoph Hellwig]

On Fri, Sep 06, 2013 at 11:43:48AM -0400, J. Bruce Fields wrote:
> diff --git a/fs/dcache.c b/fs/dcache.c
> index b949af8..934f02d 100644
> --- a/fs/dcache.c
> +++ b/fs/dcache.c
> @@ -393,7 +393,7 @@ static void __d_shrink(struct dentry *dentry)
>  {
>  	if (!d_unhashed(dentry)) {
>  		struct hlist_bl_head *b;
> -		if (unlikely(dentry->d_flags & DCACHE_DISCONNECTED))
> +		if (unlikely(IS_ROOT(dentry)))

I think this needs a comment about why the IS_ROOT check is fine,
destilled from your commit log.

Also while reviewing this I noticed that one of the two callers of
__d_shrink already has the d_unhashed check - it might make sense to
move it to the other caller as well if you touch this area anyway.
(as a separate patch).

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] dcache: Don't set DISCONNECTED on "pseudo filesystem" dentries

[Christoph Hellwig]

On Fri, Sep 06, 2013 at 11:43:49AM -0400, J. Bruce Fields wrote:
> From: "J. Bruce Fields" <bfields@redhat.com>
> 
> I can't for the life of me see any reason why anyone would care whether
> a dentry that is never hooked into the dentry cache would need
> DCACHE_DISCONNECTED set.

__d_shrink did before your PATCH 1/3.

With that fixed your patch looks fine.

> +/*
> + * For filesystems that do not actually use the dentry cache at all, and
> + * only ever deal in IS_ROOT() dentries:
> + */

If you document the function it really should be a kerneldoc comment.

Also the description, while technically correct, isn't all that useful.

It need an explanation why the filesystem is fine with unhashed
dentries, and the reason is that it never performs any lookups as it
pins the dentries in memory.

Re: [PATCH 1/3] dcache: use IS_ROOT to decide where dentry is hashed

[J. Bruce Fields]

On Fri, Sep 06, 2013 at 10:00:44AM -0700, Christoph Hellwig wrote:
> On Fri, Sep 06, 2013 at 11:43:48AM -0400, J. Bruce Fields wrote:
> > diff --git a/fs/dcache.c b/fs/dcache.c
> > index b949af8..934f02d 100644
> > --- a/fs/dcache.c
> > +++ b/fs/dcache.c
> > @@ -393,7 +393,7 @@ static void __d_shrink(struct dentry *dentry)
> >  {
> >  	if (!d_unhashed(dentry)) {
> >  		struct hlist_bl_head *b;
> > -		if (unlikely(dentry->d_flags & DCACHE_DISCONNECTED))
> > +		if (unlikely(IS_ROOT(dentry)))
> 
> I think this needs a comment about why the IS_ROOT check is fine,
> destilled from your commit log.

How about this?:

 	if (!d_unhashed(dentry)) {
 		struct hlist_bl_head *b;
+		/*
+		 * Hashed dentries are normally on the dentry hashtable,
+		 * with the exception of those newly allocated by
+		 * d_obtain_alias, which are always IS_ROOT:
+		 */
 		if (unlikely(IS_ROOT(dentry)))
 			b = &dentry->d_sb->s_anon;
 		else

> Also while reviewing this I noticed that one of the two callers of
> __d_shrink already has the d_unhashed check - it might make sense to
> move it to the other caller as well if you touch this area anyway.
> (as a separate patch).

Sure.  That'd look like the following.

--b.

diff --git a/fs/dcache.c b/fs/dcache.c
index 113c82f..81db000 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -391,23 +391,21 @@ static struct dentry *d_kill(struct dentry *dentry, struct dentry *parent)
  */
 static void __d_shrink(struct dentry *dentry)
 {
-	if (!d_unhashed(dentry)) {
-		struct hlist_bl_head *b;
-		/*
-		 * Hashed dentries are normally on the dentry hashtable,
-		 * with the exception of those newly allocated by
-		 * d_obtain_alias, which are always IS_ROOT:
-		 */
-		if (unlikely(IS_ROOT(dentry)))
-			b = &dentry->d_sb->s_anon;
-		else
-			b = d_hash(dentry->d_parent, dentry->d_name.hash);
+	struct hlist_bl_head *b;
+	/*
+	 * Hashed dentries are normally on the dentry hashtable,
+	 * with the exception of those newly allocated by
+	 * d_obtain_alias, which are always IS_ROOT:
+	 */
+	if (unlikely(IS_ROOT(dentry)))
+		b = &dentry->d_sb->s_anon;
+	else
+		b = d_hash(dentry->d_parent, dentry->d_name.hash);
 
-		hlist_bl_lock(b);
-		__hlist_bl_del(&dentry->d_hash);
-		dentry->d_hash.pprev = NULL;
-		hlist_bl_unlock(b);
-	}
+	hlist_bl_lock(b);
+	__hlist_bl_del(&dentry->d_hash);
+	dentry->d_hash.pprev = NULL;
+	hlist_bl_unlock(b);
 }
 
 /**
@@ -902,7 +900,8 @@ static void shrink_dcache_for_umount_subtree(struct dentry *dentry)
 				dentry->d_op->d_prune(dentry);
 
 			dentry_lru_del(dentry);
-			__d_shrink(dentry);
+			if (!d_unhashed(dentry))
+				__d_shrink(dentry);
 
 			if (dentry->d_lockref.count != 0) {
 				printk(KERN_ERR
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] dcache: Don't set DISCONNECTED on "pseudo filesystem" dentries

[J. Bruce Fields]

On Fri, Sep 06, 2013 at 10:03:39AM -0700, Christoph Hellwig wrote:
> On Fri, Sep 06, 2013 at 11:43:49AM -0400, J. Bruce Fields wrote:
> > From: "J. Bruce Fields" <bfields-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > 
> > I can't for the life of me see any reason why anyone would care whether
> > a dentry that is never hooked into the dentry cache would need
> > DCACHE_DISCONNECTED set.
> 
> __d_shrink did before your PATCH 1/3.

Well, d_alloc_pseudo dentries aren't even hashed, so even d_shrink
didn't actually reach that DCACHE_DISCONNECTED check.

Uh, maybe change that "would" above to a "should".

> 
> With that fixed your patch looks fine.
> 
> > +/*
> > + * For filesystems that do not actually use the dentry cache at all, and
> > + * only ever deal in IS_ROOT() dentries:
> > + */
> 
> If you document the function it really should be a kerneldoc comment.
> 
> Also the description, while technically correct, isn't all that useful.
> 
> It need an explanation why the filesystem is fine with unhashed
> dentries, and the reason is that it never performs any lookups as it
> pins the dentries in memory.

Makes sense--result follows.

Thanks for the review.

--b.

commit 089d891965374b4262973e51839fe361e2ca3cf0
Author: J. Bruce Fields <bfields-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Date:   Fri Jun 29 16:20:47 2012 -0400

    dcache: Don't set DISCONNECTED on "pseudo filesystem" dentries
    
    I can't for the life of me see any reason why anyone should care whether
    a dentry that is never hooked into the dentry cache would need
    DCACHE_DISCONNECTED set.
    
    This originates from 4b936885ab04dc6e0bb0ef35e0e23c1a7364d9e5 "fs:
    improve scalability of pseudo filesystems", which probably just made the
    false assumption the DCACHE_DISCONNECTED was meant to be set on anything
    not connected to a parent somehow.
    
    So this is just confusing.  Ideally the only uses of DCACHE_DISCONNECTED
    would be in the filehandle-lookup code, which needs it to ensure
    dentries are connected into the dentry tree before use.
    
    I left d_alloc_pseudo there even though it's now equivalent to
    __d_alloc(), just on the theory the name is better documentation of its
    intended use outside dcache.c.
    
    Cc: Nick Piggin <npiggin-tSWWG44O7X1aa/9Udqfwiw@public.gmane.org>
    Signed-off-by: J. Bruce Fields <bfields-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

diff --git a/fs/dcache.c b/fs/dcache.c
index 7208b38..2255359 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -1313,12 +1313,17 @@ struct dentry *d_alloc(struct dentry * parent, const struct qstr *name)
 }
 EXPORT_SYMBOL(d_alloc);
 
+/**
+ * d_alloc_pseudo - allocate a dentry (for lookup-less filesystems)
+ * @sb: the superblock
+ * @name: qstr of the name
+ *
+ * For a filesystem that just pins its dentries in memory and never
+ * performs lookups at all, return an unhashed IS_ROOT dentry.
+ */
 struct dentry *d_alloc_pseudo(struct super_block *sb, const struct qstr *name)
 {
-	struct dentry *dentry = __d_alloc(sb, name);
-	if (dentry)
-		dentry->d_flags |= DCACHE_DISCONNECTED;
-	return dentry;
+	return __d_alloc(sb, name);
 }
 EXPORT_SYMBOL(d_alloc_pseudo);
 
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html