cleanup the btrfs_map_block interface

cleanup the btrfs_map_block interface

[Christoph Hellwig]

Hi all,

the interface around btrfs_map_block is a bit confusion, mostly due to
the fact that it has a double-underscored complex version and two
wrappers that just take a few arguments away.  This series cleans up
a few loose ends to make this interface easier to understand.

Diffstat:
 bio.c             |    4 +--
 check-integrity.c |   19 ++++++++++------
 dev-replace.c     |    2 -
 scrub.c           |    9 ++++---
 volumes.c         |   63 ++++++++++++++++--------------------------------------
 volumes.h         |   15 ++----------
 zoned.c           |    4 +--
 7 files changed, 44 insertions(+), 72 deletions(-)

[PATCH 1/6] btrfs: remove BTRFS_MAP_DISCARD

[Christoph Hellwig]

BTRFS_MAP_DISCARD is never set, as REQ_OP_DISCARD is never passed to
btrfs_op() only only checked in two ASSERTS.

Remove it and let the catchall WARN_ON in btrfs_op() deal with accidental
REQ_OP_DISCARDs leaked into btrfs_op().

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/btrfs/volumes.c | 3 ---
 fs/btrfs/volumes.h | 3 ---
 2 files changed, 6 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index a4bfec088617ec..c236bfba0cec3b 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -6182,8 +6182,6 @@ static u64 btrfs_max_io_len(struct map_lookup *map, enum btrfs_map_op op,
 			    u64 offset, u32 *stripe_nr, u64 *stripe_offset,
 			    u64 *full_stripe_start)
 {
-	ASSERT(op != BTRFS_MAP_DISCARD);
-
 	/*
 	 * Stripe_nr is the stripe where this block falls.  stripe_offset is
 	 * the offset of this block in its stripe.
@@ -6261,7 +6259,6 @@ int __btrfs_map_block(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
 	u64 max_len;
 
 	ASSERT(bioc_ret);
-	ASSERT(op != BTRFS_MAP_DISCARD);
 
 	num_copies = btrfs_num_copies(fs_info, logical, fs_info->sectorsize);
 	if (mirror_num > num_copies)
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index 16fc640cabd3f7..e960a51abf873d 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -556,15 +556,12 @@ struct btrfs_dev_lookup_args {
 enum btrfs_map_op {
 	BTRFS_MAP_READ,
 	BTRFS_MAP_WRITE,
-	BTRFS_MAP_DISCARD,
 	BTRFS_MAP_GET_READ_MIRRORS,
 };
 
 static inline enum btrfs_map_op btrfs_op(struct bio *bio)
 {
 	switch (bio_op(bio)) {
-	case REQ_OP_DISCARD:
-		return BTRFS_MAP_DISCARD;
 	case REQ_OP_WRITE:
 	case REQ_OP_ZONE_APPEND:
 		return BTRFS_MAP_WRITE;
-- 
2.39.2

Re: [PATCH 1/6] btrfs: remove BTRFS_MAP_DISCARD

[Qu Wenruo]

On 2023/5/31 12:17, Christoph Hellwig wrote:
> BTRFS_MAP_DISCARD is never set, as REQ_OP_DISCARD is never passed to
> btrfs_op() only only checked in two ASSERTS.
>
> Remove it and let the catchall WARN_ON in btrfs_op() deal with accidental
> REQ_OP_DISCARDs leaked into btrfs_op().
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

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

Thanks,
Qu
> ---
>   fs/btrfs/volumes.c | 3 ---
>   fs/btrfs/volumes.h | 3 ---
>   2 files changed, 6 deletions(-)
>
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index a4bfec088617ec..c236bfba0cec3b 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -6182,8 +6182,6 @@ static u64 btrfs_max_io_len(struct map_lookup *map, enum btrfs_map_op op,
>   			    u64 offset, u32 *stripe_nr, u64 *stripe_offset,
>   			    u64 *full_stripe_start)
>   {
> -	ASSERT(op != BTRFS_MAP_DISCARD);
> -
>   	/*
>   	 * Stripe_nr is the stripe where this block falls.  stripe_offset is
>   	 * the offset of this block in its stripe.
> @@ -6261,7 +6259,6 @@ int __btrfs_map_block(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
>   	u64 max_len;
>
>   	ASSERT(bioc_ret);
> -	ASSERT(op != BTRFS_MAP_DISCARD);
>
>   	num_copies = btrfs_num_copies(fs_info, logical, fs_info->sectorsize);
>   	if (mirror_num > num_copies)
> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
> index 16fc640cabd3f7..e960a51abf873d 100644
> --- a/fs/btrfs/volumes.h
> +++ b/fs/btrfs/volumes.h
> @@ -556,15 +556,12 @@ struct btrfs_dev_lookup_args {
>   enum btrfs_map_op {
>   	BTRFS_MAP_READ,
>   	BTRFS_MAP_WRITE,
> -	BTRFS_MAP_DISCARD,
>   	BTRFS_MAP_GET_READ_MIRRORS,
>   };
>
>   static inline enum btrfs_map_op btrfs_op(struct bio *bio)
>   {
>   	switch (bio_op(bio)) {
> -	case REQ_OP_DISCARD:
> -		return BTRFS_MAP_DISCARD;
>   	case REQ_OP_WRITE:
>   	case REQ_OP_ZONE_APPEND:
>   		return BTRFS_MAP_WRITE;

[PATCH 2/6] btrfs: optimize simple reads in btrfsic_map_block

[Christoph Hellwig]

Pass a smap into __btrfs_map_block so that the usual case of a read that
doesn't require parity raid recovery doesn't need an extra memory
allocation for the btrfs_io_context.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/btrfs/check-integrity.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/fs/btrfs/check-integrity.c b/fs/btrfs/check-integrity.c
index b4408037b823c5..fe15367000141a 100644
--- a/fs/btrfs/check-integrity.c
+++ b/fs/btrfs/check-integrity.c
@@ -1459,13 +1459,13 @@ static int btrfsic_map_block(struct btrfsic_state *state, u64 bytenr, u32 len,
 	struct btrfs_fs_info *fs_info = state->fs_info;
 	int ret;
 	u64 length;
-	struct btrfs_io_context *multi = NULL;
+	struct btrfs_io_context *bioc = NULL;
+	struct btrfs_io_stripe smap, *map;
 	struct btrfs_device *device;
 
 	length = len;
-	ret = btrfs_map_block(fs_info, BTRFS_MAP_READ,
-			      bytenr, &length, &multi, mirror_num);
-
+	ret = __btrfs_map_block(fs_info, BTRFS_MAP_READ, bytenr, &length, &bioc,
+				NULL, &mirror_num, 0);
 	if (ret) {
 		block_ctx_out->start = 0;
 		block_ctx_out->dev_bytenr = 0;
@@ -1478,21 +1478,26 @@ static int btrfsic_map_block(struct btrfsic_state *state, u64 bytenr, u32 len,
 		return ret;
 	}
 
-	device = multi->stripes[0].dev;
+	if (bioc)
+		map = &bioc->stripes[0];
+	else
+		map = &smap;
+
+	device = map->dev;
 	if (test_bit(BTRFS_DEV_STATE_MISSING, &device->dev_state) ||
 	    !device->bdev || !device->name)
 		block_ctx_out->dev = NULL;
 	else
 		block_ctx_out->dev = btrfsic_dev_state_lookup(
 							device->bdev->bd_dev);
-	block_ctx_out->dev_bytenr = multi->stripes[0].physical;
+	block_ctx_out->dev_bytenr = map->physical;
 	block_ctx_out->start = bytenr;
 	block_ctx_out->len = len;
 	block_ctx_out->datav = NULL;
 	block_ctx_out->pagev = NULL;
 	block_ctx_out->mem_to_free = NULL;
 
-	kfree(multi);
+	kfree(bioc);
 	if (NULL == block_ctx_out->dev) {
 		ret = -ENXIO;
 		pr_info("btrfsic: error, cannot lookup dev (#1)!\n");
-- 
2.39.2

Re: [PATCH 2/6] btrfs: optimize simple reads in btrfsic_map_block

[Qu Wenruo]

On 2023/5/31 12:17, Christoph Hellwig wrote:
> Pass a smap into __btrfs_map_block so that the usual case of a read that
> doesn't require parity raid recovery doesn't need an extra memory
> allocation for the btrfs_io_context.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

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

I'm more curious on whether the check-integrity feature is still under
heavy usage.

It's from old time where we don't have a lot of sanity checks, but
nowadays it looks less worthy and can cause extra burden to maintain.

Thanks,
Qu
> ---
>   fs/btrfs/check-integrity.c | 19 ++++++++++++-------
>   1 file changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/fs/btrfs/check-integrity.c b/fs/btrfs/check-integrity.c
> index b4408037b823c5..fe15367000141a 100644
> --- a/fs/btrfs/check-integrity.c
> +++ b/fs/btrfs/check-integrity.c
> @@ -1459,13 +1459,13 @@ static int btrfsic_map_block(struct btrfsic_state *state, u64 bytenr, u32 len,
>   	struct btrfs_fs_info *fs_info = state->fs_info;
>   	int ret;
>   	u64 length;
> -	struct btrfs_io_context *multi = NULL;
> +	struct btrfs_io_context *bioc = NULL;
> +	struct btrfs_io_stripe smap, *map;
>   	struct btrfs_device *device;
>
>   	length = len;
> -	ret = btrfs_map_block(fs_info, BTRFS_MAP_READ,
> -			      bytenr, &length, &multi, mirror_num);
> -
> +	ret = __btrfs_map_block(fs_info, BTRFS_MAP_READ, bytenr, &length, &bioc,
> +				NULL, &mirror_num, 0);
>   	if (ret) {
>   		block_ctx_out->start = 0;
>   		block_ctx_out->dev_bytenr = 0;
> @@ -1478,21 +1478,26 @@ static int btrfsic_map_block(struct btrfsic_state *state, u64 bytenr, u32 len,
>   		return ret;
>   	}
>
> -	device = multi->stripes[0].dev;
> +	if (bioc)
> +		map = &bioc->stripes[0];
> +	else
> +		map = &smap;
> +
> +	device = map->dev;
>   	if (test_bit(BTRFS_DEV_STATE_MISSING, &device->dev_state) ||
>   	    !device->bdev || !device->name)
>   		block_ctx_out->dev = NULL;
>   	else
>   		block_ctx_out->dev = btrfsic_dev_state_lookup(
>   							device->bdev->bd_dev);
> -	block_ctx_out->dev_bytenr = multi->stripes[0].physical;
> +	block_ctx_out->dev_bytenr = map->physical;
>   	block_ctx_out->start = bytenr;
>   	block_ctx_out->len = len;
>   	block_ctx_out->datav = NULL;
>   	block_ctx_out->pagev = NULL;
>   	block_ctx_out->mem_to_free = NULL;
>
> -	kfree(multi);
> +	kfree(bioc);
>   	if (NULL == block_ctx_out->dev) {
>   		ret = -ENXIO;
>   		pr_info("btrfsic: error, cannot lookup dev (#1)!\n");

Re: [PATCH 2/6] btrfs: optimize simple reads in btrfsic_map_block

[Johannes Thumshirn]

On 31.05.23 10:49, Qu Wenruo wrote:
> 
> 
> On 2023/5/31 12:17, Christoph Hellwig wrote:
>> Pass a smap into __btrfs_map_block so that the usual case of a read that
>> doesn't require parity raid recovery doesn't need an extra memory
>> allocation for the btrfs_io_context.
>>
>> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Reviewed-by: Qu Wenruo <wqu@suse.com>
> 
> I'm more curious on whether the check-integrity feature is still under
> heavy usage.
> 
> It's from old time where we don't have a lot of sanity checks, but
> nowadays it looks less worthy and can cause extra burden to maintain.

I was going to ask the same question. I wouldn't mind removing it 
at all.

Re: [PATCH 2/6] btrfs: optimize simple reads in btrfsic_map_block

[Christoph Hellwig]

On Wed, May 31, 2023 at 12:31:24PM +0000, Johannes Thumshirn wrote:
> > I'm more curious on whether the check-integrity feature is still under
> > heavy usage.
> > 
> > It's from old time where we don't have a lot of sanity checks, but
> > nowadays it looks less worthy and can cause extra burden to maintain.
> 
> I was going to ask the same question. I wouldn't mind removing it 
> at all.

I've never actively used it and defintively had my fair amount of run
ins with the somewhat interesting kind of code there.

Re: [PATCH 2/6] btrfs: optimize simple reads in btrfsic_map_block

[Johannes Thumshirn]

On 31.05.23 14:35, Christoph Hellwig wrote:
> On Wed, May 31, 2023 at 12:31:24PM +0000, Johannes Thumshirn wrote:
>>> I'm more curious on whether the check-integrity feature is still under
>>> heavy usage.
>>>
>>> It's from old time where we don't have a lot of sanity checks, but
>>> nowadays it looks less worthy and can cause extra burden to maintain.
>>
>> I was going to ask the same question. I wouldn't mind removing it 
>> at all.
> 
> I've never actively used it and defintively had my fair amount of run
> ins with the somewhat interesting kind of code there.
> 

And the fact that it comes with a huge "DANGEROUS" in Kconfig doesn't
make the situation better IMHO.

I guess we should just schedule it for removal in 1-2 releases. The last
"proper" code touching check-integrity.c was 
cf90c59e680e ("Btrfs: check-int: don't complain about balanced blocks")
and that went in 9 years ago.

Josef, David, what's your take on this?

[PATCH 3/6] btrfs: remove btrfs_map_block

[Christoph Hellwig]

There are no users of btrfs_map_block left, so remove it.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/btrfs/volumes.c | 8 --------
 fs/btrfs/volumes.h | 3 ---
 2 files changed, 11 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index c236bfba0cec3b..4c6405c4ce041d 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -6481,14 +6481,6 @@ int __btrfs_map_block(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
 	return ret;
 }
 
-int btrfs_map_block(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
-		      u64 logical, u64 *length,
-		      struct btrfs_io_context **bioc_ret, int mirror_num)
-{
-	return __btrfs_map_block(fs_info, op, logical, length, bioc_ret,
-				 NULL, &mirror_num, 0);
-}
-
 /* For Scrub/replace */
 int btrfs_map_sblock(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
 		     u64 logical, u64 *length,
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index e960a51abf873d..481f3ace988c44 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -582,9 +582,6 @@ static inline unsigned long btrfs_chunk_item_size(int num_stripes)
 
 void btrfs_get_bioc(struct btrfs_io_context *bioc);
 void btrfs_put_bioc(struct btrfs_io_context *bioc);
-int btrfs_map_block(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
-		    u64 logical, u64 *length,
-		    struct btrfs_io_context **bioc_ret, int mirror_num);
 int btrfs_map_sblock(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
 		     u64 logical, u64 *length,
 		     struct btrfs_io_context **bioc_ret);
-- 
2.39.2

Re: [PATCH 3/6] btrfs: remove btrfs_map_block

[Qu Wenruo]

On 2023/5/31 12:17, Christoph Hellwig wrote:
> There are no users of btrfs_map_block left, so remove it.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

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

Thanks,
Qu
> ---
>   fs/btrfs/volumes.c | 8 --------
>   fs/btrfs/volumes.h | 3 ---
>   2 files changed, 11 deletions(-)
>
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index c236bfba0cec3b..4c6405c4ce041d 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -6481,14 +6481,6 @@ int __btrfs_map_block(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
>   	return ret;
>   }
>
> -int btrfs_map_block(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
> -		      u64 logical, u64 *length,
> -		      struct btrfs_io_context **bioc_ret, int mirror_num)
> -{
> -	return __btrfs_map_block(fs_info, op, logical, length, bioc_ret,
> -				 NULL, &mirror_num, 0);
> -}
> -
>   /* For Scrub/replace */
>   int btrfs_map_sblock(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
>   		     u64 logical, u64 *length,
> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
> index e960a51abf873d..481f3ace988c44 100644
> --- a/fs/btrfs/volumes.h
> +++ b/fs/btrfs/volumes.h
> @@ -582,9 +582,6 @@ static inline unsigned long btrfs_chunk_item_size(int num_stripes)
>
>   void btrfs_get_bioc(struct btrfs_io_context *bioc);
>   void btrfs_put_bioc(struct btrfs_io_context *bioc);
> -int btrfs_map_block(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
> -		    u64 logical, u64 *length,
> -		    struct btrfs_io_context **bioc_ret, int mirror_num);
>   int btrfs_map_sblock(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
>   		     u64 logical, u64 *length,
>   		     struct btrfs_io_context **bioc_ret);

[PATCH 4/6] btrfs: rename __btrfs_map_block to btrfs_map_block

[Christoph Hellwig]

Now that the old btrfs_map_block is gone, drop the leading underscores
from __btrfs_map_block.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/btrfs/bio.c             |  4 ++--
 fs/btrfs/check-integrity.c |  4 ++--
 fs/btrfs/dev-replace.c     |  2 +-
 fs/btrfs/volumes.c         | 16 ++++++++--------
 fs/btrfs/volumes.h         | 10 +++++-----
 5 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/fs/btrfs/bio.c b/fs/btrfs/bio.c
index ae6345668d2d01..85511a8a480194 100644
--- a/fs/btrfs/bio.c
+++ b/fs/btrfs/bio.c
@@ -622,8 +622,8 @@ static bool btrfs_submit_chunk(struct btrfs_bio *bbio, int mirror_num)
 	int error;
 
 	btrfs_bio_counter_inc_blocked(fs_info);
-	error = __btrfs_map_block(fs_info, btrfs_op(bio), logical, &map_length,
-				  &bioc, &smap, &mirror_num, 1);
+	error = btrfs_map_block(fs_info, btrfs_op(bio), logical, &map_length,
+				&bioc, &smap, &mirror_num, 1);
 	if (error) {
 		ret = errno_to_blk_status(error);
 		goto fail;
diff --git a/fs/btrfs/check-integrity.c b/fs/btrfs/check-integrity.c
index fe15367000141a..3caf339c4bb3e4 100644
--- a/fs/btrfs/check-integrity.c
+++ b/fs/btrfs/check-integrity.c
@@ -1464,8 +1464,8 @@ static int btrfsic_map_block(struct btrfsic_state *state, u64 bytenr, u32 len,
 	struct btrfs_device *device;
 
 	length = len;
-	ret = __btrfs_map_block(fs_info, BTRFS_MAP_READ, bytenr, &length, &bioc,
-				NULL, &mirror_num, 0);
+	ret = btrfs_map_block(fs_info, BTRFS_MAP_READ, bytenr, &length, &bioc,
+			      NULL, &mirror_num, 0);
 	if (ret) {
 		block_ctx_out->start = 0;
 		block_ctx_out->dev_bytenr = 0;
diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index dc3f30c79320a1..5e86bea0a9507c 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -41,7 +41,7 @@
  *   All new writes will be written to both target and source devices, so even
  *   if replace gets canceled, sources device still contains up-to-date data.
  *
- *   Location:		handle_ops_on_dev_replace() from __btrfs_map_block()
+ *   Location:		handle_ops_on_dev_replace() from btrfs_map_block()
  *   Start:		btrfs_dev_replace_start()
  *   End:		btrfs_dev_replace_finishing()
  *   Content:		Latest data/metadata
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 4c6405c4ce041d..53059ee04f9b60 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -6232,11 +6232,11 @@ static void set_io_stripe(struct btrfs_io_stripe *dst, const struct map_lookup *
 			stripe_offset + (stripe_nr << BTRFS_STRIPE_LEN_SHIFT);
 }
 
-int __btrfs_map_block(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
-		      u64 logical, u64 *length,
-		      struct btrfs_io_context **bioc_ret,
-		      struct btrfs_io_stripe *smap, int *mirror_num_ret,
-		      int need_raid_map)
+int btrfs_map_block(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
+		    u64 logical, u64 *length,
+		    struct btrfs_io_context **bioc_ret,
+		    struct btrfs_io_stripe *smap, int *mirror_num_ret,
+		    int need_raid_map)
 {
 	struct extent_map *em;
 	struct map_lookup *map;
@@ -6486,7 +6486,7 @@ int btrfs_map_sblock(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
 		     u64 logical, u64 *length,
 		     struct btrfs_io_context **bioc_ret)
 {
-	return __btrfs_map_block(fs_info, op, logical, length, bioc_ret,
+	return btrfs_map_block(fs_info, op, logical, length, bioc_ret,
 				 NULL, NULL, 1);
 }
 
@@ -8066,8 +8066,8 @@ int btrfs_map_repair_block(struct btrfs_fs_info *fs_info,
 
 	ASSERT(mirror_num > 0);
 
-	ret = __btrfs_map_block(fs_info, BTRFS_MAP_WRITE, logical, &map_length,
-				&bioc, smap, &mirror_ret, true);
+	ret = btrfs_map_block(fs_info, BTRFS_MAP_WRITE, logical, &map_length,
+			      &bioc, smap, &mirror_ret, true);
 	if (ret < 0)
 		return ret;
 
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index 481f3ace988c44..c70805c8d89554 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -585,11 +585,11 @@ void btrfs_put_bioc(struct btrfs_io_context *bioc);
 int btrfs_map_sblock(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
 		     u64 logical, u64 *length,
 		     struct btrfs_io_context **bioc_ret);
-int __btrfs_map_block(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
-		      u64 logical, u64 *length,
-		      struct btrfs_io_context **bioc_ret,
-		      struct btrfs_io_stripe *smap, int *mirror_num_ret,
-		      int need_raid_map);
+int btrfs_map_block(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
+		    u64 logical, u64 *length,
+		    struct btrfs_io_context **bioc_ret,
+		    struct btrfs_io_stripe *smap, int *mirror_num_ret,
+		    int need_raid_map);
 int btrfs_map_repair_block(struct btrfs_fs_info *fs_info,
 			   struct btrfs_io_stripe *smap, u64 logical,
 			   u32 length, int mirror_num);
-- 
2.39.2

Re: [PATCH 4/6] btrfs: rename __btrfs_map_block to btrfs_map_block

[Qu Wenruo]

On 2023/5/31 12:17, Christoph Hellwig wrote:
> Now that the old btrfs_map_block is gone, drop the leading underscores
> from __btrfs_map_block.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

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

Thanks,
Qu
> ---
>   fs/btrfs/bio.c             |  4 ++--
>   fs/btrfs/check-integrity.c |  4 ++--
>   fs/btrfs/dev-replace.c     |  2 +-
>   fs/btrfs/volumes.c         | 16 ++++++++--------
>   fs/btrfs/volumes.h         | 10 +++++-----
>   5 files changed, 18 insertions(+), 18 deletions(-)
>
> diff --git a/fs/btrfs/bio.c b/fs/btrfs/bio.c
> index ae6345668d2d01..85511a8a480194 100644
> --- a/fs/btrfs/bio.c
> +++ b/fs/btrfs/bio.c
> @@ -622,8 +622,8 @@ static bool btrfs_submit_chunk(struct btrfs_bio *bbio, int mirror_num)
>   	int error;
>
>   	btrfs_bio_counter_inc_blocked(fs_info);
> -	error = __btrfs_map_block(fs_info, btrfs_op(bio), logical, &map_length,
> -				  &bioc, &smap, &mirror_num, 1);
> +	error = btrfs_map_block(fs_info, btrfs_op(bio), logical, &map_length,
> +				&bioc, &smap, &mirror_num, 1);
>   	if (error) {
>   		ret = errno_to_blk_status(error);
>   		goto fail;
> diff --git a/fs/btrfs/check-integrity.c b/fs/btrfs/check-integrity.c
> index fe15367000141a..3caf339c4bb3e4 100644
> --- a/fs/btrfs/check-integrity.c
> +++ b/fs/btrfs/check-integrity.c
> @@ -1464,8 +1464,8 @@ static int btrfsic_map_block(struct btrfsic_state *state, u64 bytenr, u32 len,
>   	struct btrfs_device *device;
>
>   	length = len;
> -	ret = __btrfs_map_block(fs_info, BTRFS_MAP_READ, bytenr, &length, &bioc,
> -				NULL, &mirror_num, 0);
> +	ret = btrfs_map_block(fs_info, BTRFS_MAP_READ, bytenr, &length, &bioc,
> +			      NULL, &mirror_num, 0);
>   	if (ret) {
>   		block_ctx_out->start = 0;
>   		block_ctx_out->dev_bytenr = 0;
> diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
> index dc3f30c79320a1..5e86bea0a9507c 100644
> --- a/fs/btrfs/dev-replace.c
> +++ b/fs/btrfs/dev-replace.c
> @@ -41,7 +41,7 @@
>    *   All new writes will be written to both target and source devices, so even
>    *   if replace gets canceled, sources device still contains up-to-date data.
>    *
> - *   Location:		handle_ops_on_dev_replace() from __btrfs_map_block()
> + *   Location:		handle_ops_on_dev_replace() from btrfs_map_block()
>    *   Start:		btrfs_dev_replace_start()
>    *   End:		btrfs_dev_replace_finishing()
>    *   Content:		Latest data/metadata
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 4c6405c4ce041d..53059ee04f9b60 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -6232,11 +6232,11 @@ static void set_io_stripe(struct btrfs_io_stripe *dst, const struct map_lookup *
>   			stripe_offset + (stripe_nr << BTRFS_STRIPE_LEN_SHIFT);
>   }
>
> -int __btrfs_map_block(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
> -		      u64 logical, u64 *length,
> -		      struct btrfs_io_context **bioc_ret,
> -		      struct btrfs_io_stripe *smap, int *mirror_num_ret,
> -		      int need_raid_map)
> +int btrfs_map_block(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
> +		    u64 logical, u64 *length,
> +		    struct btrfs_io_context **bioc_ret,
> +		    struct btrfs_io_stripe *smap, int *mirror_num_ret,
> +		    int need_raid_map)
>   {
>   	struct extent_map *em;
>   	struct map_lookup *map;
> @@ -6486,7 +6486,7 @@ int btrfs_map_sblock(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
>   		     u64 logical, u64 *length,
>   		     struct btrfs_io_context **bioc_ret)
>   {
> -	return __btrfs_map_block(fs_info, op, logical, length, bioc_ret,
> +	return btrfs_map_block(fs_info, op, logical, length, bioc_ret,
>   				 NULL, NULL, 1);
>   }
>
> @@ -8066,8 +8066,8 @@ int btrfs_map_repair_block(struct btrfs_fs_info *fs_info,
>
>   	ASSERT(mirror_num > 0);
>
> -	ret = __btrfs_map_block(fs_info, BTRFS_MAP_WRITE, logical, &map_length,
> -				&bioc, smap, &mirror_ret, true);
> +	ret = btrfs_map_block(fs_info, BTRFS_MAP_WRITE, logical, &map_length,
> +			      &bioc, smap, &mirror_ret, true);
>   	if (ret < 0)
>   		return ret;
>
> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
> index 481f3ace988c44..c70805c8d89554 100644
> --- a/fs/btrfs/volumes.h
> +++ b/fs/btrfs/volumes.h
> @@ -585,11 +585,11 @@ void btrfs_put_bioc(struct btrfs_io_context *bioc);
>   int btrfs_map_sblock(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
>   		     u64 logical, u64 *length,
>   		     struct btrfs_io_context **bioc_ret);
> -int __btrfs_map_block(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
> -		      u64 logical, u64 *length,
> -		      struct btrfs_io_context **bioc_ret,
> -		      struct btrfs_io_stripe *smap, int *mirror_num_ret,
> -		      int need_raid_map);
> +int btrfs_map_block(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
> +		    u64 logical, u64 *length,
> +		    struct btrfs_io_context **bioc_ret,
> +		    struct btrfs_io_stripe *smap, int *mirror_num_ret,
> +		    int need_raid_map);
>   int btrfs_map_repair_block(struct btrfs_fs_info *fs_info,
>   			   struct btrfs_io_stripe *smap, u64 logical,
>   			   u32 length, int mirror_num);

[PATCH 5/6] btrfs: remove btrfs_map_sblock

[Christoph Hellwig]

btrfs_map_sblock just hardcodes three arguments and calls
btrfs_map_sblock.  Remove it as it doesn't provide any real
value, but makes following the btrfs_map_block callchains harder.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/btrfs/scrub.c   | 9 +++++----
 fs/btrfs/volumes.c | 9 ---------
 fs/btrfs/volumes.h | 3 ---
 fs/btrfs/zoned.c   | 4 ++--
 4 files changed, 7 insertions(+), 18 deletions(-)

diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
index d7d8faf1978a87..8fce48d9e07a85 100644
--- a/fs/btrfs/scrub.c
+++ b/fs/btrfs/scrub.c
@@ -882,8 +882,9 @@ static void scrub_stripe_report_errors(struct scrub_ctx *sctx,
 
 		/* For scrub, our mirror_num should always start at 1. */
 		ASSERT(stripe->mirror_num >= 1);
-		ret = btrfs_map_sblock(fs_info, BTRFS_MAP_GET_READ_MIRRORS,
-				       stripe->logical, &mapped_len, &bioc);
+		ret = btrfs_map_block(fs_info, BTRFS_MAP_GET_READ_MIRRORS,
+				      stripe->logical, &mapped_len, &bioc,
+				      NULL, NULL, 1);
 		/*
 		 * If we failed, dev will be NULL, and later detailed reports
 		 * will just be skipped.
@@ -1893,8 +1894,8 @@ static int scrub_raid56_parity_stripe(struct scrub_ctx *sctx,
 	bio->bi_end_io = raid56_scrub_wait_endio;
 
 	btrfs_bio_counter_inc_blocked(fs_info);
-	ret = btrfs_map_sblock(fs_info, BTRFS_MAP_WRITE, full_stripe_start,
-			       &length, &bioc);
+	ret = btrfs_map_block(fs_info, BTRFS_MAP_WRITE, full_stripe_start,
+			      &length, &bioc, NULL, NULL, 1);
 	if (ret < 0) {
 		btrfs_put_bioc(bioc);
 		btrfs_bio_counter_dec(fs_info);
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 53059ee04f9b60..6141a9fe5a28a0 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -6481,15 +6481,6 @@ int btrfs_map_block(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
 	return ret;
 }
 
-/* For Scrub/replace */
-int btrfs_map_sblock(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
-		     u64 logical, u64 *length,
-		     struct btrfs_io_context **bioc_ret)
-{
-	return btrfs_map_block(fs_info, op, logical, length, bioc_ret,
-				 NULL, NULL, 1);
-}
-
 static bool dev_args_match_fs_devices(const struct btrfs_dev_lookup_args *args,
 				      const struct btrfs_fs_devices *fs_devices)
 {
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index c70805c8d89554..3930ee01d6968f 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -582,9 +582,6 @@ static inline unsigned long btrfs_chunk_item_size(int num_stripes)
 
 void btrfs_get_bioc(struct btrfs_io_context *bioc);
 void btrfs_put_bioc(struct btrfs_io_context *bioc);
-int btrfs_map_sblock(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
-		     u64 logical, u64 *length,
-		     struct btrfs_io_context **bioc_ret);
 int btrfs_map_block(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
 		    u64 logical, u64 *length,
 		    struct btrfs_io_context **bioc_ret,
diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
index e311aae8f1ddca..bbde4ddd475492 100644
--- a/fs/btrfs/zoned.c
+++ b/fs/btrfs/zoned.c
@@ -1782,8 +1782,8 @@ static int read_zone_info(struct btrfs_fs_info *fs_info, u64 logical,
 	int nmirrors;
 	int i, ret;
 
-	ret = btrfs_map_sblock(fs_info, BTRFS_MAP_GET_READ_MIRRORS, logical,
-			       &mapped_length, &bioc);
+	ret = btrfs_map_block(fs_info, BTRFS_MAP_GET_READ_MIRRORS, logical,
+			      &mapped_length, &bioc, NULL, NULL, 1);
 	if (ret || !bioc || mapped_length < PAGE_SIZE) {
 		ret = -EIO;
 		goto out_put_bioc;
-- 
2.39.2

Re: [PATCH 5/6] btrfs: remove btrfs_map_sblock

[Qu Wenruo]

On 2023/5/31 12:17, Christoph Hellwig wrote:
> btrfs_map_sblock just hardcodes three arguments and calls
> btrfs_map_sblock.  Remove it as it doesn't provide any real
> value, but makes following the btrfs_map_block callchains harder.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

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

Thanks,
Qu
> ---
>   fs/btrfs/scrub.c   | 9 +++++----
>   fs/btrfs/volumes.c | 9 ---------
>   fs/btrfs/volumes.h | 3 ---
>   fs/btrfs/zoned.c   | 4 ++--
>   4 files changed, 7 insertions(+), 18 deletions(-)
>
> diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
> index d7d8faf1978a87..8fce48d9e07a85 100644
> --- a/fs/btrfs/scrub.c
> +++ b/fs/btrfs/scrub.c
> @@ -882,8 +882,9 @@ static void scrub_stripe_report_errors(struct scrub_ctx *sctx,
>
>   		/* For scrub, our mirror_num should always start at 1. */
>   		ASSERT(stripe->mirror_num >= 1);
> -		ret = btrfs_map_sblock(fs_info, BTRFS_MAP_GET_READ_MIRRORS,
> -				       stripe->logical, &mapped_len, &bioc);
> +		ret = btrfs_map_block(fs_info, BTRFS_MAP_GET_READ_MIRRORS,
> +				      stripe->logical, &mapped_len, &bioc,
> +				      NULL, NULL, 1);
>   		/*
>   		 * If we failed, dev will be NULL, and later detailed reports
>   		 * will just be skipped.
> @@ -1893,8 +1894,8 @@ static int scrub_raid56_parity_stripe(struct scrub_ctx *sctx,
>   	bio->bi_end_io = raid56_scrub_wait_endio;
>
>   	btrfs_bio_counter_inc_blocked(fs_info);
> -	ret = btrfs_map_sblock(fs_info, BTRFS_MAP_WRITE, full_stripe_start,
> -			       &length, &bioc);
> +	ret = btrfs_map_block(fs_info, BTRFS_MAP_WRITE, full_stripe_start,
> +			      &length, &bioc, NULL, NULL, 1);
>   	if (ret < 0) {
>   		btrfs_put_bioc(bioc);
>   		btrfs_bio_counter_dec(fs_info);
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 53059ee04f9b60..6141a9fe5a28a0 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -6481,15 +6481,6 @@ int btrfs_map_block(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
>   	return ret;
>   }
>
> -/* For Scrub/replace */
> -int btrfs_map_sblock(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
> -		     u64 logical, u64 *length,
> -		     struct btrfs_io_context **bioc_ret)
> -{
> -	return btrfs_map_block(fs_info, op, logical, length, bioc_ret,
> -				 NULL, NULL, 1);
> -}
> -
>   static bool dev_args_match_fs_devices(const struct btrfs_dev_lookup_args *args,
>   				      const struct btrfs_fs_devices *fs_devices)
>   {
> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
> index c70805c8d89554..3930ee01d6968f 100644
> --- a/fs/btrfs/volumes.h
> +++ b/fs/btrfs/volumes.h
> @@ -582,9 +582,6 @@ static inline unsigned long btrfs_chunk_item_size(int num_stripes)
>
>   void btrfs_get_bioc(struct btrfs_io_context *bioc);
>   void btrfs_put_bioc(struct btrfs_io_context *bioc);
> -int btrfs_map_sblock(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
> -		     u64 logical, u64 *length,
> -		     struct btrfs_io_context **bioc_ret);
>   int btrfs_map_block(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
>   		    u64 logical, u64 *length,
>   		    struct btrfs_io_context **bioc_ret,
> diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
> index e311aae8f1ddca..bbde4ddd475492 100644
> --- a/fs/btrfs/zoned.c
> +++ b/fs/btrfs/zoned.c
> @@ -1782,8 +1782,8 @@ static int read_zone_info(struct btrfs_fs_info *fs_info, u64 logical,
>   	int nmirrors;
>   	int i, ret;
>
> -	ret = btrfs_map_sblock(fs_info, BTRFS_MAP_GET_READ_MIRRORS, logical,
> -			       &mapped_length, &bioc);
> +	ret = btrfs_map_block(fs_info, BTRFS_MAP_GET_READ_MIRRORS, logical,
> +			      &mapped_length, &bioc, NULL, NULL, 1);
>   	if (ret || !bioc || mapped_length < PAGE_SIZE) {
>   		ret = -EIO;
>   		goto out_put_bioc;

[PATCH 6/6] btrfs: remove need_full_stripe

[Christoph Hellwig]

need_full_stripe is just a somewhat complicated way to say
"op != BTRFS_MAP_READ".  Just spell that explicit check out, which makes
a lot of the code currently using the helper easier to understand.

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 6141a9fe5a28a0..8137c04f31c9cd 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -6173,11 +6173,6 @@ static void handle_ops_on_dev_replace(enum btrfs_map_op op,
 	bioc->replace_nr_stripes = nr_extra_stripes;
 }
 
-static bool need_full_stripe(enum btrfs_map_op op)
-{
-	return (op == BTRFS_MAP_WRITE || op == BTRFS_MAP_GET_READ_MIRRORS);
-}
-
 static u64 btrfs_max_io_len(struct map_lookup *map, enum btrfs_map_op op,
 			    u64 offset, u32 *stripe_nr, u64 *stripe_offset,
 			    u64 *full_stripe_start)
@@ -6290,21 +6285,21 @@ int btrfs_map_block(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
 	if (map->type & BTRFS_BLOCK_GROUP_RAID0) {
 		stripe_index = stripe_nr % map->num_stripes;
 		stripe_nr /= map->num_stripes;
-		if (!need_full_stripe(op))
+		if (op == BTRFS_MAP_READ)
 			mirror_num = 1;
 	} else if (map->type & BTRFS_BLOCK_GROUP_RAID1_MASK) {
-		if (need_full_stripe(op))
+		if (op != BTRFS_MAP_READ) {
 			num_stripes = map->num_stripes;
-		else if (mirror_num)
+		} else if (mirror_num) {
 			stripe_index = mirror_num - 1;
-		else {
+		} else {
 			stripe_index = find_live_mirror(fs_info, map, 0,
 					    dev_replace_is_ongoing);
 			mirror_num = stripe_index + 1;
 		}
 
 	} else if (map->type & BTRFS_BLOCK_GROUP_DUP) {
-		if (need_full_stripe(op)) {
+		if (op != BTRFS_MAP_READ) {
 			num_stripes = map->num_stripes;
 		} else if (mirror_num) {
 			stripe_index = mirror_num - 1;
@@ -6318,7 +6313,7 @@ int btrfs_map_block(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
 		stripe_index = (stripe_nr % factor) * map->sub_stripes;
 		stripe_nr /= factor;
 
-		if (need_full_stripe(op))
+		if (op != BTRFS_MAP_READ)
 			num_stripes = map->sub_stripes;
 		else if (mirror_num)
 			stripe_index += mirror_num - 1;
@@ -6331,7 +6326,7 @@ int btrfs_map_block(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
 		}
 
 	} else if (map->type & BTRFS_BLOCK_GROUP_RAID56_MASK) {
-		if (need_raid_map && (need_full_stripe(op) || mirror_num > 1)) {
+		if (need_raid_map && (op != BTRFS_MAP_READ || mirror_num > 1)) {
 			/*
 			 * Push stripe_nr back to the start of the full stripe
 			 * For those cases needing a full stripe, @stripe_nr
@@ -6366,7 +6361,7 @@ int btrfs_map_block(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
 
 			/* We distribute the parity blocks across stripes */
 			stripe_index = (stripe_nr + stripe_index) % map->num_stripes;
-			if (!need_full_stripe(op) && mirror_num <= 1)
+			if (op == BTRFS_MAP_READ && mirror_num <= 1)
 				mirror_num = 1;
 		}
 	} else {
@@ -6406,7 +6401,7 @@ int btrfs_map_block(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
 	 */
 	if (smap && num_alloc_stripes == 1 &&
 	    !((map->type & BTRFS_BLOCK_GROUP_RAID56_MASK) && mirror_num > 1) &&
-	    (!need_full_stripe(op) || !dev_replace_is_ongoing ||
+	    (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;
@@ -6430,7 +6425,7 @@ int btrfs_map_block(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
 	 * It's still mostly the same as other profiles, just with extra rotation.
 	 */
 	if (map->type & BTRFS_BLOCK_GROUP_RAID56_MASK && need_raid_map &&
-	    (need_full_stripe(op) || mirror_num > 1)) {
+	    (op != BTRFS_MAP_READ || mirror_num > 1)) {
 		/*
 		 * For RAID56 @stripe_nr is already the number of full stripes
 		 * before us, which is also the rotation value (needs to modulo
@@ -6457,11 +6452,11 @@ int btrfs_map_block(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
 		}
 	}
 
-	if (need_full_stripe(op))
+	if (op != BTRFS_MAP_READ)
 		max_errors = btrfs_chunk_max_errors(map);
 
 	if (dev_replace_is_ongoing && dev_replace->tgtdev != NULL &&
-	    need_full_stripe(op)) {
+	    op != BTRFS_MAP_READ) {
 		handle_ops_on_dev_replace(op, bioc, dev_replace, logical,
 					  &num_stripes, &max_errors);
 	}
-- 
2.39.2

Re: [PATCH 6/6] btrfs: remove need_full_stripe

[Qu Wenruo]

On 2023/5/31 12:17, Christoph Hellwig wrote:
> need_full_stripe is just a somewhat complicated way to say
> "op != BTRFS_MAP_READ".  Just spell that explicit check out, which makes
> a lot of the code currently using the helper easier to understand.

In fact the old "need_full_stripe" can even be confusing, as
BTRFS_MAP_READ with mirror_num > 1 on RAID56 would still need all the
stripes.

So removing it completely is indeed better.

>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

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

Thanks,
Qu
> ---
>   fs/btrfs/volumes.c | 29 ++++++++++++-----------------
>   1 file changed, 12 insertions(+), 17 deletions(-)
>
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 6141a9fe5a28a0..8137c04f31c9cd 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -6173,11 +6173,6 @@ static void handle_ops_on_dev_replace(enum btrfs_map_op op,
>   	bioc->replace_nr_stripes = nr_extra_stripes;
>   }
>
> -static bool need_full_stripe(enum btrfs_map_op op)
> -{
> -	return (op == BTRFS_MAP_WRITE || op == BTRFS_MAP_GET_READ_MIRRORS);
> -}
> -
>   static u64 btrfs_max_io_len(struct map_lookup *map, enum btrfs_map_op op,
>   			    u64 offset, u32 *stripe_nr, u64 *stripe_offset,
>   			    u64 *full_stripe_start)
> @@ -6290,21 +6285,21 @@ int btrfs_map_block(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
>   	if (map->type & BTRFS_BLOCK_GROUP_RAID0) {
>   		stripe_index = stripe_nr % map->num_stripes;
>   		stripe_nr /= map->num_stripes;
> -		if (!need_full_stripe(op))
> +		if (op == BTRFS_MAP_READ)
>   			mirror_num = 1;
>   	} else if (map->type & BTRFS_BLOCK_GROUP_RAID1_MASK) {
> -		if (need_full_stripe(op))
> +		if (op != BTRFS_MAP_READ) {
>   			num_stripes = map->num_stripes;
> -		else if (mirror_num)
> +		} else if (mirror_num) {
>   			stripe_index = mirror_num - 1;
> -		else {
> +		} else {
>   			stripe_index = find_live_mirror(fs_info, map, 0,
>   					    dev_replace_is_ongoing);
>   			mirror_num = stripe_index + 1;
>   		}
>
>   	} else if (map->type & BTRFS_BLOCK_GROUP_DUP) {
> -		if (need_full_stripe(op)) {
> +		if (op != BTRFS_MAP_READ) {
>   			num_stripes = map->num_stripes;
>   		} else if (mirror_num) {
>   			stripe_index = mirror_num - 1;
> @@ -6318,7 +6313,7 @@ int btrfs_map_block(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
>   		stripe_index = (stripe_nr % factor) * map->sub_stripes;
>   		stripe_nr /= factor;
>
> -		if (need_full_stripe(op))
> +		if (op != BTRFS_MAP_READ)
>   			num_stripes = map->sub_stripes;
>   		else if (mirror_num)
>   			stripe_index += mirror_num - 1;
> @@ -6331,7 +6326,7 @@ int btrfs_map_block(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
>   		}
>
>   	} else if (map->type & BTRFS_BLOCK_GROUP_RAID56_MASK) {
> -		if (need_raid_map && (need_full_stripe(op) || mirror_num > 1)) {
> +		if (need_raid_map && (op != BTRFS_MAP_READ || mirror_num > 1)) {
>   			/*
>   			 * Push stripe_nr back to the start of the full stripe
>   			 * For those cases needing a full stripe, @stripe_nr
> @@ -6366,7 +6361,7 @@ int btrfs_map_block(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
>
>   			/* We distribute the parity blocks across stripes */
>   			stripe_index = (stripe_nr + stripe_index) % map->num_stripes;
> -			if (!need_full_stripe(op) && mirror_num <= 1)
> +			if (op == BTRFS_MAP_READ && mirror_num <= 1)
>   				mirror_num = 1;
>   		}
>   	} else {
> @@ -6406,7 +6401,7 @@ int btrfs_map_block(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
>   	 */
>   	if (smap && num_alloc_stripes == 1 &&
>   	    !((map->type & BTRFS_BLOCK_GROUP_RAID56_MASK) && mirror_num > 1) &&
> -	    (!need_full_stripe(op) || !dev_replace_is_ongoing ||
> +	    (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;
> @@ -6430,7 +6425,7 @@ int btrfs_map_block(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
>   	 * It's still mostly the same as other profiles, just with extra rotation.
>   	 */
>   	if (map->type & BTRFS_BLOCK_GROUP_RAID56_MASK && need_raid_map &&
> -	    (need_full_stripe(op) || mirror_num > 1)) {
> +	    (op != BTRFS_MAP_READ || mirror_num > 1)) {
>   		/*
>   		 * For RAID56 @stripe_nr is already the number of full stripes
>   		 * before us, which is also the rotation value (needs to modulo
> @@ -6457,11 +6452,11 @@ int btrfs_map_block(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
>   		}
>   	}
>
> -	if (need_full_stripe(op))
> +	if (op != BTRFS_MAP_READ)
>   		max_errors = btrfs_chunk_max_errors(map);
>
>   	if (dev_replace_is_ongoing && dev_replace->tgtdev != NULL &&
> -	    need_full_stripe(op)) {
> +	    op != BTRFS_MAP_READ) {
>   		handle_ops_on_dev_replace(op, bioc, dev_replace, logical,
>   					  &num_stripes, &max_errors);
>   	}

Re: [PATCH 6/6] btrfs: remove need_full_stripe

[Christoph Hellwig]

On Wed, May 31, 2023 at 04:52:53PM +0800, Qu Wenruo wrote:
>> need_full_stripe is just a somewhat complicated way to say
>> "op != BTRFS_MAP_READ".  Just spell that explicit check out, which makes
>> a lot of the code currently using the helper easier to understand.
>
> In fact the old "need_full_stripe" can even be confusing, as
> BTRFS_MAP_READ with mirror_num > 1 on RAID56 would still need all the
> stripes.

Yes, that's one of the reasons.  And that in general in the conditionals
READ/!READ tends to explain the logic better than !need_full_stripe vs
need_full_stripe.

Re: cleanup the btrfs_map_block interface

[Johannes Thumshirn]

On 31.05.23 06:17, Christoph Hellwig wrote:
> Hi all,
> 
> the interface around btrfs_map_block is a bit confusion, mostly due to
> the fact that it has a double-underscored complex version and two
> wrappers that just take a few arguments away.  This series cleans up
> a few loose ends to make this interface easier to understand.
> 
> Diffstat:
>  bio.c             |    4 +--
>  check-integrity.c |   19 ++++++++++------
>  dev-replace.c     |    2 -
>  scrub.c           |    9 ++++---
>  volumes.c         |   63 ++++++++++++++++--------------------------------------
>  volumes.h         |   15 ++----------
>  zoned.c           |    4 +--
>  7 files changed, 44 insertions(+), 72 deletions(-)
> 

For the series:
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: cleanup the btrfs_map_block interface

[David Sterba]

On Wed, May 31, 2023 at 06:17:33AM +0200, Christoph Hellwig wrote:
> Hi all,
> 
> the interface around btrfs_map_block is a bit confusion, mostly due to
> the fact that it has a double-underscored complex version and two
> wrappers that just take a few arguments away.  This series cleans up
> a few loose ends to make this interface easier to understand.

Added to misc-next, thanks.