| Message ID | 4564c119bf29d7646033a292c208ffcab6589414.1687851190.git.wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: add comments for btrfs_map_block() | expand |
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,
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?
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
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.
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
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.
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'.
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,
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(+)