[PATCH] btrfs: add comments for btrfs_map_block()

[PATCH] btrfs: add comments for btrfs_map_block()

[Qu Wenruo]

The function btrfs_map_block() is a critical part of the btrfs storage
layer, which handles mapping of logical ranges to physical ranges.

Thus it's better to have some basic explanation, especially on the
following points:

- Segment split by various boundaries
  As a continuous logical range may be split into different segments,
  due to various factors like zones and RAID0/5/6/10 boundaries.

- The meaning of @mirror_num

- The possible single stripe optimization

- One deprecated parameter @need_raid_map
  Just explicitly mark it deprecated so we're aware of the problem.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
There would be a follow up patch, to add one new case for
@mirror_num, where for RAID56 we allow mirror_num > num_copies, as a way
to read P/Q stripes directly for the incoming scrub_logical.

Thus I believe it's better to explicit explain @mirror_num_ret at least.

Also if @need_raid_map can be finally removed, we can also remove the
corner case for special op == READ && !need_raid_map case where
mirror_num == 2 means P stripe.
---
 fs/btrfs/volumes.c | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index ed15c89d4350..efac9293446d 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -6227,6 +6227,45 @@ static void set_io_stripe(struct btrfs_io_stripe *dst, const struct map_lookup *
 			stripe_offset + ((u64)stripe_nr << BTRFS_STRIPE_LEN_SHIFT);
 }
 
+/*
+ * Map one logical range to one or more physical ranges.
+ *
+ * @length:		(Mandatory) mapped length of this run.
+ *			One logical range can be split into different segments
+ *			due to factors like zones and RAID0/5/6/10 stripe
+ *			boundaries.
+ *
+ * @bioc_ret:		(Mandatory) returned btrfs_io_context structure.
+ *			which has one or more physical ranges (btrfs_io_stripe)
+ *			recorded inside.
+ *			Caller should call btrfs_put_bioc() to free it after use.
+ *
+ * @smap:		(Optional) single physical range optimization.
+ *			If the map request can be fulfilled by one single
+ *			physical range, and this is parameter is not NULL,
+ *			then @bioc_ret would be NULL, and @smap would be
+ *			updated.
+ *
+ * @mirror_num_ret:	(Mandatory) returned mirror number if the original
+ *			value is 0.
+ *
+ *			Mirror number 0 means to choose any live mirrors.
+ *
+ *			For non-RAID56 profiles, non-zero mirror_num means
+ *			the Nth mirror. (e.g. mirror_num 1 means the first
+ *			copy).
+ *
+ *			For RAID56 profile, mirror 1 means rebuild from P and
+ *			the remaingin data stripes.
+ *
+ *			For RAID6 profile, mirror > 2 means mark another
+ *			data/P stripe error and rebuild from the remaining
+ *			stripes..
+ *
+ * @need_raid_map:	(Deprecated) whether the map wants a full stripe map
+ *			(including all data and P/Q stripes) for RAID56.
+ *			Should always be 1 except for legacy call sites.
+ */
 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,
-- 
2.41.0

Re: [PATCH] btrfs: add comments for btrfs_map_block()

[Qu Wenruo]

On 2023/6/27 15:34, Qu Wenruo wrote:
> The function btrfs_map_block() is a critical part of the btrfs storage
> layer, which handles mapping of logical ranges to physical ranges.
> 
> Thus it's better to have some basic explanation, especially on the
> following points:
> 
> - Segment split by various boundaries
>    As a continuous logical range may be split into different segments,
>    due to various factors like zones and RAID0/5/6/10 boundaries.
> 
> - The meaning of @mirror_num
> 
> - The possible single stripe optimization
> 
> - One deprecated parameter @need_raid_map
>    Just explicitly mark it deprecated so we're aware of the problem.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
> There would be a follow up patch, to add one new case for
> @mirror_num, where for RAID56 we allow mirror_num > num_copies, as a way
> to read P/Q stripes directly for the incoming scrub_logical.

It turns out that, if we allow mirror_num -1 to indicate P, and -2 for Q 
it can be much easier to do the calculation.

But that future feature should not affect this patch anyway.

Thanks,
Qu

