* 4 DCACHE_DISCONNECTED patches
@ 2013-09-07 18:45 J. Bruce Fields
2013-09-07 18:45 ` [PATCH 2/4] dcache: make __d_shrink callers check d_unhashed() J. Bruce Fields
` (2 more replies)
0 siblings, 3 replies; 19+ messages in thread
From: J. Bruce Fields @ 2013-09-07 18:45 UTC (permalink / raw)
To: Al Viro; +Cc: linux-nfs, linux-fsdevel, Christoph Hellwig
This is a resend of some DCACHE_DISCONNECTED patches incorporating
suggestions from Christoph.
--b.
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 1/4] dcache: use IS_ROOT to decide where dentry is hashed
2013-09-07 18:45 4 DCACHE_DISCONNECTED patches J. Bruce Fields
@ 2013-09-07 18:45 ` J. Bruce Fields
[not found] ` <1378579561-26868-1-git-send-email-bfields-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2013-09-07 18:46 ` [PATCH 4/4] dcache: don't clear DCACHE_DISCONNECTED too early J. Bruce Fields
2 siblings, 0 replies; 19+ messages in thread
From: J. Bruce Fields @ 2013-09-07 18:45 UTC (permalink / raw)
To: Al Viro
Cc: linux-nfs-u79uwXL29TY76Z2rM5mHXA,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA, Christoph Hellwig,
J. Bruce Fields, Nick Piggin
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".
Also add a comment while we're here.
Cc: Nick Piggin <npiggin-tSWWG44O7X1aa/9Udqfwiw@public.gmane.org>
Cc: Christoph Hellwig <hch-wEGCiKHe2LqWVfeAwA7xHQ@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 | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/fs/dcache.c b/fs/dcache.c
index b949af8..7208b38 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -393,7 +393,12 @@ static void __d_shrink(struct dentry *dentry)
{
if (!d_unhashed(dentry)) {
struct hlist_bl_head *b;
- if (unlikely(dentry->d_flags & DCACHE_DISCONNECTED))
+ /*
+ * 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);
--
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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 1/4] dcache: use IS_ROOT to decide where dentry is hashed
@ 2013-09-07 18:45 ` J. Bruce Fields
0 siblings, 0 replies; 19+ messages in thread
From: J. Bruce Fields @ 2013-09-07 18:45 UTC (permalink / raw)
To: Al Viro
Cc: linux-nfs, linux-fsdevel, Christoph Hellwig, J. Bruce Fields,
Nick Piggin
From: "J. Bruce Fields" <bfields@redhat.com>
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".
Also add a comment while we're here.
Cc: Nick Piggin <npiggin@kernel.dk>
Cc: Christoph Hellwig <hch@infradead.org>
Reviewed-by: NeilBrown <neilb@suse.de>
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
fs/dcache.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/fs/dcache.c b/fs/dcache.c
index b949af8..7208b38 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -393,7 +393,12 @@ static void __d_shrink(struct dentry *dentry)
{
if (!d_unhashed(dentry)) {
struct hlist_bl_head *b;
- if (unlikely(dentry->d_flags & DCACHE_DISCONNECTED))
+ /*
+ * 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);
--
1.7.9.5
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 2/4] dcache: make __d_shrink callers check d_unhashed()
2013-09-07 18:45 4 DCACHE_DISCONNECTED patches J. Bruce Fields
@ 2013-09-07 18:45 ` J. Bruce Fields
[not found] ` <1378579561-26868-1-git-send-email-bfields-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2013-09-07 18:46 ` [PATCH 4/4] dcache: don't clear DCACHE_DISCONNECTED too early J. Bruce Fields
2 siblings, 0 replies; 19+ messages in thread
From: J. Bruce Fields @ 2013-09-07 18:45 UTC (permalink / raw)
To: Al Viro; +Cc: linux-nfs, linux-fsdevel, Christoph Hellwig, J. Bruce Fields
From: "J. Bruce Fields" <bfields@redhat.com>
__d_shrink only has two callers, one of whom already does the check, so
may as well make this the caller's responsibility.
Cc: Christoph Hellwig <hch@infradead.org>
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
fs/dcache.c | 33 ++++++++++++++++-----------------
1 file changed, 16 insertions(+), 17 deletions(-)
diff --git a/fs/dcache.c b/fs/dcache.c
index 7208b38..d9e4fba 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);
}
/**
@@ -905,7 +903,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
--
1.7.9.5
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 3/4] dcache: Don't set DISCONNECTED on "pseudo filesystem" dentries
2013-09-07 18:45 4 DCACHE_DISCONNECTED patches J. Bruce Fields
@ 2013-09-07 18:46 ` J. Bruce Fields
[not found] ` <1378579561-26868-1-git-send-email-bfields-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2013-09-07 18:46 ` [PATCH 4/4] dcache: don't clear DCACHE_DISCONNECTED too early J. Bruce Fields
2 siblings, 0 replies; 19+ messages in thread
From: J. Bruce Fields @ 2013-09-07 18:46 UTC (permalink / raw)
To: Al Viro
Cc: linux-nfs-u79uwXL29TY76Z2rM5mHXA,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA, Christoph Hellwig,
J. Bruce Fields, Nick Piggin
From: "J. Bruce Fields" <bfields-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
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>
Cc: Christoph Hellwig <hch-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>
Signed-off-by: J. Bruce Fields <bfields-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
fs/dcache.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/fs/dcache.c b/fs/dcache.c
index d9e4fba..a53f55d 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -1312,12 +1312,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);
--
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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 3/4] dcache: Don't set DISCONNECTED on "pseudo filesystem" dentries
@ 2013-09-07 18:46 ` J. Bruce Fields
0 siblings, 0 replies; 19+ messages in thread
From: J. Bruce Fields @ 2013-09-07 18:46 UTC (permalink / raw)
To: Al Viro
Cc: linux-nfs, linux-fsdevel, Christoph Hellwig, J. Bruce Fields,
Nick Piggin
From: "J. Bruce Fields" <bfields@redhat.com>
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@kernel.dk>
Cc: Christoph Hellwig <hch@infradead.org>
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
fs/dcache.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/fs/dcache.c b/fs/dcache.c
index d9e4fba..a53f55d 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -1312,12 +1312,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);
--
1.7.9.5
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 4/4] dcache: don't clear DCACHE_DISCONNECTED too early
2013-09-07 18:45 4 DCACHE_DISCONNECTED patches J. Bruce Fields
2013-09-07 18:45 ` [PATCH 2/4] dcache: make __d_shrink callers check d_unhashed() J. Bruce Fields
[not found] ` <1378579561-26868-1-git-send-email-bfields-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
@ 2013-09-07 18:46 ` J. Bruce Fields
[not found] ` <1378579561-26868-5-git-send-email-bfields-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2 siblings, 1 reply; 19+ messages in thread
From: J. Bruce Fields @ 2013-09-07 18:46 UTC (permalink / raw)
To: Al Viro; +Cc: linux-nfs, linux-fsdevel, Christoph Hellwig, 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 a53f55d..a4560ca 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -2404,7 +2404,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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: 4 DCACHE_DISCONNECTED patches
2013-09-07 18:45 4 DCACHE_DISCONNECTED patches J. Bruce Fields
@ 2013-09-08 9:16 ` Christoph Hellwig
[not found] ` <1378579561-26868-1-git-send-email-bfields-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2013-09-07 18:46 ` [PATCH 4/4] dcache: don't clear DCACHE_DISCONNECTED too early J. Bruce Fields
2 siblings, 0 replies; 19+ messages in thread
From: Christoph Hellwig @ 2013-09-08 9:16 UTC (permalink / raw)
To: J. Bruce Fields
Cc: Al Viro, linux-nfs-u79uwXL29TY76Z2rM5mHXA,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA, Christoph Hellwig
On Sat, Sep 07, 2013 at 02:45:57PM -0400, J. Bruce Fields wrote:
> This is a resend of some DCACHE_DISCONNECTED patches incorporating
> suggestions from Christoph.
The whole series looks good to me,
Reviewed-by: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
--
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: 4 DCACHE_DISCONNECTED patches
@ 2013-09-08 9:16 ` Christoph Hellwig
0 siblings, 0 replies; 19+ messages in thread
From: Christoph Hellwig @ 2013-09-08 9:16 UTC (permalink / raw)
To: J. Bruce Fields; +Cc: Al Viro, linux-nfs, linux-fsdevel, Christoph Hellwig
On Sat, Sep 07, 2013 at 02:45:57PM -0400, J. Bruce Fields wrote:
> This is a resend of some DCACHE_DISCONNECTED patches incorporating
> suggestions from Christoph.
The whole series looks good to me,
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 4/4] dcache: don't clear DCACHE_DISCONNECTED too early
2013-09-07 18:46 ` [PATCH 4/4] dcache: don't clear DCACHE_DISCONNECTED too early J. Bruce Fields
@ 2013-09-09 0:46 ` Al Viro
0 siblings, 0 replies; 19+ messages in thread
From: Al Viro @ 2013-09-09 0:46 UTC (permalink / raw)
To: J. Bruce Fields
Cc: linux-nfs-u79uwXL29TY76Z2rM5mHXA,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA, Christoph Hellwig
On Sat, Sep 07, 2013 at 02:46:01PM -0400, J. Bruce Fields wrote:
> From: "J. Bruce Fields" <bfields-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>
> 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.
Then you probably want this
if (!IS_ROOT(pd)) {
/* must have found a connected parent - great */
spin_lock(&pd->d_lock);
pd->d_flags &= ~DCACHE_DISCONNECTED;
spin_unlock(&pd->d_lock);
noprogress = 0;
to go through all intermediates, clearing DCACHE_DISCONNECTED on all of
them; O(depth^2) can suck when we have a long chain of directories...
--
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 4/4] dcache: don't clear DCACHE_DISCONNECTED too early
@ 2013-09-09 0:46 ` Al Viro
0 siblings, 0 replies; 19+ messages in thread
From: Al Viro @ 2013-09-09 0:46 UTC (permalink / raw)
To: J. Bruce Fields; +Cc: linux-nfs, linux-fsdevel, Christoph Hellwig
On Sat, Sep 07, 2013 at 02:46:01PM -0400, J. Bruce Fields wrote:
> 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.
Then you probably want this
if (!IS_ROOT(pd)) {
/* must have found a connected parent - great */
spin_lock(&pd->d_lock);
pd->d_flags &= ~DCACHE_DISCONNECTED;
spin_unlock(&pd->d_lock);
noprogress = 0;
to go through all intermediates, clearing DCACHE_DISCONNECTED on all of
them; O(depth^2) can suck when we have a long chain of directories...
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 4/4] dcache: don't clear DCACHE_DISCONNECTED too early
2013-09-09 0:46 ` Al Viro
(?)
@ 2013-09-09 7:45 ` Christoph Hellwig
-1 siblings, 0 replies; 19+ messages in thread
From: Christoph Hellwig @ 2013-09-09 7:45 UTC (permalink / raw)
To: Al Viro; +Cc: J. Bruce Fields, linux-nfs, linux-fsdevel, Christoph Hellwig
On Mon, Sep 09, 2013 at 01:46:47AM +0100, Al Viro wrote:
> On Sat, Sep 07, 2013 at 02:46:01PM -0400, J. Bruce Fields wrote:
> > 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.
>
> Then you probably want this
> if (!IS_ROOT(pd)) {
> /* must have found a connected parent - great */
> spin_lock(&pd->d_lock);
> pd->d_flags &= ~DCACHE_DISCONNECTED;
> spin_unlock(&pd->d_lock);
> noprogress = 0;
> to go through all intermediates, clearing DCACHE_DISCONNECTED on all of
> them; O(depth^2) can suck when we have a long chain of directories...
Aka call reconnect_path() from fs/exportfs/expfs.c on it.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 4/4] dcache: don't clear DCACHE_DISCONNECTED too early
2013-09-09 0:46 ` Al Viro
@ 2013-09-09 20:46 ` J. Bruce Fields
-1 siblings, 0 replies; 19+ messages in thread
From: J. Bruce Fields @ 2013-09-09 20:46 UTC (permalink / raw)
To: Al Viro
Cc: J. Bruce Fields, linux-nfs-u79uwXL29TY76Z2rM5mHXA,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA, Christoph Hellwig
On Mon, Sep 09, 2013 at 01:46:47AM +0100, Al Viro wrote:
> On Sat, Sep 07, 2013 at 02:46:01PM -0400, J. Bruce Fields wrote:
> > From: "J. Bruce Fields" <bfields-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> >
> > 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.
>
> Then you probably want this
> if (!IS_ROOT(pd)) {
> /* must have found a connected parent - great */
> spin_lock(&pd->d_lock);
> pd->d_flags &= ~DCACHE_DISCONNECTED;
> spin_unlock(&pd->d_lock);
> noprogress = 0;
> to go through all intermediates, clearing DCACHE_DISCONNECTED on all of
> them; O(depth^2) can suck when we have a long chain of directories...
I'm not sure what you mean--something like the following?
Not having seen complaints about filehandle lookup performance I'm
inclined to leave it alone, though.
--b.
diff --git a/fs/exportfs/expfs.c b/fs/exportfs/expfs.c
index 5816e19..72ed705 100644
--- a/fs/exportfs/expfs.c
+++ b/fs/exportfs/expfs.c
@@ -90,6 +90,22 @@ find_disconnected_root(struct dentry *dentry)
return dentry;
}
+static void clear_disconnected(struct dentry *dentry)
+{
+ dget(dentry);
+ while (dentry->d_flags & DCACHE_DISCONNECTED) {
+ struct dentry *parent = dget_parent(dentry);
+
+ spin_lock(&dentry->d_lock);
+ dentry->d_flags &= ~DCACHE_DISCONNECTED;
+ spin_unlock(&dentry->d_lock);
+
+ dput(dentry);
+ dentry = parent;
+ }
+ dput(dentry);
+}
+
/*
* Make sure target_dir is fully connected to the dentry tree.
*
@@ -114,15 +130,11 @@ reconnect_path(struct vfsmount *mnt, struct dentry *target_dir, char *nbuf)
if (!IS_ROOT(pd)) {
/* must have found a connected parent - great */
- spin_lock(&pd->d_lock);
- pd->d_flags &= ~DCACHE_DISCONNECTED;
- spin_unlock(&pd->d_lock);
+ clear_disconnected(target_dir);
noprogress = 0;
} else if (pd == mnt->mnt_sb->s_root) {
printk(KERN_ERR "export: Eeek filesystem root is not connected, impossible\n");
- spin_lock(&pd->d_lock);
- pd->d_flags &= ~DCACHE_DISCONNECTED;
- spin_unlock(&pd->d_lock);
+ clear_disconnected(target_dir);
noprogress = 0;
} else {
/*
--
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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 4/4] dcache: don't clear DCACHE_DISCONNECTED too early
@ 2013-09-09 20:46 ` J. Bruce Fields
0 siblings, 0 replies; 19+ messages in thread
From: J. Bruce Fields @ 2013-09-09 20:46 UTC (permalink / raw)
To: Al Viro; +Cc: J. Bruce Fields, linux-nfs, linux-fsdevel, Christoph Hellwig
On Mon, Sep 09, 2013 at 01:46:47AM +0100, Al Viro wrote:
> On Sat, Sep 07, 2013 at 02:46:01PM -0400, J. Bruce Fields wrote:
> > 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.
>
> Then you probably want this
> if (!IS_ROOT(pd)) {
> /* must have found a connected parent - great */
> spin_lock(&pd->d_lock);
> pd->d_flags &= ~DCACHE_DISCONNECTED;
> spin_unlock(&pd->d_lock);
> noprogress = 0;
> to go through all intermediates, clearing DCACHE_DISCONNECTED on all of
> them; O(depth^2) can suck when we have a long chain of directories...
I'm not sure what you mean--something like the following?
Not having seen complaints about filehandle lookup performance I'm
inclined to leave it alone, though.
--b.
diff --git a/fs/exportfs/expfs.c b/fs/exportfs/expfs.c
index 5816e19..72ed705 100644
--- a/fs/exportfs/expfs.c
+++ b/fs/exportfs/expfs.c
@@ -90,6 +90,22 @@ find_disconnected_root(struct dentry *dentry)
return dentry;
}
+static void clear_disconnected(struct dentry *dentry)
+{
+ dget(dentry);
+ while (dentry->d_flags & DCACHE_DISCONNECTED) {
+ struct dentry *parent = dget_parent(dentry);
+
+ spin_lock(&dentry->d_lock);
+ dentry->d_flags &= ~DCACHE_DISCONNECTED;
+ spin_unlock(&dentry->d_lock);
+
+ dput(dentry);
+ dentry = parent;
+ }
+ dput(dentry);
+}
+
/*
* Make sure target_dir is fully connected to the dentry tree.
*
@@ -114,15 +130,11 @@ reconnect_path(struct vfsmount *mnt, struct dentry *target_dir, char *nbuf)
if (!IS_ROOT(pd)) {
/* must have found a connected parent - great */
- spin_lock(&pd->d_lock);
- pd->d_flags &= ~DCACHE_DISCONNECTED;
- spin_unlock(&pd->d_lock);
+ clear_disconnected(target_dir);
noprogress = 0;
} else if (pd == mnt->mnt_sb->s_root) {
printk(KERN_ERR "export: Eeek filesystem root is not connected, impossible\n");
- spin_lock(&pd->d_lock);
- pd->d_flags &= ~DCACHE_DISCONNECTED;
- spin_unlock(&pd->d_lock);
+ clear_disconnected(target_dir);
noprogress = 0;
} else {
/*
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 4/4] dcache: don't clear DCACHE_DISCONNECTED too early
2013-09-09 20:46 ` J. Bruce Fields
@ 2013-10-11 16:02 ` J. Bruce Fields
-1 siblings, 0 replies; 19+ messages in thread
From: J. Bruce Fields @ 2013-10-11 16:02 UTC (permalink / raw)
To: Al Viro
Cc: J. Bruce Fields, linux-nfs-u79uwXL29TY76Z2rM5mHXA,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA, Christoph Hellwig
On Mon, Sep 09, 2013 at 04:46:55PM -0400, J. Bruce Fields wrote:
> On Mon, Sep 09, 2013 at 01:46:47AM +0100, Al Viro wrote:
> > On Sat, Sep 07, 2013 at 02:46:01PM -0400, J. Bruce Fields wrote:
> > > From: "J. Bruce Fields" <bfields-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > >
> > > 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.
> >
> > Then you probably want this
> > if (!IS_ROOT(pd)) {
> > /* must have found a connected parent - great */
> > spin_lock(&pd->d_lock);
> > pd->d_flags &= ~DCACHE_DISCONNECTED;
> > spin_unlock(&pd->d_lock);
> > noprogress = 0;
> > to go through all intermediates, clearing DCACHE_DISCONNECTED on all of
> > them; O(depth^2) can suck when we have a long chain of directories...
>
> I'm not sure what you mean--something like the following?
>
> Not having seen complaints about filehandle lookup performance I'm
> inclined to leave it alone, though.
But just for fun--I did some cleanup and fixed some other quadratic
behavior here and can notice a difference on lookups of very deep
subdirectories.
For example I'm seeing an uncached lookup of an 8000-deep directory
taking about 6 seconds, and can get that down to a tenth of a second.
I'm not sure yet if the difference on less extreme examples is really
significant, I need to experiment some more.
I'll do some more review and post patches and results.
--b.
> diff --git a/fs/exportfs/expfs.c b/fs/exportfs/expfs.c
> index 5816e19..72ed705 100644
> --- a/fs/exportfs/expfs.c
> +++ b/fs/exportfs/expfs.c
> @@ -90,6 +90,22 @@ find_disconnected_root(struct dentry *dentry)
> return dentry;
> }
>
> +static void clear_disconnected(struct dentry *dentry)
> +{
> + dget(dentry);
> + while (dentry->d_flags & DCACHE_DISCONNECTED) {
> + struct dentry *parent = dget_parent(dentry);
> +
> + spin_lock(&dentry->d_lock);
> + dentry->d_flags &= ~DCACHE_DISCONNECTED;
> + spin_unlock(&dentry->d_lock);
> +
> + dput(dentry);
> + dentry = parent;
> + }
> + dput(dentry);
> +}
> +
> /*
> * Make sure target_dir is fully connected to the dentry tree.
> *
> @@ -114,15 +130,11 @@ reconnect_path(struct vfsmount *mnt, struct dentry *target_dir, char *nbuf)
>
> if (!IS_ROOT(pd)) {
> /* must have found a connected parent - great */
> - spin_lock(&pd->d_lock);
> - pd->d_flags &= ~DCACHE_DISCONNECTED;
> - spin_unlock(&pd->d_lock);
> + clear_disconnected(target_dir);
> noprogress = 0;
> } else if (pd == mnt->mnt_sb->s_root) {
> printk(KERN_ERR "export: Eeek filesystem root is not connected, impossible\n");
> - spin_lock(&pd->d_lock);
> - pd->d_flags &= ~DCACHE_DISCONNECTED;
> - spin_unlock(&pd->d_lock);
> + clear_disconnected(target_dir);
> noprogress = 0;
> } else {
> /*
--
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 4/4] dcache: don't clear DCACHE_DISCONNECTED too early
@ 2013-10-11 16:02 ` J. Bruce Fields
0 siblings, 0 replies; 19+ messages in thread
From: J. Bruce Fields @ 2013-10-11 16:02 UTC (permalink / raw)
To: Al Viro; +Cc: J. Bruce Fields, linux-nfs, linux-fsdevel, Christoph Hellwig
On Mon, Sep 09, 2013 at 04:46:55PM -0400, J. Bruce Fields wrote:
> On Mon, Sep 09, 2013 at 01:46:47AM +0100, Al Viro wrote:
> > On Sat, Sep 07, 2013 at 02:46:01PM -0400, J. Bruce Fields wrote:
> > > 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.
> >
> > Then you probably want this
> > if (!IS_ROOT(pd)) {
> > /* must have found a connected parent - great */
> > spin_lock(&pd->d_lock);
> > pd->d_flags &= ~DCACHE_DISCONNECTED;
> > spin_unlock(&pd->d_lock);
> > noprogress = 0;
> > to go through all intermediates, clearing DCACHE_DISCONNECTED on all of
> > them; O(depth^2) can suck when we have a long chain of directories...
>
> I'm not sure what you mean--something like the following?
>
> Not having seen complaints about filehandle lookup performance I'm
> inclined to leave it alone, though.
But just for fun--I did some cleanup and fixed some other quadratic
behavior here and can notice a difference on lookups of very deep
subdirectories.
For example I'm seeing an uncached lookup of an 8000-deep directory
taking about 6 seconds, and can get that down to a tenth of a second.
I'm not sure yet if the difference on less extreme examples is really
significant, I need to experiment some more.
I'll do some more review and post patches and results.
--b.
> diff --git a/fs/exportfs/expfs.c b/fs/exportfs/expfs.c
> index 5816e19..72ed705 100644
> --- a/fs/exportfs/expfs.c
> +++ b/fs/exportfs/expfs.c
> @@ -90,6 +90,22 @@ find_disconnected_root(struct dentry *dentry)
> return dentry;
> }
>
> +static void clear_disconnected(struct dentry *dentry)
> +{
> + dget(dentry);
> + while (dentry->d_flags & DCACHE_DISCONNECTED) {
> + struct dentry *parent = dget_parent(dentry);
> +
> + spin_lock(&dentry->d_lock);
> + dentry->d_flags &= ~DCACHE_DISCONNECTED;
> + spin_unlock(&dentry->d_lock);
> +
> + dput(dentry);
> + dentry = parent;
> + }
> + dput(dentry);
> +}
> +
> /*
> * Make sure target_dir is fully connected to the dentry tree.
> *
> @@ -114,15 +130,11 @@ reconnect_path(struct vfsmount *mnt, struct dentry *target_dir, char *nbuf)
>
> if (!IS_ROOT(pd)) {
> /* must have found a connected parent - great */
> - spin_lock(&pd->d_lock);
> - pd->d_flags &= ~DCACHE_DISCONNECTED;
> - spin_unlock(&pd->d_lock);
> + clear_disconnected(target_dir);
> noprogress = 0;
> } else if (pd == mnt->mnt_sb->s_root) {
> printk(KERN_ERR "export: Eeek filesystem root is not connected, impossible\n");
> - spin_lock(&pd->d_lock);
> - pd->d_flags &= ~DCACHE_DISCONNECTED;
> - spin_unlock(&pd->d_lock);
> + clear_disconnected(target_dir);
> noprogress = 0;
> } else {
> /*
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 4/4] dcache: don't clear DCACHE_DISCONNECTED too early
2013-10-11 16:02 ` J. Bruce Fields
@ 2013-10-12 8:42 ` Christoph Hellwig
-1 siblings, 0 replies; 19+ messages in thread
From: Christoph Hellwig @ 2013-10-12 8:42 UTC (permalink / raw)
To: J. Bruce Fields
Cc: Al Viro, J. Bruce Fields, linux-nfs-u79uwXL29TY76Z2rM5mHXA,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA, Christoph Hellwig
On Fri, Oct 11, 2013 at 12:02:37PM -0400, J. Bruce Fields wrote:
> But just for fun--I did some cleanup and fixed some other quadratic
> behavior here and can notice a difference on lookups of very deep
> subdirectories.
>
> For example I'm seeing an uncached lookup of an 8000-deep directory
> taking about 6 seconds, and can get that down to a tenth of a second.
>
> I'm not sure yet if the difference on less extreme examples is really
> significant, I need to experiment some more.
>
> I'll do some more review and post patches and results.
This sounds like and awesome improvement. How much code do you have to
add for it?
--
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 4/4] dcache: don't clear DCACHE_DISCONNECTED too early
@ 2013-10-12 8:42 ` Christoph Hellwig
0 siblings, 0 replies; 19+ messages in thread
From: Christoph Hellwig @ 2013-10-12 8:42 UTC (permalink / raw)
To: J. Bruce Fields
Cc: Al Viro, J. Bruce Fields, linux-nfs, linux-fsdevel, Christoph Hellwig
On Fri, Oct 11, 2013 at 12:02:37PM -0400, J. Bruce Fields wrote:
> But just for fun--I did some cleanup and fixed some other quadratic
> behavior here and can notice a difference on lookups of very deep
> subdirectories.
>
> For example I'm seeing an uncached lookup of an 8000-deep directory
> taking about 6 seconds, and can get that down to a tenth of a second.
>
> I'm not sure yet if the difference on less extreme examples is really
> significant, I need to experiment some more.
>
> I'll do some more review and post patches and results.
This sounds like and awesome improvement. How much code do you have to
add for it?
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 4/4] dcache: don't clear DCACHE_DISCONNECTED too early
2013-10-12 8:42 ` Christoph Hellwig
(?)
@ 2013-10-15 19:29 ` J. Bruce Fields
-1 siblings, 0 replies; 19+ messages in thread
From: J. Bruce Fields @ 2013-10-15 19:29 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Al Viro, J. Bruce Fields, linux-nfs, linux-fsdevel
On Sat, Oct 12, 2013 at 01:42:35AM -0700, Christoph Hellwig wrote:
> On Fri, Oct 11, 2013 at 12:02:37PM -0400, J. Bruce Fields wrote:
> > But just for fun--I did some cleanup and fixed some other quadratic
> > behavior here and can notice a difference on lookups of very deep
> > subdirectories.
> >
> > For example I'm seeing an uncached lookup of an 8000-deep directory
> > taking about 6 seconds, and can get that down to a tenth of a second.
> >
> > I'm not sure yet if the difference on less extreme examples is really
> > significant, I need to experiment some more.
> >
> > I'll do some more review and post patches and results.
>
> This sounds like and awesome improvement.
Nobody should be nesting that deep, but maybe it's useful to have the
insurance against bad behavior anyway.
> How much code do you have to
> add for it?
fs/exportfs/expfs.c | 224 ++++++++++++++++++++++++++--------------------------
1 file changed, 112 insertions(+), 112 deletions(-)
But maybe I removed some necessary complication. I'll post the
patches....
--b.
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2013-10-15 19:29 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-09-07 18:45 4 DCACHE_DISCONNECTED patches J. Bruce Fields
2013-09-07 18:45 ` [PATCH 2/4] dcache: make __d_shrink callers check d_unhashed() J. Bruce Fields
[not found] ` <1378579561-26868-1-git-send-email-bfields-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2013-09-07 18:45 ` [PATCH 1/4] dcache: use IS_ROOT to decide where dentry is hashed J. Bruce Fields
2013-09-07 18:45 ` J. Bruce Fields
2013-09-07 18:46 ` [PATCH 3/4] dcache: Don't set DISCONNECTED on "pseudo filesystem" dentries J. Bruce Fields
2013-09-07 18:46 ` J. Bruce Fields
2013-09-08 9:16 ` 4 DCACHE_DISCONNECTED patches Christoph Hellwig
2013-09-08 9:16 ` Christoph Hellwig
2013-09-07 18:46 ` [PATCH 4/4] dcache: don't clear DCACHE_DISCONNECTED too early J. Bruce Fields
[not found] ` <1378579561-26868-5-git-send-email-bfields-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2013-09-09 0:46 ` Al Viro
2013-09-09 0:46 ` Al Viro
2013-09-09 7:45 ` Christoph Hellwig
[not found] ` <20130909004647.GK13318-3bDd1+5oDREiFSDQTTA3OLVCufUGDwFn@public.gmane.org>
2013-09-09 20:46 ` J. Bruce Fields
2013-09-09 20:46 ` J. Bruce Fields
[not found] ` <20130909204655.GA10599-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org>
2013-10-11 16:02 ` J. Bruce Fields
2013-10-11 16:02 ` J. Bruce Fields
[not found] ` <20131011160237.GA21258-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org>
2013-10-12 8:42 ` Christoph Hellwig
2013-10-12 8:42 ` Christoph Hellwig
2013-10-15 19:29 ` J. Bruce Fields
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.