[RESEND] btrfs: Fix exception handling in relocating_repair_kthread()

Commit Message

Markus Elfring March 3, 2025, 8:31 p.m. UTC

From: Markus Elfring <[e-mail]>
Date: Wed, 22 Mar 2023 20:10:09 +0100

The label “out” was used to jump to another pointer check despite of
the detail in the implementation of the function
“relocating_repair_kthread” that it was determined already that
a corresponding variable contained a null pointer because of
a failed call of the function “btrfs_lookup_block_group”.

* Thus use more appropriate labels instead.

* Delete a redundant check.


This issue was detected by using the Coccinelle software.

Fixes: f7ef5287a63d ("btrfs: zoned: relocate block group to repair IO failure in zoned filesystems")
Signed-off-by: Markus Elfring <[e-mail]>
---
 fs/btrfs/volumes.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

--
2.40.0

Comments

Naohiro Aota March 4, 2025, 1:34 a.m. UTC | #1

On Tue Mar 4, 2025 at 5:31 AM JST, Markus Elfring wrote:
> From: Markus Elfring <[e-mail]>
> Date: Wed, 22 Mar 2023 20:10:09 +0100
>
> The label “out” was used to jump to another pointer check despite of
> the detail in the implementation of the function
> “relocating_repair_kthread” that it was determined already that
> a corresponding variable contained a null pointer because of
> a failed call of the function “btrfs_lookup_block_group”.
>
> * Thus use more appropriate labels instead.
>
> * Delete a redundant check.
>
>
> This issue was detected by using the Coccinelle software.

Since this function is local to the zoned feature, could I have "zoned: "
added to the subject line?

Other than that, it looks reasonable to me.

Reviewed-by: Naohiro Aota <[e-mail]>

>
> Fixes: f7ef5287a63d ("btrfs: zoned: relocate block group to repair IO failure in zoned filesystems")
> Signed-off-by: Markus Elfring <[e-mail]>
> ---
>  fs/btrfs/volumes.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 6d0124b6e79e..de11ad6c8740 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -8096,23 +8096,22 @@ static int relocating_repair_kthread(void *data)
>  	/* Ensure block group still exists */
>  	cache = btrfs_lookup_block_group(fs_info, target);
>  	if (!cache)
> -		goto out;
> +		goto unlock;
>
>  	if (!test_bit(BLOCK_GROUP_FLAG_RELOCATING_REPAIR, &cache->runtime_flags))
> -		goto out;
> +		goto put_block_group;
>
>  	ret = btrfs_may_alloc_data_chunk(fs_info, target);
>  	if (ret < 0)
> -		goto out;
> +		goto put_block_group;
>
>  	btrfs_info(fs_info,
>  		   "zoned: relocating block group %llu to repair IO failure",
>  		   target);
>  	ret = btrfs_relocate_chunk(fs_info, target);
> -
> -out:
> -	if (cache)
> -		btrfs_put_block_group(cache);
> +put_block_group:
> +	btrfs_put_block_group(cache);
> +unlock:
>  	mutex_unlock(&fs_info->reclaim_bgs_lock);
>  	btrfs_exclop_finish(fs_info);
>  	sb_end_write(fs_info->sb);
> --
> 2.40.0

Dan Carpenter March 4, 2025, 6:38 a.m. UTC | #2

On Tue, Mar 04, 2025 at 01:34:00AM +0000, Naohiro Aota wrote:
> On Tue Mar 4, 2025 at 5:31 AM JST, Markus Elfring wrote:
> > From: Markus Elfring <[e-mail]>
> > Date: Wed, 22 Mar 2023 20:10:09 +0100
> >
> > The label “out” was used to jump to another pointer check despite of
> > the detail in the implementation of the function
> > “relocating_repair_kthread” that it was determined already that
> > a corresponding variable contained a null pointer because of
> > a failed call of the function “btrfs_lookup_block_group”.
> >
> > * Thus use more appropriate labels instead.
> >
> > * Delete a redundant check.
> >
> >
> > This issue was detected by using the Coccinelle software.
> 
> Since this function is local to the zoned feature, could I have "zoned: "
> added to the subject line?
> 
> Other than that, it looks reasonable to me.
> 
> Reviewed-by: Naohiro Aota <[e-mail]>
> 
> >
> > Fixes: f7ef5287a63d ("btrfs: zoned: relocate block group to repair IO failure in zoned filesystems")
> > Signed-off-by: Markus Elfring <[e-mail]>

Don't use Fixes tags, if it's not a bug fix.

regards,
dan carpenter

Patch

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 6d0124b6e79e..de11ad6c8740 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -8096,23 +8096,22 @@  static int relocating_repair_kthread(void *data)
 	/* Ensure block group still exists */
 	cache = btrfs_lookup_block_group(fs_info, target);
 	if (!cache)
-		goto out;
+		goto unlock;

 	if (!test_bit(BLOCK_GROUP_FLAG_RELOCATING_REPAIR, &cache->runtime_flags))
-		goto out;
+		goto put_block_group;

 	ret = btrfs_may_alloc_data_chunk(fs_info, target);
 	if (ret < 0)
-		goto out;
+		goto put_block_group;

 	btrfs_info(fs_info,
 		   "zoned: relocating block group %llu to repair IO failure",
 		   target);
 	ret = btrfs_relocate_chunk(fs_info, target);
-
-out:
-	if (cache)
-		btrfs_put_block_group(cache);
+put_block_group:
+	btrfs_put_block_group(cache);
+unlock:
 	mutex_unlock(&fs_info->reclaim_bgs_lock);
 	btrfs_exclop_finish(fs_info);
 	sb_end_write(fs_info->sb);