> 
> Thus I believe it's better to explicit explain @mirror_num_ret at least.
> 
> Also if @need_raid_map can be finally removed, we can also remove the
> corner case for special op == READ && !need_raid_map case where
> mirror_num == 2 means P stripe.
> ---
>   fs/btrfs/volumes.c | 39 +++++++++++++++++++++++++++++++++++++++
>   1 file changed, 39 insertions(+)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index ed15c89d4350..efac9293446d 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -6227,6 +6227,45 @@ static void set_io_stripe(struct btrfs_io_stripe *dst, const struct map_lookup *
>   			stripe_offset + ((u64)stripe_nr << BTRFS_STRIPE_LEN_SHIFT);
>   }
>   
> +/*
> + * Map one logical range to one or more physical ranges.
> + *
> + * @length:		(Mandatory) mapped length of this run.
> + *			One logical range can be split into different segments
> + *			due to factors like zones and RAID0/5/6/10 stripe
> + *			boundaries.
> + *
> + * @bioc_ret:		(Mandatory) returned btrfs_io_context structure.
> + *			which has one or more physical ranges (btrfs_io_stripe)
> + *			recorded inside.
> + *			Caller should call btrfs_put_bioc() to free it after use.
> + *
> + * @smap:		(Optional) single physical range optimization.
> + *			If the map request can be fulfilled by one single
> + *			physical range, and this is parameter is not NULL,
> + *			then @bioc_ret would be NULL, and @smap would be
> + *			updated.
> + *
> + * @mirror_num_ret:	(Mandatory) returned mirror number if the original
> + *			value is 0.
> + *
> + *			Mirror number 0 means to choose any live mirrors.
> + *
> + *			For non-RAID56 profiles, non-zero mirror_num means
> + *			the Nth mirror. (e.g. mirror_num 1 means the first
> + *			copy).
> + *
> + *			For RAID56 profile, mirror 1 means rebuild from P and
> + *			the remaingin data stripes.
> + *
> + *			For RAID6 profile, mirror > 2 means mark another
> + *			data/P stripe error and rebuild from the remaining
> + *			stripes..
> + *
> + * @need_raid_map:	(Deprecated) whether the map wants a full stripe map
> + *			(including all data and P/Q stripes) for RAID56.
> + *			Should always be 1 except for legacy call sites.
> + */
>   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,

Re: [PATCH] btrfs: add comments for btrfs_map_block()

[Christoph Hellwig]

On Tue, Jun 27, 2023 at 03:34:31PM +0800, Qu Wenruo wrote:
> + * @mirror_num_ret:	(Mandatory) returned mirror number if the original
> + *			value is 0.

This one is optional.

> + *
> + * @need_raid_map:	(Deprecated) whether the map wants a full stripe map
> + *			(including all data and P/Q stripes) for RAID56.
> + *			Should always be 1 except for legacy call sites.
> + */

Saying deprecated for a paramter to function with just a few callers
feels weird.  For this patch I'd just stick to a functional description.

As far as I can tell need_raid_map just creates extra overhead when
used on a paritty RAID BG, but is othr wise harmless and only
btrfsic_map_block clears it to 0.  Maybe add a follow on patch to just
remove the paramter and waste the little bit of extra effort to build
the raid map for btrfsic_map_block?

Re: [PATCH] btrfs: add comments for btrfs_map_block()

[Qu Wenruo]

On 2023/6/28 00:07, Christoph Hellwig wrote:
> On Tue, Jun 27, 2023 at 03:34:31PM +0800, Qu Wenruo wrote:
>> + * @mirror_num_ret:	(Mandatory) returned mirror number if the original
>> + *			value is 0.
>
> This one is optional.
>
>> + *
>> + * @need_raid_map:	(Deprecated) whether the map wants a full stripe map
>> + *			(including all data and P/Q stripes) for RAID56.
>> + *			Should always be 1 except for legacy call sites.
>> + */
>
> Saying deprecated for a paramter to function with just a few callers
> feels weird.  For this patch I'd just stick to a functional description.
>
> As far as I can tell need_raid_map just creates extra overhead when
> used on a paritty RAID BG, but is othr wise harmless and only
> btrfsic_map_block clears it to 0.

Yep, and in fact whether we need to allocate the full stripe mapping
should not be determined by this parameter.
@op and @mirror_num is really determining if we need the full stripe
mapping.

And even if we want a way to return P/Q stripe directly (later
scrub_logical would need it), we can go single stripe fast path for such
case.

Really it's btrfsic_map_block() the only exception.

>  Maybe add a follow on patch to just
> remove the paramter and waste the little bit of extra effort to build
> the raid map for btrfsic_map_block?
>

Well, I'd prefer to remove btrfsic_map_block() and the check_int thing
completely.

Thanks,
Qu

Re: [PATCH] btrfs: add comments for btrfs_map_block()

[Christoph Hellwig]

On Wed, Jun 28, 2023 at 06:00:56AM +0800, Qu Wenruo wrote:
> Really it's btrfsic_map_block() the only exception.
> 
> >  Maybe add a follow on patch to just
> > remove the paramter and waste the little bit of extra effort to build
> > the raid map for btrfsic_map_block?
> > 
> 
> Well, I'd prefer to remove btrfsic_map_block() and the check_int thing
> completely.

I'm perfectly fine with removing it.  But I don't see why that needs
to delay dropping the raid_map argument.

Re: [PATCH] btrfs: add comments for btrfs_map_block()

[Qu Wenruo]

