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