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

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Fri, Mar 20, 2020 at 03:41:08PM +0100, Pavel Reichl wrote:
> On Wed, Mar 18, 2020 at 7:50 PM Darrick J. Wong <darrick.wong@oracle.com> wrote:
> >
> > On Wed, Mar 18, 2020 at 12:46:37PM -0500, Eric Sandeen wrote:
> > > On 3/18/20 12:13 PM, Pavel Reichl wrote:
> > > > On Fri, Feb 28, 2020 at 6:10 PM Darrick J. Wong <darrick.wong@oracle.com> wrote:
> > >
> > > ...
> > >
> > > >> So, this function's call signature should change so that callers can
> > > >> communicate both _SHARED and _EXCL; and then you can pick the correct
> > > >
> > > > Thanks for the suggestion...but that's how v5 signature looked like
> > > > before Christoph and Eric requested change...on the grounds that
> > > > there're:
> > > > *  confusion over a (true, true) set of args
> > > > *  confusion of what happens if we pass (false, false).
> >
> > Yeah.  I don't mean adding back the dual booleans, I meant refactoring
> > the way we define the lock constants so that you can use bit shifting
> > and masking:
> >
> > #define XFS_IOLOCK_SHIFT        0
> > #define XFS_ILOCK_SHIFT         2
> > #define XFS_MMAPLOCK_SHIFT      4
> >
> > #define XFS_SHARED_LOCK_SHIFT   1
> >
> > #define XFS_IOLOCK_EXCL     (1 << (XFS_IOLOCK_SHIFT))
> > #define XFS_IOLOCK_SHARED   (1 << (XFS_IOLOCK_SHIFT + XFS_SHARED_LOCK_SHIFT))
> > #define XFS_ILOCK_EXCL      (1 << (XFS_ILOCK_SHIFT))
> > #define XFS_ILOCK_SHARED    (1 << (XFS_ILOCK_SHIFT + XFS_SHARED_LOCK_SHIFT))
> > #define XFS_MMAPLOCK_EXCL   (1 << (XFS_MMAPLOCK_SHIFT))
> > #define XFS_MMAPLOCK_SHARED (1 << (XFS_MMAPLOCK_SHIFT + XFS_SHARED_LOCK_SHIFT))
> 
> Thank you for the code - now I see what you meant and I like it,
> however allow me a question:
> Are you aware that XFS_IOLOCK_SHIFT, XFS_MMAPLOCK_SHIFT,
> XFS_ILOCK_SHIFT are already defined with different values and used in
> xfs_lock_inumorder()?
> 
> I have no trouble to investigate the code and see if it is OK i.g.
> XFS_IOLOCK_EXCL to be 21 (I guess I should check that no bit arrays
> are used to store the value, etc)
> 
> Or maybe I should just rewrite  the '#define XFS_IOLOCK_SHIFT
> 0' to something like '#define XFS_IOLOCK_TYPE_SHIFT        0' ?
> 
> Do you have any thoughts about that?

XFS_IOLOCK_TYPE_SHIFT seems fine to me to avoid clashing with lockdep. :)

(perhaps XFS_IOLOCK_FLAG_SHIFT?)

--D

> 
> Thanks!
> 
> 
> >
> > Because then in the outer xfs_isilocked function you can do:
> >
> > if (lock_flags & (XFS_ILOCK_EXCL | XFS_ILOCK_SHARED))
> >         return __isilocked(&ip->i_lock, lock_flags >> XFS_ILOCK_SHIFT);
> >
> > if (lock_flags & (XFS_MMAPLOCK_EXCL | XFS_MMAPLOCK_SHARED))
> >         return __isilocked(&ip->i_mmaplock, lock_flags >> XFS_MMAPLOCK_SHIFT);
> >
> > if (lock_flags & (XFS_IOLOCK_EXCL | XFS_IOLOCK_SHARED))
> >         return __isilocked(&VFS_I(ip)->i_rwsem, lock_flags >> XFS_IOLOCK_SHIFT);
> >
> > And finally in __isilocked you can do:
> >
> > static inline bool __isilocked(rwsem, lock_flags)
> > {
> >         int     arg;
> >
> >         if (!debug_locks)
> >                 return rwsem_is_locked(rwsem);
> >
> >         if (lock_flags & (1 << XFS_SHARED_LOCK_SHIFT)) {
> >                 /*
> >                  * The caller could be asking if we have (shared | excl)
> >                  * access to the lock.  Ask lockdep if the rwsem is
> >                  * locked either for read or write access.
> >                  *
> >                  * The caller could also be asking if we have only
> >                  * shared access to the lock.  Holding a rwsem
> >                  * write-locked implies read access as well, so the
> >                  * request to lockdep is the same for this case.
> >                  */
> >                 arg = -1;
> >         } else {
> >                 /*
> >                  * The caller is asking if we have only exclusive access
> >                  * to the lock.  Ask lockdep if the rwsem is locked for
> >                  * write access.
> >                  */
> >                 arg = 0;
> >         }
> >         return lockdep_is_held_type(rwsem, arg);
> > }
> >
> > > >> "r" parameter value for the lockdep_is_held_type() call.  Then all of
> > > >> this becomes:
> > > >>
> > > >>         if !debug_locks:
> > > >>                 return rwsem_is_locked(rwsem)
> > > >>
> > > >>         if shared and excl:
> > > >>                 r = -1
> > > >>         elif shared:
> > > >>                 r = 1
> > > >>         else:
> > > >>                 r = 0
> > > >>         return lockdep_is_held_type(rwsem, r)
> > > >
> > > > I tried to create a table for this code as well:
> > >
> > > <adding back the table headers>
> > >
> > > > (nolockdep corresponds to debug_locks == 0)
> > > >
> > > > RWSEM STATE             PARAMETERS TO XFS_ISILOCKED:
> > > >                         SHARED  EXCL    SHARED | EXCL
> > > > readlocked              y       n       y
> > > > writelocked             *n*     y       y
> > > > unlocked                n       n       n
> > > > nolockdep readlocked    y       y       y
> > > > nolockdep writelocked   y       y       y
> > > > nolockdep unlocked      n       n       n
> > > >
> > > > I think that when we query writelocked lock for being shared having
> > > > 'no' for an answer may not be expected...or at least this is how I
> > > > read the code.
> > >
> > > This might be ok, because
> > > a) it is technically correct (is it shared? /no/ it is exclusive), and
> > > b) in the XFS code today we never call:
> > >
> > >       xfs_isilocked(ip, XFS_ILOCK_SHARED);
> > >
> > > it's always:
> > >
> > >       xfs_isilocked(ip, XFS_ILOCK_SHARED | XFS_ILOCK_EXCL);
> > >
> > > So I think that if we document the behavior clearly, the truth table above
> > > would be ok.
> > >
> > > Thoughts?
> >
> > No, Pavel's right, I got the pseudocode wrong, because holding a write
> > lock means you also hold the read lock.
> >
> >         if !debug_locks:
> >                 return rwsem_is_locked(rwsem)
> >
> >         if shared:
> >                 r = -1
> >         else:
> >                 r = 0
> >         return lockdep_is_held_type(rwsem, r)
> >
> > --D
> >
> > > -Eric
> >
>