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

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

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@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@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

[RFC PATCH] ocfs2: don't depend on DCACHE_DISCONNECTED

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@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@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

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

[Joel Becker]

On Tue, Jul 31, 2012 at 06:33:23PM -0400, J. Bruce Fields wrote:
> From: "J. Bruce Fields" <bfields@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@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

Re: [RFC PATCH] ocfs2: don't depend on DCACHE_DISCONNECTED

[Joel Becker]

On Tue, Jul 31, 2012 at 06:33:23PM -0400, J. Bruce Fields wrote:
> From: "J. Bruce Fields" <bfields@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@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@evilplan.org

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

[J. Bruce Fields]

On Thu, Aug 02, 2012 at 12:57:44AM -0700, Joel Becker wrote:
> On Tue, Jul 31, 2012 at 06:33:23PM -0400, J. Bruce Fields wrote:
> > From: "J. Bruce Fields" <bfields@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@redhat.com>
> 
> 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.)

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

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

Re: [RFC PATCH] ocfs2: don't depend on DCACHE_DISCONNECTED

[J. Bruce Fields]

On Thu, Aug 02, 2012 at 12:57:44AM -0700, Joel Becker wrote:
> On Tue, Jul 31, 2012 at 06:33:23PM -0400, J. Bruce Fields wrote:
> > From: "J. Bruce Fields" <bfields@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@redhat.com>
> 
> 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.)

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

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

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

[Joel Becker]

On Thu, Aug 02, 2012 at 08:59:10AM -0400, J. Bruce Fields wrote:
> On Thu, Aug 02, 2012 at 12:57:44AM -0700, Joel Becker wrote:
> > On Tue, Jul 31, 2012 at 06:33:23PM -0400, J. Bruce Fields wrote:
> > > From: "J. Bruce Fields" <bfields@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@redhat.com>
> > 
> > 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.)
> 
> > 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....
> 
> > 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

-- 

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

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

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

[Joel Becker]

On Thu, Aug 02, 2012 at 08:59:10AM -0400, J. Bruce Fields wrote:
> On Thu, Aug 02, 2012 at 12:57:44AM -0700, Joel Becker wrote:
> > On Tue, Jul 31, 2012 at 06:33:23PM -0400, J. Bruce Fields wrote:
> > > From: "J. Bruce Fields" <bfields@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@redhat.com>
> > 
> > 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.)
> 
> > 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....
> 
> > 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

-- 

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

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

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

[J. Bruce Fields]

On Wed, Aug 15, 2012 at 03:22:42AM -0700, Joel Becker wrote:
> 	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.

[PATCH] ocfs2: comment missing-cluster-lock warning

[J. Bruce Fields]

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