On 2/21/20 11:49 AM, Pavel Reichl wrote:
> On Wed, Feb 19, 2020 at 7:40 PM Christoph Hellwig <hch@xxxxxxxxxxxxx> 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
