Re: [PATCH v2 2/7] xfs: Update checking excl. locks for ilock

[Eric Sandeen <sandeen@sandeen.net>]

On 2/4/20 3:06 PM, Dave Chinner wrote:
> On Tue, Feb 04, 2020 at 10:08:45AM -0600, Eric Sandeen wrote:
>> On 2/4/20 12:21 AM, Christoph Hellwig wrote:
>>>> -	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
>>>> +	ASSERT(xfs_is_ilocked(ip, XFS_ILOCK_EXCL));
>>>
>>> I think this is a very bad interface.  Either we keep our good old
>>> xfs_isilocked that just operates on the inode and lock flags, or
>>> we use something that gets the actual lock passed.  But an interface
>>> that encodes the lock in both the function called and the flags, and
>>> one that doesn't follow neither the XFS lock flags conventions nor
>>> the core kernel convention is just not very useful.
>>
>> I think this came out of Dave's suggestion on the previous patchset,
>> but I agree with you Chrisoph.  Even if there is a future reason to
>> split it out into a function for each type, I don't see a reason to
>> do it now, and this interface is awkward.
>>
>> I'd prefer to keep xfs_isilocked() with the current calling convention and
>> just change its internals to use lockdep.  Dave spotted a bug in the
>> current implementation, but I think that can be fixed.
>>
>> Splitting out the 3 lock testing functions seems to me like complexity
>> creep that doesn't need to be in this series.
>>
>> Dave, thoughts?
> 
> All I care about is that we get rid of the mrlock_t. I'm not
> interested in bikeshedding the details to death. I've put my 2c
> worth in, if you don't like it, then that's fine and I'm not going
> to get upset about that.

In the short term I'd suggest something like this. It keeps the helper,
but we don't have to change the callsites, and breaking out separate types
etc can be done after the mrlock removal patchset is done - in a separate
series if/when needed.

static inline bool
__xfs_rwsem_islocked(
        struct rw_semaphore     *rwsem,
        bool                    shared,
        bool                    excl)
{
        bool locked = false;

        if (!rwsem_is_locked(rwsem))
                return false;

        if (!debug_locks)
                return true;

        if (shared)
                locked = lockdep_is_held_type(rwsem, 0);

        if (excl)
                locked |= lockdep_is_held_type(rwsem, 1);

        return locked;
}

bool
xfs_isilocked(
        struct xfs_inode        *ip,
        uint                    lock_flags)
{       
        if (lock_flags & (XFS_ILOCK_EXCL|XFS_ILOCK_SHARED)) {
                return __xfs_rwsem_islocked(&ip->i_lock.mr_lock,
                                (lock_flags & XFS_ILOCK_SHARED),
                                (lock_flags & XFS_ILOCK_EXCL));
        }
        if (lock_flags & (XFS_MMAPLOCK_EXCL|XFS_MMAPLOCK_SHARED)) {
                return __xfs_rwsem_islocked(&ip->i_mmlock.mr_lock,
                                (lock_flags & XFS_MMAPLOCK_SHARED),
                                (lock_flags & XFS_MMAPLOCK_EXCL));
        }
        if (lock_flags & (XFS_IOLOCK_EXCL|XFS_IOLOCK_SHARED)) {
                return __xfs_rwsem_islocked(&VFS_I(ip)->i_rwsem,
                                (lock_flags & XFS_IOLOCK_SHARED),
                                (lock_flags & XFS_IOLOCK_EXCL));
        }
}

That still has the problem/bug with the existing

	ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL | XFS_ILOCK_EXCL));

callsite since it doesn't actually test both types but again that is a separate
bugfix - it could be changed to accumulate a true/false state for all the flags
in the lock_flags argument in another bugfix patch.

-Eric