On 2023/6/28 12:59, Christoph Hellwig wrote:
> On Wed, Jun 28, 2023 at 06:00:56AM +0800, Qu Wenruo wrote:
>> Really it's btrfsic_map_block() the only exception.
>>
>>>   Maybe add a follow on patch to just
>>> remove the paramter and waste the little bit of extra effort to build
>>> the raid map for btrfsic_map_block?
>>>
>>
>> Well, I'd prefer to remove btrfsic_map_block() and the check_int thing
>> completely.
>
> I'm perfectly fine with removing it.  But I don't see why that needs
> to delay dropping the raid_map argument.
>
If we're dropping check_int completely, wouldn't it make no sense to
update the only caller of @need_raid_map?

We can just wait the check_int drop, then safely drop @need_raid_map
argument.

Thanks,
Qu

Re: [PATCH] btrfs: add comments for btrfs_map_block()

[David Sterba]

On Tue, Jun 27, 2023 at 03:34:31PM +0800, Qu Wenruo wrote:
> The function btrfs_map_block() is a critical part of the btrfs storage
> layer, which handles mapping of logical ranges to physical ranges.
> 
> Thus it's better to have some basic explanation, especially on the
> following points:
> 
> - Segment split by various boundaries
>   As a continuous logical range may be split into different segments,
>   due to various factors like zones and RAID0/5/6/10 boundaries.
> 
> - The meaning of @mirror_num
> 
> - The possible single stripe optimization
> 
> - One deprecated parameter @need_raid_map
>   Just explicitly mark it deprecated so we're aware of the problem.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
> There would be a follow up patch, to add one new case for
> @mirror_num, where for RAID56 we allow mirror_num > num_copies, as a way
> to read P/Q stripes directly for the incoming scrub_logical.
> 
> Thus I believe it's better to explicit explain @mirror_num_ret at least.
> 
> Also if @need_raid_map can be finally removed, we can also remove the
> corner case for special op == READ && !need_raid_map case where
> mirror_num == 2 means P stripe.

Ok. Added to misc-next, thanks.

> ---
>  fs/btrfs/volumes.c | 39 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index ed15c89d4350..efac9293446d 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -6227,6 +6227,45 @@ static void set_io_stripe(struct btrfs_io_stripe *dst, const struct map_lookup *
>  			stripe_offset + ((u64)stripe_nr << BTRFS_STRIPE_LEN_SHIFT);
>  }
>  
> +/*
> + * Map one logical range to one or more physical ranges.
> + *
> + * @length:		(Mandatory) mapped length of this run.
> + *			One logical range can be split into different segments
> + *			due to factors like zones and RAID0/5/6/10 stripe
> + *			boundaries.
> + *
> + * @bioc_ret:		(Mandatory) returned btrfs_io_context structure.
> + *			which has one or more physical ranges (btrfs_io_stripe)
> + *			recorded inside.
> + *			Caller should call btrfs_put_bioc() to free it after use.
> + *
> + * @smap:		(Optional) single physical range optimization.
> + *			If the map request can be fulfilled by one single
> + *			physical range, and this is parameter is not NULL,
> + *			then @bioc_ret would be NULL, and @smap would be
> + *			updated.
> + *
> + * @mirror_num_ret:	(Mandatory) returned mirror number if the original
> + *			value is 0.
> + *
> + *			Mirror number 0 means to choose any live mirrors.
> + *
> + *			For non-RAID56 profiles, non-zero mirror_num means
> + *			the Nth mirror. (e.g. mirror_num 1 means the first
> + *			copy).
> + *
> + *			For RAID56 profile, mirror 1 means rebuild from P and
> + *			the remaingin data stripes.
> + *
> + *			For RAID6 profile, mirror > 2 means mark another
> + *			data/P stripe error and rebuild from the remaining
> + *			stripes..
> + *
> + * @need_raid_map:	(Deprecated) whether the map wants a full stripe map
> + *			(including all data and P/Q stripes) for RAID56.
> + *			Should always be 1 except for legacy call sites.
> + */
>  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,

There are some parameters that are not explained, though they're self
explaining and given how much text is needed for the others I don't
think we need to mention them.

Re: [PATCH] btrfs: add comments for btrfs_map_block()

[David Sterba]

On Tue, Jun 27, 2023 at 09:07:32AM -0700, Christoph Hellwig wrote:
> On Tue, Jun 27, 2023 at 03:34:31PM +0800, Qu Wenruo wrote:
> > + * @mirror_num_ret:	(Mandatory) returned mirror number if the original
> > + *			value is 0.
> 
> This one is optional.
> 
> > + *
> > + * @need_raid_map:	(Deprecated) whether the map wants a full stripe map
> > + *			(including all data and P/Q stripes) for RAID56.
> > + *			Should always be 1 except for legacy call sites.
> > + */
> 
> Saying deprecated for a paramter to function with just a few callers
> feels weird.  For this patch I'd just stick to a functional description.

Agreed, I've updated the comment to refer to integrity checker instead
of 'deprecated'.