Skip to contentSkip to conversation

ocfs2: don't depend on DCACHE_DISCONNECTED

RFC PATCH In review J. Bruce Fields 2 participants 6 messages Jul 31, 2012 – Aug 16, 2012

Subsystems (2)
VFS OCFS2
lore
Share & tools
lore

J. Bruce Fields

author
#0 lore raw

RFC PATCH 1/1 ocfs2: don't depend on DCACHE_DISCONNECTED

From: "J. Bruce Fields" <[email protected]>

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 <[email protected]>
---
 fs/ocfs2/dcache.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
fs/ocfs2/dcache.c +1 -1
index e5ba348..2a66620 100644
@@ -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)
Signature · 1 line
1.7.11.2
maintainer
#1 lore raw
Quoted 8 lines · J. Bruce Fields

From: "J. Bruce Fields" <[email protected]>

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 <[email protected]>

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

Signature · 7 lines
"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
author
#2 lore raw
Quoted 11 lines · Joel Becker
Quoted 8 lines · J. Bruce Fields

From: "J. Bruce Fields" <[email protected]>

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 <[email protected]>

NAK. DISCONNECTED is cleared when the dentry is materialized.

Are you sure? ocfs2 uses d_splice_alias in its lookup method, which doesn't clear DISCONNECTED.

(d_materialise_unique does something similar to d_splice_alias and also clears DISCONNECTED. However, I'm almost certain that's a bug in d_materialise_unique. The export code connects a looked-up-by-filehandle directory by doing lookups in parents one step at a time up to the root, only clearing DISCONNECTED once we know that the dentry is connected all the way up to the root. Clearing DISCONNECTED as soon as a single dentry is connected to parents will lead to bugs.)

Quoted 4 lines

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.

Alas, I'm not following you. I'll stare at some of the code and try to understand....

Quoted 3 lines

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.

So you also depend on d_kill setting DISCONNECTED, huh.

--b.

maintainer
#3 lore raw
Quoted 30 lines · J. Bruce Fields
Quoted 11 lines · Joel Becker
Quoted 8 lines · J. Bruce Fields

From: "J. Bruce Fields" <[email protected]>

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 <[email protected]>

NAK. DISCONNECTED is cleared when the dentry is materialized.

Are you sure? ocfs2 uses d_splice_alias in its lookup method, which doesn't clear DISCONNECTED.

(d_materialise_unique does something similar to d_splice_alias and also clears DISCONNECTED. However, I'm almost certain that's a bug in d_materialise_unique. The export code connects a looked-up-by-filehandle directory by doing lookups in parents one step at a time up to the root, only clearing DISCONNECTED once we know that the dentry is connected all the way up to the root. Clearing DISCONNECTED as soon as a single dentry is connected to parents will lead to bugs.)

Quoted 4 lines

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.

Quoted 8 lines

Alas, I'm not following you. I'll stare at some of the code and try to understand....

Quoted 3 lines

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.

So you also depend on d_kill setting DISCONNECTED, huh.

So, I think you are right that we can't be relying on it *that*
much, because splicing the alias doesn't clear it right away. In other words, we rely on other mechanisms to ensure we have our lock attached when the dentry is reachable, but if we're dropping an unreachable dentry, we might not have the lock attached, and we need to detect that.
So your original point, that the code "can't be right", is
really that the code is overly permissive. If we have a reachable tree with DISCONNECTED not yet cleared, that lock should be attached, but this check won't catch it. That's fine. We rely on other code. Conversely, we *know* we can get here with DISCONNECTED set from nfs or d_kill, and we don't want to print errors for a sane state.

Joel

Signature · 5 lines
"You don't make the poor richer by making the rich poorer."
	- Sir Winston Churchill

			http://www.jlbec.org/
			jlbec at evilplan.org
author
#4 lore raw
Quoted 11 lines · Joel Becker

So, I think you are right that we can't be relying on it *that*
much, because splicing the alias doesn't clear it right away. In other words, we rely on other mechanisms to ensure we have our lock attached when the dentry is reachable, but if we're dropping an unreachable dentry, we might not have the lock attached, and we need to detect that.
So your original point, that the code "can't be right", is
really that the code is overly permissive. If we have a reachable tree with DISCONNECTED not yet cleared, that lock should be attached, but this check won't catch it. That's fine. We rely on other code. Conversely, we *know* we can get here with DISCONNECTED set from nfs or d_kill, and we don't want to print errors for a sane state.

OK, so we're depending on the DCACHE_DISCONNECTED check *only* to decide whether to warn, and you don't mind missing some warnings as long as you never warn when you shouldn't. Makes sense, thanks!

--b.

author
#5 lore raw

PATCH 1/1 ocfs2: comment missing-cluster-lock warning

From: "J. Bruce Fields" <[email protected]>

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 <[email protected]>
---
 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.

fs/ocfs2/dcache.c +25 -15
index 8db4b58..27677ab 100644
@@ -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;
 	}
Signature · 1 line
1.7.9.5