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

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

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

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