Ocfs2 devel - Jul 2012 - [RFC PATCH] ocfs2: don't depend on DCACHE_DISCONNECTED

From: "J. Bruce Fields" <bfields at redhat.com>

XXX: I don't understand this code, but I also can't see how it can be
right as is: a dentry marked DCACHE_DISCONNECTED can in fact be a
fully-connected member of the dcache.  Is IS_ROOT() the right check
instead?

Signed-off-by: J. Bruce Fields <bfields at redhat.com>
---
 fs/ocfs2/dcache.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ocfs2/dcache.c b/fs/ocfs2/dcache.c
index e5ba348..2a66620 100644
--- a/fs/ocfs2/dcache.c
+++ b/fs/ocfs2/dcache.c
@@ -461,7 +461,7 @@ static void ocfs2_dentry_iput(struct dentry *dentry, struct
inode *inode)
 		 * No dentry lock is ok if we're disconnected or
 		 * unhashed.
 		 */
-		if (!(dentry->d_flags & DCACHE_DISCONNECTED) &&
+		if (!IS_ROOT(dentry)) &&
 		    !d_unhashed(dentry)) {
 			unsigned long long ino = 0ULL;
 			if (inode)
-- 
1.7.11.2

On Tue, Jul 31, 2012 at 06:33:23PM -0400, J. Bruce Fields
wrote:
> From: "J. Bruce Fields" <bfields at redhat.com>
> 
> XXX: I don't understand this code, but I also can't see how it can
be
> right as is: a dentry marked DCACHE_DISCONNECTED can in fact be a
> fully-connected member of the dcache.  Is IS_ROOT() the right check
> instead?
> 
> Signed-off-by: J. Bruce Fields <bfields at redhat.com>
NAK.  DISCONNECTED is cleared when the dentry is materialized.

Here's the context.  When an ocfs2 dentry is discoverable via the tree
(lookup or splicing an alias), we hold a cluster lock (the "dentry
lock").  This is why we override d_move(), to make sure that state is
kept sane.  That way, other nodes can communicate unlink to this node.
They notify our node via the locking system, which does a
d_delete()+dput(), which sets DISCONNECTED.  When the dentry gets its
final put, we can properly accept an empty lock.

Joel

-- 

"The one important thing i have learned over the years is the
 difference between taking one's work seriously and taking one's self
 seriously.  The first is imperative and the second is disastrous."
	-Margot Fonteyn

			http://www.jlbec.org/
			jlbec at evilplan.org

From: "J. Bruce Fields" <bfields@redhat.com>

Add some explanation of the DCACHE_DISCONNECTED use here.  And hide away
the boring warning code in a helper function.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/ocfs2/dcache.c |   40 +++++++++++++++++++++++++---------------
 1 file changed, 25 insertions(+), 15 deletions(-)

By the way, something like this might save me a little time next time I
run across this and forget this conversation.  But, no big deal if you
think it''s overkill.  Untested except by a make fs/ocfs2/dcache.o.

diff --git a/fs/ocfs2/dcache.c b/fs/ocfs2/dcache.c
index 8db4b58..27677ab 100644
--- a/fs/ocfs2/dcache.c
+++ b/fs/ocfs2/dcache.c
@@ -446,26 +446,36 @@ void ocfs2_dentry_lock_put(struct ocfs2_super *osb,
 		ocfs2_drop_dentry_lock(osb, dl);
 }
 
+static void ocfs2_warn_if_missing_lock(struct dentry *dentry, struct inode
*inode)
+{
+	unsigned long long ino = 0ULL;
+
+	/* It''s OK not to have the dentry lock on an unhashed dentry: */
+	if (d_unhashed(dentry))
+		return;
+	/*
+	 * It''s also fine if we''re disconnected.  (Note: a dentry
may be
+	 * marked DCACHE_DISCONNECTED for a brief time after it''s been
+	 * fully connected, so we may fail to warn in some cases we
+	 * should.  We prefer that to warning when we shouldn''t.)
+	 */
+	if (dentry->d_flags & DCACHE_DISCONNECTED)
+		return;
+
+	if (inode)
+		ino = (unsigned long long)OCFS2_I(inode)->ip_blkno;
+	mlog(ML_ERROR, "Dentry is missing cluster lock. "
+	     "inode: %llu, d_flags: 0x%x, d_name: %.*s\n",
+	     ino, dentry->d_flags, dentry->d_name.len,
+	     dentry->d_name.name);
+}
+
 static void ocfs2_dentry_iput(struct dentry *dentry, struct inode *inode)
 {
 	struct ocfs2_dentry_lock *dl = dentry->d_fsdata;
 
 	if (!dl) {
-		/*
-		 * No dentry lock is ok if we''re disconnected or
-		 * unhashed.
-		 */
-		if (!(dentry->d_flags & DCACHE_DISCONNECTED) &&
-		    !d_unhashed(dentry)) {
-			unsigned long long ino = 0ULL;
-			if (inode)
-				ino = (unsigned long long)OCFS2_I(inode)->ip_blkno;
-			mlog(ML_ERROR, "Dentry is missing cluster lock. "
-			     "inode: %llu, d_flags: 0x%x, d_name: %.*s\n",
-			     ino, dentry->d_flags, dentry->d_name.len,
-			     dentry->d_name.name);
-		}
-
+		ocfs2_warn_if_missing_lock(dentry, inode);
 		goto out;
 	}
 
-- 
1.7.9.5

--
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