Re: [PATCH resent] bcache: Fix exception handling in mca_alloc()

Re: [PATCH resent] bcache: Fix exception handling in mca_alloc()

[Coly Li]

On 3/25/23 5:31 PM, Markus Elfring wrote:
> Date: Mon, 20 Mar 2023 13:13:37 +0100
>
> The label “err” was used to jump to another pointer check despite of
> the detail in the implementation of the function “mca_alloc”
> that it was determined already that a corresponding variable contained
> a null pointer because of a failed function call “mca_bucket_alloc”.


Hmm, I don't get the exact point from the above long sentence. Could you 
please describe a bit more specific where the problem is at exact line 
number of the code?

> * Thus use a more appropriate label instead.

So far I am not convinced the modified label is more appropriate.

>
> * Delete a redundant check.

Where is the location of the redundant check?


>
>
> This issue was detected by using the Coccinelle software.

Just curious, what is the warning reported by the mentioned software ?


>
> Fixes: cafe563591446cf80bfbc2fe3bc72a2e36cf1060 ("bcache: A block layer cache")
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>   drivers/md/bcache/btree.c | 11 +++++------
>   1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/md/bcache/btree.c b/drivers/md/bcache/btree.c
> index 147c493a989a..166afd7ec499 100644
> --- a/drivers/md/bcache/btree.c
> +++ b/drivers/md/bcache/btree.c
> @@ -921,18 +921,18 @@ static struct btree *mca_alloc(struct cache_set *c, struct btree_op *op,
>   		if (!mca_reap(b, 0, false)) {
>   			mca_data_alloc(b, k, __GFP_NOWARN|GFP_NOIO);
>   			if (!b->keys.set[0].data)
> -				goto err;
> +				goto unlock;
>   			else
>   				goto out;
>   		}
>
>   	b = mca_bucket_alloc(c, k, __GFP_NOWARN|GFP_NOIO);
>   	if (!b)
> -		goto err;
> +		goto unlock;
>
>   	BUG_ON(!down_write_trylock(&b->lock));
>   	if (!b->keys.set->data)
> -		goto err;
> +		goto unlock;
>   out:
>   	BUG_ON(b->io_mutex.count != 1);
>
> @@ -955,9 +955,8 @@ static struct btree *mca_alloc(struct cache_set *c, struct btree_op *op,
>   				    &b->c->expensive_debug_checks);
>
>   	return b;
> -err:
> -	if (b)
> -		rw_unlock(true, b);
> +unlock:
> +	rw_unlock(true, b);

If b is NULL, is it a bit fishing to send the NULL pointer into 
rw_unlock() ?


Thanks in advance for more information.


Coly Li

Re: [PATCH v2] bcache: Fix exception handling in mca_alloc()

[Coly Li]

> 2023年3月25日 20:21，Markus Elfring <Markus.Elfring@web.de> 写道：
> 
> Date: Sat, 25 Mar 2023 13:08:01 +0100
> 
> The label “err” was used to jump to another pointer check despite of
> the detail in the implementation of the function “mca_alloc”
> that it was determined already that a corresponding variable contained
> a null pointer because of a failed function call “mca_bucket_alloc”.
> 
> 1. Thus use more appropriate labels instead.

It is not convinced to me that the new added labels are more appropriate. IMHO, the change just makes the code to be more complicated.


> 
> 2. Delete a repeated check (for the variable “b”)
>   which became unnecessary with this refactoring.
> 

To remove one line ‘if’ check, 13 lines are changed. IMHO this is not worthy. Yes the extra ‘if’ check can be avoided, but the code is more simple before adding labels unlock and cannibalize_mca.

The ‘if’ check is in error handling, which is not hot code path. Comparing to avoid an ‘if’ check, I prefer more for more simpler code. I am not supportive to this change.


Thanks.

Coly Li


> 
> This issue was detected by using the Coccinelle software.
> 
> Fixes: cafe563591446cf80bfbc2fe3bc72a2e36cf1060 ("bcache: A block layer cache")
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
> V2:
> Use another label.
> 
> drivers/md/bcache/btree.c | 13 ++++++-------
> 1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/md/bcache/btree.c b/drivers/md/bcache/btree.c
> index 147c493a989a..c6a20595302f 100644
> --- a/drivers/md/bcache/btree.c
> +++ b/drivers/md/bcache/btree.c
> @@ -921,18 +921,18 @@ static struct btree *mca_alloc(struct cache_set *c, struct btree_op *op,
> if (!mca_reap(b, 0, false)) {
> mca_data_alloc(b, k, __GFP_NOWARN|GFP_NOIO);
> if (!b->keys.set[0].data)
> - goto err;
> + goto unlock;
> else
> goto out;
> }
> 
> b = mca_bucket_alloc(c, k, __GFP_NOWARN|GFP_NOIO);
> if (!b)
> - goto err;
> + goto cannibalize_mca;
> 
> BUG_ON(!down_write_trylock(&b->lock));
> if (!b->keys.set->data)
> - goto err;
> + goto unlock;
> out:
> BUG_ON(b->io_mutex.count != 1);
> 
> @@ -955,10 +955,9 @@ static struct btree *mca_alloc(struct cache_set *c, struct btree_op *op,
>    &b->c->expensive_debug_checks);
> 
> return b;
> -err:
> - if (b)
> - rw_unlock(true, b);
> -
> +unlock:
> + rw_unlock(true, b);
> +cannibalize_mca:
> b = mca_cannibalize(c, op, k);
> if (!IS_ERR(b))
> goto out;
> --
> 2.40.0
>