Re: [Patch v2 05/42] btrfs: refactor submit_extent_page() to make bio and its flag tracing easier
From: David Sterba <hidden>
Date: 2021-05-13 23:05:35
On Wed, Apr 28, 2021 at 07:03:12AM +0800, Qu Wenruo wrote:
quoted hunk ↗ jump to hunk
There are a lot of code inside extent_io.c needs both "struct bio **bio_ret" and "unsigned long prev_bio_flags", along with some parameter like "unsigned long bio_flags". Such strange parameters are here for bio assembly. For example, we have such inode page layout: 0 4K 8K 12K |<-- Extent A-->|<- EB->| Then what we do is: - Page [0, 4K) *bio_ret = NULL So we allocate a new bio to bio_ret, Add page [0, 4K) to *bio_ret. - Page [4K, 8K) *bio_ret != NULL We found this page is continuous to *bio_ret, and if we're not at stripe boundary, we add page [4K, 8K) to *bio_ret. - Page [8K, 12K) *bio_ret != NULL But we found this page is not continuous, so we submit *bio_ret, then allocate a new bio, and add page [8K, 12K) to the new bio. This means we need to record both the bio and its bio_flag, but we record them manually using those strange parameter list, other than encapsulating them into their own structure. So this patch will introduce a new structure, btrfs_bio_ctrl, to record both the bio, and its bio_flags. Also, in above case, for all pages added to the bio, we need to check if the new page crosses stripe boundary. This check itself can be time consuming, and we don't really need to do that for each page. This patch also integrate the stripe boundary check into btrfs_bio_ctrl. When a new bio is allocated, the stripe and ordered extent boundary is also calculated, so no matter how large the bio will be, we only calculate the boundaries once, to save some CPU time. The following functions/structures are affected: - struct extent_page_data Replace its bio pointer with structure btrfs_bio_ctrl (embedded structure, not pointer) - end_write_bio() - flush_write_bio() Just change how bio is fetched - btrfs_bio_add_page() Use pre-calculated boundaries instead of re-calculating them. And use @bio_ctrl to replace @bio and @prev_bio_flags. - calc_bio_boundaries() New function - submit_extent_page() callers - btrfs_do_readpage() callers - contiguous_readpages() callers To Use @bio_ctrl to replace @bio and @prev_bio_flags, and how to grab bio. - btrfs_bio_fits_in_ordered_extent() Removed, as now the ordered extent size limit is done at bio allocation time, no need to check for each page range. Signed-off-by: Qu Wenruo <redacted> --- fs/btrfs/ctree.h | 2 - fs/btrfs/extent_io.c | 214 ++++++++++++++++++++++++++++--------------- fs/btrfs/extent_io.h | 13 ++- fs/btrfs/inode.c | 36 +------- 4 files changed, 154 insertions(+), 111 deletions(-)diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h index 278e0cbc9a98..b94790583008 100644 --- a/fs/btrfs/ctree.h +++ b/fs/btrfs/ctree.h@@ -3146,8 +3146,6 @@ void btrfs_split_delalloc_extent(struct inode *inode, struct extent_state *orig, u64 split); int btrfs_bio_fits_in_stripe(struct page *page, size_t size, struct bio *bio, unsigned long bio_flags); -bool btrfs_bio_fits_in_ordered_extent(struct page *page, struct bio *bio, - unsigned int size); void btrfs_set_range_writeback(struct extent_io_tree *tree, u64 start, u64 end); vm_fault_t btrfs_page_mkwrite(struct vm_fault *vmf); int btrfs_readpage(struct file *file, struct page *page);diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c index 8e18dc9a415d..949b603e7aa3 100644 --- a/fs/btrfs/extent_io.c +++ b/fs/btrfs/extent_io.c@@ -136,7 +136,7 @@ struct tree_entry { }; struct extent_page_data { - struct bio *bio; + struct btrfs_bio_ctrl bio_ctrl; /* tells writepage not to lock the state bits for this range * it still does the unlocking */@@ -185,10 +185,12 @@ int __must_check submit_one_bio(struct bio *bio, int mirror_num, /* Cleanup unsubmitted bios */ static void end_write_bio(struct extent_page_data *epd, int ret) { - if (epd->bio) { - epd->bio->bi_status = errno_to_blk_status(ret); - bio_endio(epd->bio); - epd->bio = NULL; + struct bio *bio = epd->bio_ctrl.bio;
We've been using _ctl suffix for the control structures that affect behaviour of functions that take it as a parameter. I haven't modified the patch to avoid conflicts with other patches but as a further cleanup it would be good to get that unified to _ctl.
static int submit_extent_page(unsigned int opf, struct writeback_control *wbc, + struct btrfs_bio_ctrl *bio_ctrl, struct page *page, u64 disk_bytenr, size_t size, unsigned long pg_offset, - struct bio **bio_ret, bio_end_io_t end_io_func, int mirror_num, - unsigned long prev_bio_flags, unsigned long bio_flags, bool force_bio_submit)
This function has been a challenge for anybody who likes to do cleanups. I have a few branches that try to do so but no luck yet, so I appreciate any reduction of the parameters with potential to move more of them to the control structure.