[Cluster-devel] [PATCH] GFS2: Hold gl_hash_table bucket locks in glock_hash_walk

[Steven Whitehouse <swhiteho@redhat.com>]

Hi,

On 12/11/15 20:11, Bob Peterson wrote:
> ----- Original Message -----
>> This lockdep splat was being triggered on umount:
>>
>> [55715.973122] ===============================
>> [55715.980169] [ INFO: suspicious RCU usage. ]
>> [55715.981021] 4.3.0-11553-g8d3de01-dirty #15 Tainted: G        W
>> [55715.982353] -------------------------------
>> [55715.983301] fs/gfs2/glock.c:1427 suspicious rcu_dereference_protected()
>> usage!
>>
>> The code it refers to is the rht_for_each_entry_safe usage in
>> glock_hash_walk. The condition that triggers the warning is
>> lockdep_rht_bucket_is_held(tbl, hash) which is checked in the
>> __rcu_dereference_protected macro.
>>
>> lockdep_rht_bucket_is_held returns false when rht_bucket_lock(tbl, hash)
>> is not held, which suggests that glock_hash_walk should be holding the
>> lock for each rhashtable bucket it looks at. Holding those locks clears
>> up the warning.
>>
>> Signed-off-by: Andrew Price <anprice@redhat.com>
>> ---
>>   fs/gfs2/glock.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
>> index 32e7471..7384cbb 100644
>> --- a/fs/gfs2/glock.c
>> +++ b/fs/gfs2/glock.c
>> @@ -1424,11 +1424,13 @@ static void glock_hash_walk(glock_examiner examiner,
>> const struct gfs2_sbd *sdp)
>>   	rcu_read_lock();
>>   	tbl = rht_dereference_rcu(gl_hash_table.tbl, &gl_hash_table);
>>   	for (i = 0; i < tbl->size; i++) {
>> +		spin_lock(rht_bucket_lock(tbl, i));
>>   		rht_for_each_entry_safe(gl, pos, next, tbl, i, gl_node) {
>>   			if ((gl->gl_name.ln_sbd == sdp) &&
>>   			    lockref_get_not_dead(&gl->gl_lockref))
>>   				examiner(gl);
>>   		}
>> +		spin_unlock(rht_bucket_lock(tbl, i));
>>   	}
>>   	rcu_read_unlock();
>>   	cond_resched();
>> --
Not sure that this is the correct fix. There is nothing changing in the 
examiner function that I can see which would require the bucket lock, 
and we already have this with an rcu_read_lock, which is correct since 
we are only reading the hash lists, not changing them. So I'm wondering 
why this triggers the warning, and whether there is some annotation 
issue perhaps?

Steve.

>> 2.4.3
>>
>>
> Hi Andy,
>
> Thanks. I'll hold onto this until after the merge window is closed.
>
> Regards,
>
> Bob Peterson
> Red Hat File Systems
>