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