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

[Eric Sandeen <sandeen@sandeen.net>]

On 2/21/20 11:49 AM, Pavel Reichl wrote:
> On Wed, Feb 19, 2020 at 7:40 PM Christoph Hellwig <hch@infradead.org> wrote:
>>
>> On Tue, Feb 18, 2020 at 08:48:21PM -0800, Darrick J. Wong wrote:
>>>>> +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;
>>>>
>>>> This could use some comments explaining the logic, especially why we
>>>> need the shared and excl flags, which seems very confusing given that
>>>> a lock can be held either shared or exclusive, but not neither and not
>>>> both.
>>>
>>> Yes, this predicate should document that callers are allowed to pass in
>>> shared==true and excl==true when the caller wants to assert that either
>>> lock type (shared or excl) of a given lock class (e.g. iolock) are held.
>>
>> Looking at the lockdep_is_held_type implementation, and our existing
>> code for i_rwsem I really don't see the point of the extra shared
>> check.  Something like:
>>
>> static inline bool
>> __xfs_rwsem_islocked(
>>         struct rw_semaphore     *rwsem,
>>         bool                    excl)
>> {
>>         if (rwsem_is_locked(rwsem)) {
>>                 if (debug_locks && excl)
>>                         return lockdep_is_held_type(rwsem, 1);
>>                 return true;
>>         }
>>
>>         return false;
>> }
>>
>> should be all that we really need.
>>
> 
> You don't see the point of extra shared check, but if we want to check
> that the semaphore is locked for reading and not writing? Having the
> semaphore locked for writing would make the code safe from race
> condition but could be a performance hit, right?

So, I raised this question with Pavel but I think maybe it was borne
of my misunderstanding.

Ok let me think this through.  Today we have:

int
xfs_isilocked(
        xfs_inode_t             *ip,
        uint                    lock_flags)
{
        if (lock_flags & (XFS_ILOCK_EXCL|XFS_ILOCK_SHARED)) {
                if (!(lock_flags & XFS_ILOCK_SHARED))
                        return !!ip->i_lock.mr_writer;
                return rwsem_is_locked(&ip->i_lock.mr_lock);
        }
        ....

If we assert xfs_isilocked(ip, XFS_ILOCK_SHARED) I guess we /already/ get a positive
result if the inode is actually locked XFS_ILOCK_EXCL.  So perhaps Christoph's
suggestion really just keeps implementing what we already have today.

It might be a reasonable question re: whether we ever want to know that we are locked
shared and NOT locked exclusive, but we can't do that today, so I guess it shouldn't
complicate this patchset.

... do I have this right?

Thanks,
-Eric