2020-04-15 23:03:00

by Zhiqiang Liu

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

From: Zhiqiang Liu <[email protected]>

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 <[email protected]>
---
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);

--
2.19.1

2020-04-20 06:57:50

by Zhiqiang Liu

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

friendly ping...

On 2020/4/15 19:42, Zhiqiang Liu wrote:
> From: Zhiqiang Liu <[email protected]>
>
> 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 <[email protected]>
> ---
> 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);
>

2020-04-20 07:14:04

by Coly Li

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

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 <[email protected]>
>>
>> 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 <[email protected]>
>> ---
>> 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);
>>
>

2020-04-20 07:50:19

by Zhiqiang Liu

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

On 2020/4/20 15:10, Coly Li wrote:
> 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

Thanks for your reply. I will try my best to rewrite the commit log.

Regards.
Zhiqiang Liu.

>
>
>> On 2020/4/15 19:42, Zhiqiang Liu wrote:
>>> From: Zhiqiang Liu <[email protected]>
>>>
>>> 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 <[email protected]>
>>> ---
>>> 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);
>>>
>>
>
>