On 2/19/20 12:40 PM, Christoph Hellwig 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.
I think that's a lot more clear. In addition to the slight confusion over a (true, true)
set of args, the current proposal also has the extra confusion of what happens if we pass
(false, false), for example.
One other thought, since debug_locks getting turned off by lockdep means that
an exclusive test reverts to a shared|exclusive test, would it be worth adding
a WARN_ON_ONCE to make it clear when xfs rwsem lock testing coverage has been
reduced?
-Eric
