btrfs_map_block tidyups

btrfs_map_block tidyups

[Christoph Hellwig]

Hi all,

Dan reported a static type checker complained about the num_mirror_ret
output argument in btrfs_map_block, so the first patch cleans that up.
The second one simplifies a conditional right next to it.

Diffstat:
 volumes.c |    7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

[PATCH 1/2] btrfs: be a bit more careful when setting mirror_num_ret in btrfs_map_block

[Christoph Hellwig]

The mirror_num_ret is allowed to be NULL, although it has to be set when
smap is set.  Unfortunately that is not a well enough specifiable
invariant for static type checkers, so add a NULL check to make sure they
are fine.

Fixes: 03793cbbc80f ("btrfs: add fast path for single device io in __btrfs_map_block")
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/btrfs/volumes.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index a536d0e0e05566..0d386ed44279ce 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -6406,7 +6406,8 @@ int btrfs_map_block(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
 	    (op == BTRFS_MAP_READ || !dev_replace_is_ongoing ||
 	     !dev_replace->tgtdev)) {
 		set_io_stripe(smap, map, stripe_index, stripe_offset, stripe_nr);
-		*mirror_num_ret = mirror_num;
+		if (mirror_num_ret)
+			*mirror_num_ret = mirror_num;
 		*bioc_ret = NULL;
 		ret = 0;
 		goto out;
-- 
2.39.2

Re: [PATCH 1/2] btrfs: be a bit more careful when setting mirror_num_ret in btrfs_map_block

[Qu Wenruo]

On 2023/6/27 14:13, Christoph Hellwig wrote:
> The mirror_num_ret is allowed to be NULL, although it has to be set when
> smap is set.  Unfortunately that is not a well enough specifiable
> invariant for static type checkers, so add a NULL check to make sure they
> are fine.
>
> Fixes: 03793cbbc80f ("btrfs: add fast path for single device io in __btrfs_map_block")
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu
> ---
>   fs/btrfs/volumes.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index a536d0e0e05566..0d386ed44279ce 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -6406,7 +6406,8 @@ int btrfs_map_block(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
>   	    (op == BTRFS_MAP_READ || !dev_replace_is_ongoing ||
>   	     !dev_replace->tgtdev)) {
>   		set_io_stripe(smap, map, stripe_index, stripe_offset, stripe_nr);
> -		*mirror_num_ret = mirror_num;
> +		if (mirror_num_ret)
> +			*mirror_num_ret = mirror_num;
>   		*bioc_ret = NULL;
>   		ret = 0;
>   		goto out;

Re: [PATCH 1/2] btrfs: be a bit more careful when setting mirror_num_ret in btrfs_map_block

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

[PATCH 2/2] btrfs: simplify the no-bioc fast path condition in btrfs_map_block

[Christoph Hellwig]

nr_alloc_stripes can't be one if we are writing to a replacement device,
as it is incremented for that case right above.  Remove the duplicate
checks.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/btrfs/volumes.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 0d386ed44279ce..4907ed9809109d 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -6402,9 +6402,7 @@ int btrfs_map_block(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
 	 * I/O context structure.
 	 */
 	if (smap && num_alloc_stripes == 1 &&
-	    !((map->type & BTRFS_BLOCK_GROUP_RAID56_MASK) && mirror_num > 1) &&
-	    (op == BTRFS_MAP_READ || !dev_replace_is_ongoing ||
-	     !dev_replace->tgtdev)) {
+	    !((map->type & BTRFS_BLOCK_GROUP_RAID56_MASK) && mirror_num > 1)) {
 		set_io_stripe(smap, map, stripe_index, stripe_offset, stripe_nr);
 		if (mirror_num_ret)
 			*mirror_num_ret = mirror_num;
-- 
2.39.2

Re: [PATCH 2/2] btrfs: simplify the no-bioc fast path condition in btrfs_map_block

[Qu Wenruo]

On 2023/6/27 14:13, Christoph Hellwig wrote:
> nr_alloc_stripes can't be one if we are writing to a replacement device,
> as it is incremented for that case right above.  Remove the duplicate
> checks.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu

> ---
>   fs/btrfs/volumes.c | 4 +---
>   1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 0d386ed44279ce..4907ed9809109d 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -6402,9 +6402,7 @@ int btrfs_map_block(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
>   	 * I/O context structure.
>   	 */
>   	if (smap && num_alloc_stripes == 1 &&
> -	    !((map->type & BTRFS_BLOCK_GROUP_RAID56_MASK) && mirror_num > 1) &&
> -	    (op == BTRFS_MAP_READ || !dev_replace_is_ongoing ||
> -	     !dev_replace->tgtdev)) {
> +	    !((map->type & BTRFS_BLOCK_GROUP_RAID56_MASK) && mirror_num > 1)) {
>   		set_io_stripe(smap, map, stripe_index, stripe_offset, stripe_nr);
>   		if (mirror_num_ret)
>   			*mirror_num_ret = mirror_num;

Re: [PATCH 2/2] btrfs: simplify the no-bioc fast path condition in btrfs_map_block

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: btrfs_map_block tidyups

[David Sterba]

On Tue, Jun 27, 2023 at 08:13:22AM +0200, Christoph Hellwig wrote:
> Hi all,
> 
> Dan reported a static type checker complained about the num_mirror_ret
> output argument in btrfs_map_block, so the first patch cleans that up.
> The second one simplifies a conditional right next to it.

Added to misc-next, thanks.