Re: [PATCH 05/11] XFS: Fix lock ASSERT on UP

[Dave Chinner <david@fromorbit.com>]

On Mon, Mar 19, 2012 at 07:28:12PM -0700, Andi Kleen wrote:
> > So this means we only ever check that the spinlock is held when
> > lockdep is turned on instead of whenever CONFIG_XFS_DEBUG is set?
> 
> You should regularly test with lockdep anyways.

I do. Once or twice a release cycle I'll do a test cycle with
lockdep enabled. i don't use it on day-to-day tst cycles because of
it's overhead, and well, I understand the lock ordering of most of
the code I'm modifying....

> If you don't you clearly
> have a testing gap. lockdep is likely to find many more locking bugs
> than any of your very sparse manual annotations.

That's not true. Lockdep validates lock order but it doesn't find
race conditions. Nor does lockdep tell us that the correct lock is
held when entering the function. Nor does it tell us that the state
of a RCU freed object is correct when it is reallocated out of the
kmem_cache it belongs to. These things need manual annotations, and
they find more problems during development than lockdep does.

Not to mention that we have to annotate our read/write locks with
extra state when CONFIG_XFS_DEBUG is enabled to ensure that the lock
mode is correct because rwsems can't tell us the difference between
a read hold and a write hold.

Hell, we only check the inode lock state in about 40 different
locations via xfs_isilocked(), but many of these locations have
multiple callers. e.g.  there's a check in xfs_trans_ijoin(), and it
has about 60 callers.  So the "very sparse manual annotations" for
locking behaviour in XFS are far more comprehensive than you claim
and catch many more problems early in development under
CONFIG_XFS_DEBUG than lockdep is even aware exists....

> > That means it will rarely get checked during development instead of
> > all the time. That's not an improvement IMO....
> 
> It's an improvement that an !CONFIG_SMP && CONFIG_XFS_DEBUG kernel
> will not blow up anymore.

<shrug>

Whatever, it's not worth arguing over. There's only two places that
XFS uses spin_is_locked(), and there are other checks around it that
will catch the same race conditions with CONFIG_XFS_DEBUG enabled,
so go ahead and use lockdep for them.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com