Re: [PATCH v5 1/4] xfs: Refactor xfs_isilocked()

[Pavel Reichl <preichl@redhat.com>]

On Wed, Feb 19, 2020 at 7:40 PM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Tue, Feb 18, 2020 at 08:48:21PM -0800, Darrick J. Wong wrote:
> > > > +static inline bool
> > > > +__xfs_rwsem_islocked(
> > > > + struct rw_semaphore     *rwsem,
> > > > + bool                    shared,
> > > > + bool                    excl)
> > > > +{
> > > > + bool locked = false;
> > > > +
> > > > + if (!rwsem_is_locked(rwsem))
> > > > +         return false;
> > > > +
> > > > + if (!debug_locks)
> > > > +         return true;
> > > > +
> > > > + if (shared)
> > > > +         locked = lockdep_is_held_type(rwsem, 0);
> > > > +
> > > > + if (excl)
> > > > +         locked |= lockdep_is_held_type(rwsem, 1);
> > > > +
> > > > + return locked;
> > >
> > > This could use some comments explaining the logic, especially why we
> > > need the shared and excl flags, which seems very confusing given that
> > > a lock can be held either shared or exclusive, but not neither and not
> > > both.
> >
> > Yes, this predicate should document that callers are allowed to pass in
> > shared==true and excl==true when the caller wants to assert that either
> > lock type (shared or excl) of a given lock class (e.g. iolock) are held.
>
> Looking at the lockdep_is_held_type implementation, and our existing
> code for i_rwsem I really don't see the point of the extra shared
> check.  Something like:
>
> static inline bool
> __xfs_rwsem_islocked(
>         struct rw_semaphore     *rwsem,
>         bool                    excl)
> {
>         if (rwsem_is_locked(rwsem)) {
>                 if (debug_locks && excl)
>                         return lockdep_is_held_type(rwsem, 1);
>                 return true;
>         }
>
>         return false;
> }
>
> should be all that we really need.
>

You don't see the point of extra shared check, but if we want to check
that the semaphore is locked for reading and not writing? Having the
semaphore locked for writing would make the code safe from race
condition but could be a performance hit, right?

Thanks for comments.