Skip to contentSkip to conversation

fs: add a DCACHE_NEED_LOOKUP flag for d_flags

PATCH In review Josef Bacik 3 participants 7 messages May 26, 2011 1 message not in the archive

Subsystems (2)
lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected], [email protected]

#1 lore raw

PATCH 1/2 fs: add a DCACHE_NEED_LOOKUP flag for d_flags

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

Thats a savings of 1300 seconds, or 22 minutes. That is a significant savings. Thanks,

---
 fs/dcache.c            |   34 ++++++++++++++++++++++++++++++-
 fs/namei.c             |   51 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/dcache.h |    7 ++++++
 3 files changed, 90 insertions(+), 2 deletions(-)
fs/dcache.c +32 -2
index 22a0ef4..7fc0e30 100644
@@ -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.
@@ -431,8 +449,13 @@ repeat:
  	if (d_unhashed(dentry))
 		goto kill_it;
 
-	/* Otherwise leave it cached and ensure it's on the LRU */
-	dentry->d_flags |= DCACHE_REFERENCED;
+	/*
+	 * If this dentry needs lookup, don't set the referenced flag so that it
+	 * is more likely to be cleaned up by the dcache shrinker in case of
+	 * memory pressure.
+	 */
+	if (!d_need_lookup(dentry))
+		dentry->d_flags |= DCACHE_REFERENCED;
 	dentry_lru_add(dentry);
 
 	dentry->d_count--;
@@ -1703,6 +1726,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.
 	 */
fs/namei.c +51 -0

81 lines not on this page · raw message

include/linux/dcache.h +7 -0
index 19d90a5..5fa5bd3 100644
@@ -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;
Signature · 1 line
1.7.2.3
#2 lore raw

PATCH 2/2 Btrfs: load the key from the dir item in readdir into a fake dentry

In reply to a message not in the archive

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,

---
 fs/btrfs/inode.c |   47 +++++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 45 insertions(+), 2 deletions(-)
fs/btrfs/inode.c +45 -2
index 6228a30..2a1a028 100644
@@ -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,
 };
Signature · 1 line
1.7.2.3
#3 lore raw

Re: [PATCH 2/2] Btrfs: load the key from the dir item in readdir into a fake dentry

