Josef Bacik
2011-May-20 17:44 UTC
[PATCH 1/2] fs: add a DCACHE_NEED_LOOKUP flag for d_flags V2
Btrfs (and I''d venture most other fs''s) stores its indexes in
nice disk order
for readdir, but unfortunately in the case of anything that stats the files in
order that readdir spits back (like oh say ls) that means we still have to do
the normal lookup of the file, which means looking up our other index and then
looking up the inode. What I want is a way to create dummy dentries when we
find them in readdir so that when ls or anything else subsequently does a
stat(), we already have the location information in the dentry and can go
straight to the inode itself. The lookup stuff just assumes that if it finds a
dentry it is done, it doesn''t perform a lookup. So add a
DCACHE_NEED_LOOKUP
flag so that the lookup code knows it still needs to run i_op->lookup() on
the
parent to get the inode for the dentry. I have tested this with btrfs and I
went from something that looks like this
http://people.redhat.com/jwhiter/ls-noreada.png
To this
http://people.redhat.com/jwhiter/ls-good.png
Or 2100 seconds to 900 seconds. This was on a 40gb fs that was completely full
of empty files. Thanks,
Signed-off-by: Josef Bacik <josef@redhat.com>
---
V1->V2: Add a d_clear_need_lookup helper to clear the need lookup flag. This
also does it _after_ unhashing the dentry so we don''t race with
lookups.
fs/dcache.c | 25 ++++++++++++++++++++
fs/namei.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++
include/linux/dcache.h | 7 +++++
3 files changed, 90 insertions(+), 0 deletions(-)
diff --git a/fs/dcache.c b/fs/dcache.c
index 22a0ef4..131f20a 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -343,6 +343,24 @@ void d_drop(struct dentry *dentry)
EXPORT_SYMBOL(d_drop);
/*
+ * d_clear_need_lookup - drop a dentry from cache and clear the need lookup
flag
+ * @dentry: dentry to drop
+ *
+ * This is called when we do a lookup on a placeholder dentry that needed to be
+ * looked up. The dentry should have been hashed in order for it to be found
by
+ * the lookup code, but now needs to be unhashed while we do the actual lookup
+ * and clear the DCACHE_NEED_LOOKUP flag.
+ */
+void d_clear_need_lookup(struct dentry *dentry)
+{
+ spin_lock(&dentry->d_lock);
+ __d_drop(dentry);
+ dentry->d_flags &= ~DCACHE_NEED_LOOKUP;
+ spin_unlock(&dentry->d_lock);
+}
+EXPORT_SYMBOL(d_clear_need_lookup);
+
+/*
* Finish off a dentry we''ve decided to kill.
* dentry->d_lock must be held, returns with it unlocked.
* If ref is non-zero, then decrement the refcount too.
@@ -1703,6 +1721,13 @@ struct dentry *d_add_ci(struct dentry *dentry, struct
inode *inode,
}
/*
+ * We are going to instantiate this dentry, unhash it and clear the
+ * lookup flag so we can do that.
+ */
+ if (unlikely(d_need_lookup(found)))
+ d_clear_need_lookup(found);
+
+ /*
* Negative dentry: instantiate it unless the inode is a directory and
* already has a dentry.
*/
diff --git a/fs/namei.c b/fs/namei.c
index e3c4f11..d417217 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -1198,6 +1198,30 @@ static struct dentry *d_alloc_and_lookup(struct dentry
*parent,
}
/*
+ * We already have a dentry, but require a lookup to be performed on the parent
+ * directory to fill in d_inode. Returns the new dentry, or ERR_PTR on error.
+ * parent->d_inode->i_mutex must be held. d_lookup must have verified
that no
+ * child exists while under i_mutex.
+ */
+static struct dentry *d_inode_lookup(struct dentry *parent, struct dentry
*dentry,
+ struct nameidata *nd)
+{
+ struct inode *inode = parent->d_inode;
+ struct dentry *old;
+
+ /* Don''t create child dentry for a dead directory. */
+ if (unlikely(IS_DEADDIR(inode)))
+ return ERR_PTR(-ENOENT);
+
+ old = inode->i_op->lookup(inode, dentry, nd);
+ if (unlikely(old)) {
+ dput(dentry);
+ dentry = old;
+ }
+ return dentry;
+}
+
+/*
* It''s more convoluted than I''d like it to be, but...
it''s still fairly
* small and for now I''d prefer to have fast path as straight as
possible.
* It _is_ time-critical.
@@ -1236,6 +1260,13 @@ static int do_lookup(struct nameidata *nd, struct qstr
*name,
goto unlazy;
}
}
+ if (unlikely(d_need_lookup(dentry))) {
+ if (nameidata_dentry_drop_rcu(nd, dentry))
+ return -ECHILD;
+ dput(dentry);
+ dentry = NULL;
+ goto retry;
+ }
path->mnt = mnt;
path->dentry = dentry;
if (likely(__follow_mount_rcu(nd, path, inode, false)))
@@ -1250,6 +1281,12 @@ unlazy:
}
} else {
dentry = __d_lookup(parent, name);
+ if (unlikely(!dentry))
+ goto retry;
+ if (unlikely(d_need_lookup(dentry))) {
+ dput(dentry);
+ dentry = NULL;
+ }
}
retry:
@@ -1268,6 +1305,17 @@ retry:
/* known good */
need_reval = 0;
status = 1;
+ } else if (unlikely(d_need_lookup(dentry))) {
+ struct dentry *old;
+
+ dentry = d_inode_lookup(parent, dentry, nd);
+ if (IS_ERR(dentry)) {
+ mutex_unlock(&dir->i_mutex);
+ return PTR_ERR(dentry);
+ }
+ /* known good */
+ need_reval = 0;
+ status = 1;
}
mutex_unlock(&dir->i_mutex);
}
@@ -1755,6 +1803,16 @@ static struct dentry *__lookup_hash(struct qstr *name,
*/
dentry = d_lookup(base, name);
+ if (dentry && d_need_lookup(dentry)) {
+ /*
+ * __lookup_hash is called with the parent dir''s i_mutex already
+ * held, so we are good to go here.
+ */
+ dentry = d_inode_lookup(base, dentry, nd);
+ if (IS_ERR(dentry))
+ return dentry;
+ }
+
if (dentry && (dentry->d_flags & DCACHE_OP_REVALIDATE))
dentry = do_revalidate(dentry, nd);
diff --git a/include/linux/dcache.h b/include/linux/dcache.h
index 19d90a5..5fa5bd3 100644
--- a/include/linux/dcache.h
+++ b/include/linux/dcache.h
@@ -216,6 +216,7 @@ struct dentry_operations {
#define DCACHE_MOUNTED 0x10000 /* is a mountpoint */
#define DCACHE_NEED_AUTOMOUNT 0x20000 /* handle automount on this dir */
#define DCACHE_MANAGE_TRANSIT 0x40000 /* manage transit from this dirent */
+#define DCACHE_NEED_LOOKUP 0x80000 /* dentry requires i_op->lookup */
#define DCACHE_MANAGED_DENTRY \
(DCACHE_MOUNTED|DCACHE_NEED_AUTOMOUNT|DCACHE_MANAGE_TRANSIT)
@@ -416,6 +417,12 @@ static inline bool d_mountpoint(struct dentry *dentry)
return dentry->d_flags & DCACHE_MOUNTED;
}
+static inline bool d_need_lookup(struct dentry *dentry)
+{
+ return dentry->d_flags & DCACHE_NEED_LOOKUP;
+}
+
+extern void d_clear_need_lookup(struct dentry *dentry);
extern struct dentry *lookup_create(struct nameidata *nd, int is_dir);
extern int sysctl_vfs_cache_pressure;
--
1.7.2.3
--
To unsubscribe from this list: send the line "unsubscribe
linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Josef Bacik
2011-May-20 17:44 UTC
[PATCH 2/2] Btrfs: load the key from the dir item in readdir into a fake dentry V2
In btrfs we have 2 indexes for inodes. One is for readdir, it''s in
this nice
sequential order and works out brilliantly for readdir. However if you use ls,
it usually stat''s each file it gets from readdir. This is where the
second
index comes in, which is based on a hash of the name of the file. So then the
lookup has to lookup this index, and then lookup the inode. The index lookup is
going to be in random order (since its based on the name hash), which gives us
less than stellar performance. Since we know the inode location from the
readdir index, I create a dummy dentry and copy the location key into
dentry->d_fsdata. Then on lookup if we have d_fsdata we use that location to
lookup the inode, avoiding looking up the other directory index. Thanks,
Signed-off-by: Josef Bacik <josef@redhat.com>
---
V1->V2: use the d_clear_need_lookup() helper instead of d_drop.
fs/btrfs/inode.c | 47 +++++++++++++++++++++++++++++++++++++++++++++--
1 files changed, 45 insertions(+), 2 deletions(-)
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 6228a30..2a1a028 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -4120,12 +4120,19 @@ struct inode *btrfs_lookup_dentry(struct inode *dir,
struct dentry *dentry)
struct btrfs_root *sub_root = root;
struct btrfs_key location;
int index;
- int ret;
+ int ret = 0;
if (dentry->d_name.len > BTRFS_NAME_LEN)
return ERR_PTR(-ENAMETOOLONG);
- ret = btrfs_inode_by_name(dir, dentry, &location);
+ if (unlikely(d_need_lookup(dentry))) {
+ memcpy(&location, dentry->d_fsdata, sizeof(struct btrfs_key));
+ kfree(dentry->d_fsdata);
+ dentry->d_fsdata = NULL;
+ d_clear_need_lookup(dentry);
+ } else {
+ ret = btrfs_inode_by_name(dir, dentry, &location);
+ }
if (ret < 0)
return ERR_PTR(ret);
@@ -4180,6 +4187,12 @@ static int btrfs_dentry_delete(const struct dentry
*dentry)
return 0;
}
+static void btrfs_dentry_release(struct dentry *dentry)
+{
+ if (dentry->d_fsdata)
+ kfree(dentry->d_fsdata);
+}
+
static struct dentry *btrfs_lookup(struct inode *dir, struct dentry *dentry,
struct nameidata *nd)
{
@@ -4206,6 +4219,7 @@ static int btrfs_real_readdir(struct file *filp, void
*dirent,
struct btrfs_key key;
struct btrfs_key found_key;
struct btrfs_path *path;
+ struct qstr q;
int ret;
struct extent_buffer *leaf;
int slot;
@@ -4284,6 +4298,7 @@ static int btrfs_real_readdir(struct file *filp, void
*dirent,
while (di_cur < di_total) {
struct btrfs_key location;
+ struct dentry *tmp;
if (verify_dir_item(root, leaf, di))
break;
@@ -4304,6 +4319,33 @@ static int btrfs_real_readdir(struct file *filp, void
*dirent,
d_type = btrfs_filetype_table[btrfs_dir_type(leaf, di)];
btrfs_dir_item_key_to_cpu(leaf, di, &location);
+ q.name = name_ptr;
+ q.len = name_len;
+ q.hash = full_name_hash(q.name, q.len);
+ tmp = d_lookup(filp->f_dentry, &q);
+ if (!tmp) {
+ struct btrfs_key *newkey;
+
+ newkey = kzalloc(sizeof(struct btrfs_key),
+ GFP_NOFS);
+ if (!newkey)
+ goto no_dentry;
+ tmp = d_alloc(filp->f_dentry, &q);
+ if (!tmp) {
+ kfree(newkey);
+ dput(tmp);
+ goto no_dentry;
+ }
+ memcpy(newkey, &location,
+ sizeof(struct btrfs_key));
+ tmp->d_fsdata = newkey;
+ tmp->d_flags |= DCACHE_NEED_LOOKUP;
+ d_rehash(tmp);
+ dput(tmp);
+ } else {
+ dput(tmp);
+ }
+no_dentry:
/* is this a reference to our own snapshot? If so
* skip it
*/
@@ -7566,4 +7608,5 @@ static const struct inode_operations
btrfs_symlink_inode_operations = {
const struct dentry_operations btrfs_dentry_operations = {
.d_delete = btrfs_dentry_delete,
+ .d_release = btrfs_dentry_release,
};
--
1.7.2.3
--
To unsubscribe from this list: send the line "unsubscribe
linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Al Viro
2011-May-22 02:11 UTC
Re: [PATCH 1/2] fs: add a DCACHE_NEED_LOOKUP flag for d_flags V2
On Fri, May 20, 2011 at 01:44:30PM -0400, Josef Bacik wrote:> + if (unlikely(d_need_lookup(dentry))) { > + if (nameidata_dentry_drop_rcu(nd, dentry)) > + return -ECHILD; > + dput(dentry); > + dentry = NULL; > + goto retry;Yecchhh... How about simple goto unlazy; here instead and doing the rest there? Especially since you have the same kind of thing elsewhere in the same sucker. It had been bloody painful to untangle that thing; let''s not add to the rat''s nest that still remains... -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Josef Bacik
2011-May-23 16:13 UTC
Re: [PATCH 1/2] fs: add a DCACHE_NEED_LOOKUP flag for d_flags V2
On 05/21/2011 10:11 PM, Al Viro wrote:> On Fri, May 20, 2011 at 01:44:30PM -0400, Josef Bacik wrote: >> + if (unlikely(d_need_lookup(dentry))) { >> + if (nameidata_dentry_drop_rcu(nd, dentry)) >> + return -ECHILD; >> + dput(dentry); >> + dentry = NULL; >> + goto retry; > > Yecchhh... How about simple goto unlazy; here instead and doing the rest > there? Especially since you have the same kind of thing elsewhere in the > same sucker. It had been bloody painful to untangle that thing; let''s not > add to the rat''s nest that still remains...This is where I was a little confused, which is why I added this code. It seems that having goto unlazy; will mean that we will come down to this section if (unlikely(status <= 0 )) { if (status < 0) { dput(dentry); return status; } if (!d_invalidate(dentry)) { dput(dentry); dentry = NULL; need_reval = 1; goto retry; } } and d_invalidate will unhash us so we won''t find our new dentry in the cache which makes this whole exercise useless. Is there a different way you''d like this cleaned up? Thanks, Josef -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Al Viro
2011-May-26 10:35 UTC
Re: [PATCH 1/2] fs: add a DCACHE_NEED_LOOKUP flag for d_flags V2
On Mon, May 23, 2011 at 12:13:07PM -0400, Josef Bacik wrote:> On 05/21/2011 10:11 PM, Al Viro wrote: > > On Fri, May 20, 2011 at 01:44:30PM -0400, Josef Bacik wrote: > >> + if (unlikely(d_need_lookup(dentry))) { > >> + if (nameidata_dentry_drop_rcu(nd, dentry)) > >> + return -ECHILD; > >> + dput(dentry); > >> + dentry = NULL; > >> + goto retry; > > > > Yecchhh... How about simple goto unlazy; here instead and doing the rest > > there? Especially since you have the same kind of thing elsewhere in the > > same sucker. It had been bloody painful to untangle that thing; let''s not > > add to the rat''s nest that still remains... > > This is where I was a little confused, which is why I added this code. It > seems that having goto unlazy; will mean that we will come down to this > section > > if (unlikely(status <= 0 )) { > if (status < 0) { > dput(dentry); > return status; > } > if (!d_invalidate(dentry)) { > dput(dentry); > dentry = NULL; > need_reval = 1; > goto retry; > } > } > > and d_invalidate will unhash us so we won''t find our new dentry in the cache > which makes this whole exercise useless. Is there a different way you''d > like > this cleaned up? Thanks,Not *into* the loop; just before the beginning of that loop. IOW, put if (dentry && unlikely(dentry->d_flags & DCACHE_NEED_LOOKUP)) { dput(dentry); dentry = NULL; } just before retry: instead of doing it in non-lazy branch. Voila - your code in the lazy branch becomes if (unlikely(dentry->d_flags & DCACHE_NEED_LOOKUP)) goto unlazy; and that''s it. Can you resend it with such modifications? ObMemoryPressureIssues: I really hoped to get Dave''s patch (per-sb shrinkers) in that cycle, but it''ll probably have to wait for the next one... -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html