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@xxxxxxxxxxxxx
>
