Re: next-20090220: XFS: inconsistent lock state

[Felix Blyakher]

On Mar 3, 2009, at 10:00 AM, Eric Sandeen wrote:

> Christoph Hellwig wrote:
> > On Fri, Feb 20, 2009 at 08:52:59PM +0300, Alexander Beregalov wrote:
> > > Hi
> > >
> > > [ INFO: inconsistent lock state ]
> > > 2.6.29-rc5-next-20090220 #2
> > > ---------------------------------
> > > inconsistent {RECLAIM_FS-ON-W} -> {IN-RECLAIM_FS-R} usage.
> > > kswapd0/324 [HC0[0]:SC0[0]:HE1:SE1] takes:
> > > (&(&ip->i_lock)->mr_lock){+++++?}, at: [<ffffffff803ca60a>]
> > > xfs_ilock+0xaa/0x120
> > > {RECLAIM_FS-ON-W} state was registered at:
> >
> > That's a false positive.  While the ilock can be taken in reclaim the
> > allocation here is done before the inode is added to the inode cache.
> >
> > The patch below should help avoiding the warning:
>
> Seems ok to me.  I hate to see the BUG() added but I guess in this  
> case
> something truly bizarre would have to happen for the ilock to fail on
> this inode.
>
> on irc you sugggested ASSERT(0); instead of BUG();

That would mean that instead of bombing out here, we do it
in xfs debug kernels only, which is a good thing. However,
do we just silently ignore it in non debug kernels, and
later try to unlock without locking first?
Maybe the following be better:

	if (lock_flags) {
		if (!xfs_ilock_nowait(ip, lock_flags)) {
			ASSERT(0);
			error = EAGAIN;
			goto out_destroy;
		}
	}
				
Or just keep the BUG(); , as it shouldn't happen (we hope).

Reviewed-by: Felix Blyakher <felixb@xxxxxxx>


> I might prefer that
> but either way:
>
> Reviewed-by: Eric Sandeen <sandeen@xxxxxxxxxxx>
>
> > Index: xfs/fs/xfs/xfs_iget.c
> > ===================================================================
> > --- xfs.orig/fs/xfs/xfs_iget.c	2009-02-24 20:56:00.716027739 +0100
> > +++ xfs/fs/xfs/xfs_iget.c	2009-02-24 20:56:46.089031360 +0100
> > @@ -246,9 +246,6 @@ xfs_iget_cache_miss(
> > 		goto out_destroy;
> > 	}
> >
> > -	if (lock_flags)
> > -		xfs_ilock(ip, lock_flags);
> > -
> > 	/*
> > 	 * Preload the radix tree so we can insert safely under the
> > 	 * write spinlock. Note that we cannot sleep inside the preload
> > @@ -259,6 +256,15 @@ xfs_iget_cache_miss(
> > 		goto out_unlock;
> > 	}
> >
> > +	/*
> > +	 * Because the inode hasn't been added to the radix-tree yet it  
> > can't
> > +	 * be found by another thread, so we can do the non-sleeping lock  
> > here.
> > +	 */
> > +	if (lock_flags) {
> > +		if (!xfs_ilock_nowait(ip, lock_flags))
> > +			BUG();
> > +	}
> > +
> > 	mask = ~(((XFS_INODE_CLUSTER_SIZE(mp) >> mp->m_sb.sb_inodelog)) -  
> > 1);
> > 	first_index = agino & mask;
> > 	write_lock(&pag->pag_ici_lock);
> >
> > _______________________________________________
> > xfs mailing list
> > xfs@xxxxxxxxxxx
> > http://oss.sgi.com/mailman/listinfo/xfs
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux- 
> kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/