xfs
[Top] [All Lists]

Re: XFS reclaim lock order bug

To: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
Subject: Re: XFS reclaim lock order bug
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Thu, 25 Nov 2010 21:32:00 +1100
Cc: Nick Piggin <npiggin@xxxxxxxxx>, Christoph Hellwig <hch@xxxxxxxxxxxxx>, xfs@xxxxxxxxxxx, Ingo Molnar <mingo@xxxxxxxxxx>
In-reply-to: <1290670097.2072.554.camel@laptop>
References: <20101123121802.GA4785@amd> <20101123211258.GY22876@dastard> <20101124200341.GA2493@xxxxxxxxxxxxx> <20101125034824.GA3359@amd> <1290666325.2072.535.camel@laptop> <20101125070817.GA4120@amd> <1290670097.2072.554.camel@laptop>
User-agent: Mutt/1.5.20 (2009-06-14)
On Thu, Nov 25, 2010 at 08:28:17AM +0100, Peter Zijlstra wrote:
> On Thu, 2010-11-25 at 18:08 +1100, Nick Piggin wrote:
> > > +static struct lock_class_key xfs_dead_inode;
> > > +
> > >  STATIC void
> > >  xfs_fs_evict_inode(
> > >       struct inode            *inode)
> > > @@ -1118,6 +1120,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_dead_inode, 
> > > +                     "xfd_dead_inode");
> > >  
> > >       xfs_inactive(ip);
> > >  }
> > 
> > With this change, I assume the mrlock_init can go? (it would be nice
> > to have a wrapper to allocate the class by itself)
> 
> 
> mrlock_init() does allocate a class (well rwsem_init, really), but sets
> the name to a stringified version of the lock argument.
> 
> The lockdep_set_class*() interface is only guaranteed to work on a
> freshly initialized lock structure -- which in this case is a bit of a
> waste, but for debugging purposes would allow setting a clearer name.
> 
> Alternatively, you can write the code like:
> 
>   xfs_inode_t dead_ip = XFS_I(inode);
> 
>   mrlock_init(&dead_ip->i_iolock, ...);
> 
> In which case its also obvious, as that would result in:
> 
>    (&(&dead_ip->i_iolock)->mr_lock)
> 
> as opposed to:
> 
>    (&(&ip->i_iolock)->mr_lock)

Ok, that's a handy trick to know. I'll try and sort this out
tomorrow and make use of this trick to help identify the different
lock classes.

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

<Prev in Thread] Current Thread [Next in Thread>