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

[Eric Sandeen <sandeen@sandeen.net>]

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