Thread (42 messages) 42 messages, 7 authors, 2013-05-14

Re: [PATCH v6 04/31] dcache: remove dentries from LRU before putting on dispose list

From: Dave Chinner <david@fromorbit.com>
Date: 2013-05-14 02:02:48
Also in: linux-fsdevel, linux-mm

On Sun, May 12, 2013 at 10:13:25PM +0400, Glauber Costa wrote:
quoted hunk ↗ jump to hunk
From: Dave Chinner <redacted>

One of the big problems with modifying the way the dcache shrinker
and LRU implementation works is that the LRU is abused in several
ways. One of these is shrink_dentry_list().

Basically, we can move a dentry off the LRU onto a different list
without doing any accounting changes, and then use dentry_lru_prune()
to remove it from what-ever list it is now on to do the LRU
accounting at that point.

This makes it -really hard- to change the LRU implementation. The
use of the per-sb LRU lock serialises movement of the dentries
between the different lists and the removal of them, and this is the
only reason that it works. If we want to break up the dentry LRU
lock and lists into, say, per-node lists, we remove the only
serialisation that allows this lru list/dispose list abuse to work.

To make this work effectively, the dispose list has to be isolated
from the LRU list - dentries have to be removed from the LRU
*before* being placed on the dispose list. This means that the LRU
accounting and isolation is completed before disposal is started,
and that means we can change the LRU implementation freely in
future.

This means that dentries *must* be marked with DCACHE_SHRINK_LIST
when they are placed on the dispose list so that we don't think that
parent dentries found in try_prune_one_dentry() are on the LRU when
the are actually on the dispose list. This would result in
accounting the dentry to the LRU a second time. Hence
dentry_lru_prune() has to handle the DCACHE_SHRINK_LIST case
differently because the dentry isn't on the LRU list.

[ v2: don't decrement nr unused twice, spotted by Sha Zhengju ]
Signed-off-by: Dave Chinner <redacted>
Signed-off-by: Glauber Costa <redacted>
Acked-by: Mel Gorman <mgorman@suse.de>
---
 fs/dcache.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 71 insertions(+), 22 deletions(-)
diff --git a/fs/dcache.c b/fs/dcache.c
index 795c15d..868abf9 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -331,7 +331,6 @@ static void dentry_lru_add(struct dentry *dentry)
 static void __dentry_lru_del(struct dentry *dentry)
 {
 	list_del_init(&dentry->d_lru);
-	dentry->d_flags &= ~DCACHE_SHRINK_LIST;
 	dentry->d_sb->s_nr_dentry_unused--;
 	this_cpu_dec(nr_dentry_unused);
 }
@@ -341,6 +340,8 @@ static void __dentry_lru_del(struct dentry *dentry)
  */
 static void dentry_lru_del(struct dentry *dentry)
 {
+	BUG_ON(dentry->d_flags & DCACHE_SHRINK_LIST);
+
 	if (!list_empty(&dentry->d_lru)) {
 		spin_lock(&dentry->d_sb->s_dentry_lru_lock);
 		__dentry_lru_del(dentry);
@@ -348,15 +349,39 @@ static void dentry_lru_del(struct dentry *dentry)
 	}
 }
 
+static void dentry_lru_prune(struct dentry *dentry)
+{
+	/*
+	 * inform the fs via d_prune that this dentry is about to be
+	 * unhashed and destroyed.
+	 */
+	if (dentry->d_flags & DCACHE_OP_PRUNE)
+		dentry->d_op->d_prune(dentry);
+
+	if (list_empty(&dentry->d_lru))
+		return;
+
+	if ((dentry->d_flags & DCACHE_SHRINK_LIST)) {
+		list_del_init(&dentry->d_lru);
+		dentry->d_flags &= ~DCACHE_SHRINK_LIST;
+	} else {
+		spin_lock(&dentry->d_sb->s_dentry_lru_lock);
+		__dentry_lru_del(dentry);
+		spin_unlock(&dentry->d_sb->s_dentry_lru_lock);
+	}
+}
Re-adding this function is wrong - it went away because the act of
calling ->d_prune is independent of the presence of the dentry on
the LRU.
quoted hunk ↗ jump to hunk
@@ -479,14 +504,8 @@ relock:
 
 	if (ref)
 		dentry->d_count--;
-	/*
-	 * inform the fs via d_prune that this dentry is about to be
-	 * unhashed and destroyed.
-	 */
-	if (dentry->d_flags & DCACHE_OP_PRUNE)
-		dentry->d_op->d_prune(dentry);
 
-	dentry_lru_del(dentry);
+	dentry_lru_prune(dentry);
 	/* if it was on the hash then remove it */
 	__d_drop(dentry);
 	return d_kill(dentry, parent);
So this change is not necessary.
quoted hunk ↗ jump to hunk
@@ -914,14 +970,7 @@ static void shrink_dcache_for_umount_subtree(struct dentry *dentry)
 		do {
 			struct inode *inode;
 
-			/*
-			 * inform the fs that this dentry is about to be
-			 * unhashed and destroyed.
-			 */
-			if (dentry->d_flags & DCACHE_OP_PRUNE)
-				dentry->d_op->d_prune(dentry);
-
-			dentry_lru_del(dentry);
+			dentry_lru_prune(dentry);
 			__d_shrink(dentry);
 
 			if (dentry->d_count != 0) {
Nor is this one.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help