[PATCH] d_prune dentry_operation

[PATCH] d_prune dentry_operation

[Sage Weil]

Ceph goes to great lengths to keep its client-side cache coherent, 
allowing many operations (lookup, creations, readdir) to be performed 
without any server interaction when the client has the proper leases. 

Sadly, this functionality is all currently disabled because we cannot 
handle races between dcache pruning and any of those activities with the 
current VFS interface.

This patch adds a d_prune hook that allows the filesystem to be informed 
before a dentry is removed from the cache.  Merging this for 3.2-rc1 
will make Ceph users and their metadata-intensive workload very happy.

If anybody has any issues at all with this, please tell me, so I can 
make my case or revise my approach.

Thanks!
sage


Sage Weil (1):
  vfs: add d_prune dentry operation

 Documentation/filesystems/Locking |    1 +
 fs/dcache.c                       |    8 ++++++++
 include/linux/dcache.h            |    3 +++
 3 files changed, 12 insertions(+), 0 deletions(-)

-- 
1.7.2.5

[PATCH] vfs: add d_prune dentry operation

[Sage Weil]

This adds a d_prune dentry operation that is called by the VFS prior to
pruning (i.e. unhashing and killing) a hashed dentry from the dcache.  This
will be used by Ceph to maintain a flag indicating whether the complete
contents of a directory are contained in the dcache, allowing it to satisfy
lookups and readdir without addition server communication.

Signed-off-by: Sage Weil <sage@newdream.net>
---
 Documentation/filesystems/Locking |    1 +
 fs/dcache.c                       |    8 ++++++++
 include/linux/dcache.h            |    3 +++
 3 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/Documentation/filesystems/Locking b/Documentation/filesystems/Locking
index 57d827d..b3fa7c8 100644
--- a/Documentation/filesystems/Locking
+++ b/Documentation/filesystems/Locking
@@ -29,6 +29,7 @@ d_hash		no		no		no		maybe
 d_compare:	yes		no		no		maybe
 d_delete:	no		yes		no		no
 d_release:	no		no		yes		no
+d_prune:        no              yes             no              no
 d_iput:		no		no		yes		no
 d_dname:	no		no		no		no
 d_automount:	no		no		yes		no
diff --git a/fs/dcache.c b/fs/dcache.c
index fbdcbca..84e1756 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -673,6 +673,8 @@ static void try_prune_one_dentry(struct dentry *dentry)
 			spin_unlock(&dentry->d_lock);
 			return;
 		}
+		if (dentry->d_flags & DCACHE_OP_PRUNE)
+			dentry->d_op->d_prune(dentry);
 		dentry = dentry_kill(dentry, 1);
 	}
 }
@@ -879,6 +881,8 @@ static void shrink_dcache_for_umount_subtree(struct dentry *dentry)
 
 	/* detach this root from the system */
 	spin_lock(&dentry->d_lock);
+	if (dentry->d_flags & DCACHE_OP_PRUNE)
+		dentry->d_op->d_prune(dentry);
 	dentry_lru_del(dentry);
 	__d_drop(dentry);
 	spin_unlock(&dentry->d_lock);
@@ -895,6 +899,8 @@ static void shrink_dcache_for_umount_subtree(struct dentry *dentry)
 					    d_u.d_child) {
 				spin_lock_nested(&loop->d_lock,
 						DENTRY_D_LOCK_NESTED);
+				if (dentry->d_flags & DCACHE_OP_PRUNE)
+					dentry->d_op->d_prune(dentry);
 				dentry_lru_del(loop);
 				__d_drop(loop);
 				spin_unlock(&loop->d_lock);
@@ -1363,6 +1369,8 @@ void d_set_d_op(struct dentry *dentry, const struct dentry_operations *op)
 		dentry->d_flags |= DCACHE_OP_REVALIDATE;
 	if (op->d_delete)
 		dentry->d_flags |= DCACHE_OP_DELETE;
+	if (op->d_prune)
+		dentry->d_flags |= DCACHE_OP_PRUNE;
 
 }
 EXPORT_SYMBOL(d_set_d_op);
diff --git a/include/linux/dcache.h b/include/linux/dcache.h
index 19d90a5..681f46f 100644
--- a/include/linux/dcache.h
+++ b/include/linux/dcache.h
@@ -165,6 +165,7 @@ struct dentry_operations {
 			unsigned int, const char *, const struct qstr *);
 	int (*d_delete)(const struct dentry *);
 	void (*d_release)(struct dentry *);
+	void (*d_prune)(struct dentry *);
 	void (*d_iput)(struct dentry *, struct inode *);
 	char *(*d_dname)(struct dentry *, char *, int);
 	struct vfsmount *(*d_automount)(struct path *);