fs/btrfs/inode.c static int btrfs_real_readdir(struct file *filp, void *dirent, · line 4333

Quoted 6 lines · Josef Bacik
fs/btrfs/inode.c +6 -0
+
+				newkey = kzalloc(sizeof(struct btrfs_key),
+						 GFP_NOFS);
+				if (!newkey)
+					goto no_dentry;
+				tmp = d_alloc(filp->f_dentry, &q);

This doesn't seem to address the "find / fills all memory with dentries" concerns brought up earlier at all.

d_alloc uses a normal GFP_KERNEL, which is quite in appropiate for this.

It should at least reclaim and probably more, but even then it's risky.

-Andi

Signature · 1 line
[email protected] -- Speaking for myself only
#4 lore raw

Re: [PATCH 2/2] Btrfs: load the key from the dir item in readdir into a fake dentry

Quoted 11 lines · Andi Kleen

fs/btrfs/inode.c static int btrfs_real_readdir(struct file *filp, void *dirent, · line 4333

Quoted 6 lines · Josef Bacik
fs/btrfs/inode.c +6 -0
+
+				newkey = kzalloc(sizeof(struct btrfs_key),
+						 GFP_NOFS);
+				if (!newkey)
+					goto no_dentry;
+				tmp = d_alloc(filp->f_dentry, &q);

This doesn't seem to address the "find / fills all memory with dentries" concerns brought up earlier at all.

Nope, this part does in patch 1/1

+ /*
+ * If this dentry needs lookup, don't set the referenced flag so that it
+ * is more likely to be cleaned up by the dcache shrinker in case of
+ * memory pressure.
+ */
+ if (!d_need_lookup(dentry))
+ dentry->d_flags |= DCACHE_REFERENCED;

Quoted 5 lines

d_alloc uses a normal GFP_KERNEL, which is quite in appropiate for this.

It should at least reclaim and probably more, but even then it's risky.

Ah yeah I guess I should have probably used GFP_KERNEL. Sorry about that,

Josef

#5 lore raw

Re: [PATCH 2/2] Btrfs: load the key from the dir item in readdir into a fake dentry

Review

Quoted 7 lines · Josef Bacik
(quoted diff) +7 -0
+	/*
+	 * If this dentry needs lookup, don't set the referenced flag so that it
+	 * is more likely to be cleaned up by the dcache shrinker in case of
+	 * memory pressure.
+	 */
+	if (!d_need_lookup(dentry))
+		dentry->d_flags |= DCACHE_REFERENCED;

No it doesn't at all. The allocation will just push everything else out.

Really you cannot view this by only looking at the dcache. You have to look at the complete VM behaviour. All the caches and the other memory interact.

Quoted 9 lines
Quoted 7 lines

d_alloc uses a normal GFP_KERNEL, which is quite in appropiate for this.

It should at least reclaim and probably more, but even then it's risky.

Ah yeah I guess I should have probably used GFP_KERNEL. Sorry about that,

GFP_KERNEL is already used, but it's wrong. I'm not sure any of the existing GFP_* flags will give you the semantics you need in fact. The new flag Minchan added for readahead may come near, but even that is probably not enough.

-Andi

Signature · 1 line
[email protected] -- Speaking for myself only.
#6 lore raw

Re: [PATCH 2/2] Btrfs: load the key from the dir item in readdir into a fake dentry

Quoted 12 lines · Andi Kleen

Review

Quoted 7 lines · Josef Bacik
(quoted diff) +7 -0
+	/*
+	 * If this dentry needs lookup, don't set the referenced flag so that it
+	 * is more likely to be cleaned up by the dcache shrinker in case of
+	 * memory pressure.
+	 */
+	if (!d_need_lookup(dentry))
+		dentry->d_flags |= DCACHE_REFERENCED;

No it doesn't at all. The allocation will just push everything else out.

Quoted 3 lines

Really you cannot view this by only looking at the dcache. You have to look at the complete VM behaviour. All the caches and the other memory interact.

Agreed, but this makes it monumentally easier to push these entries out of the cache, which is the best I can do with what I have for now.

Quoted 8 lines
Quoted 8 lines
Quoted 7 lines

d_alloc uses a normal GFP_KERNEL, which is quite in appropiate for this.

It should at least reclaim and probably more, but even then it's risky.

Quoted 6 lines
Quoted 1 line

Ah yeah I guess I should have probably used GFP_KERNEL. Sorry about that,

GFP_KERNEL is already used, but it's wrong. I'm not sure any of the existing GFP_* flags will give you the semantics you need in fact. The new flag Minchan added for readahead may come near, but even that is probably not enough.

Yeah if there was a GFP_DONTTRYTOHARD I would use that but there isn't.
Maybe I'll code that up. Thanks,

Josef

#7 lore raw

Re: [PATCH 2/2] Btrfs: load the key from the dir item in readdir into a fake dentry

In reply to Andi Kleen
Quoted 12 lines · Andi Kleen

Review

Quoted 7 lines · Josef Bacik
(quoted diff) +7 -0
+	/*
+	 * If this dentry needs lookup, don't set the referenced flag so that it
+	 * is more likely to be cleaned up by the dcache shrinker in case of
+	 * memory pressure.
+	 */
+	if (!d_need_lookup(dentry))
+		dentry->d_flags |= DCACHE_REFERENCED;

No it doesn't at all. The allocation will just push everything else out.

Quoted 3 lines

Really you cannot view this by only looking at the dcache. You have to look at the complete VM behaviour. All the caches and the other memory interact.

Even without this patch, if you are doing "find /" there will be considerable memory pressure from all of the pages that are accessed by readdir(), and to a lesser extent the directory dentry/inode entries. I'm not sure whether the issue you raise is going to be significant in the end or not.

Taking this development a bit further, I've long thought it would be quite interesting if these dcache entries could be linked into a readdir-ordered linked list, and then discard the actual directory pages from cache entirely. This would potentially allow later readdir operations to work just by walking the readdir-ordered linked list and not touching the page cache at all.

Since normal operations like "ls -l" currently instantiate both a pagecache for readdir, and a dentry for each dirent, it could potentially even reduce memory if there was no a need to keep the directory pages in cache anymore.

Quoted 6 lines
Quoted 6 lines
Quoted 5 lines

d_alloc uses a normal GFP_KERNEL, which is quite in appropiate for this.

It should at least reclaim and probably more, but even then it's risky.

Quoted 15 lines
Quoted 1 line

Ah yeah I guess I should have probably used GFP_KERNEL. Sorry about that,

GFP_KERNEL is already used, but it's wrong. I'm not sure any of the existing GFP_* flags will give you the semantics you need in fact. The new flag Minchan added for readahead may come near, but even that is probably not enough.

-Andi

Signature · 5 lines
[email protected] -- Speaking for myself only.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Cheers, Andreas