This is a resend of some DCACHE_DISCONNECTED patches incorporating
suggestions from Christoph.
--b.
From: "J. Bruce Fields" <[email protected]>
__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 <[email protected]>
Signed-off-by: J. Bruce Fields <[email protected]>
---
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
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" <[email protected]>
> >
> > 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.
From: "J. Bruce Fields" <[email protected]>
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 <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Signed-off-by: J. Bruce Fields <[email protected]>
---
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
From: "J. Bruce Fields" <[email protected]>
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 <[email protected]>
---
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
On Sat, Sep 07, 2013 at 02:46:01PM -0400, J. Bruce Fields wrote:
> From: "J. Bruce Fields" <[email protected]>
>
> 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...
From: "J. Bruce Fields" <[email protected]>
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 <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Reviewed-by: NeilBrown <[email protected]>
Signed-off-by: J. Bruce Fields <[email protected]>
---
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
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" <[email protected]>
> >
> > 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 {
/*
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 <[email protected]>
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.
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?
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" <[email protected]>
> > >
> > > 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 {
> /*