Messages in this thread | | | Date | Wed, 21 Mar 2012 11:44:39 +1100 | From | Dave Chinner <> | Subject | Re: [PATCH 05/11] XFS: Fix lock ASSERT on UP |
| |
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
| |