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

[Dave Chinner <david@fromorbit.com>]

On Tue, Jan 28, 2020 at 08:42:00AM -0800, Darrick J. Wong wrote:
> On Tue, Jan 28, 2020 at 03:55:25PM +0100, Pavel Reichl wrote:
> > mr_writer is obsolete and the information it contains is accesible
> > from mr_lock.
> > 
> > Signed-off-by: Pavel Reichl <preichl@redhat.com>
> > ---
> >  fs/xfs/xfs_inode.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > index c5077e6326c7..32fac6152dc3 100644
> > --- a/fs/xfs/xfs_inode.c
> > +++ b/fs/xfs/xfs_inode.c
> > @@ -352,13 +352,17 @@ xfs_isilocked(
> >  {
> >  	if (lock_flags & (XFS_ILOCK_EXCL|XFS_ILOCK_SHARED)) {
> >  		if (!(lock_flags & XFS_ILOCK_SHARED))
> > -			return !!ip->i_lock.mr_writer;
> > +			return !debug_locks ||
> > +				lockdep_is_held_type(&ip->i_lock.mr_lock, 0);
> 
> Why do we reference debug_locks here directly?  It looks as though that
> variable exists to shut up lockdep assertions WARN_ONs, but
> xfs_isilocked is a predicate (and not itself an assertion), so why can't
> we 'return lockdep_is_held_type(...);' directly?

It's because that's the way lockdep is structured. That is, lockdep
turns off when the first error is reported, and debug_locks is the
variable used to turn lockdep warnings/checking off once an error
has occurred.

It is normally wrapped in lockdep_assert...() macros so you don't
see it, but it is not referenced at all inside the lockdep functions
that do the actual lock state checking. Hence to replicate lockdep
behaviour, we have to check it, too.

The lockdep code now has these wrappers for rwsems:

#define lockdep_assert_held_write(l)    do {                    \
                WARN_ON(debug_locks && !lockdep_is_held_type(l, 0));    \
        } while (0)

#define lockdep_assert_held_read(l)     do {                            \
                WARN_ON(debug_locks && !lockdep_is_held_type(l, 1));    \
        } while (0)

But xfs_isilocked() is called from within ASSERT() calls, so we
don't want WARN_ON() calls within the ASSERT() calls which provide
their own WARN/BUG handling.

IOWs, we essentially open coded lockdep_assert_held_read (and now
_write) to fit into our own framework of lock checking.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com