Re: [PATCH 1/4] xfs: change xfs_isilocked() to always use lockdep()

[Dave Chinner <david@fromorbit.com>]

On Wed, Jan 29, 2020 at 11:44:24PM -0800, Christoph Hellwig wrote:
> On Thu, Jan 30, 2020 at 09:18:19AM +1100, Dave Chinner wrote:
> > This captures both read and write locks on the rwsem, and doesn't
> > discriminate at all. Now we don't have explicit writer lock checking
> > in CONFIG_XFS_DEBUG=y kernels, I think we need to at least check
> > that the rwsem is locked in all cases to catch cases where we are
> > calling a function without the lock held. That will ctach most
> > programming mistakes, and then lockdep will provide the
> > read-vs-write discrimination to catch the "hold the wrong lock type"
> > mistakes.
> > 
> > Hence I think this code should end up looking like this:
> > 
> > 	if (lock_flags & (XFS_ILOCK_EXCL|XFS_ILOCK_SHARED)) {
> > 		bool locked = false;
> > 
> > 		if (!rwsem_is_locked(&ip->i_lock))
> > 			return false;
> > 		if (!debug_locks)
> > 			return true;
> > 		if (lock_flags & XFS_ILOCK_EXCL)
> > 			locked = lockdep_is_held_type(&ip->i_lock, 0);
> > 		if (lock_flags & XFS_ILOCK_SHARED)
> > 			locked |= lockdep_is_held_type(&ip->i_lock, 1);
> > 		return locked;
> > 	}
> > 
> > Thoughts?
> 
> I like the idea, but I really think that this does not belong into XFS,
> but into the core rwsem code.  That means replacing the lock_flags with
> a bool exclusive, picking a good name for it (can't think of one right
> now, except for re-using rwsem_is_locked), and adding a kerneldoc
> comment explaining the semantics and use cases in detail.

I'd say that's the step after removing mrlocks in XFS. Get this
patchset sorted, then lift the rwsem checking function to the core
code as a separate patchset that can be handled indepedently to the
changes we need to make to XFS...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com