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?
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
>
