Re: bcache: unlock new_nodes[i]->write_lock in btree_gc_coalesce

[Coly Li]

On 2020/4/20 14:55, Zhiqiang Liu wrote:
> friendly ping...
> 

The patch is good. Just it would be better if the commit log can be more
accurate and understandable for other readers who are not very familiar
with bcache code.

Thanks.


Coly Li


> On 2020/4/15 19:42, Zhiqiang Liu wrote:
>> From: Zhiqiang Liu <liuzhiqiang26@xxxxxxxxxx>
>>
>> coccicheck reports:
>>   drivers/md//bcache/btree.c:1538:1-7:ÂprecedingÂlockÂonÂlineÂ1417
>>
>> After obtaining new_nodes[i]->write_lock, we may go to out_nocoalesce
>> tag without releasing lock. Here, we add a new tag
>> 'out_unlock_nocoalesce' before out_nocoalesce tag to release
>> new_nodes[i]->write_lock.
>>
>> Fixes: 2a285686c1 ("bcache: btree locking rework")
>> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@xxxxxxxxxx>
>> ---
>>  drivers/md/bcache/btree.c | 8 ++++++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/md/bcache/btree.c b/drivers/md/bcache/btree.c
>> index fa872df4e770..cad8b0b97e33 100644
>> --- a/drivers/md/bcache/btree.c
>> +++ b/drivers/md/bcache/btree.c
>> @@ -1447,7 +1447,7 @@ static int btree_gc_coalesce(struct btree *b, struct btree_op *op,
>>  			if (__set_blocks(n1, n1->keys + n2->keys,
>>  					 block_bytes(b->c)) >
>>  			    btree_blocks(new_nodes[i]))
>> -				goto out_nocoalesce;
>> +				goto out_unlock_nocoalesce;
>>
>>  			keys = n2->keys;
>>  			/* Take the key of the node we're getting rid of */
>> @@ -1476,7 +1476,7 @@ static int btree_gc_coalesce(struct btree *b, struct btree_op *op,
>>
>>  		if (__bch_keylist_realloc(&keylist,
>>  					  bkey_u64s(&new_nodes[i]->key)))
>> -			goto out_nocoalesce;
>> +			goto out_unlock_nocoalesce;
>>
>>  		bch_btree_node_write(new_nodes[i], &cl);
>>  		bch_keylist_add(&keylist, &new_nodes[i]->key);
>> @@ -1522,6 +1522,10 @@ static int btree_gc_coalesce(struct btree *b, struct btree_op *op,
>>  	/* Invalidated our iterator */
>>  	return -EINTR;
>>
>> +out_unlock_nocoalesce:
>> +	for (i = 0; i < nodes; i++)
>> +		mutex_unlock(&new_nodes[i]->write_lock);
>> +
>>  out_nocoalesce:
>>  	closure_sync(&cl);
>>
>