| Message ID | 9509878ff5631eb2563855c0de694f296e23e0f2.1676985912.git.johannes.thumshirn@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: reflow calc_bio_boundaries | expand |
On 2/21/23 21:25, Johannes Thumshirn wrote: > calc_bio_boundaries() is only used for guaranteeing the one bio equals to one > ordered extent rule for uncompressed Zone Append bios. > > Re-flow the function to exit early in case we're not operating on an > uncompressed Zone Append and then calculate the boundary. > > This imposes no functional changes but improves readability. > > Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> > --- > fs/btrfs/extent_io.c | 25 ++++++++++++++----------- > 1 file changed, 14 insertions(+), 11 deletions(-) > > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c > index c25fa74d7615..c0442f2ed150 100644 > --- a/fs/btrfs/extent_io.c > +++ b/fs/btrfs/extent_io.c > @@ -956,19 +956,22 @@ static void calc_bio_boundaries(struct btrfs_bio_ctrl *bio_ctrl, > * Compressed bios aren't submitted directly, so it doesn't apply to > * them. > */ > - if (bio_ctrl->compress_type == BTRFS_COMPRESS_NONE && > - btrfs_use_zone_append(btrfs_bio(bio_ctrl->bio))) { > - ordered = btrfs_lookup_ordered_extent(inode, file_offset); > - if (ordered) { > - bio_ctrl->len_to_oe_boundary = min_t(u32, U32_MAX, > + bio_ctrl->len_to_oe_boundary = U32_MAX; Should bio_ctrl->len_to_oe_boundary be set here? It appears to be unused until its value is overwritten a few lines later. > + > + if (bio_ctrl->compress_type != BTRFS_COMPRESS_NONE) > + return; > + > + if (!btrfs_use_zone_append(btrfs_bio(bio_ctrl->bio))) > + return; > + > + ordered = btrfs_lookup_ordered_extent(inode, file_offset); > + if (!ordered) > + return; > + > + bio_ctrl->len_to_oe_boundary = min_t(u32, U32_MAX, > ordered->file_offset + > ordered->disk_num_bytes - file_offset); Here. Thanks, Anand > - btrfs_put_ordered_extent(ordered); > - return; > - } > - } > - > - bio_ctrl->len_to_oe_boundary = U32_MAX; > + btrfs_put_ordered_extent(ordered); > } > > static void alloc_new_bio(struct btrfs_inode *inode,
On 21.02.23 14:57, Anand Jain wrote: >> - if (bio_ctrl->compress_type == BTRFS_COMPRESS_NONE && >> - btrfs_use_zone_append(btrfs_bio(bio_ctrl->bio))) { >> - ordered = btrfs_lookup_ordered_extent(inode, file_offset); >> - if (ordered) { >> - bio_ctrl->len_to_oe_boundary = min_t(u32, U32_MAX, > >> + bio_ctrl->len_to_oe_boundary = U32_MAX; > > Should bio_ctrl->len_to_oe_boundary be set here? > It appears to be unused until its value is overwritten a > few lines later. > >> + >> + if (bio_ctrl->compress_type != BTRFS_COMPRESS_NONE) >> + return; >> + >> + if (!btrfs_use_zone_append(btrfs_bio(bio_ctrl->bio))) >> + return; >> + >> + ordered = btrfs_lookup_ordered_extent(inode, file_offset); >> + if (!ordered) >> + return; >> + > >> + bio_ctrl->len_to_oe_boundary = min_t(u32, U32_MAX, >> ordered->file_offset + >> ordered->disk_num_bytes - file_offset); > > > Here. > If you have a look at the original code, the setting was at the end of the function [1]. So yes it will get overwritten in case we have a Zone Append bio but I think the impact of that is neglectable compared to the better readability of the re-flowed version. [1] https://github.com/kdave/btrfs-devel/blob/misc-next/fs/btrfs/extent_io.c#L971
On Tue, Feb 21, 2023 at 05:25:36AM -0800, Johannes Thumshirn wrote: > calc_bio_boundaries() is only used for guaranteeing the one bio equals to one > ordered extent rule for uncompressed Zone Append bios. > > Re-flow the function to exit early in case we're not operating on an > uncompressed Zone Append and then calculate the boundary. > > This imposes no functional changes but improves readability. This looks correct but doesn't really read much better to me. My mid-term plan here is to instead keep a refrence to the ordered extent in the bio_ctrl and get rid of the len_to_oe_boundary value, in which case this is just a bit more churn. But I'm not sure I'm going to get to that yet for the 6.4 merge window.
On 2/21/23 22:09, Johannes Thumshirn wrote: > On 21.02.23 14:57, Anand Jain wrote: > >>> - if (bio_ctrl->compress_type == BTRFS_COMPRESS_NONE && >>> - btrfs_use_zone_append(btrfs_bio(bio_ctrl->bio))) { >>> - ordered = btrfs_lookup_ordered_extent(inode, file_offset); >>> - if (ordered) { >>> - bio_ctrl->len_to_oe_boundary = min_t(u32, U32_MAX, >> >>> + bio_ctrl->len_to_oe_boundary = U32_MAX; >> >> Should bio_ctrl->len_to_oe_boundary be set here? >> It appears to be unused until its value is overwritten a >> few lines later. >> >>> + >>> + if (bio_ctrl->compress_type != BTRFS_COMPRESS_NONE) >>> + return; >>> + >>> + if (!btrfs_use_zone_append(btrfs_bio(bio_ctrl->bio))) >>> + return; >>> + >>> + ordered = btrfs_lookup_ordered_extent(inode, file_offset); >>> + if (!ordered) >>> + return; >>> + >> >>> + bio_ctrl->len_to_oe_boundary = min_t(u32, U32_MAX, >>> ordered->file_offset + >>> ordered->disk_num_bytes - file_offset); >> >> >> Here. >> > > If you have a look at the original code, the setting was at the end > of the function [1]. > > So yes it will get overwritten in case we have a Zone Append bio but > I think the impact of that is neglectable compared to the better readability > of the re-flowed version. > > [1] https://github.com/kdave/btrfs-devel/blob/misc-next/fs/btrfs/extent_io.c#L971 > Yeah. btrfs_bio_add_page() :: ASSERT(bio_ctrl->len_to_oe_boundary); Changes looks good.
Reviewed-by: Anand Jain <anand.jain@oracle.com>
On 21.02.23 15:26, Christoph Hellwig wrote: > On Tue, Feb 21, 2023 at 05:25:36AM -0800, Johannes Thumshirn wrote: >> calc_bio_boundaries() is only used for guaranteeing the one bio equals to one >> ordered extent rule for uncompressed Zone Append bios. >> >> Re-flow the function to exit early in case we're not operating on an >> uncompressed Zone Append and then calculate the boundary. >> >> This imposes no functional changes but improves readability. > > This looks correct but doesn't really read much better to me. > I knew this was controversial. IMHO it reads a lot better as we're not cramping 95% of the function's body on the right side of the editor window. > My mid-term plan here is to instead keep a refrence to the ordered > extent in the bio_ctrl and get rid of the len_to_oe_boundary value, > in which case this is just a bit more churn. But I'm not sure I'm > going to get to that yet for the 6.4 merge window. > Another approach would be sinking calc_bio_boundaries into alloc_new_bio() altogether: diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c index c25fa74d7615..ec027097aa05 100644 --- a/fs/btrfs/extent_io.c +++ b/fs/btrfs/extent_io.c @@ -946,31 +946,6 @@ static int btrfs_bio_add_page(struct btrfs_bio_ctrl *bio_ctrl, return bio_add_page(bio, page, real_size, pg_offset); } -static void calc_bio_boundaries(struct btrfs_bio_ctrl *bio_ctrl, - struct btrfs_inode *inode, u64 file_offset) -{ - struct btrfs_ordered_extent *ordered; - - /* - * Limit the extent to the ordered boundary for Zone Append. - * Compressed bios aren't submitted directly, so it doesn't apply to - * them. - */ - if (bio_ctrl->compress_type == BTRFS_COMPRESS_NONE && - btrfs_use_zone_append(btrfs_bio(bio_ctrl->bio))) { - ordered = btrfs_lookup_ordered_extent(inode, file_offset); - if (ordered) { - bio_ctrl->len_to_oe_boundary = min_t(u32, U32_MAX, - ordered->file_offset + - ordered->disk_num_bytes - file_offset); - btrfs_put_ordered_extent(ordered); - return; - } - } - - bio_ctrl->len_to_oe_boundary = U32_MAX; -} - static void alloc_new_bio(struct btrfs_inode *inode, struct btrfs_bio_ctrl *bio_ctrl, struct writeback_control *wbc, blk_opf_t opf, @@ -993,7 +968,24 @@ static void alloc_new_bio(struct btrfs_inode *inode, btrfs_bio(bio)->file_offset = file_offset; bio_ctrl->bio = bio; bio_ctrl->compress_type = compress_type; - calc_bio_boundaries(bio_ctrl, inode, file_offset); + bio_ctrl->len_to_oe_boundary = U32_MAX; + + /* + * Limit the extent to the ordered boundary for Zone Append. + * Compressed bios aren't submitted directly, so it doesn't apply to + * them. + */ + if (bio_ctrl->compress_type == BTRFS_COMPRESS_NONE && + btrfs_use_zone_append(bio)) { + struct btrfs_ordered_extent *ordered = + btrfs_lookup_ordered_extent(inode, file_offset); + if (ordered) { + bio_ctrl->len_to_oe_boundary = min_t(u32, U32_MAX, + ordered->file_offset + + ordered->disk_num_bytes - file_offset); + btrfs_put_ordered_extent(ordered); + } + } if (wbc) { /*
On Tue, Feb 21, 2023 at 04:00:41PM +0000, Johannes Thumshirn wrote: > Another approach would be sinking calc_bio_boundaries into > alloc_new_bio() altogether: I like that.
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c index c25fa74d7615..c0442f2ed150 100644 --- a/fs/btrfs/extent_io.c +++ b/fs/btrfs/extent_io.c @@ -956,19 +956,22 @@ static void calc_bio_boundaries(struct btrfs_bio_ctrl *bio_ctrl, * Compressed bios aren't submitted directly, so it doesn't apply to * them. */ - if (bio_ctrl->compress_type == BTRFS_COMPRESS_NONE && - btrfs_use_zone_append(btrfs_bio(bio_ctrl->bio))) { - ordered = btrfs_lookup_ordered_extent(inode, file_offset); - if (ordered) { - bio_ctrl->len_to_oe_boundary = min_t(u32, U32_MAX, + bio_ctrl->len_to_oe_boundary = U32_MAX; + + if (bio_ctrl->compress_type != BTRFS_COMPRESS_NONE) + return; + + if (!btrfs_use_zone_append(btrfs_bio(bio_ctrl->bio))) + return; + + ordered = btrfs_lookup_ordered_extent(inode, file_offset); + if (!ordered) + return; + + bio_ctrl->len_to_oe_boundary = min_t(u32, U32_MAX, ordered->file_offset + ordered->disk_num_bytes - file_offset); - btrfs_put_ordered_extent(ordered); - return; - } - } - - bio_ctrl->len_to_oe_boundary = U32_MAX; + btrfs_put_ordered_extent(ordered); } static void alloc_new_bio(struct btrfs_inode *inode,
calc_bio_boundaries() is only used for guaranteeing the one bio equals to one ordered extent rule for uncompressed Zone Append bios. Re-flow the function to exit early in case we're not operating on an uncompressed Zone Append and then calculate the boundary. This imposes no functional changes but improves readability. Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> --- fs/btrfs/extent_io.c | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-)