[PATCH 00/10] dcache cleanups

[PATCH 00/10] dcache cleanups

[Christoph Hellwig]

A couple of dcache cleanups in preparation of the dcache_lock splitup.
Note that I already sent the first patch individually, but this series
contains a fixed version.

[PATCH 01/10] [PATCH] fs: take dcache_lock inside __d_path

[Christoph Hellwig]

[-- Attachment #1: take-dcache_lock-in-__d_path --]
[-- Type: text/plain, Size: 2855 bytes --]

All callers take dcache_lock just around the call to __d_path, so
take the lock into it in preparation of getting rid of dcache_lock.

Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: linux-2.6/fs/dcache.c
===================================================================
--- linux-2.6.orig/fs/dcache.c	2010-10-09 22:59:55.482254091 +0200
+++ linux-2.6/fs/dcache.c	2010-10-10 09:36:07.338004055 +0200
@@ -1994,7 +1994,7 @@ global_root:
  * Returns a pointer into the buffer or an error code if the
  * path was too long.
  *
- * "buflen" should be positive. Caller holds the dcache_lock.
+ * "buflen" should be positive.
  *
  * If path is not reachable from the supplied root, then the value of
  * root is changed (without modifying refcounts).
@@ -2006,10 +2006,12 @@ char *__d_path(const struct path *path,
 	int error;
 
 	prepend(&res, &buflen, "\0", 1);
+	spin_lock(&dcache_lock);
 	error = prepend_path(path, root, &res, &buflen);
+	spin_unlock(&dcache_lock);
+
 	if (error)
 		return ERR_PTR(error);
-
 	return res;
 }
 
Index: linux-2.6/fs/seq_file.c
===================================================================
--- linux-2.6.orig/fs/seq_file.c	2010-10-09 22:59:55.490010272 +0200
+++ linux-2.6/fs/seq_file.c	2010-10-09 23:00:38.952004057 +0200
@@ -462,9 +462,7 @@ int seq_path_root(struct seq_file *m, st
 	if (size) {
 		char *p;
 
-		spin_lock(&dcache_lock);
 		p = __d_path(path, root, buf, size);
-		spin_unlock(&dcache_lock);
 		res = PTR_ERR(p);
 		if (!IS_ERR(p)) {
 			char *end = mangle_path(buf, p, esc);
Index: linux-2.6/security/apparmor/path.c
===================================================================
--- linux-2.6.orig/security/apparmor/path.c	2010-10-09 22:59:55.500012996 +0200
+++ linux-2.6/security/apparmor/path.c	2010-10-09 23:00:38.958004057 +0200
@@ -72,10 +72,8 @@ static int d_namespace_path(struct path
 		path_get(&root);
 	}
 
-	spin_lock(&dcache_lock);
 	tmp = root;
 	res = __d_path(path, &tmp, buf, buflen);
-	spin_unlock(&dcache_lock);
 
 	*name = res;
 	/* handle error conditions - and still allow a partial path to
Index: linux-2.6/security/tomoyo/realpath.c
===================================================================
--- linux-2.6.orig/security/tomoyo/realpath.c	2010-10-09 22:59:55.518025777 +0200
+++ linux-2.6/security/tomoyo/realpath.c	2010-10-09 23:00:38.961023403 +0200
@@ -127,10 +127,8 @@ char *tomoyo_realpath_from_path(struct p
 		/* If we don't have a vfsmount, we can't calculate. */
 		if (!path->mnt)
 			break;
-		spin_lock(&dcache_lock);
 		/* go to whatever namespace root we are under */
 		pos = __d_path(path, &ns_root, buf, buf_len);
-		spin_unlock(&dcache_lock);
 		/* Prepend "/proc" prefix if using internal proc vfs mount. */
 		if (!IS_ERR(pos) && (path->mnt->mnt_flags & MNT_INTERNAL) &&
 		    (path->mnt->mnt_sb->s_magic == PROC_SUPER_MAGIC)) {

[PATCH 02/10] fs: simplify __d_free

[Christoph Hellwig]

[-- Attachment #1: cleanup-d_free --]
[-- Type: text/plain, Size: 1348 bytes --]

Remove d_callback and always call __d_free with a RCU head.

Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: linux-2.6/fs/dcache.c
===================================================================
--- linux-2.6.orig/fs/dcache.c	2010-10-09 10:20:46.650004474 +0200
+++ linux-2.6/fs/dcache.c	2010-10-09 10:24:36.806004683 +0200
@@ -67,20 +67,16 @@ struct dentry_stat_t dentry_stat = {
 	.age_limit = 45,
 };
 
-static void __d_free(struct dentry *dentry)
+static void __d_free(struct rcu_head *head)
 {
+	struct dentry *dentry = container_of(head, struct dentry, d_u.d_rcu);
+
 	WARN_ON(!list_empty(&dentry->d_alias));
 	if (dname_external(dentry))
 		kfree(dentry->d_name.name);
 	kmem_cache_free(dentry_cache, dentry); 
 }
 
-static void d_callback(struct rcu_head *head)
-{
-	struct dentry * dentry = container_of(head, struct dentry, d_u.d_rcu);
-	__d_free(dentry);
-}
-
 /*
  * no dcache_lock, please.  The caller must decrement dentry_stat.nr_dentry
  * inside dcache_lock.
@@ -91,9 +87,9 @@ static void d_free(struct dentry *dentry
 		dentry->d_op->d_release(dentry);
 	/* if dentry was never inserted into hash, immediate free is OK */
 	if (hlist_unhashed(&dentry->d_hash))
-		__d_free(dentry);
+		__d_free(&dentry->d_u.d_rcu);
 	else
-		call_rcu(&dentry->d_u.d_rcu, d_callback);
+		call_rcu(&dentry->d_u.d_rcu, __d_free);
 }
 
 /*

[PATCH 03/10] fs: use percpu counter for nr_dentry and nr_dentry_unused

[Christoph Hellwig]

[-- Attachment #1: nr_dentries-percpu --]
[-- Type: text/plain, Size: 6489 bytes --]

The nr_dentry stat is a globally touched cacheline and atomic operation
twice over the lifetime of a dentry. It is used for the benfit of userspace
only. Turn it into a per-cpu counter and always decrement it in d_free instead
of doing various batching operations to reduce lock hold times in the callers.

Based on an earlier patch from Nick Piggin <npiggin@suse.de>.

Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: linux-2.6/fs/dcache.c
===================================================================
--- linux-2.6.orig/fs/dcache.c	2010-10-10 09:36:28.918003777 +0200
+++ linux-2.6/fs/dcache.c	2010-10-10 09:36:31.325003987 +0200
@@ -67,6 +67,19 @@ struct dentry_stat_t dentry_stat = {
 	.age_limit = 45,
 };
 
+static struct percpu_counter nr_dentry __cacheline_aligned_in_smp;
+static struct percpu_counter nr_dentry_unused __cacheline_aligned_in_smp;
+
+#if defined(CONFIG_SYSCTL) && defined(CONFIG_PROC_FS)
+int proc_nr_dentry(ctl_table *table, int write, void __user *buffer,
+		   size_t *lenp, loff_t *ppos)
+{
+	dentry_stat.nr_dentry = percpu_counter_sum_positive(&nr_dentry);
+	dentry_stat.nr_unused = percpu_counter_sum_positive(&nr_dentry_unused);
+	return proc_dointvec(table, write, buffer, lenp, ppos);
+}
+#endif
+
 static void __d_free(struct rcu_head *head)
 {
 	struct dentry *dentry = container_of(head, struct dentry, d_u.d_rcu);
@@ -78,13 +91,14 @@ static void __d_free(struct rcu_head *he
 }
 
 /*
- * no dcache_lock, please.  The caller must decrement dentry_stat.nr_dentry
- * inside dcache_lock.
+ * no dcache_lock, please.
  */
 static void d_free(struct dentry *dentry)
 {
+	percpu_counter_dec(&nr_dentry);
 	if (dentry->d_op && dentry->d_op->d_release)
 		dentry->d_op->d_release(dentry);
+
 	/* if dentry was never inserted into hash, immediate free is OK */
 	if (hlist_unhashed(&dentry->d_hash))
 		__d_free(&dentry->d_u.d_rcu);
@@ -125,14 +139,14 @@ static void dentry_lru_add(struct dentry
 {
 	list_add(&dentry->d_lru, &dentry->d_sb->s_dentry_lru);
 	dentry->d_sb->s_nr_dentry_unused++;
-	dentry_stat.nr_unused++;
+	percpu_counter_inc(&nr_dentry_unused);
 }
 
 static void dentry_lru_add_tail(struct dentry *dentry)
 {
 	list_add_tail(&dentry->d_lru, &dentry->d_sb->s_dentry_lru);
 	dentry->d_sb->s_nr_dentry_unused++;
-	dentry_stat.nr_unused++;
+	percpu_counter_inc(&nr_dentry_unused);
 }
 
 static void dentry_lru_del(struct dentry *dentry)
@@ -140,7 +154,7 @@ static void dentry_lru_del(struct dentry
 	if (!list_empty(&dentry->d_lru)) {
 		list_del(&dentry->d_lru);
 		dentry->d_sb->s_nr_dentry_unused--;
-		dentry_stat.nr_unused--;
+		percpu_counter_dec(&nr_dentry_unused);
 	}
 }
 
@@ -149,7 +163,7 @@ static void dentry_lru_del_init(struct d
 	if (likely(!list_empty(&dentry->d_lru))) {
 		list_del_init(&dentry->d_lru);
 		dentry->d_sb->s_nr_dentry_unused--;
-		dentry_stat.nr_unused--;
+		percpu_counter_dec(&nr_dentry_unused);
 	}
 }
 
@@ -168,7 +182,6 @@ static struct dentry *d_kill(struct dent
 	struct dentry *parent;
 
 	list_del(&dentry->d_u.d_child);
-	dentry_stat.nr_dentry--;	/* For d_free, below */
 	/*drops the locks, at that point nobody can reach this dentry */
 	dentry_iput(dentry);
 	if (IS_ROOT(dentry))
@@ -314,7 +327,6 @@ int d_invalidate(struct dentry * dentry)
 EXPORT_SYMBOL(d_invalidate);
 
 /* This should be called _only_ with dcache_lock held */
-
 static inline struct dentry * __dget_locked(struct dentry *dentry)
 {
 	atomic_inc(&dentry->d_count);
@@ -534,7 +546,7 @@ static void prune_dcache(int count)
 {
 	struct super_block *sb, *p = NULL;
 	int w_count;
-	int unused = dentry_stat.nr_unused;
+	int unused = percpu_counter_sum_positive(&nr_dentry_unused);
 	int prune_ratio;
 	int pruned;
 
@@ -699,20 +711,13 @@ static void shrink_dcache_for_umount_sub
 			 * otherwise we ascend to the parent and move to the
 			 * next sibling if there is one */
 			if (!parent)
-				goto out;
-
+				return;
 			dentry = parent;
-
 		} while (list_empty(&dentry->d_subdirs));
 
 		dentry = list_entry(dentry->d_subdirs.next,
 				    struct dentry, d_u.d_child);
 	}
-out:
-	/* several dentries were freed, need to correct nr_dentry */
-	spin_lock(&dcache_lock);
-	dentry_stat.nr_dentry -= detached;
-	spin_unlock(&dcache_lock);
 }
 
 /*
@@ -896,12 +901,16 @@ EXPORT_SYMBOL(shrink_dcache_parent);
  */
 static int shrink_dcache_memory(struct shrinker *shrink, int nr, gfp_t gfp_mask)
 {
+	int nr_unused;
+
 	if (nr) {
 		if (!(gfp_mask & __GFP_FS))
 			return -1;
 		prune_dcache(nr);
 	}
-	return (dentry_stat.nr_unused / 100) * sysctl_vfs_cache_pressure;
+
+	nr_unused = percpu_counter_sum_positive(&nr_dentry_unused);
+	return (nr_unused / 100) * sysctl_vfs_cache_pressure;
 }
 
 static struct shrinker dcache_shrinker = {
@@ -968,9 +977,10 @@ struct dentry *d_alloc(struct dentry * p
 	spin_lock(&dcache_lock);
 	if (parent)
 		list_add(&dentry->d_u.d_child, &parent->d_subdirs);
-	dentry_stat.nr_dentry++;
 	spin_unlock(&dcache_lock);
 
+	percpu_counter_inc(&nr_dentry);
+
 	return dentry;
 }
 EXPORT_SYMBOL(d_alloc);
@@ -2417,6 +2427,9 @@ static void __init dcache_init(void)
 {
 	int loop;
 
+	percpu_counter_init(&nr_dentry, 0);
+	percpu_counter_init(&nr_dentry_unused, 0);
+
 	/* 
 	 * A constructor could be added for stable state like the lists,
 	 * but it is probably not worth it because of the cache nature
Index: linux-2.6/include/linux/fs.h
===================================================================
--- linux-2.6.orig/include/linux/fs.h	2010-10-10 09:36:05.400003777 +0200
+++ linux-2.6/include/linux/fs.h	2010-10-10 09:36:31.326004057 +0200
@@ -2483,6 +2483,8 @@ ssize_t simple_attr_write(struct file *f
 struct ctl_table;
 int proc_nr_files(struct ctl_table *table, int write,
 		  void __user *buffer, size_t *lenp, loff_t *ppos);
+int proc_nr_dentry(struct ctl_table *table, int write,
+		  void __user *buffer, size_t *lenp, loff_t *ppos);
 int proc_nr_inodes(struct ctl_table *table, int write,
 		   void __user *buffer, size_t *lenp, loff_t *ppos);
 int __init get_filesystem_list(char *buf);
Index: linux-2.6/kernel/sysctl.c
===================================================================
--- linux-2.6.orig/kernel/sysctl.c	2010-10-10 09:36:05.412003707 +0200
+++ linux-2.6/kernel/sysctl.c	2010-10-10 09:36:31.331004057 +0200
@@ -1377,7 +1377,7 @@ static struct ctl_table fs_table[] = {
 		.data		= &dentry_stat,
 		.maxlen		= 6*sizeof(int),
 		.mode		= 0444,
-		.proc_handler	= proc_dointvec,
+		.proc_handler	= proc_nr_dentry,
 	},
 	{
 		.procname	= "overflowuid",

[PATCH 04/10] fs: improve DCACHE_REFERENCED usage

[Christoph Hellwig]

[-- Attachment #1: dcache-improve-DCACHE_REFERENCE --]
[-- Type: text/plain, Size: 1075 bytes --]

From: Nick Piggin <npiggin@suse.de>

dentry referenced bit is only set when installing the dentry back
onto the LRU. However with lazy LRU, the dentry can already be on
the LRU list at dput time, thus missing out on setting the referenced
bit. Fix this.

Signed-off-by: Nick Piggin <npiggin@suse.de>
Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: linux-2.6/fs/dcache.c
===================================================================
--- linux-2.6.orig/fs/dcache.c	2010-10-09 10:26:11.000000000 +0200
+++ linux-2.6/fs/dcache.c	2010-10-09 10:29:04.611003707 +0200
@@ -246,13 +246,16 @@ repeat:
 		if (dentry->d_op->d_delete(dentry))
 			goto unhash_it;
 	}
+
 	/* Unreachable? Get rid of it */
  	if (d_unhashed(dentry))
 		goto kill_it;
-  	if (list_empty(&dentry->d_lru)) {
-  		dentry->d_flags |= DCACHE_REFERENCED;
+
+	/* Otherwise leave it cached and ensure it's on the LRU */
+	dentry->d_flags |= DCACHE_REFERENCED;
+  	if (list_empty(&dentry->d_lru))
 		dentry_lru_add(dentry);
-  	}
+
  	spin_unlock(&dentry->d_lock);
 	spin_unlock(&dcache_lock);
 	return;

[PATCH 05/10] fs: split __shrink_dcache_sb

[Christoph Hellwig]

[-- Attachment #1: dcache-split-__shrink_dcache_sb --]
[-- Type: text/plain, Size: 5342 bytes --]

Currently __shrink_dcache_sb has an extremly awkward calling convention
because it tries to please very different callers.  Split out the
main loop into a shrink_dentry_list helper, which gets called directly
from shrink_dcache_sb for the cases where all dentries need to be pruned,
or from __shrink_dcache_sb for pruning only a certain number of dentries.

Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: linux-2.6/fs/dcache.c
===================================================================
--- linux-2.6.orig/fs/dcache.c	2010-10-09 23:00:40.729003636 +0200
+++ linux-2.6/fs/dcache.c	2010-10-09 23:02:16.223005663 +0200
@@ -459,66 +459,20 @@ static void prune_one_dentry(struct dent
 	}
 }
 
-/*
- * Shrink the dentry LRU on a given superblock.
- * @sb   : superblock to shrink dentry LRU.
- * @count: If count is NULL, we prune all dentries on superblock.
- * @flags: If flags is non-zero, we need to do special processing based on
- * which flags are set. This means we don't need to maintain multiple
- * similar copies of this loop.
- */
-static void __shrink_dcache_sb(struct super_block *sb, int *count, int flags)
+static void shrink_dentry_list(struct list_head *list)
 {
-	LIST_HEAD(referenced);
-	LIST_HEAD(tmp);
 	struct dentry *dentry;
-	int cnt = 0;
-
-	BUG_ON(!sb);
-	BUG_ON((flags & DCACHE_REFERENCED) && count == NULL);
-	spin_lock(&dcache_lock);
-	if (count != NULL)
-		/* called from prune_dcache() and shrink_dcache_parent() */
-		cnt = *count;
-restart:
-	if (count == NULL)
-		list_splice_init(&sb->s_dentry_lru, &tmp);
-	else {
-		while (!list_empty(&sb->s_dentry_lru)) {
-			dentry = list_entry(sb->s_dentry_lru.prev,
-					struct dentry, d_lru);
-			BUG_ON(dentry->d_sb != sb);
 
-			spin_lock(&dentry->d_lock);
-			/*
-			 * If we are honouring the DCACHE_REFERENCED flag and
-			 * the dentry has this flag set, don't free it. Clear
-			 * the flag and put it back on the LRU.
-			 */
-			if ((flags & DCACHE_REFERENCED)
-				&& (dentry->d_flags & DCACHE_REFERENCED)) {
-				dentry->d_flags &= ~DCACHE_REFERENCED;
-				list_move(&dentry->d_lru, &referenced);
-				spin_unlock(&dentry->d_lock);
-			} else {
-				list_move_tail(&dentry->d_lru, &tmp);
-				spin_unlock(&dentry->d_lock);
-				cnt--;
-				if (!cnt)
-					break;
-			}
-			cond_resched_lock(&dcache_lock);
-		}
-	}
-	while (!list_empty(&tmp)) {
-		dentry = list_entry(tmp.prev, struct dentry, d_lru);
+	while (!list_empty(list)) {
+		dentry = list_entry(list->prev, struct dentry, d_lru);
 		dentry_lru_del_init(dentry);
-		spin_lock(&dentry->d_lock);
+
 		/*
 		 * We found an inuse dentry which was not removed from
 		 * the LRU because of laziness during lookup.  Do not free
 		 * it - just keep it off the LRU list.
 		 */
+		spin_lock(&dentry->d_lock);
 		if (atomic_read(&dentry->d_count)) {
 			spin_unlock(&dentry->d_lock);
 			continue;
@@ -527,13 +481,60 @@ restart:
 		/* dentry->d_lock was dropped in prune_one_dentry() */
 		cond_resched_lock(&dcache_lock);
 	}
-	if (count == NULL && !list_empty(&sb->s_dentry_lru))
-		goto restart;
-	if (count != NULL)
-		*count = cnt;
+}
+
+/**
+ * __shrink_dcache_sb - shrink the dentry LRU on a given superblock
+ * @sb:		superblock to shrink dentry LRU.
+ * @count:	number of entries to prune
+ * @flags:	flags to control the dentry processing
+ *
+ * If flags contains DCACHE_REFERENCED reference dentries will not be pruned.
+ */
+static void __shrink_dcache_sb(struct super_block *sb, int *count, int flags)
+{
+	/* called from prune_dcache() and shrink_dcache_parent() */
+	struct dentry *dentry;
+	LIST_HEAD(referenced);
+	LIST_HEAD(tmp);
+	int cnt = *count;
+
+	spin_lock(&dcache_lock);
+	while (!list_empty(&sb->s_dentry_lru)) {
+		dentry = list_entry(sb->s_dentry_lru.prev,
+				struct dentry, d_lru);
+		BUG_ON(dentry->d_sb != sb);
+
+		/*
+		 * If we are honouring the DCACHE_REFERENCED flag and the
+		 * dentry has this flag set, don't free it.  Clear the flag
+		 * and put it back on the LRU.
+		 */
+		if (flags & DCACHE_REFERENCED) {
+			spin_lock(&dentry->d_lock);
+			if (dentry->d_flags & DCACHE_REFERENCED) {
+				dentry->d_flags &= ~DCACHE_REFERENCED;
+				list_move(&dentry->d_lru, &referenced);
+				spin_unlock(&dentry->d_lock);
+				cond_resched_lock(&dcache_lock);
+				continue;
+			}
+			spin_unlock(&dentry->d_lock);
+		}
+
+		list_move_tail(&dentry->d_lru, &tmp);
+		if (!--cnt)
+			break;
+		cond_resched_lock(&dcache_lock);
+	}
+
+	*count = cnt;
+	shrink_dentry_list(&tmp);
+
 	if (!list_empty(&referenced))
 		list_splice(&referenced, &sb->s_dentry_lru);
 	spin_unlock(&dcache_lock);
+
 }
 
 /**
@@ -619,13 +620,19 @@ static void prune_dcache(int count)
  * shrink_dcache_sb - shrink dcache for a superblock
  * @sb: superblock
  *
- * Shrink the dcache for the specified super block. This
- * is used to free the dcache before unmounting a file
- * system
+ * Shrink the dcache for the specified super block. This is used to free
+ * the dcache before unmounting a file system.
  */
-void shrink_dcache_sb(struct super_block * sb)
+void shrink_dcache_sb(struct super_block *sb)
 {
-	__shrink_dcache_sb(sb, NULL, 0);
+	LIST_HEAD(tmp);
+
+	spin_lock(&dcache_lock);
+	while (!list_empty(&sb->s_dentry_lru)) {
+		list_splice_init(&sb->s_dentry_lru, &tmp);
+		shrink_dentry_list(&tmp);
+	}
+	spin_unlock(&dcache_lock);
 }
 EXPORT_SYMBOL(shrink_dcache_sb);
 

[PATCH 06/10] fs: clean up dentry lru modification

[Christoph Hellwig]

[-- Attachment #1: dcache-simplify-lru --]
[-- Type: text/plain, Size: 4379 bytes --]

Always do a list_del_init on the LRU to make sure the list_empty invariant for
not beeing on the LRU always holds true, and fold dentry_lru_del_init into
dentry_lru_del.  Replace the dentry_lru_add_tail primitive with a
dentry_lru_move_tail operations that simpler when the dentry already is one
the list, which is always is.  Move the list_empty into dentry_lru_add to
fit the scheme of the other lru helpers, and simplify locking once we
move to a separate LRU lock.

Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: linux-2.6/fs/dcache.c
===================================================================
--- linux-2.6.orig/fs/dcache.c	2010-10-09 23:02:16.223005663 +0200
+++ linux-2.6/fs/dcache.c	2010-10-10 09:35:59.292254021 +0200
@@ -133,37 +133,34 @@ static void dentry_iput(struct dentry *
 }
 
 /*
- * dentry_lru_(add|add_tail|del|del_init) must be called with dcache_lock held.
+ * dentry_lru_(add|del|move_tail) must be called with dcache_lock held.
  */
 static void dentry_lru_add(struct dentry *dentry)
 {
-	list_add(&dentry->d_lru, &dentry->d_sb->s_dentry_lru);
-	dentry->d_sb->s_nr_dentry_unused++;
-	percpu_counter_inc(&nr_dentry_unused);
-}
-
-static void dentry_lru_add_tail(struct dentry *dentry)
-{
-	list_add_tail(&dentry->d_lru, &dentry->d_sb->s_dentry_lru);
-	dentry->d_sb->s_nr_dentry_unused++;
-	percpu_counter_inc(&nr_dentry_unused);
+  	if (list_empty(&dentry->d_lru)) {
+		list_add(&dentry->d_lru, &dentry->d_sb->s_dentry_lru);
+		dentry->d_sb->s_nr_dentry_unused++;
+		percpu_counter_inc(&nr_dentry_unused);
+	}
 }
 
 static void dentry_lru_del(struct dentry *dentry)
 {
 	if (!list_empty(&dentry->d_lru)) {
-		list_del(&dentry->d_lru);
+		list_del_init(&dentry->d_lru);
 		dentry->d_sb->s_nr_dentry_unused--;
 		percpu_counter_dec(&nr_dentry_unused);
 	}
 }
 
-static void dentry_lru_del_init(struct dentry *dentry)
+static void dentry_lru_move_tail(struct dentry *dentry)
 {
-	if (likely(!list_empty(&dentry->d_lru))) {
-		list_del_init(&dentry->d_lru);
-		dentry->d_sb->s_nr_dentry_unused--;
-		percpu_counter_dec(&nr_dentry_unused);
+	if (list_empty(&dentry->d_lru)) {
+		list_add_tail(&dentry->d_lru, &dentry->d_sb->s_dentry_lru);
+		dentry->d_sb->s_nr_dentry_unused++;
+		percpu_counter_inc(&nr_dentry_unused);
+	} else {
+		list_move_tail(&dentry->d_lru, &dentry->d_sb->s_dentry_lru);
 	}
 }
 
@@ -253,8 +250,7 @@ repeat:
 
 	/* Otherwise leave it cached and ensure it's on the LRU */
 	dentry->d_flags |= DCACHE_REFERENCED;
-  	if (list_empty(&dentry->d_lru))
-		dentry_lru_add(dentry);
+	dentry_lru_add(dentry);
 
  	spin_unlock(&dentry->d_lock);
 	spin_unlock(&dcache_lock);
@@ -333,7 +329,7 @@ EXPORT_SYMBOL(d_invalidate);
 static inline struct dentry * __dget_locked(struct dentry *dentry)
 {
 	atomic_inc(&dentry->d_count);
-	dentry_lru_del_init(dentry);
+	dentry_lru_del(dentry);
 	return dentry;
 }
 
@@ -452,7 +448,7 @@ static void prune_one_dentry(struct dent
 
 		if (dentry->d_op && dentry->d_op->d_delete)
 			dentry->d_op->d_delete(dentry);
-		dentry_lru_del_init(dentry);
+		dentry_lru_del(dentry);
 		__d_drop(dentry);
 		dentry = d_kill(dentry);
 		spin_lock(&dcache_lock);
@@ -465,7 +461,7 @@ static void shrink_dentry_list(struct li
 
 	while (!list_empty(list)) {
 		dentry = list_entry(list->prev, struct dentry, d_lru);
-		dentry_lru_del_init(dentry);
+		dentry_lru_del(dentry);
 
 		/*
 		 * We found an inuse dentry which was not removed from
@@ -650,7 +646,7 @@ static void shrink_dcache_for_umount_sub
 
 	/* detach this root from the system */
 	spin_lock(&dcache_lock);
-	dentry_lru_del_init(dentry);
+	dentry_lru_del(dentry);
 	__d_drop(dentry);
 	spin_unlock(&dcache_lock);
 
@@ -664,7 +660,7 @@ static void shrink_dcache_for_umount_sub
 			spin_lock(&dcache_lock);
 			list_for_each_entry(loop, &dentry->d_subdirs,
 					    d_u.d_child) {
-				dentry_lru_del_init(loop);
+				dentry_lru_del(loop);
 				__d_drop(loop);
 				cond_resched_lock(&dcache_lock);
 			}
@@ -841,14 +837,15 @@ resume:
 		struct dentry *dentry = list_entry(tmp, struct dentry, d_u.d_child);
 		next = tmp->next;
 
-		dentry_lru_del_init(dentry);
 		/* 
 		 * move only zero ref count dentries to the end 
 		 * of the unused list for prune_dcache
 		 */
 		if (!atomic_read(&dentry->d_count)) {
-			dentry_lru_add_tail(dentry);
+			dentry_lru_move_tail(dentry);
 			found++;
+		} else {
+			dentry_lru_del(dentry);
 		}
 
 		/*

[PATCH 07/10] fs: use RCU read side protection in d_validate

[Christoph Hellwig]

[-- Attachment #1: dcache-d_validate-use-rcu --]
[-- Type: text/plain, Size: 1678 bytes --]

d_validate does a purely read lookup in the dentry hash, so use RCU read side
locking instead of dcache_lock.  Split out from a larget patch by
Nick Piggin <npiggin@suse.de>.

Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: linux-2.6/fs/dcache.c
===================================================================
--- linux-2.6.orig/fs/dcache.c	2010-10-09 23:02:58.688004125 +0200
+++ linux-2.6/fs/dcache.c	2010-10-09 23:05:04.988254510 +0200
@@ -1491,33 +1491,26 @@ out:
  * This is used by ncpfs in its readdir implementation.
  * Zero is returned in the dentry is invalid.
  */
- 
-int d_validate(struct dentry *dentry, struct dentry *dparent)
+int d_validate(struct dentry *dentry, struct dentry *parent)
 {
-	struct hlist_head *base;
-	struct hlist_node *lhp;
+	struct hlist_head *head = d_hash(parent, dentry->d_name.hash);
+	struct hlist_node *node;
+	struct dentry *d;
 
 	/* Check whether the ptr might be valid at all.. */
 	if (!kmem_ptr_validate(dentry_cache, dentry))
-		goto out;
+		return 0;
+	if (dentry->d_parent != parent)
+		return 0;
 
-	if (dentry->d_parent != dparent)
-		goto out;
-
-	spin_lock(&dcache_lock);
-	base = d_hash(dparent, dentry->d_name.hash);
-	hlist_for_each(lhp,base) { 
-		/* hlist_for_each_entry_rcu() not required for d_hash list
-		 * as it is parsed under dcache_lock
-		 */
-		if (dentry == hlist_entry(lhp, struct dentry, d_hash)) {
-			__dget_locked(dentry);
-			spin_unlock(&dcache_lock);
+	rcu_read_lock();
+	hlist_for_each_entry_rcu(d, node, head, d_hash) {
+		if (d == dentry) {
+			dget(dentry);
 			return 1;
 		}
 	}
-	spin_unlock(&dcache_lock);
-out:
+ 	rcu_read_unlock();
 	return 0;
 }
 EXPORT_SYMBOL(d_validate);

[PATCH 08/10] exportfs: use dget_parent

[Christoph Hellwig]

[-- Attachment #1: exportfs-use-dget_parent --]
[-- Type: text/plain, Size: 1178 bytes --]

Use dget_parent instead of opencoding it.  This simplifies the code, but
more importanly prepares for the more complicated locking for a parent
dget in the dcache scale patch series.

Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: linux-2.6/fs/exportfs/expfs.c
===================================================================
--- linux-2.6.orig/fs/exportfs/expfs.c	2010-10-10 09:51:24.580004056 +0200
+++ linux-2.6/fs/exportfs/expfs.c	2010-10-10 09:52:34.490016907 +0200
@@ -74,21 +74,20 @@ static struct dentry *
 find_disconnected_root(struct dentry *dentry)
 {
 	dget(dentry);
-	spin_lock(&dentry->d_lock);
-	while (!IS_ROOT(dentry) &&
-	       (dentry->d_parent->d_flags & DCACHE_DISCONNECTED)) {
-		struct dentry *parent = dentry->d_parent;
-		dget(parent);
-		spin_unlock(&dentry->d_lock);
+	while (!IS_ROOT(dentry)) {
+		struct dentry *parent = dget_parent(dentry);
+	       
+		if (parent->d_flags & DCACHE_DISCONNECTED) {
+			dput(parent);
+			break;
+		}
+
 		dput(dentry);
 		dentry = parent;
-		spin_lock(&dentry->d_lock);
 	}
-	spin_unlock(&dentry->d_lock);
 	return dentry;
 }
 
-
 /*
  * Make sure target_dir is fully connected to the dentry tree.
  *

[PATCH 09/10] smbfs: use dget_parent

[Christoph Hellwig]

[-- Attachment #1: smbfs-use-dget_parent --]
[-- Type: text/plain, Size: 2323 bytes --]

Use dget_parent instead of opencoding it.  This simplifies the code, but
more importanly prepares for the more complicated locking for a parent
dget in the dcache scale patch series.

Note that the d_time assignment in smb_renew_times moves out of d_lock,
but it's a single atomic 32-bit value, and that's what other sites
setting it do already.

Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: linux-2.6/fs/smbfs/dir.c
===================================================================
--- linux-2.6.orig/fs/smbfs/dir.c	2010-10-09 12:58:31.000263030 +0200
+++ linux-2.6/fs/smbfs/dir.c	2010-10-09 13:03:42.478255626 +0200
@@ -406,21 +406,15 @@ void
 smb_renew_times(struct dentry * dentry)
 {
 	dget(dentry);
-	spin_lock(&dentry->d_lock);
-	for (;;) {
-		struct dentry *parent;
+	dentry->d_time = jiffies;
 
-		dentry->d_time = jiffies;
-		if (IS_ROOT(dentry))
-			break;
-		parent = dentry->d_parent;
-		dget(parent);
-		spin_unlock(&dentry->d_lock);
+	while (!IS_ROOT(dentry)) {
+		struct dentry *parent = dget_parent(dentry);
 		dput(dentry);
 		dentry = parent;
-		spin_lock(&dentry->d_lock);
+
+		dentry->d_time = jiffies;
 	}
-	spin_unlock(&dentry->d_lock);
 	dput(dentry);
 }
 
Index: linux-2.6/fs/smbfs/proc.c
===================================================================
--- linux-2.6.orig/fs/smbfs/proc.c	2010-10-09 12:58:31.008254859 +0200
+++ linux-2.6/fs/smbfs/proc.c	2010-10-09 13:02:14.917005452 +0200
@@ -332,16 +332,15 @@ static int smb_build_path(struct smb_sb_
 	 * and store it in reversed order [see reverse_string()]
 	 */
 	dget(entry);
-	spin_lock(&entry->d_lock);
 	while (!IS_ROOT(entry)) {
 		struct dentry *parent;
 
 		if (maxlen < (3<<unicode)) {
-			spin_unlock(&entry->d_lock);
 			dput(entry);
 			return -ENAMETOOLONG;
 		}
 
+		spin_lock(&entry->d_lock);
 		len = server->ops->convert(path, maxlen-2, 
 				      entry->d_name.name, entry->d_name.len,
 				      server->local_nls, server->remote_nls);
@@ -359,15 +358,12 @@ static int smb_build_path(struct smb_sb_
 		}
 		*path++ = '\\';
 		maxlen -= len+1;
-
-		parent = entry->d_parent;
-		dget(parent);
 		spin_unlock(&entry->d_lock);
+
+		parent = dget_parent(entry);
 		dput(entry);
 		entry = parent;
-		spin_lock(&entry->d_lock);
 	}
-	spin_unlock(&entry->d_lock);
 	dput(entry);
 	reverse_string(buf, path-buf);
 

[PATCH 10/10] fsnotify: use dget_parent

[Christoph Hellwig]

[-- Attachment #1: fsnotify-use-dget_parent --]
[-- Type: text/plain, Size: 2375 bytes --]

Use dget_parent instead of opencoding it.  This simplifies the code, but
more importanly prepares for the more complicated locking for a parent
dget in the dcache scale patch series.

It means we do grab a reference to the parent now if need to be watched,
but not with the specified mask.  If this turns out to be a problem
we'll have to revisit it, but for now let's keep as much as possible
dcache internals inside dcache.[ch].

Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: linux-2.6/fs/notify/fsnotify.c
===================================================================
--- linux-2.6.orig/fs/notify/fsnotify.c	2010-10-10 09:35:58.488004195 +0200
+++ linux-2.6/fs/notify/fsnotify.c	2010-10-10 09:53:53.103010272 +0200
@@ -88,8 +88,6 @@ void __fsnotify_parent(struct path *path
 {
 	struct dentry *parent;
 	struct inode *p_inode;
-	bool send = false;
-	bool should_update_children = false;
 
 	if (!dentry)
 		dentry = path->dentry;
@@ -97,29 +95,12 @@ void __fsnotify_parent(struct path *path
 	if (!(dentry->d_flags & DCACHE_FSNOTIFY_PARENT_WATCHED))
 		return;
 
-	spin_lock(&dentry->d_lock);
-	parent = dentry->d_parent;
+	parent = dget_parent(dentry);
 	p_inode = parent->d_inode;
 
-	if (fsnotify_inode_watches_children(p_inode)) {
-		if (p_inode->i_fsnotify_mask & mask) {
-			dget(parent);
-			send = true;
-		}
-	} else {
-		/*
-		 * The parent doesn't care about events on it's children but
-		 * at least one child thought it did.  We need to run all the
-		 * children and update their d_flags to let them know p_inode
-		 * doesn't care about them any more.
-		 */
-		dget(parent);
-		should_update_children = true;
-	}
-
-	spin_unlock(&dentry->d_lock);
-
-	if (send) {
+	if (unlikely(!fsnotify_inode_watches_children(p_inode)))
+		__fsnotify_update_child_dentry_flags(p_inode);
+	else if (p_inode->i_fsnotify_mask & mask) {
 		/* we are notifying a parent so come up with the new mask which
 		 * specifies these are events which came from a child. */
 		mask |= FS_EVENT_ON_CHILD;
@@ -130,13 +111,9 @@ void __fsnotify_parent(struct path *path
 		else
 			fsnotify(p_inode, mask, dentry->d_inode, FSNOTIFY_EVENT_INODE,
 				 dentry->d_name.name, 0);
-		dput(parent);
 	}
 
-	if (unlikely(should_update_children)) {
-		__fsnotify_update_child_dentry_flags(p_inode);
-		dput(parent);
-	}
+	dput(parent);
 }
 EXPORT_SYMBOL_GPL(__fsnotify_parent);
 

Re: [PATCH 10/10] fsnotify: use dget_parent

[Eric Paris]

On Sun, 2010-10-10 at 05:36 -0400, Christoph Hellwig wrote:
> plain text document attachment (fsnotify-use-dget_parent)
> Use dget_parent instead of opencoding it.  This simplifies the code, but
> more importanly prepares for the more complicated locking for a parent
> dget in the dcache scale patch series.
> 
> It means we do grab a reference to the parent now if need to be watched,
> but not with the specified mask.  If this turns out to be a problem
> we'll have to revisit it, but for now let's keep as much as possible
> dcache internals inside dcache.[ch].
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Nope this is fine, we were doing lots of extra work for little benefit
before.

Acked-by: Eric Paris <eparis@redhat.com>

> Index: linux-2.6/fs/notify/fsnotify.c
> ===================================================================
> --- linux-2.6.orig/fs/notify/fsnotify.c	2010-10-10 09:35:58.488004195 +0200
> +++ linux-2.6/fs/notify/fsnotify.c	2010-10-10 09:53:53.103010272 +0200
> @@ -88,8 +88,6 @@ void __fsnotify_parent(struct path *path
>  {
>  	struct dentry *parent;
>  	struct inode *p_inode;
> -	bool send = false;
> -	bool should_update_children = false;
>  
>  	if (!dentry)
>  		dentry = path->dentry;
> @@ -97,29 +95,12 @@ void __fsnotify_parent(struct path *path
>  	if (!(dentry->d_flags & DCACHE_FSNOTIFY_PARENT_WATCHED))
>  		return;
>  
> -	spin_lock(&dentry->d_lock);
> -	parent = dentry->d_parent;
> +	parent = dget_parent(dentry);
>  	p_inode = parent->d_inode;
>  
> -	if (fsnotify_inode_watches_children(p_inode)) {
> -		if (p_inode->i_fsnotify_mask & mask) {
> -			dget(parent);
> -			send = true;
> -		}
> -	} else {
> -		/*
> -		 * The parent doesn't care about events on it's children but
> -		 * at least one child thought it did.  We need to run all the
> -		 * children and update their d_flags to let them know p_inode
> -		 * doesn't care about them any more.
> -		 */
> -		dget(parent);
> -		should_update_children = true;
> -	}
> -
> -	spin_unlock(&dentry->d_lock);
> -
> -	if (send) {
> +	if (unlikely(!fsnotify_inode_watches_children(p_inode)))
> +		__fsnotify_update_child_dentry_flags(p_inode);
> +	else if (p_inode->i_fsnotify_mask & mask) {
>  		/* we are notifying a parent so come up with the new mask which
>  		 * specifies these are events which came from a child. */
>  		mask |= FS_EVENT_ON_CHILD;
> @@ -130,13 +111,9 @@ void __fsnotify_parent(struct path *path
>  		else
>  			fsnotify(p_inode, mask, dentry->d_inode, FSNOTIFY_EVENT_INODE,
>  				 dentry->d_name.name, 0);
> -		dput(parent);
>  	}
>  
> -	if (unlikely(should_update_children)) {
> -		__fsnotify_update_child_dentry_flags(p_inode);
> -		dput(parent);
> -	}
> +	dput(parent);
>  }
>  EXPORT_SYMBOL_GPL(__fsnotify_parent);
>  
> 

Re: [PATCH 00/10] dcache cleanups

[Dave Chinner]

On Sun, Oct 10, 2010 at 05:36:20AM -0400, Christoph Hellwig wrote:
> A couple of dcache cleanups in preparation of the dcache_lock splitup.
> Note that I already sent the first patch individually, but this series
> contains a fixed version.

This applies on top of my inode-scale branch, right?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 00/10] dcache cleanups

[Dave Chinner]

On Sun, Oct 10, 2010 at 05:36:20AM -0400, Christoph Hellwig wrote:
> A couple of dcache cleanups in preparation of the dcache_lock splitup.
> Note that I already sent the first patch individually, but this series
> contains a fixed version.

Something in this series causes xfsdump to get really unhappy
(xfstests 026) - probably the get_parent changes. This message is
just continually spewed and the xfsdump process cannot be killed:

  [  884.392813] export: Eeek filesystem root is not connected, impossible
  [  884.393873] export: Eeek filesystem root is not connected, impossible
  [  884.394669] export: Eeek filesystem root is not connected, impossible
  [  884.395648] export: Eeek filesystem root is not connected, impossible
  [  884.396482] export: Eeek filesystem root is not connected, impossible
  [  884.397487] export: Eeek filesystem root is not connected, impossible
  [  884.398241] export: Eeek filesystem root is not connected, impossible
  [  884.399036] export: Eeek filesystem root is not connected, impossible
  [  884.400148] export: Eeek filesystem root is not connected, impossible
  [  884.401186] export: Eeek filesystem root is not connected, impossible

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 00/10] dcache cleanups

[Christoph Hellwig]

On Wed, Oct 13, 2010 at 12:04:59PM +1100, Dave Chinner wrote:
> On Sun, Oct 10, 2010 at 05:36:20AM -0400, Christoph Hellwig wrote:
> > A couple of dcache cleanups in preparation of the dcache_lock splitup.
> > Note that I already sent the first patch individually, but this series
> > contains a fixed version.
> 
> This applies on top of my inode-scale branch, right?

Yes.

Re: [PATCH 00/10] dcache cleanups

[Christoph Hellwig]

On Wed, Oct 13, 2010 at 12:46:02PM +1100, Dave Chinner wrote:
> On Sun, Oct 10, 2010 at 05:36:20AM -0400, Christoph Hellwig wrote:
> > A couple of dcache cleanups in preparation of the dcache_lock splitup.
> > Note that I already sent the first patch individually, but this series
> > contains a fixed version.
> 
> Something in this series causes xfsdump to get really unhappy
> (xfstests 026) - probably the get_parent changes. This message is
> just continually spewed and the xfsdump process cannot be killed:

Hmm, I did a couple of xfstests run both with XFS and ext3 and even did
some nfs testing.  Do you still see it with the exportfs patch removed?

Re: [PATCH 00/10] dcache cleanups

[Christoph Hellwig]

Found the bug.  I did a small cleanup after testing the patch
and inverted a condition.  The correct one is below:

---
From: Christoph Hellwig <hch@lst.de>
Subject: exportfs: use dget_parent

Use dget_parent instead of opencoding it.  This simplifies the code, but
more importanly prepares for the more complicated locking for a parent
dget in the dcache scale patch series.

Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: linux-2.6/fs/exportfs/expfs.c
===================================================================
--- linux-2.6.orig/fs/exportfs/expfs.c	2010-10-11 16:58:59.533088586 -0400
+++ linux-2.6/fs/exportfs/expfs.c	2010-10-13 09:54:41.070259168 -0400
@@ -74,21 +74,20 @@ static struct dentry *
 find_disconnected_root(struct dentry *dentry)
 {
 	dget(dentry);
-	spin_lock(&dentry->d_lock);
-	while (!IS_ROOT(dentry) &&
-	       (dentry->d_parent->d_flags & DCACHE_DISCONNECTED)) {
-		struct dentry *parent = dentry->d_parent;
-		dget(parent);
-		spin_unlock(&dentry->d_lock);
+	while (!IS_ROOT(dentry)) {
+		struct dentry *parent = dget_parent(dentry);
+
+		if (!(parent->d_flags & DCACHE_DISCONNECTED)) {
+			dput(parent);
+			break;
+		}
+
 		dput(dentry);
 		dentry = parent;
-		spin_lock(&dentry->d_lock);
 	}
-	spin_unlock(&dentry->d_lock);
 	return dentry;
 }
 
-
 /*
  * Make sure target_dir is fully connected to the dentry tree.
  *

Re: [PATCH 00/10] dcache cleanups

[Nick Piggin]

Well thanks for review on inode series so far, I've attempted to
cover everything but I'll probably end up going back over all the
feedback a couple more times.

Not quite finished taking a look at this dcache series, but thanks
for it too I'll get some comments out soon.