[PATCH] xfs: provide a inode iolock lockdep class

[PATCH] xfs: provide a inode iolock lockdep class

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

The XFS iolock needs to be re-initialised to a new lock class before
it enters reclaim to prevent lockdep false positives. Unfortunately,
this is not sufficient protection as inodes in the XFS_IRECLAIMABLE
state can be recycled and not re-initialised before being reused.

We need to re-initialise the lock state when transfering out of
XFS_IRECLAIMABLE state to XFS_INEW, but we need to keep the same
class as if the inode was just allocated. Hence we need a specific
lockdep class variable for the iolock so that both initialisations
use the same class.

While there, add a specific class for inodes in the reclaim state so
that it is easy to tell from lockdep reports what state the inode
was in that generated the report.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/linux-2.6/xfs_super.c |    2 ++
 fs/xfs/xfs_iget.c            |   19 +++++++++++++++++++
 fs/xfs/xfs_inode.h           |    2 ++
 3 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/fs/xfs/linux-2.6/xfs_super.c b/fs/xfs/linux-2.6/xfs_super.c
index 064f964..c45b323 100644
--- a/fs/xfs/linux-2.6/xfs_super.c
+++ b/fs/xfs/linux-2.6/xfs_super.c
@@ -1118,6 +1118,8 @@ xfs_fs_evict_inode(
 	 */
 	ASSERT(!rwsem_is_locked(&ip->i_iolock.mr_lock));
 	mrlock_init(&ip->i_iolock, MRLOCK_BARRIER, "xfsio", ip->i_ino);
+	lockdep_set_class_and_name(&ip->i_iolock.mr_lock,
+			&xfs_iolock_reclaimable, "xfs_iolock_reclaimable");
 
 	xfs_inactive(ip);
 }
diff --git a/fs/xfs/xfs_iget.c b/fs/xfs/xfs_iget.c
index 0cdd269..cdb1c25 100644
--- a/fs/xfs/xfs_iget.c
+++ b/fs/xfs/xfs_iget.c
@@ -43,6 +43,17 @@
 
 
 /*
+ * Define xfs inode iolock lockdep classes. We need to ensure that all active
+ * inodes are considered the same for lockdep purposes, including inodes that
+ * are recycled through the XFS_IRECLAIMABLE state. This is the the only way to
+ * guarantee the locks are considered the same when there are multiple lock
+ * initialisation siteѕ. Also, define a reclaimable inode class so it is
+ * obvious in lockdep reports which class the report is against.
+ */
+static struct lock_class_key xfs_iolock_active;
+struct lock_class_key xfs_iolock_reclaimable;
+
+/*
  * Allocate and initialise an xfs_inode.
  */
 STATIC struct xfs_inode *
@@ -71,6 +82,8 @@ xfs_inode_alloc(
 	ASSERT(completion_done(&ip->i_flush));
 
 	mrlock_init(&ip->i_iolock, MRLOCK_BARRIER, "xfsio", ip->i_ino);
+	lockdep_set_class_and_name(&ip->i_iolock.mr_lock,
+			&xfs_iolock_active, "xfs_iolock_active");
 
 	/* initialise the xfs inode */
 	ip->i_ino = ino;
@@ -218,6 +231,12 @@ xfs_iget_cache_hit(
 		ip->i_flags |= XFS_INEW;
 		__xfs_inode_clear_reclaim_tag(mp, pag, ip);
 		inode->i_state = I_NEW;
+
+		ASSERT(!rwsem_is_locked(&ip->i_iolock.mr_lock));
+		mrlock_init(&ip->i_iolock, MRLOCK_BARRIER, "xfsio", ip->i_ino);
+		lockdep_set_class_and_name(&ip->i_iolock.mr_lock,
+				&xfs_iolock_active, "xfs_iolock_active");
+
 		spin_unlock(&ip->i_flags_lock);
 		write_unlock(&pag->pag_ici_lock);
 	} else {
diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
index fb2ca2e..1c6514d 100644
--- a/fs/xfs/xfs_inode.h
+++ b/fs/xfs/xfs_inode.h
@@ -438,6 +438,8 @@ static inline void xfs_ifunlock(xfs_inode_t *ip)
 #define XFS_IOLOCK_DEP(flags)	(((flags) & XFS_IOLOCK_DEP_MASK) >> XFS_IOLOCK_SHIFT)
 #define XFS_ILOCK_DEP(flags)	(((flags) & XFS_ILOCK_DEP_MASK) >> XFS_ILOCK_SHIFT)
 
+extern struct lock_class_key xfs_iolock_reclaimable;
+
 /*
  * Flags for xfs_itruncate_start().
  */
-- 
1.7.2.3

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: provide a inode iolock lockdep class

[Christoph Hellwig]

On Mon, Dec 13, 2010 at 12:07:11PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> The XFS iolock needs to be re-initialised to a new lock class before
> it enters reclaim to prevent lockdep false positives. Unfortunately,
> this is not sufficient protection as inodes in the XFS_IRECLAIMABLE
> state can be recycled and not re-initialised before being reused.
> 
> We need to re-initialise the lock state when transfering out of
> XFS_IRECLAIMABLE state to XFS_INEW, but we need to keep the same
> class as if the inode was just allocated. Hence we need a specific
> lockdep class variable for the iolock so that both initialisations
> use the same class.
> 
> While there, add a specific class for inodes in the reclaim state so
> that it is easy to tell from lockdep reports what state the inode
> was in that generated the report.

Looks good to me.  As long as we have the mrlock abstraction we might as
well hide this behind it, but as I plan on killing the abstraction
that's probably not worth the effort.


Reviewed-by: Christoph Hellwig <hch@lst.de>

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: provide a inode iolock lockdep class

[Dave Chinner]

On Wed, Dec 15, 2010 at 07:18:11AM -0500, Christoph Hellwig wrote:
> On Mon, Dec 13, 2010 at 12:07:11PM +1100, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > The XFS iolock needs to be re-initialised to a new lock class before
> > it enters reclaim to prevent lockdep false positives. Unfortunately,
> > this is not sufficient protection as inodes in the XFS_IRECLAIMABLE
> > state can be recycled and not re-initialised before being reused.
> > 
> > We need to re-initialise the lock state when transfering out of
> > XFS_IRECLAIMABLE state to XFS_INEW, but we need to keep the same
> > class as if the inode was just allocated. Hence we need a specific
> > lockdep class variable for the iolock so that both initialisations
> > use the same class.
> > 
> > While there, add a specific class for inodes in the reclaim state so
> > that it is easy to tell from lockdep reports what state the inode
> > was in that generated the report.
> 
> Looks good to me.  As long as we have the mrlock abstraction we might as
> well hide this behind it, but as I plan on killing the abstraction
> that's probably not worth the effort.

I thought about doing that, too, after looking at what the abstraction
still provides us with. I think it only provides an extra "is write
locked" debug check which I think could be added to the generic
rwsem code pretty easily. But I've got a big enough patch stack out
for review right now. ;)

> Reviewed-by: Christoph Hellwig <hch@lst.de>

Thanks, I'll add it to the for-2.6.38 stack.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs