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

[David Sterba <dsterba@suse.cz>]

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.