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

[Bill O'Donnell <billodo@redhat.com>]

On Fri, Jan 31, 2020 at 07:14:47AM +1100, Dave Chinner wrote:
> 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...

I agree with this approach, with modification of rwsem checking code as
as separate follow-on patchset.
Thanks-
Bill

> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
>