@@ -219,6 +220,8 @@ struct dentry_operations {
 #define DCACHE_MANAGED_DENTRY \
 	(DCACHE_MOUNTED|DCACHE_NEED_AUTOMOUNT|DCACHE_MANAGE_TRANSIT)
 
+#define DCACHE_OP_PRUNE         0x80000
+
 extern seqlock_t rename_lock;
 
 static inline int dname_external(struct dentry *dentry)
-- 
1.7.2.5

Re: [PATCH] vfs: add d_prune dentry operation

[Christoph Hellwig]

On Wed, Oct 05, 2011 at 09:26:10PM -0700, Sage Weil wrote:
> This adds a d_prune dentry operation that is called by the VFS prior to
> pruning (i.e. unhashing and killing) a hashed dentry from the dcache.  This
> will be used by Ceph to maintain a flag indicating whether the complete
> contents of a directory are contained in the dcache, allowing it to satisfy
> lookups and readdir without addition server communication.

What tree is the patch against?  I seems to fail to apply against Linus'
latests.  

It also seem like it basically should not be be opencoded but in a
wrapper around dentry_lru_del for all cases but the lazy removal from
LRU in case that is referenced, with some comments explaining the whole
thing.

Re: [PATCH] vfs: add d_prune dentry operation

[Sage Weil]

On Thu, 6 Oct 2011, Christoph Hellwig wrote:
> On Wed, Oct 05, 2011 at 09:26:10PM -0700, Sage Weil wrote:
> > This adds a d_prune dentry operation that is called by the VFS prior to
> > pruning (i.e. unhashing and killing) a hashed dentry from the dcache.  This
> > will be used by Ceph to maintain a flag indicating whether the complete
> > contents of a directory are contained in the dcache, allowing it to satisfy
> > lookups and readdir without addition server communication.
> 
> What tree is the patch against?  I seems to fail to apply against Linus'
> latests.  

Whoops, I rebased against v3.0 instead of latest master.

> It also seem like it basically should not be be opencoded but in a
> wrapper around dentry_lru_del for all cases but the lazy removal from
> LRU in case that is referenced, with some comments explaining the whole
> thing.

Yeah, that's a bit better.  There are four dentry_lru_del() callers, two 
where we there is a reference, and two where we want to ->d_prune too.

How does the below look?

Thanks!
sage


>From 182e20331b9b2be15dbecdff7465be26ac00a81c Mon Sep 17 00:00:00 2001
From: Sage Weil <sage@newdream.net>
Date: Thu, 6 Oct 2011 15:18:21 -0700
Subject: [PATCH] vfs: add d_prune dentry operation

This adds a d_prune dentry operation that is called by the VFS prior to
pruning (i.e. unhashing and killing) a hashed dentry from the dcache.
Wrap dentry_lru_del() and use the new _prune() helper in the cases where we
are about to unhash and kill the dentry.

This will be used by Ceph to maintain a flag indicating whether the
complete contents of a directory are contained in the dcache, allowing it
to satisfy lookups and readdir without addition server communication.

Signed-off-by: Sage Weil <sage@newdream.net>
---
 Documentation/filesystems/Locking |    1 +
 fs/dcache.c                       |   33 ++++++++++++++++++++++++++++-----
 include/linux/dcache.h            |    3 +++
 3 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/Documentation/filesystems/Locking b/Documentation/filesystems/Locking
index 6533807..d819ba1 100644
--- a/Documentation/filesystems/Locking
+++ b/Documentation/filesystems/Locking
@@ -29,6 +29,7 @@ d_hash		no		no		no		maybe
 d_compare:	yes		no		no		maybe
 d_delete:	no		yes		no		no
 d_release:	no		no		yes		no
+d_prune:        no              yes             no              no
 d_iput:		no		no		yes		no
 d_dname:	no		no		no		no
 d_automount:	no		no		yes		no
diff --git a/fs/dcache.c b/fs/dcache.c
index a88948b..a348f64 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -225,7 +225,7 @@ static void dentry_unlink_inode(struct dentry * dentry)
 }
 
 /*
- * dentry_lru_(add|del|move_tail) must be called with d_lock held.
+ * dentry_lru_(add|del|prune|move_tail) must be called with d_lock held.
  */
 static void dentry_lru_add(struct dentry *dentry)
 {
@@ -254,6 +254,24 @@ static void dentry_lru_del(struct dentry *dentry)
 	}
 }
 
