cleanup btrfs_add_compressed_bio_pages

cleanup btrfs_add_compressed_bio_pages

[Christoph Hellwig]

Hi all,

when I factored out btrfs_add_compressed_bio_pages from the two duplicate
copies a while ago I left the code very much as it was then except for
splitting out the helper.  But this helper is very convoluted for what
it does, so this tiny series cleans it up a bit.

Diffstat:
 compression.c |   45 ++++++++++++---------------------------------
 1 file changed, 12 insertions(+), 33 deletions(-)

[PATCH 1/2] btrfs: move the bi_sector assingment out of btrfs_add_compressed_bio_pages

[Christoph Hellwig]

Adding pages to a bio has nothing to do with the sector.  Move the
assignment to the two callers in preparation for cleaning up
btrfs_add_compressed_bio_pages.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/btrfs/compression.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
index c5839d04690d67..1487c9413e6942 100644
--- a/fs/btrfs/compression.c
+++ b/fs/btrfs/compression.c
@@ -256,14 +256,13 @@ static void end_compressed_bio_write(struct btrfs_bio *bbio)
 	queue_work(fs_info->compressed_write_workers, &cb->write_end_work);
 }
 
-static void btrfs_add_compressed_bio_pages(struct compressed_bio *cb,
-					   u64 disk_bytenr)
+static void btrfs_add_compressed_bio_pages(struct compressed_bio *cb)
 {
 	struct btrfs_fs_info *fs_info = cb->bbio.inode->root->fs_info;
 	struct bio *bio = &cb->bbio.bio;
+	u64 disk_bytenr = bio->bi_iter.bi_sector << SECTOR_SHIFT;
 	u64 cur_disk_byte = disk_bytenr;
 
-	bio->bi_iter.bi_sector = disk_bytenr >> SECTOR_SHIFT;
 	while (cur_disk_byte < disk_bytenr + cb->compressed_len) {
 		u64 offset = cur_disk_byte - disk_bytenr;
 		unsigned int index = offset >> PAGE_SHIFT;
@@ -331,8 +330,9 @@ void btrfs_submit_compressed_write(struct btrfs_inode *inode, u64 start,
 	cb->writeback = writeback;
 	INIT_WORK(&cb->write_end_work, btrfs_finish_compressed_write_work);
 	cb->nr_pages = nr_pages;
+	cb->bbio.bio.bi_iter.bi_sector = disk_start >> SECTOR_SHIFT;
+	btrfs_add_compressed_bio_pages(cb);
 
-	btrfs_add_compressed_bio_pages(cb, disk_start);
 	btrfs_submit_bio(&cb->bbio, 0);
 
 	if (blkcg_css)
@@ -506,7 +506,6 @@ void btrfs_submit_compressed_read(struct btrfs_bio *bbio, int mirror_num)
 	struct extent_map_tree *em_tree = &inode->extent_tree;
 	struct compressed_bio *cb;
 	unsigned int compressed_len;
-	const u64 disk_bytenr = bbio->bio.bi_iter.bi_sector << SECTOR_SHIFT;
 	u64 file_offset = bbio->file_offset;
 	u64 em_len;
 	u64 em_start;
@@ -560,8 +559,8 @@ void btrfs_submit_compressed_read(struct btrfs_bio *bbio, int mirror_num)
 
 	/* include any pages we added in add_ra-bio_pages */
 	cb->len = bbio->bio.bi_iter.bi_size;
-
-	btrfs_add_compressed_bio_pages(cb, disk_bytenr);
+	cb->bbio.bio.bi_iter.bi_sector = bbio->bio.bi_iter.bi_sector;
+	btrfs_add_compressed_bio_pages(cb);
 
 	if (memstall)
 		psi_memstall_leave(&pflags);
-- 
2.39.2

[PATCH 2/2] btrfs: btrfs_add_compressed_bio_pages

[Christoph Hellwig]

btrfs_add_compressed_bio_pages is neededlyless complicated.  Instead
of iterating over the logic disk offset just to add pages to the bio
use a simple offset starting at 0, which also removes most of the
claiming.  Additionally __bio_add_pages already takes care of the
assert that the bio is always properly sized, and btrfs_submit_bio
called right after asserts that the bio size is non-zero.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/btrfs/compression.c | 34 +++++++---------------------------
 1 file changed, 7 insertions(+), 27 deletions(-)

diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
index 1487c9413e6942..44c4276741ceda 100644
--- a/fs/btrfs/compression.c
+++ b/fs/btrfs/compression.c
@@ -258,37 +258,17 @@ static void end_compressed_bio_write(struct btrfs_bio *bbio)
 
 static void btrfs_add_compressed_bio_pages(struct compressed_bio *cb)
 {
-	struct btrfs_fs_info *fs_info = cb->bbio.inode->root->fs_info;
 	struct bio *bio = &cb->bbio.bio;
-	u64 disk_bytenr = bio->bi_iter.bi_sector << SECTOR_SHIFT;
-	u64 cur_disk_byte = disk_bytenr;
+	u32 offset = 0;
 
-	while (cur_disk_byte < disk_bytenr + cb->compressed_len) {
-		u64 offset = cur_disk_byte - disk_bytenr;
-		unsigned int index = offset >> PAGE_SHIFT;
-		unsigned int real_size;
-		unsigned int added;
-		struct page *page = cb->compressed_pages[index];
+	while (offset < cb->compressed_len) {
+		u32 len = min_t(u32, cb->compressed_len - offset, PAGE_SIZE);
 
-		/*
-		 * We have various limit on the real read size:
-		 * - page boundary
-		 * - compressed length boundary
-		 */
-		real_size = min_t(u64, U32_MAX, PAGE_SIZE - offset_in_page(offset));
-		real_size = min_t(u64, real_size, cb->compressed_len - offset);
-		ASSERT(IS_ALIGNED(real_size, fs_info->sectorsize));
-
-		added = bio_add_page(bio, page, real_size, offset_in_page(offset));
-		/*
-		 * Maximum compressed extent is smaller than bio size limit,
-		 * thus bio_add_page() should always success.
-		 */
-		ASSERT(added == real_size);
-		cur_disk_byte += added;
+		/* Maximum compressed extent is smaller than bio size limit. */
+		__bio_add_page(bio, cb->compressed_pages[offset >> PAGE_SHIFT],
+			       len, 0);
+		offset += len;
 	}
-
-	ASSERT(bio->bi_iter.bi_size);
 }
 
 /*
-- 
2.39.2

Re: cleanup btrfs_add_compressed_bio_pages

[David Sterba]

On Tue, Mar 14, 2023 at 05:51:08PM +0100, Christoph Hellwig wrote:
> Hi all,
> 
> when I factored out btrfs_add_compressed_bio_pages from the two duplicate
> copies a while ago I left the code very much as it was then except for
> splitting out the helper.  But this helper is very convoluted for what
> it does, so this tiny series cleans it up a bit.
> 
> Diffstat:
>  compression.c |   45 ++++++++++++---------------------------------
>  1 file changed, 12 insertions(+), 33 deletions(-)

Added to misc-next, thanks.

Re: [PATCH 2/2] btrfs: btrfs_add_compressed_bio_pages

[Anand Jain]

On 3/15/23 00:51, Christoph Hellwig wrote:
> btrfs_add_compressed_bio_pages is neededlyless complicated.  Instead
> of iterating over the logic disk offset just to add pages to the bio
> use a simple offset starting at 0, which also removes most of the

> claiming.  Additionally __bio_add_pages already takes care of the
                                         ^__bio_add_page
> assert that the bio is always properly sized, and btrfs_submit_bio

WARN_ON_ONCE() will be triggered if the bio is a CLONED bio or is full
when passed to __bio_add_page().

And, in our case, we do not require the functionality of 
__bio_try_merge_page()?


> called right after asserts that the bio size is non-zero.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Other than the nitpick as above. Looks good.

Thanks, Anand

> ---
>   fs/btrfs/compression.c | 34 +++++++---------------------------
>   1 file changed, 7 insertions(+), 27 deletions(-)
> 
> diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
> index 1487c9413e6942..44c4276741ceda 100644
> --- a/fs/btrfs/compression.c
> +++ b/fs/btrfs/compression.c
> @@ -258,37 +258,17 @@ static void end_compressed_bio_write(struct btrfs_bio *bbio)
>   
>   static void btrfs_add_compressed_bio_pages(struct compressed_bio *cb)
>   {
> -	struct btrfs_fs_info *fs_info = cb->bbio.inode->root->fs_info;
>   	struct bio *bio = &cb->bbio.bio;
> -	u64 disk_bytenr = bio->bi_iter.bi_sector << SECTOR_SHIFT;
> -	u64 cur_disk_byte = disk_bytenr;
> +	u32 offset = 0;
>   
> -	while (cur_disk_byte < disk_bytenr + cb->compressed_len) {
> -		u64 offset = cur_disk_byte - disk_bytenr;
> -		unsigned int index = offset >> PAGE_SHIFT;
> -		unsigned int real_size;
> -		unsigned int added;
> -		struct page *page = cb->compressed_pages[index];
> +	while (offset < cb->compressed_len) {
> +		u32 len = min_t(u32, cb->compressed_len - offset, PAGE_SIZE);
>   
> -		/*
> -		 * We have various limit on the real read size:
> -		 * - page boundary
> -		 * - compressed length boundary
> -		 */
> -		real_size = min_t(u64, U32_MAX, PAGE_SIZE - offset_in_page(offset));
> -		real_size = min_t(u64, real_size, cb->compressed_len - offset);
> -		ASSERT(IS_ALIGNED(real_size, fs_info->sectorsize));
> -
> -		added = bio_add_page(bio, page, real_size, offset_in_page(offset));
> -		/*
> -		 * Maximum compressed extent is smaller than bio size limit,
> -		 * thus bio_add_page() should always success.
> -		 */
> -		ASSERT(added == real_size);
> -		cur_disk_byte += added;
> +		/* Maximum compressed extent is smaller than bio size limit. */
> +		__bio_add_page(bio, cb->compressed_pages[offset >> PAGE_SHIFT],
> +			       len, 0);
> +		offset += len;
>   	}
> -
> -	ASSERT(bio->bi_iter.bi_size);
>   }
>   
>   /*

Re: [PATCH 1/2] btrfs: move the bi_sector assingment out of btrfs_add_compressed_bio_pages

[Anand Jain]

On 3/15/23 00:51, Christoph Hellwig wrote:
> Adding pages to a bio has nothing to do with the sector.  Move the
> assignment to the two callers in preparation for cleaning up
> btrfs_add_compressed_bio_pages.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks good.
Reviewed-by: Anand Jain <anand.jain@oracle.com>

Re: [PATCH 2/2] btrfs: btrfs_add_compressed_bio_pages

[Christoph Hellwig]

On Thu, Mar 16, 2023 at 04:57:39PM +0800, Anand Jain wrote:
> And, in our case, we do not require the functionality of 
> __bio_try_merge_page()?

Yes.  These are separately allocated pages, and we have enough
space for them.  In the unlikely case that they are contiguous,
they will still be coalesced when merging into the scatterlist.