+static void dentry_lru_prune(struct dentry *dentry)
+{
+	if (!list_empty(&dentry->d_lru)) {
+		/*
+		 * Notify the fs this dentry is about to be pruned
+		 * before removing from the LRU.  This should happen
+		 * before we unhash it (if it was hashed to begin
+		 * with).
+		 */
+		if (dentry->d_flags & DCACHE_OP_PRUNE)
+			dentry->d_op->d_prune(dentry);
+
+		spin_lock(&dcache_lru_lock);
+		__dentry_lru_del(dentry);
+		spin_unlock(&dcache_lru_lock);
+	}
+}
+
 static void dentry_lru_move_tail(struct dentry *dentry)
 {
 	spin_lock(&dcache_lru_lock);
@@ -403,8 +421,11 @@ relock:
 
 	if (ref)
 		dentry->d_count--;
-	/* if dentry was on the d_lru list delete it from there */
-	dentry_lru_del(dentry);
+	/*
+	 * if dentry was on the d_lru list delete it from there and
+	 * inform the fs via d_prune.
+	 */
+	dentry_lru_prune(dentry);
 	/* if it was on the hash then remove it */
 	__d_drop(dentry);
 	return d_kill(dentry, parent);
@@ -854,8 +875,8 @@ static void shrink_dcache_for_umount_subtree(struct dentry *dentry)
 		do {
 			struct inode *inode;
 
-			/* detach from the system */
-			dentry_lru_del(dentry);
+			/* detach from the system, inform the fs via d_prune */
+			dentry_lru_prune(dentry);
 			__d_shrink(dentry);
 
 			if (dentry->d_count != 0) {
@@ -1283,6 +1304,8 @@ void d_set_d_op(struct dentry *dentry, const struct dentry_operations *op)
 		dentry->d_flags |= DCACHE_OP_REVALIDATE;
 	if (op->d_delete)
 		dentry->d_flags |= DCACHE_OP_DELETE;
+	if (op->d_prune)
+		dentry->d_flags |= DCACHE_OP_PRUNE;
 
 }
 EXPORT_SYMBOL(d_set_d_op);
diff --git a/include/linux/dcache.h b/include/linux/dcache.h
index 62157c0..69ee43a 100644
--- a/include/linux/dcache.h
+++ b/include/linux/dcache.h
@@ -165,6 +165,7 @@ struct dentry_operations {
 			unsigned int, const char *, const struct qstr *);
 	int (*d_delete)(const struct dentry *);
 	void (*d_release)(struct dentry *);
+	void (*d_prune)(struct dentry *);
 	void (*d_iput)(struct dentry *, struct inode *);
 	char *(*d_dname)(struct dentry *, char *, int);
 	struct vfsmount *(*d_automount)(struct path *);
@@ -216,6 +217,8 @@ struct dentry_operations {
 #define DCACHE_MANAGED_DENTRY \
 	(DCACHE_MOUNTED|DCACHE_NEED_AUTOMOUNT|DCACHE_MANAGE_TRANSIT)
 
+#define DCACHE_OP_PRUNE         0x80000
+
 extern seqlock_t rename_lock;
 
 static inline int dname_external(struct dentry *dentry)
-- 
1.7.0

Re: [PATCH] vfs: add d_prune dentry operation

[Christoph Hellwig]

On Thu, Oct 06, 2011 at 03:20:37PM -0700, Sage Weil wrote:
> > It also seem like it basically should not be be opencoded but in a
> > wrapper around dentry_lru_del for all cases but the lazy removal from
> > LRU in case that is referenced, with some comments explaining the whole
> > thing.
> 
> Yeah, that's a bit better.  There are four dentry_lru_del() callers, two 
> where we there is a reference, and two where we want to ->d_prune too.
> 
> How does the below look?

This looks much better.

It will clash a bit with Dave's dcache series that were part of his
shrinker/lru series, which I really hope he is going to resumit for
3.2 (at least the dcache cleanup parts).

Al, any progress on getting the current VFS queue up somewhere for
review?

Re: [PATCH] vfs: add d_prune dentry operation

[Dave Chinner]

On Thu, Oct 06, 2011 at 03:20:37PM -0700, Sage Weil wrote:
> On Thu, 6 Oct 2011, Christoph Hellwig wrote:
> > On Wed, Oct 05, 2011 at 09:26:10PM -0700, Sage Weil wrote:
> > > This adds a d_prune dentry operation that is called by the VFS prior to
> > > pruning (i.e. unhashing and killing) a hashed dentry from the dcache.  This
> > > will be used by Ceph to maintain a flag indicating whether the complete
> > > contents of a directory are contained in the dcache, allowing it to satisfy
> > > lookups and readdir without addition server communication.
> > 
> > What tree is the patch against?  I seems to fail to apply against Linus'
> > latests.  
> 
> Whoops, I rebased against v3.0 instead of latest master.
> 
> > It also seem like it basically should not be be opencoded but in a
> > wrapper around dentry_lru_del for all cases but the lazy removal from
> > LRU in case that is referenced, with some comments explaining the whole
> > thing.
> 
> Yeah, that's a bit better.  There are four dentry_lru_del() callers, two 
> where we there is a reference, and two where we want to ->d_prune too.

The code in the patch doesn't explain to me why you'd need to call
dentry_lru_prune() rather than dentry_lru_del()? It's something to
do with the difference between active and inactive LRU removal, but
I can't really tell. My patchset removes the LRU abuse from
select_parent, so I'm kind of wondering what the correct thing is to
do there. 

Hence, can you add a bit of documentation to those functions
explaining why and when you should use one or the other?  i.e
document the situations where the FS needs to be notified of
pruning, rather than leaving anyone who is reading the code
guessing.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] vfs: add d_prune dentry operation

[Christoph Hellwig]

On Mon, Oct 10, 2011 at 04:11:41PM +1100, Dave Chinner wrote:
> The code in the patch doesn't explain to me why you'd need to call
> dentry_lru_prune() rather than dentry_lru_del()? It's something to
> do with the difference between active and inactive LRU removal, but

Whenever we actually remove a object it needs to be removed, if it
only gets deleted from the LRU because it now has a reference and
doesn't belong onto the LRU it doesn't.

But yes, this really should be documented better in the code.

Re: [PATCH] vfs: add d_prune dentry operation

[Sage Weil]

On Mon, 10 Oct 2011, Dave Chinner wrote:
> On Thu, Oct 06, 2011 at 03:20:37PM -0700, Sage Weil wrote:
> > On Thu, 6 Oct 2011, Christoph Hellwig wrote:
> > > On Wed, Oct 05, 2011 at 09:26:10PM -0700, Sage Weil wrote:
> > > > This adds a d_prune dentry operation that is called by the VFS prior to
> > > > pruning (i.e. unhashing and killing) a hashed dentry from the dcache.  This
> > > > will be used by Ceph to maintain a flag indicating whether the complete
> > > > contents of a directory are contained in the dcache, allowing it to satisfy
> > > > lookups and readdir without addition server communication.
> > > 
> > > What tree is the patch against?  I seems to fail to apply against Linus'
> > > latests.  
> > 
> > Whoops, I rebased against v3.0 instead of latest master.
> > 
> > > It also seem like it basically should not be be opencoded but in a
> > > wrapper around dentry_lru_del for all cases but the lazy removal from
> > > LRU in case that is referenced, with some comments explaining the whole
> > > thing.
> > 
> > Yeah, that's a bit better.  There are four dentry_lru_del() callers, two 
> > where we there is a reference, and two where we want to ->d_prune too.
> 
> The code in the patch doesn't explain to me why you'd need to call
> dentry_lru_prune() rather than dentry_lru_del()? It's something to
> do with the difference between active and inactive LRU removal, but
> I can't really tell. My patchset removes the LRU abuse from
> select_parent, so I'm kind of wondering what the correct thing is to
> do there. 
> 
> Hence, can you add a bit of documentation to those functions
> explaining why and when you should use one or the other?  i.e
> document the situations where the FS needs to be notified of
> pruning, rather than leaving anyone who is reading the code
> guessing.

Let me know if the below is clearer.  The requirement is that ->d_prune be 
called prior to unhashing (and then destroying) a victim dentry.  I'm a 
little worried about mixing that in with the lru helpers because it is 
only indirectly related to whether the dentry is on the LRU, and that may 
confuse people.  A cleaned up opencoded patch is here:

	https://github.com/NewDreamNetwork/ceph-client/commit/784a6aa6dc7baf2069c40988d79130dba17c7068

and the updated dentry_lru_prune() wrapper version is below.

Thanks-
sage



>From 619707e0b6fca69f165d1d4f048ab8211531d559 Mon Sep 17 00:00:00 2001
From: Sage Weil <sage@newdream.net>
Date: Mon, 10 Oct 2011 09:18:16 -0700
Subject: [PATCH] vfs: add d_prune dentry operation

This adds a d_prune dentry operation that is called by the VFS prior to
pruning (i.e. unhashing and killing) a hashed dentry from the dcache.
Wrap dentry_lru_del() and use the new _prune() helper in the cases where we
are about to unhash and kill the dentry.

This will be used by Ceph to maintain a flag indicating whether the
complete contents of a directory are contained in the dcache, allowing it
to satisfy lookups and readdir without addition server communication.

Signed-off-by: Sage Weil <sage@newdream.net>
---
 Documentation/filesystems/Locking |    1 +
 fs/dcache.c                       |   40 ++++++++++++++++++++++++++++++++----
 include/linux/dcache.h            |    3 ++
 3 files changed, 39 insertions(+), 5 deletions(-)

diff --git a/Documentation/filesystems/Locking b/Documentation/filesystems/Locking
index 6533807..d819ba1 100644
--- a/Documentation/filesystems/Locking
+++ b/Documentation/filesystems/Locking
@@ -29,6 +29,7 @@ d_hash		no		no		no		maybe
 d_compare:	yes		no		no		maybe
 d_delete:	no		yes		no		no
 d_release:	no		no		yes		no
+d_prune:        no              yes             no              no
 d_iput:		no		no		yes		no
 d_dname:	no		no		no		no
 d_automount:	no		no		yes		no
diff --git a/fs/dcache.c b/fs/dcache.c
index a88948b..274f13e 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -225,7 +225,7 @@ static void dentry_unlink_inode(struct dentry * dentry)
 }
 
 /*
- * dentry_lru_(add|del|move_tail) must be called with d_lock held.
+ * dentry_lru_(add|del|prune|move_tail) must be called with d_lock held.
  */
 static void dentry_lru_add(struct dentry *dentry)
 {
@@ -245,6 +245,9 @@ static void __dentry_lru_del(struct dentry *dentry)
 	dentry_stat.nr_unused--;
 }
 
+/*
+ * Remove a dentry with references from the LRU.
+ */
 static void dentry_lru_del(struct dentry *dentry)
 {
 	if (!list_empty(&dentry->d_lru)) {
@@ -254,6 +257,23 @@ static void dentry_lru_del(struct dentry *dentry)
 	}
 }
 
+/*
+ * Remove a dentry that is unreferenced and about to be pruned
+ * (unhashed and destroyed) from the LRU, and inform the file system.
+ * This wrapper should be called _prior_ to unhashing a victim dentry.
+ */
+static void dentry_lru_prune(struct dentry *dentry)
+{
+	if (!list_empty(&dentry->d_lru)) {
+		if (dentry->d_flags & DCACHE_OP_PRUNE)
+			dentry->d_op->d_prune(dentry);
+
+		spin_lock(&dcache_lru_lock);
+		__dentry_lru_del(dentry);
+		spin_unlock(&dcache_lru_lock);
+	}
+}
+
 static void dentry_lru_move_tail(struct dentry *dentry)
 {
 	spin_lock(&dcache_lru_lock);
@@ -403,8 +423,12 @@ relock:
 
 	if (ref)
 		dentry->d_count--;
-	/* if dentry was on the d_lru list delete it from there */
-	dentry_lru_del(dentry);
+	/*
+	 * if dentry was on the d_lru list delete it from there.
+	 * inform the fs via d_prune that this dentry is about to be
+	 * unhashed and destroyed.
+	 */
+	dentry_lru_prune(dentry);
 	/* if it was on the hash then remove it */
 	__d_drop(dentry);
 	return d_kill(dentry, parent);
@@ -854,8 +878,12 @@ static void shrink_dcache_for_umount_subtree(struct dentry *dentry)
 		do {
 			struct inode *inode;
 
-			/* detach from the system */
-			dentry_lru_del(dentry);
+			/*
+			 * remove the dentry from the lru, and inform
+			 * the fs that this dentry is about to be
+			 * unhashed and destroyed.
+			 */
+			dentry_lru_prune(dentry);
 			__d_shrink(dentry);
 
 			if (dentry->d_count != 0) {
@@ -1283,6 +1311,8 @@ void d_set_d_op(struct dentry *dentry, const struct dentry_operations *op)
 		dentry->d_flags |= DCACHE_OP_REVALIDATE;
 	if (op->d_delete)
 		dentry->d_flags |= DCACHE_OP_DELETE;
+	if (op->d_prune)
+		dentry->d_flags |= DCACHE_OP_PRUNE;
 
 }
 EXPORT_SYMBOL(d_set_d_op);
diff --git a/include/linux/dcache.h b/include/linux/dcache.h
index 62157c0..69ee43a 100644
--- a/include/linux/dcache.h
+++ b/include/linux/dcache.h
@@ -165,6 +165,7 @@ struct dentry_operations {
 			unsigned int, const char *, const struct qstr *);
 	int (*d_delete)(const struct dentry *);
 	void (*d_release)(struct dentry *);
+	void (*d_prune)(struct dentry *);
 	void (*d_iput)(struct dentry *, struct inode *);
 	char *(*d_dname)(struct dentry *, char *, int);
 	struct vfsmount *(*d_automount)(struct path *);
@@ -216,6 +217,8 @@ struct dentry_operations {
 #define DCACHE_MANAGED_DENTRY \
 	(DCACHE_MOUNTED|DCACHE_NEED_AUTOMOUNT|DCACHE_MANAGE_TRANSIT)
 
+#define DCACHE_OP_PRUNE         0x80000
+
 extern seqlock_t rename_lock;
 
 static inline int dname_external(struct dentry *dentry)
-- 
1.7.0

Re: [PATCH] vfs: add d_prune dentry operation

[Christoph Hellwig]

On Mon, Oct 10, 2011 at 09:19:43AM -0700, Sage Weil wrote:
> Let me know if the below is clearer.  The requirement is that ->d_prune be 
> called prior to unhashing (and then destroying) a victim dentry.  I'm a 
> little worried about mixing that in with the lru helpers because it is 
> only indirectly related to whether the dentry is on the LRU, and that may 
> confuse people.  A cleaned up opencoded patch is here:
> 
> 	https://github.com/NewDreamNetwork/ceph-client/commit/784a6aa6dc7baf2069c40988d79130dba17c7068
> 
> and the updated dentry_lru_prune() wrapper version is below.

Long term what I really want is a helper for doing more of the real
removal in single place.  This is one step towards it, and the series
from Dave is another.

Re: [PATCH] vfs: add d_prune dentry operation

[Sage Weil]

On Mon, 10 Oct 2011, Christoph Hellwig wrote:
> On Mon, Oct 10, 2011 at 09:19:43AM -0700, Sage Weil wrote:
> > Let me know if the below is clearer.  The requirement is that ->d_prune be 
> > called prior to unhashing (and then destroying) a victim dentry.  I'm a 
> > little worried about mixing that in with the lru helpers because it is 
> > only indirectly related to whether the dentry is on the LRU, and that may 
> > confuse people.  A cleaned up opencoded patch is here:
> > 
> > 	https://github.com/NewDreamNetwork/ceph-client/commit/784a6aa6dc7baf2069c40988d79130dba17c7068
> > 
> > and the updated dentry_lru_prune() wrapper version is below.
> 
> Long term what I really want is a helper for doing more of the real
> removal in single place.  This is one step towards it, and the series
> from Dave is another.

Ah, makes sense.

Dave, is the revised patch more clear?  

sage

Re: [PATCH] vfs: add d_prune dentry operation

[Dave Chinner]

On Tue, Oct 11, 2011 at 08:39:24AM -0700, Sage Weil wrote:
> On Mon, 10 Oct 2011, Christoph Hellwig wrote:
> > On Mon, Oct 10, 2011 at 09:19:43AM -0700, Sage Weil wrote:
> > > Let me know if the below is clearer.  The requirement is that ->d_prune be 
> > > called prior to unhashing (and then destroying) a victim dentry.  I'm a 
> > > little worried about mixing that in with the lru helpers because it is 
> > > only indirectly related to whether the dentry is on the LRU, and that may 
> > > confuse people.  A cleaned up opencoded patch is here:
> > > 
> > > 	https://github.com/NewDreamNetwork/ceph-client/commit/784a6aa6dc7baf2069c40988d79130dba17c7068
> > > 
> > > and the updated dentry_lru_prune() wrapper version is below.
> > 
> > Long term what I really want is a helper for doing more of the real
> > removal in single place.  This is one step towards it, and the series
> > from Dave is another.
> 
> Ah, makes sense.
> 
> Dave, is the revised patch more clear?  

Yeah, I kind of understand what it is doing now, and the comments
will remind me when I start digging in this code again. Thanks.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] vfs: add d_prune dentry operation

[Christoph Hellwig]

This variant seems to crash btrfs in xfstests 010:

010 0s ... 0s
[ 8093.504387] BUG: unable to handle kernel NULL pointer dereference at (null)
[ 8093.505618] IP: [<          (null)>]           (null)
[ 8093.505930] PGD 196f5067 PUD 5507d067 PMD 0 
[ 8093.505930] Oops: 0010 [#1] SMP 
[ 8093.505930] CPU 0 
[ 8093.505930] Modules linked in:
[ 8093.505930] 
[ 8093.505930] Pid: 26328, comm: umount Not tainted 3.1.0+ #90 Bochs Bochs
[ 8093.505930] RIP: 0010:[<0000000000000000>]  [<          (null)>] (null)
[ 8093.505930] RSP: 0018:ffff880009f63dc0  EFLAGS: 00010206
[ 8093.505930] RAX: ffffffff81ae7780 RBX: ffff88002b93a840 RCX: 000000010023ad06
[ 8093.505930] RDX: ffff88002b93a8e0 RSI: ffffffff8114fef0 RDI: ffff88002b93a840
[ 8093.505930] RBP: ffff880009f63dd8 R08: 0000000000000000 R09: 002dffe1ea51a56d
[ 8093.505930] R10: ffff880058c7f680 R11: 0000000000000004 R12: ffff88002b93a8c0
[ 8093.505930] R13: 0000000000000000 R14: ffff88005a56d0c0 R15: ffff88001a896000
[ 8093.505930] FS:  0000000000000000(0000) GS:ffff88005d800000(0063) knlGS:00000000f75c6700
[ 8093.505930] CS:  0010 DS: 002b ES: 002b CR0: 0000000080050033
[ 8093.505930] CR2: 0000000000000000 CR3: 0000000041ddc000 CR4: 00000000000006f0
[ 8093.505930] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 8093.505930] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
[ 8093.505930] Process umount (pid: 26328, threadinfo ffff880009f62000, task ffff88003abd6500)
[ 8093.505930] Stack:
[ 8093.505930]  ffffffff8114efb7 ffff88002b93a840 ffff88005a3c4500 ffff880009f63e08
[ 8093.505930]  ffffffff811516ea ffff880009f63f08 0000000000000000 ffff88001a896000
[ 8093.505930]  ffffffff81ae40c0 ffff880009f63e28 ffffffff8115192e ffff880009f63e38
[ 8093.505930] Call Trace:
[ 8093.505930]  [<ffffffff8114efb7>] ? dentry_lru_prune+0x87/0x90
[ 8093.505930]  [<ffffffff811516ea>] shrink_dcache_for_umount_subtree+0x7a/0x1e0
[ 8093.505930]  [<ffffffff8115192e>] shrink_dcache_for_umount+0x2e/0x60
[ 8093.505930]  [<ffffffff8113d977>] generic_shutdown_super+0x27/0xd0
[ 8093.505930]  [<ffffffff8113dab1>] kill_anon_super+0x11/0x20
[ 8093.505930]  [<ffffffff8113de25>] deactivate_locked_super+0x45/0x70
[ 8093.505930]  [<ffffffff8113e7e9>] deactivate_super+0x49/0x70
[ 8093.505930]  [<ffffffff81157f7b>] mntput_no_expire+0xab/0xf0
[ 8093.505930]  [<ffffffff81158c1a>] sys_umount+0x6a/0x360
[ 8093.505930]  [<ffffffff81158f1b>] sys_oldumount+0xb/0x10
[ 8093.505930]  [<ffffffff819e2f05>] sysenter_dispatch+0x7/0x2b
[ 8093.505930] Code:  Bad RIP value.
[ 8093.505930] RIP  [<          (null)>]           (null)

Re: [PATCH] vfs: add d_prune dentry operation

[Sage Weil]

On Fri, 28 Oct 2011, Christoph Hellwig wrote:
> This variant seems to crash btrfs in xfstests 010:

This got rebased enough times that DCACHE_OP_PRUNE collided with 
DCACHE_NEEDS_LOOKUP.  The version below fixes that, and also renumbers a 
few of those constants to group it with the other DCACHE_OP_* guys.  I 
assume it's okay to switch those up?

sage


>From 2c39148ba12ce13442d66319f9d1ba91ce62974d Mon Sep 17 00:00:00 2001
From: Sage Weil <sage@newdream.net>
Date: Fri, 28 Oct 2011 09:55:21 -0700
Subject: [PATCH] vfs: add d_prune dentry operation

This adds a d_prune dentry operation that is called by the VFS prior to
pruning (i.e. unhashing and killing) a hashed dentry from the dcache.
Wrap dentry_lru_del() and use the new _prune() helper in the cases where we
are about to unhash and kill the dentry.

This will be used by Ceph to maintain a flag indicating whether the
complete contents of a directory are contained in the dcache, allowing it
to satisfy lookups and readdir without addition server communication.

Renumber a few DCACHE_* #defines to group DCACHE_OP_PRUNE with the other
DCACHE_OP_ bits.

Signed-off-by: Sage Weil <sage@newdream.net>
---

v4: fixed DCACHE_OP_PRUNE collision with DCACHE_NEEDS_LOOKUP.
v3: better comments for dentry_lru_{del,prune}
v2: move prune call into dentry_lru_prune() wrapper

 Documentation/filesystems/Locking |    1 +
 fs/dcache.c                       |   40 ++++++++++++++++++++++++++++++++----
 include/linux/dcache.h            |    8 ++++--
 3 files changed, 41 insertions(+), 8 deletions(-)

diff --git a/Documentation/filesystems/Locking b/Documentation/filesystems/Locking
index 6533807..d819ba1 100644
--- a/Documentation/filesystems/Locking
+++ b/Documentation/filesystems/Locking
@@ -29,6 +29,7 @@ d_hash		no		no		no		maybe
 d_compare:	yes		no		no		maybe
 d_delete:	no		yes		no		no
 d_release:	no		no		yes		no
+d_prune:        no              yes             no              no
 d_iput:		no		no		yes		no
 d_dname:	no		no		no		no
 d_automount:	no		no		yes		no
diff --git a/fs/dcache.c b/fs/dcache.c
index a88948b..274f13e 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -225,7 +225,7 @@ static void dentry_unlink_inode(struct dentry * dentry)
 }
 
 /*
- * dentry_lru_(add|del|move_tail) must be called with d_lock held.
+ * dentry_lru_(add|del|prune|move_tail) must be called with d_lock held.
  */
 static void dentry_lru_add(struct dentry *dentry)
 {
@@ -245,6 +245,9 @@ static void __dentry_lru_del(struct dentry *dentry)
 	dentry_stat.nr_unused--;
 }
 
+/*
+ * Remove a dentry with references from the LRU.
+ */
 static void dentry_lru_del(struct dentry *dentry)
 {
 	if (!list_empty(&dentry->d_lru)) {
@@ -254,6 +257,23 @@ static void dentry_lru_del(struct dentry *dentry)
 	}
 }
 
+/*
+ * Remove a dentry that is unreferenced and about to be pruned
+ * (unhashed and destroyed) from the LRU, and inform the file system.
+ * This wrapper should be called _prior_ to unhashing a victim dentry.
+ */
+static void dentry_lru_prune(struct dentry *dentry)
+{
+	if (!list_empty(&dentry->d_lru)) {
+		if (dentry->d_flags & DCACHE_OP_PRUNE)
+			dentry->d_op->d_prune(dentry);
+
+		spin_lock(&dcache_lru_lock);
+		__dentry_lru_del(dentry);
+		spin_unlock(&dcache_lru_lock);
+	}
+}
+
 static void dentry_lru_move_tail(struct dentry *dentry)
 {
 	spin_lock(&dcache_lru_lock);
@@ -403,8 +423,12 @@ relock:
 
 	if (ref)
 		dentry->d_count--;
-	/* if dentry was on the d_lru list delete it from there */
-	dentry_lru_del(dentry);
+	/*
+	 * if dentry was on the d_lru list delete it from there.
+	 * inform the fs via d_prune that this dentry is about to be
+	 * unhashed and destroyed.
+	 */
+	dentry_lru_prune(dentry);
 	/* if it was on the hash then remove it */
 	__d_drop(dentry);
 	return d_kill(dentry, parent);
@@ -854,8 +878,12 @@ static void shrink_dcache_for_umount_subtree(struct dentry *dentry)
 		do {
 			struct inode *inode;
 
-			/* detach from the system */
-			dentry_lru_del(dentry);
+			/*
+			 * remove the dentry from the lru, and inform
+			 * the fs that this dentry is about to be
+			 * unhashed and destroyed.
+			 */
+			dentry_lru_prune(dentry);
 			__d_shrink(dentry);
 
 			if (dentry->d_count != 0) {
@@ -1283,6 +1311,8 @@ void d_set_d_op(struct dentry *dentry, const struct dentry_operations *op)
 		dentry->d_flags |= DCACHE_OP_REVALIDATE;
 	if (op->d_delete)
 		dentry->d_flags |= DCACHE_OP_DELETE;
+	if (op->d_prune)
+		dentry->d_flags |= DCACHE_OP_PRUNE;
 
 }
 EXPORT_SYMBOL(d_set_d_op);
diff --git a/include/linux/dcache.h b/include/linux/dcache.h
index 62157c0..4df9261 100644
--- a/include/linux/dcache.h
+++ b/include/linux/dcache.h
@@ -165,6 +165,7 @@ struct dentry_operations {
 			unsigned int, const char *, const struct qstr *);
 	int (*d_delete)(const struct dentry *);
 	void (*d_release)(struct dentry *);
+	void (*d_prune)(struct dentry *);
 	void (*d_iput)(struct dentry *, struct inode *);
 	char *(*d_dname)(struct dentry *, char *, int);
 	struct vfsmount *(*d_automount)(struct path *);
@@ -184,8 +185,9 @@ struct dentry_operations {
 #define DCACHE_OP_COMPARE	0x0002
 #define DCACHE_OP_REVALIDATE	0x0004
 #define DCACHE_OP_DELETE	0x0008
+#define DCACHE_OP_PRUNE         0x0010
 
-#define	DCACHE_DISCONNECTED	0x0010
+#define	DCACHE_DISCONNECTED	0x0020
      /* This dentry is possibly not currently connected to the dcache tree, in
       * which case its parent will either be itself, or will have this flag as
       * well.  nfsd will not use a dentry with this bit set, but will first
@@ -196,8 +198,8 @@ struct dentry_operations {
       * dentry into place and return that dentry rather than the passed one,
       * typically using d_splice_alias. */
 
-#define DCACHE_REFERENCED	0x0020  /* Recently used, don't discard. */
-#define DCACHE_RCUACCESS	0x0040	/* Entry has ever been RCU-visible */
+#define DCACHE_REFERENCED	0x0040  /* Recently used, don't discard. */
+#define DCACHE_RCUACCESS	0x0080	/* Entry has ever been RCU-visible */
 
 #define DCACHE_CANT_MOUNT	0x0100
 #define DCACHE_GENOCIDE		0x0200
-- 
1.7.0

[PATCH] vfs: add d_prune dentry operation

[Sage Weil]

This adds a d_prune dentry operation that is called by the VFS prior to 
pruning (i.e. unhashing and killing) a hashed dentry from the dcache. This 
allows the file system to maintain visibility into the contents and 
consistency of the dcache:

 - dentry eviction/pruning now calls into the fs
 - any modifications (unlink, rename, create, etc.) are protected by 
   i_mutex and call into the fs

This will be used by Ceph to maintain a flag indicating whether the 
complete contents of a directory are contained in the dcache, allowing it 
to satisfy lookups and readdir with cached results without additional 
server communication.

Signed-off-by: Sage Weil <sage@newdream.net>
---
 Documentation/filesystems/Locking |    1 +
 fs/dcache.c                       |    8 ++++++++
 include/linux/dcache.h            |    3 +++
 3 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/Documentation/filesystems/Locking b/Documentation/filesystems/Locking
index 57d827d..b3fa7c8 100644
--- a/Documentation/filesystems/Locking
+++ b/Documentation/filesystems/Locking
@@ -29,6 +29,7 @@ d_hash		no		no		no		maybe
 d_compare:	yes		no		no		maybe
 d_delete:	no		yes		no		no
 d_release:	no		no		yes		no
+d_prune:        no              yes             no              no
 d_iput:		no		no		yes		no
 d_dname:	no		no		no		no
 d_automount:	no		no		yes		no
diff --git a/fs/dcache.c b/fs/dcache.c
index 37f72ee..74d1a30 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -673,6 +673,8 @@ static void try_prune_one_dentry(struct dentry *dentry)
 			spin_unlock(&dentry->d_lock);
 			return;
 		}
+		if (dentry->d_flags & DCACHE_OP_PRUNE)
+			dentry->d_op->d_prune(dentry);
 		dentry = dentry_kill(dentry, 1);
 	}
 }
@@ -879,6 +881,8 @@ static void shrink_dcache_for_umount_subtree(struct dentry *dentry)
 
 	/* detach this root from the system */
 	spin_lock(&dentry->d_lock);
+	if (dentry->d_flags & DCACHE_OP_PRUNE)
+		dentry->d_op->d_prune(dentry);
 	dentry_lru_del(dentry);
 	__d_drop(dentry);
 	spin_unlock(&dentry->d_lock);
@@ -895,6 +899,8 @@ static void shrink_dcache_for_umount_subtree(struct dentry *dentry)
 					    d_u.d_child) {
 				spin_lock_nested(&loop->d_lock,
 						DENTRY_D_LOCK_NESTED);
+				if (dentry->d_flags & DCACHE_OP_PRUNE)
+					dentry->d_op->d_prune(dentry);
 				dentry_lru_del(loop);
 				__d_drop(loop);
 				spin_unlock(&loop->d_lock);
@@ -1363,6 +1369,8 @@ void d_set_d_op(struct dentry *dentry, const struct dentry_operations *op)
 		dentry->d_flags |= DCACHE_OP_REVALIDATE;
 	if (op->d_delete)
 		dentry->d_flags |= DCACHE_OP_DELETE;
+	if (op->d_prune)
+		dentry->d_flags |= DCACHE_OP_PRUNE;
 
 }
 EXPORT_SYMBOL(d_set_d_op);
diff --git a/include/linux/dcache.h b/include/linux/dcache.h
index 19d90a5..681f46f 100644
--- a/include/linux/dcache.h
+++ b/include/linux/dcache.h
@@ -165,6 +165,7 @@ struct dentry_operations {
 			unsigned int, const char *, const struct qstr *);
 	int (*d_delete)(const struct dentry *);
 	void (*d_release)(struct dentry *);
+	void (*d_prune)(struct dentry *);
 	void (*d_iput)(struct dentry *, struct inode *);
 	char *(*d_dname)(struct dentry *, char *, int);
 	struct vfsmount *(*d_automount)(struct path *);
@@ -219,6 +220,8 @@ struct dentry_operations {
 #define DCACHE_MANAGED_DENTRY \
 	(DCACHE_MOUNTED|DCACHE_NEED_AUTOMOUNT|DCACHE_MANAGE_TRANSIT)
 
+#define DCACHE_OP_PRUNE         0x80000
+
 extern seqlock_t rename_lock;
 
 static inline int dname_external(struct dentry *dentry)
-- 
1.7.0

Re: [PATCH] vfs: add d_prune dentry operation

[Sage Weil]

Hi Al,

Any chance we can get this in 3.1-rc1?  Not being able to use the dcache 
fully isn't doing good things to our metadata performance.

sage



On Fri, 8 Jul 2011, Sage Weil wrote:

> This adds a d_prune dentry operation that is called by the VFS prior to 
> pruning (i.e. unhashing and killing) a hashed dentry from the dcache. This 
> allows the file system to maintain visibility into the contents and 
> consistency of the dcache:
> 
>  - dentry eviction/pruning now calls into the fs
>  - any modifications (unlink, rename, create, etc.) are protected by 
>    i_mutex and call into the fs
> 
> This will be used by Ceph to maintain a flag indicating whether the 
> complete contents of a directory are contained in the dcache, allowing it 
> to satisfy lookups and readdir with cached results without additional 
> server communication.
> 
> Signed-off-by: Sage Weil <sage@newdream.net>
> ---
>  Documentation/filesystems/Locking |    1 +
>  fs/dcache.c                       |    8 ++++++++
>  include/linux/dcache.h            |    3 +++
>  3 files changed, 12 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/filesystems/Locking b/Documentation/filesystems/Locking
> index 57d827d..b3fa7c8 100644
> --- a/Documentation/filesystems/Locking
> +++ b/Documentation/filesystems/Locking
> @@ -29,6 +29,7 @@ d_hash		no		no		no		maybe
>  d_compare:	yes		no		no		maybe
>  d_delete:	no		yes		no		no
>  d_release:	no		no		yes		no
> +d_prune:        no              yes             no              no
>  d_iput:		no		no		yes		no
>  d_dname:	no		no		no		no
>  d_automount:	no		no		yes		no
> diff --git a/fs/dcache.c b/fs/dcache.c
> index 37f72ee..74d1a30 100644
> --- a/fs/dcache.c
> +++ b/fs/dcache.c
> @@ -673,6 +673,8 @@ static void try_prune_one_dentry(struct dentry *dentry)
>  			spin_unlock(&dentry->d_lock);
>  			return;
>  		}
> +		if (dentry->d_flags & DCACHE_OP_PRUNE)
> +			dentry->d_op->d_prune(dentry);
>  		dentry = dentry_kill(dentry, 1);
>  	}
>  }
> @@ -879,6 +881,8 @@ static void shrink_dcache_for_umount_subtree(struct dentry *dentry)
>  
>  	/* detach this root from the system */
>  	spin_lock(&dentry->d_lock);
> +	if (dentry->d_flags & DCACHE_OP_PRUNE)
> +		dentry->d_op->d_prune(dentry);
>  	dentry_lru_del(dentry);
>  	__d_drop(dentry);
>  	spin_unlock(&dentry->d_lock);
> @@ -895,6 +899,8 @@ static void shrink_dcache_for_umount_subtree(struct dentry *dentry)
>  					    d_u.d_child) {
>  				spin_lock_nested(&loop->d_lock,
>  						DENTRY_D_LOCK_NESTED);
> +				if (dentry->d_flags & DCACHE_OP_PRUNE)
> +					dentry->d_op->d_prune(dentry);
>  				dentry_lru_del(loop);
>  				__d_drop(loop);
>  				spin_unlock(&loop->d_lock);
> @@ -1363,6 +1369,8 @@ void d_set_d_op(struct dentry *dentry, const struct dentry_operations *op)
>  		dentry->d_flags |= DCACHE_OP_REVALIDATE;
>  	if (op->d_delete)
>  		dentry->d_flags |= DCACHE_OP_DELETE;
> +	if (op->d_prune)
> +		dentry->d_flags |= DCACHE_OP_PRUNE;
>  
>  }
>  EXPORT_SYMBOL(d_set_d_op);
> diff --git a/include/linux/dcache.h b/include/linux/dcache.h
> index 19d90a5..681f46f 100644
> --- a/include/linux/dcache.h
> +++ b/include/linux/dcache.h
> @@ -165,6 +165,7 @@ struct dentry_operations {
>  			unsigned int, const char *, const struct qstr *);
>  	int (*d_delete)(const struct dentry *);
>  	void (*d_release)(struct dentry *);
> +	void (*d_prune)(struct dentry *);
>  	void (*d_iput)(struct dentry *, struct inode *);
>  	char *(*d_dname)(struct dentry *, char *, int);
>  	struct vfsmount *(*d_automount)(struct path *);
> @@ -219,6 +220,8 @@ struct dentry_operations {
>  #define DCACHE_MANAGED_DENTRY \
>  	(DCACHE_MOUNTED|DCACHE_NEED_AUTOMOUNT|DCACHE_MANAGE_TRANSIT)
>  
> +#define DCACHE_OP_PRUNE         0x80000
> +
>  extern seqlock_t rename_lock;
>  
>  static inline int dname_external(struct dentry *dentry)
> -- 
> 1.7.0
> 
> --
> 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
> 
>