[PATCH 0/9] btrfs: compression: refactor and enhancement preparing for subpage compression support

[PATCH 0/9] btrfs: compression: refactor and enhancement preparing for subpage compression support

[Qu Wenruo]

There are quite some problems in compression code:

- Weird compressed_bio::pending_bios dance
  If we just don't want compressed_bio being freed halfway, we have more
  sane methods, just like btrfs_subpage::readers.

  So here we fix it by introducing compressed_bio::io_sectors to do the
  job.

- BUG_ON()s inside btrfs_submit_compressed_*()
  Even they are just ENOMEM, we should handle them.
  With io_sectors introduced, we have a way to finish compressed_bio
  all by ourselves, as long as we haven't submitted last bio.

  If we have last bio submitted, then endio will handle it well.

- Duplicated code for compressed bio allocation and submission
  Just small refactor can handle it

- Stripe boundary is checked every time one page is added
  This is overkilled.
  Just learn from extent_io.c refactor which use bio_ctrl to do the
  boundary check only once for each bio.

  Although in compression context, we don't need extra checks in
  extent_io.c, thus we don't need bio_ctrl structure, but
  can afford to do it locally.

- Dead code removal
  One dead comment and a new zombie function,
  btrfs_bio_fits_in_stripe(), can be removed now.

Qu Wenruo (9):
  btrfs: remove a dead comment for btrfs_decompress_bio()
  btrfs: add compressed_bio::io_sectors to trace compressed bio more
    elegantly
  btrfs: hunt down the BUG_ON()s inside btrfs_submit_compressed_read()
  btrfs: hunt down the BUG_ON()s inside btrfs_submit_compressed_write()
  btrfs: introduce submit_compressed_bio() for compression
  btrfs: introduce alloc_submit_compressed_bio() for compression
  btrfs: make btrfs_submit_compressed_read() to determine stripe
    boundary at bio allocation time
  btrfs: make btrfs_submit_compressed_read() to determine stripe
    boundary at bio allocation time
  btrfs: remove unused function btrfs_bio_fits_in_stripe()

 fs/btrfs/compression.c | 591 +++++++++++++++++++++++++----------------
 fs/btrfs/compression.h |  13 +-
 fs/btrfs/ctree.h       |   2 -
 fs/btrfs/inode.c       |  42 ---
 4 files changed, 369 insertions(+), 279 deletions(-)

-- 
2.32.0

[PATCH 1/9] btrfs: remove a dead comment for btrfs_decompress_bio()

[Qu Wenruo]

Since commit 8140dc30a432 ("btrfs: btrfs_decompress_bio() could accept
compressed_bio instead"), btrfs_decompress_bio() accepts
"struct compressed_bio" other than open-coded parameter list.

Thus the comments for the parameter list is no longer needed.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/compression.c | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
index 831e6ae92940..fc4f37adb7b7 100644
--- a/fs/btrfs/compression.c
+++ b/fs/btrfs/compression.c
@@ -1208,20 +1208,6 @@ int btrfs_compress_pages(unsigned int type_level, struct address_space *mapping,
 	return ret;
 }
 
-/*
- * pages_in is an array of pages with compressed data.
- *
- * disk_start is the starting logical offset of this array in the file
- *
- * orig_bio contains the pages from the file that we want to decompress into
- *
- * srclen is the number of bytes in pages_in
- *
- * The basic idea is that we have a bio that was created by readpages.
- * The pages in the bio are for the uncompressed data, and they may not
- * be contiguous.  They all correspond to the range of bytes covered by
- * the compressed extent.
- */
 static int btrfs_decompress_bio(struct compressed_bio *cb)
 {
 	struct list_head *workspace;
-- 
2.32.0

Re: [PATCH 1/9] btrfs: remove a dead comment for btrfs_decompress_bio()

[Anand Jain]

On 11/6/21 9:31 am, Qu Wenruo wrote:
> Since commit 8140dc30a432 ("btrfs: btrfs_decompress_bio() could accept
> compressed_bio instead"), btrfs_decompress_bio() accepts
> "struct compressed_bio" other than open-coded parameter list.
> 
> Thus the comments for the parameter list is no longer needed.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

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


> ---
>   fs/btrfs/compression.c | 14 --------------
>   1 file changed, 14 deletions(-)
> 
> diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
> index 831e6ae92940..fc4f37adb7b7 100644
> --- a/fs/btrfs/compression.c
> +++ b/fs/btrfs/compression.c
> @@ -1208,20 +1208,6 @@ int btrfs_compress_pages(unsigned int type_level, struct address_space *mapping,
>   	return ret;
>   }
>   
> -/*
> - * pages_in is an array of pages with compressed data.
> - *
> - * disk_start is the starting logical offset of this array in the file
> - *
> - * orig_bio contains the pages from the file that we want to decompress into
> - *
> - * srclen is the number of bytes in pages_in
> - *
> - * The basic idea is that we have a bio that was created by readpages.
> - * The pages in the bio are for the uncompressed data, and they may not
> - * be contiguous.  They all correspond to the range of bytes covered by
> - * the compressed extent.
> - */
>   static int btrfs_decompress_bio(struct compressed_bio *cb)
>   {
>   	struct list_head *workspace;
> 

[PATCH 2/9] btrfs: add compressed_bio::io_sectors to trace compressed bio more elegantly

[Qu Wenruo]

For btrfs_submit_compressed_read() and btrfs_submit_compressed_write(),
we have a pretty weird dance around compressed_bio::io_sectors:

  btrfs_submit_compressed_read/write()
  {
	cb = kmalloc()
	refcount_set(&cb->pending_bios, 0);
	bio = btrfs_alloc_bio();

	/* NOTE here, we haven't yet submitted any bio */
	refcount_set(&cb->pending_bios, 1);

	for (pg_index = 0; pg_index < cb->nr_pages; pg_index++) {
		if (submit) {
			/* Here we submit bio, but we always have one
			 * extra pending_bios */
			refcount_inc(&cb->pending_bios);
			ret = btrfs_map_bio();
		}
	}

	/* Submit the last bio */
	ret = btrfs_map_bio();
  }

There are two reasons why we do this:

- compressed_bio::pending_bios is a refcount
  Thus if it's reduced to 0, it can not be increased again.

- To ensure the compressed_bio is not freed by some submitted bios
  If the submitted bio is finished before the next bio submitted,
  we can free the compressed_bio completely.

But the above code is sometimes confusing, and we can do it better by
just introduce a new member, compressed_bio::io_sectors.

With that member, we can easily distinguish if we're really the last
bio at endio time, and even allows us to remove some BUG_ON() later,
as we now know how many bytes are not yet submitted.

With this new member, now compressed_bio::pending_bios really indicates
the pending bios, without any special handling needed.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/compression.c | 71 ++++++++++++++++++++++--------------------
 fs/btrfs/compression.h | 10 +++++-
 2 files changed, 47 insertions(+), 34 deletions(-)

diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
index fc4f37adb7b7..c9dbe306f6ba 100644
--- a/fs/btrfs/compression.c
+++ b/fs/btrfs/compression.c
@@ -193,6 +193,34 @@ static int check_compressed_csum(struct btrfs_inode *inode, struct bio *bio,
 	return 0;
 }
 
+/*
+ * Reduce bio and io accounting for a compressed_bio with its coresponding bio.
+ *
+ * Return true if there is no pending bio nor io.
+ * Return false otherwise.
+ */
+static bool dec_and_test_compressed_bio(struct compressed_bio *cb,
+					struct bio *bio)
+{
+	struct btrfs_fs_info *fs_info = btrfs_sb(cb->inode->i_sb);
+	unsigned int bi_size = bio->bi_iter.bi_size;
+	bool last_bio = false;
+	bool last_io = false;
+
+	if (bio->bi_status)
+		cb->errors = 1;
+
+	last_bio = atomic_dec_and_test(&cb->pending_bios);
+	last_io = atomic_sub_and_test(bi_size >> fs_info->sectorsize_bits,
+				       &cb->io_sectors);
+
+	/*
+	 * We can only finish the compressed bio if no pending bio and all io
+	 * submitted.
+	 */
+	return last_bio && last_io;
+}
+
 /* when we finish reading compressed pages from the disk, we
  * decompress them and then run the bio end_io routines on the
  * decompressed pages (in the inode address space).
@@ -212,13 +240,7 @@ static void end_compressed_bio_read(struct bio *bio)
 	unsigned int mirror = btrfs_io_bio(bio)->mirror_num;
 	int ret = 0;
 
-	if (bio->bi_status)
-		cb->errors = 1;
-
-	/* if there are more bios still pending for this compressed
-	 * extent, just exit
-	 */
-	if (!refcount_dec_and_test(&cb->pending_bios))
+	if (!dec_and_test_compressed_bio(cb, bio))
 		goto out;
 
 	/*
@@ -336,13 +358,7 @@ static void end_compressed_bio_write(struct bio *bio)
 	struct page *page;
 	unsigned long index;
 
-	if (bio->bi_status)
-		cb->errors = 1;
-
-	/* if there are more bios still pending for this compressed
-	 * extent, just exit
-	 */
-	if (!refcount_dec_and_test(&cb->pending_bios))
+	if (!dec_and_test_compressed_bio(cb, bio))
 		goto out;
 
 	/* ok, we're the last bio for this extent, step one is to
@@ -408,7 +424,8 @@ blk_status_t btrfs_submit_compressed_write(struct btrfs_inode *inode, u64 start,
 	cb = kmalloc(compressed_bio_size(fs_info, compressed_len), GFP_NOFS);
 	if (!cb)
 		return BLK_STS_RESOURCE;
-	refcount_set(&cb->pending_bios, 0);
+	atomic_set(&cb->pending_bios, 0);
+	atomic_set(&cb->io_sectors, compressed_len >> fs_info->sectorsize_bits);
 	cb->errors = 0;
 	cb->inode = &inode->vfs_inode;
 	cb->start = start;
@@ -441,7 +458,6 @@ blk_status_t btrfs_submit_compressed_write(struct btrfs_inode *inode, u64 start,
 		bio->bi_opf |= REQ_CGROUP_PUNT;
 		kthread_associate_blkcg(blkcg_css);
 	}
-	refcount_set(&cb->pending_bios, 1);
 
 	/* create and submit bios for the compressed pages */
 	bytes_left = compressed_len;
@@ -462,13 +478,7 @@ blk_status_t btrfs_submit_compressed_write(struct btrfs_inode *inode, u64 start,
 
 		page->mapping = NULL;
 		if (submit || len < PAGE_SIZE) {
-			/*
-			 * inc the count before we submit the bio so
-			 * we know the end IO handler won't happen before
-			 * we inc the count.  Otherwise, the cb might get
-			 * freed before we're done setting it up
-			 */
-			refcount_inc(&cb->pending_bios);
+			atomic_inc(&cb->pending_bios);
 			ret = btrfs_bio_wq_end_io(fs_info, bio,
 						  BTRFS_WQ_ENDIO_DATA);
 			BUG_ON(ret); /* -ENOMEM */
@@ -506,6 +516,7 @@ blk_status_t btrfs_submit_compressed_write(struct btrfs_inode *inode, u64 start,
 		cond_resched();
 	}
 
+	atomic_inc(&cb->pending_bios);
 	ret = btrfs_bio_wq_end_io(fs_info, bio, BTRFS_WQ_ENDIO_DATA);
 	BUG_ON(ret); /* -ENOMEM */
 
@@ -689,7 +700,8 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
 	if (!cb)
 		goto out;
 
-	refcount_set(&cb->pending_bios, 0);
+	atomic_set(&cb->pending_bios, 0);
+	atomic_set(&cb->io_sectors, compressed_len >> fs_info->sectorsize_bits);
 	cb->errors = 0;
 	cb->inode = inode;
 	cb->mirror_num = mirror_num;
@@ -734,7 +746,6 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
 	comp_bio->bi_opf = REQ_OP_READ;
 	comp_bio->bi_private = cb;
 	comp_bio->bi_end_io = end_compressed_bio_read;
-	refcount_set(&cb->pending_bios, 1);
 
 	for (pg_index = 0; pg_index < nr_pages; pg_index++) {
 		u32 pg_len = PAGE_SIZE;
@@ -763,18 +774,11 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
 		if (submit || bio_add_page(comp_bio, page, pg_len, 0) < pg_len) {
 			unsigned int nr_sectors;
 
+			atomic_inc(&cb->pending_bios);
 			ret = btrfs_bio_wq_end_io(fs_info, comp_bio,
 						  BTRFS_WQ_ENDIO_DATA);
 			BUG_ON(ret); /* -ENOMEM */
 
-			/*
-			 * inc the count before we submit the bio so
-			 * we know the end IO handler won't happen before
-			 * we inc the count.  Otherwise, the cb might get
-			 * freed before we're done setting it up
-			 */
-			refcount_inc(&cb->pending_bios);
-
 			ret = btrfs_lookup_bio_sums(inode, comp_bio, sums);
 			BUG_ON(ret); /* -ENOMEM */
 
@@ -798,6 +802,7 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
 		cur_disk_byte += pg_len;
 	}
 
+	atomic_inc(&cb->pending_bios);
 	ret = btrfs_bio_wq_end_io(fs_info, comp_bio, BTRFS_WQ_ENDIO_DATA);
 	BUG_ON(ret); /* -ENOMEM */
 
diff --git a/fs/btrfs/compression.h b/fs/btrfs/compression.h
index 8001b700ea3a..3df3262fedcd 100644
--- a/fs/btrfs/compression.h
+++ b/fs/btrfs/compression.h
@@ -29,7 +29,15 @@ struct btrfs_inode;
 
 struct compressed_bio {
 	/* number of bios pending for this compressed extent */
-	refcount_t pending_bios;
+	atomic_t pending_bios;
+
+	/*
+	 * Number of sectors which hasn't finished.
+	 *
+	 * Combined with pending_bios, we can manually finish the compressed_bio
+	 * if we hit some error while there is still some pages not added.
+	 */
+	atomic_t io_sectors;
 
 	/* the pages with the compressed data on them */
 	struct page **compressed_pages;
-- 
2.32.0

Re: [PATCH 2/9] btrfs: add compressed_bio::io_sectors to trace compressed bio more elegantly

[Anand Jain]

On 11/6/21 9:31 am, Qu Wenruo wrote:
> For btrfs_submit_compressed_read() and btrfs_submit_compressed_write(),
> we have a pretty weird dance around compressed_bio::io_sectors:

I don't see io_sectors member for the struct compressed_bio in the 
current misc-next. It is not mentioned in the cover letter, but is this 
series a part of some other patch set yet to integrate into misc-next? 
OR can this series be re-based on current misc-next?

Thanks, Anand


> 
>    btrfs_submit_compressed_read/write()
>    {
> 	cb = kmalloc()
> 	refcount_set(&cb->pending_bios, 0);
> 	bio = btrfs_alloc_bio();
> 
> 	/* NOTE here, we haven't yet submitted any bio */
> 	refcount_set(&cb->pending_bios, 1);
> 
> 	for (pg_index = 0; pg_index < cb->nr_pages; pg_index++) {
> 		if (submit) {
> 			/* Here we submit bio, but we always have one
> 			 * extra pending_bios */
> 			refcount_inc(&cb->pending_bios);
> 			ret = btrfs_map_bio();
> 		}
> 	}
> 
> 	/* Submit the last bio */
> 	ret = btrfs_map_bio();
>    }
> 
> There are two reasons why we do this:
> 
> - compressed_bio::pending_bios is a refcount
>    Thus if it's reduced to 0, it can not be increased again.
> 
> - To ensure the compressed_bio is not freed by some submitted bios
>    If the submitted bio is finished before the next bio submitted,
>    we can free the compressed_bio completely.
> 
> But the above code is sometimes confusing, and we can do it better by
> just introduce a new member, compressed_bio::io_sectors.
> 
> With that member, we can easily distinguish if we're really the last
> bio at endio time, and even allows us to remove some BUG_ON() later,
> as we now know how many bytes are not yet submitted.
> 
> With this new member, now compressed_bio::pending_bios really indicates
> the pending bios, without any special handling needed.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>   fs/btrfs/compression.c | 71 ++++++++++++++++++++++--------------------
>   fs/btrfs/compression.h | 10 +++++-
>   2 files changed, 47 insertions(+), 34 deletions(-)
> 
> diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
> index fc4f37adb7b7..c9dbe306f6ba 100644
> --- a/fs/btrfs/compression.c
> +++ b/fs/btrfs/compression.c
> @@ -193,6 +193,34 @@ static int check_compressed_csum(struct btrfs_inode *inode, struct bio *bio,
>   	return 0;
>   }
>   
> +/*
> + * Reduce bio and io accounting for a compressed_bio with its coresponding bio.
> + *
> + * Return true if there is no pending bio nor io.
> + * Return false otherwise.
> + */
> +static bool dec_and_test_compressed_bio(struct compressed_bio *cb,
> +					struct bio *bio)
> +{
> +	struct btrfs_fs_info *fs_info = btrfs_sb(cb->inode->i_sb);
> +	unsigned int bi_size = bio->bi_iter.bi_size;
> +	bool last_bio = false;
> +	bool last_io = false;
> +
> +	if (bio->bi_status)
> +		cb->errors = 1;
> +
> +	last_bio = atomic_dec_and_test(&cb->pending_bios);
> +	last_io = atomic_sub_and_test(bi_size >> fs_info->sectorsize_bits,
> +				       &cb->io_sectors);
> +
> +	/*
> +	 * We can only finish the compressed bio if no pending bio and all io
> +	 * submitted.
> +	 */
> +	return last_bio && last_io;
> +}
> +
>   /* when we finish reading compressed pages from the disk, we
>    * decompress them and then run the bio end_io routines on the
>    * decompressed pages (in the inode address space).
> @@ -212,13 +240,7 @@ static void end_compressed_bio_read(struct bio *bio)
>   	unsigned int mirror = btrfs_io_bio(bio)->mirror_num;
>   	int ret = 0;
>   
> -	if (bio->bi_status)
> -		cb->errors = 1;
> -
> -	/* if there are more bios still pending for this compressed
> -	 * extent, just exit
> -	 */
> -	if (!refcount_dec_and_test(&cb->pending_bios))
> +	if (!dec_and_test_compressed_bio(cb, bio))
>   		goto out;
>   
>   	/*
> @@ -336,13 +358,7 @@ static void end_compressed_bio_write(struct bio *bio)
>   	struct page *page;
>   	unsigned long index;
>   
> -	if (bio->bi_status)
> -		cb->errors = 1;
> -
> -	/* if there are more bios still pending for this compressed
> -	 * extent, just exit
> -	 */
> -	if (!refcount_dec_and_test(&cb->pending_bios))
> +	if (!dec_and_test_compressed_bio(cb, bio))
>   		goto out;
>   
>   	/* ok, we're the last bio for this extent, step one is to
> @@ -408,7 +424,8 @@ blk_status_t btrfs_submit_compressed_write(struct btrfs_inode *inode, u64 start,
>   	cb = kmalloc(compressed_bio_size(fs_info, compressed_len), GFP_NOFS);
>   	if (!cb)
>   		return BLK_STS_RESOURCE;
> -	refcount_set(&cb->pending_bios, 0);
> +	atomic_set(&cb->pending_bios, 0);
> +	atomic_set(&cb->io_sectors, compressed_len >> fs_info->sectorsize_bits);
>   	cb->errors = 0;
>   	cb->inode = &inode->vfs_inode;
>   	cb->start = start;
> @@ -441,7 +458,6 @@ blk_status_t btrfs_submit_compressed_write(struct btrfs_inode *inode, u64 start,
>   		bio->bi_opf |= REQ_CGROUP_PUNT;
>   		kthread_associate_blkcg(blkcg_css);
>   	}
> -	refcount_set(&cb->pending_bios, 1);
>   
>   	/* create and submit bios for the compressed pages */
>   	bytes_left = compressed_len;
> @@ -462,13 +478,7 @@ blk_status_t btrfs_submit_compressed_write(struct btrfs_inode *inode, u64 start,
>   
>   		page->mapping = NULL;
>   		if (submit || len < PAGE_SIZE) {
> -			/*
> -			 * inc the count before we submit the bio so
> -			 * we know the end IO handler won't happen before
> -			 * we inc the count.  Otherwise, the cb might get
> -			 * freed before we're done setting it up
> -			 */
> -			refcount_inc(&cb->pending_bios);
> +			atomic_inc(&cb->pending_bios);
>   			ret = btrfs_bio_wq_end_io(fs_info, bio,
>   						  BTRFS_WQ_ENDIO_DATA);
>   			BUG_ON(ret); /* -ENOMEM */
> @@ -506,6 +516,7 @@ blk_status_t btrfs_submit_compressed_write(struct btrfs_inode *inode, u64 start,
>   		cond_resched();
>   	}
>   
> +	atomic_inc(&cb->pending_bios);
>   	ret = btrfs_bio_wq_end_io(fs_info, bio, BTRFS_WQ_ENDIO_DATA);
>   	BUG_ON(ret); /* -ENOMEM */
>   
> @@ -689,7 +700,8 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
>   	if (!cb)
>   		goto out;
>   
> -	refcount_set(&cb->pending_bios, 0);
> +	atomic_set(&cb->pending_bios, 0);
> +	atomic_set(&cb->io_sectors, compressed_len >> fs_info->sectorsize_bits);
>   	cb->errors = 0;
>   	cb->inode = inode;
>   	cb->mirror_num = mirror_num;
> @@ -734,7 +746,6 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
>   	comp_bio->bi_opf = REQ_OP_READ;
>   	comp_bio->bi_private = cb;
>   	comp_bio->bi_end_io = end_compressed_bio_read;
> -	refcount_set(&cb->pending_bios, 1);
>   
>   	for (pg_index = 0; pg_index < nr_pages; pg_index++) {
>   		u32 pg_len = PAGE_SIZE;
> @@ -763,18 +774,11 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
>   		if (submit || bio_add_page(comp_bio, page, pg_len, 0) < pg_len) {
>   			unsigned int nr_sectors;
>   
> +			atomic_inc(&cb->pending_bios);
>   			ret = btrfs_bio_wq_end_io(fs_info, comp_bio,
>   						  BTRFS_WQ_ENDIO_DATA);
>   			BUG_ON(ret); /* -ENOMEM */
>   
> -			/*
> -			 * inc the count before we submit the bio so
> -			 * we know the end IO handler won't happen before
> -			 * we inc the count.  Otherwise, the cb might get
> -			 * freed before we're done setting it up
> -			 */
> -			refcount_inc(&cb->pending_bios);
> -
>   			ret = btrfs_lookup_bio_sums(inode, comp_bio, sums);
>   			BUG_ON(ret); /* -ENOMEM */
>   
> @@ -798,6 +802,7 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
>   		cur_disk_byte += pg_len;
>   	}
>   
> +	atomic_inc(&cb->pending_bios);
>   	ret = btrfs_bio_wq_end_io(fs_info, comp_bio, BTRFS_WQ_ENDIO_DATA);
>   	BUG_ON(ret); /* -ENOMEM */
>   
> diff --git a/fs/btrfs/compression.h b/fs/btrfs/compression.h
> index 8001b700ea3a..3df3262fedcd 100644
> --- a/fs/btrfs/compression.h
> +++ b/fs/btrfs/compression.h
> @@ -29,7 +29,15 @@ struct btrfs_inode;
>   
>   struct compressed_bio {
>   	/* number of bios pending for this compressed extent */
> -	refcount_t pending_bios;
> +	atomic_t pending_bios;
> +
> +	/*
> +	 * Number of sectors which hasn't finished.
> +	 *
> +	 * Combined with pending_bios, we can manually finish the compressed_bio
> +	 * if we hit some error while there is still some pages not added.
> +	 */
> +	atomic_t io_sectors;
>   
>   	/* the pages with the compressed data on them */
>   	struct page **compressed_pages;
> 

Re: [PATCH 2/9] btrfs: add compressed_bio::io_sectors to trace compressed bio more elegantly

[Qu Wenruo]

On 2021/6/11 下午12:18, Anand Jain wrote:
> On 11/6/21 9:31 am, Qu Wenruo wrote:
>> For btrfs_submit_compressed_read() and btrfs_submit_compressed_write(),
>> we have a pretty weird dance around compressed_bio::io_sectors:
> 
> I don't see io_sectors member for the struct compressed_bio in the 
> current misc-next. It is not mentioned in the cover letter, but is this 
> series a part of some other patch set yet to integrate into misc-next? 
> OR can this series be re-based on current misc-next?

My bad, the wrong variable name in the commit message.

It should be compressed_bio::pending_bios.

The io_sectors is added to address the problem.

Thanks,
Qu
> 
> Thanks, Anand
> 
> 
>>
>>    btrfs_submit_compressed_read/write()
>>    {
>>     cb = kmalloc()
>>     refcount_set(&cb->pending_bios, 0);
>>     bio = btrfs_alloc_bio();
>>
>>     /* NOTE here, we haven't yet submitted any bio */
>>     refcount_set(&cb->pending_bios, 1);
>>
>>     for (pg_index = 0; pg_index < cb->nr_pages; pg_index++) {
>>         if (submit) {
>>             /* Here we submit bio, but we always have one
>>              * extra pending_bios */
>>             refcount_inc(&cb->pending_bios);
>>             ret = btrfs_map_bio();
>>         }
>>     }
>>
>>     /* Submit the last bio */
>>     ret = btrfs_map_bio();
>>    }
>>
>> There are two reasons why we do this:
>>
>> - compressed_bio::pending_bios is a refcount
>>    Thus if it's reduced to 0, it can not be increased again.
>>
>> - To ensure the compressed_bio is not freed by some submitted bios
>>    If the submitted bio is finished before the next bio submitted,
>>    we can free the compressed_bio completely.
>>
>> But the above code is sometimes confusing, and we can do it better by
>> just introduce a new member, compressed_bio::io_sectors.
>>
>> With that member, we can easily distinguish if we're really the last
>> bio at endio time, and even allows us to remove some BUG_ON() later,
>> as we now know how many bytes are not yet submitted.
>>
>> With this new member, now compressed_bio::pending_bios really indicates
>> the pending bios, without any special handling needed.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>   fs/btrfs/compression.c | 71 ++++++++++++++++++++++--------------------
>>   fs/btrfs/compression.h | 10 +++++-
>>   2 files changed, 47 insertions(+), 34 deletions(-)
>>
>> diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
>> index fc4f37adb7b7..c9dbe306f6ba 100644
>> --- a/fs/btrfs/compression.c
>> +++ b/fs/btrfs/compression.c
>> @@ -193,6 +193,34 @@ static int check_compressed_csum(struct 
>> btrfs_inode *inode, struct bio *bio,
>>       return 0;
>>   }
>> +/*
>> + * Reduce bio and io accounting for a compressed_bio with its 
>> coresponding bio.
>> + *
>> + * Return true if there is no pending bio nor io.
>> + * Return false otherwise.
>> + */
>> +static bool dec_and_test_compressed_bio(struct compressed_bio *cb,
>> +                    struct bio *bio)
>> +{
>> +    struct btrfs_fs_info *fs_info = btrfs_sb(cb->inode->i_sb);
>> +    unsigned int bi_size = bio->bi_iter.bi_size;
>> +    bool last_bio = false;
>> +    bool last_io = false;
>> +
>> +    if (bio->bi_status)
>> +        cb->errors = 1;
>> +
>> +    last_bio = atomic_dec_and_test(&cb->pending_bios);
>> +    last_io = atomic_sub_and_test(bi_size >> fs_info->sectorsize_bits,
>> +                       &cb->io_sectors);
>> +
>> +    /*
>> +     * We can only finish the compressed bio if no pending bio and 
>> all io
>> +     * submitted.
>> +     */
>> +    return last_bio && last_io;
>> +}
>> +
>>   /* when we finish reading compressed pages from the disk, we
>>    * decompress them and then run the bio end_io routines on the
>>    * decompressed pages (in the inode address space).
>> @@ -212,13 +240,7 @@ static void end_compressed_bio_read(struct bio *bio)
>>       unsigned int mirror = btrfs_io_bio(bio)->mirror_num;
>>       int ret = 0;
>> -    if (bio->bi_status)
>> -        cb->errors = 1;
>> -
>> -    /* if there are more bios still pending for this compressed
>> -     * extent, just exit
>> -     */
>> -    if (!refcount_dec_and_test(&cb->pending_bios))
>> +    if (!dec_and_test_compressed_bio(cb, bio))
>>           goto out;
>>       /*
>> @@ -336,13 +358,7 @@ static void end_compressed_bio_write(struct bio 
>> *bio)
>>       struct page *page;
>>       unsigned long index;
>> -    if (bio->bi_status)
>> -        cb->errors = 1;
>> -
>> -    /* if there are more bios still pending for this compressed
>> -     * extent, just exit
>> -     */
>> -    if (!refcount_dec_and_test(&cb->pending_bios))
>> +    if (!dec_and_test_compressed_bio(cb, bio))
>>           goto out;
>>       /* ok, we're the last bio for this extent, step one is to
>> @@ -408,7 +424,8 @@ blk_status_t btrfs_submit_compressed_write(struct 
>> btrfs_inode *inode, u64 start,
>>       cb = kmalloc(compressed_bio_size(fs_info, compressed_len), 
>> GFP_NOFS);
>>       if (!cb)
>>           return BLK_STS_RESOURCE;
>> -    refcount_set(&cb->pending_bios, 0);
>> +    atomic_set(&cb->pending_bios, 0);
>> +    atomic_set(&cb->io_sectors, compressed_len >> 
>> fs_info->sectorsize_bits);
>>       cb->errors = 0;
>>       cb->inode = &inode->vfs_inode;
>>       cb->start = start;
>> @@ -441,7 +458,6 @@ blk_status_t btrfs_submit_compressed_write(struct 
>> btrfs_inode *inode, u64 start,
>>           bio->bi_opf |= REQ_CGROUP_PUNT;
>>           kthread_associate_blkcg(blkcg_css);
>>       }
>> -    refcount_set(&cb->pending_bios, 1);
>>       /* create and submit bios for the compressed pages */
>>       bytes_left = compressed_len;
>> @@ -462,13 +478,7 @@ blk_status_t btrfs_submit_compressed_write(struct 
>> btrfs_inode *inode, u64 start,
>>           page->mapping = NULL;
>>           if (submit || len < PAGE_SIZE) {
>> -            /*
>> -             * inc the count before we submit the bio so
>> -             * we know the end IO handler won't happen before
>> -             * we inc the count.  Otherwise, the cb might get
>> -             * freed before we're done setting it up
>> -             */
>> -            refcount_inc(&cb->pending_bios);
>> +            atomic_inc(&cb->pending_bios);
>>               ret = btrfs_bio_wq_end_io(fs_info, bio,
>>                             BTRFS_WQ_ENDIO_DATA);
>>               BUG_ON(ret); /* -ENOMEM */
>> @@ -506,6 +516,7 @@ blk_status_t btrfs_submit_compressed_write(struct 
>> btrfs_inode *inode, u64 start,
>>           cond_resched();
>>       }
>> +    atomic_inc(&cb->pending_bios);
>>       ret = btrfs_bio_wq_end_io(fs_info, bio, BTRFS_WQ_ENDIO_DATA);
>>       BUG_ON(ret); /* -ENOMEM */
>> @@ -689,7 +700,8 @@ blk_status_t btrfs_submit_compressed_read(struct 
>> inode *inode, struct bio *bio,
>>       if (!cb)
>>           goto out;
>> -    refcount_set(&cb->pending_bios, 0);
>> +    atomic_set(&cb->pending_bios, 0);
>> +    atomic_set(&cb->io_sectors, compressed_len >> 
>> fs_info->sectorsize_bits);
>>       cb->errors = 0;
>>       cb->inode = inode;
>>       cb->mirror_num = mirror_num;
>> @@ -734,7 +746,6 @@ blk_status_t btrfs_submit_compressed_read(struct 
>> inode *inode, struct bio *bio,
>>       comp_bio->bi_opf = REQ_OP_READ;
>>       comp_bio->bi_private = cb;
>>       comp_bio->bi_end_io = end_compressed_bio_read;
>> -    refcount_set(&cb->pending_bios, 1);
>>       for (pg_index = 0; pg_index < nr_pages; pg_index++) {
>>           u32 pg_len = PAGE_SIZE;
>> @@ -763,18 +774,11 @@ blk_status_t btrfs_submit_compressed_read(struct 
>> inode *inode, struct bio *bio,
>>           if (submit || bio_add_page(comp_bio, page, pg_len, 0) < 
>> pg_len) {
>>               unsigned int nr_sectors;
>> +            atomic_inc(&cb->pending_bios);
>>               ret = btrfs_bio_wq_end_io(fs_info, comp_bio,
>>                             BTRFS_WQ_ENDIO_DATA);
>>               BUG_ON(ret); /* -ENOMEM */
>> -            /*
>> -             * inc the count before we submit the bio so
>> -             * we know the end IO handler won't happen before
>> -             * we inc the count.  Otherwise, the cb might get
>> -             * freed before we're done setting it up
>> -             */
>> -            refcount_inc(&cb->pending_bios);
>> -
>>               ret = btrfs_lookup_bio_sums(inode, comp_bio, sums);
>>               BUG_ON(ret); /* -ENOMEM */
>> @@ -798,6 +802,7 @@ blk_status_t btrfs_submit_compressed_read(struct 
>> inode *inode, struct bio *bio,
>>           cur_disk_byte += pg_len;
>>       }
>> +    atomic_inc(&cb->pending_bios);
>>       ret = btrfs_bio_wq_end_io(fs_info, comp_bio, BTRFS_WQ_ENDIO_DATA);
>>       BUG_ON(ret); /* -ENOMEM */
>> diff --git a/fs/btrfs/compression.h b/fs/btrfs/compression.h
>> index 8001b700ea3a..3df3262fedcd 100644
>> --- a/fs/btrfs/compression.h
>> +++ b/fs/btrfs/compression.h
>> @@ -29,7 +29,15 @@ struct btrfs_inode;
>>   struct compressed_bio {
>>       /* number of bios pending for this compressed extent */
>> -    refcount_t pending_bios;
>> +    atomic_t pending_bios;
>> +
>> +    /*
>> +     * Number of sectors which hasn't finished.
>> +     *
>> +     * Combined with pending_bios, we can manually finish the 
>> compressed_bio
>> +     * if we hit some error while there is still some pages not added.
>> +     */
>> +    atomic_t io_sectors;
>>       /* the pages with the compressed data on them */
>>       struct page **compressed_pages;
>>
> 

Re: [PATCH 2/9] btrfs: add compressed_bio::io_sectors to trace compressed bio more elegantly

[Johannes Thumshirn]

On 11/06/2021 03:31, Qu Wenruo wrote:
> For btrfs_submit_compressed_read() and btrfs_submit_compressed_write(),
> we have a pretty weird dance around compressed_bio::io_sectors:
> 
>   btrfs_submit_compressed_read/write()
>   {
> 	cb = kmalloc()
> 	refcount_set(&cb->pending_bios, 0);
> 	bio = btrfs_alloc_bio();
> 
> 	/* NOTE here, we haven't yet submitted any bio */
> 	refcount_set(&cb->pending_bios, 1);
> 
> 	for (pg_index = 0; pg_index < cb->nr_pages; pg_index++) {
> 		if (submit) {
> 			/* Here we submit bio, but we always have one
> 			 * extra pending_bios */
> 			refcount_inc(&cb->pending_bios);
> 			ret = btrfs_map_bio();
> 		}
> 	}
> 
> 	/* Submit the last bio */
> 	ret = btrfs_map_bio();
>   }
> 
> There are two reasons why we do this:
> 
> - compressed_bio::pending_bios is a refcount
>   Thus if it's reduced to 0, it can not be increased again.
> 
> - To ensure the compressed_bio is not freed by some submitted bios
>   If the submitted bio is finished before the next bio submitted,
>   we can free the compressed_bio completely.
> 
> But the above code is sometimes confusing, and we can do it better by
> just introduce a new member, compressed_bio::io_sectors.
> 
> With that member, we can easily distinguish if we're really the last
> bio at endio time, and even allows us to remove some BUG_ON() later,
> as we now know how many bytes are not yet submitted.
> 
> With this new member, now compressed_bio::pending_bios really indicates
> the pending bios, without any special handling needed.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---

This doesn't apply cleanly on current misc-next.

[PATCH 3/9] btrfs: hunt down the BUG_ON()s inside btrfs_submit_compressed_read()

[Qu Wenruo]

There are quite some BUG_ON()s inside btrfs_submit_compressed_read(),
naming all errors inside the for() loop relies on BUG_ON() to handle
-ENOMEM.

Hunt down these BUG_ON()s properly by:

- Introduce compressed_bio::pending_bios_wait
  This allows us to wait for any submitted bio to finish, while still
  keeps the compressed_bio not freed, as we should have
  compressed_bio::io_sectors not zero.

- Introduce finish_compressed_bio_read() to finish the compressed_bio

- Properly end the bio and finish compressed_bio when error happens

Now in btrfs_submit_compressed_read() even when the bio submission
failed, we can properly handle the error without triggering BUG_ON().

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/compression.c | 119 ++++++++++++++++++++++++++---------------
 fs/btrfs/compression.h |   3 ++
 2 files changed, 79 insertions(+), 43 deletions(-)

diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
index c9dbe306f6ba..f1b9c5fd7779 100644
--- a/fs/btrfs/compression.c
+++ b/fs/btrfs/compression.c
@@ -214,6 +214,7 @@ static bool dec_and_test_compressed_bio(struct compressed_bio *cb,
 	last_io = atomic_sub_and_test(bi_size >> fs_info->sectorsize_bits,
 				       &cb->io_sectors);
 
+	wake_up(&cb->pending_bio_wait);
 	/*
 	 * We can only finish the compressed bio if no pending bio and all io
 	 * submitted.
@@ -221,6 +222,44 @@ static bool dec_and_test_compressed_bio(struct compressed_bio *cb,
 	return last_bio && last_io;
 }
 
+static void finish_compressed_bio_read(struct compressed_bio *cb,
+				       struct bio *bio)
+{
+	unsigned long index;
+	struct page *page;
+
+	/* release the compressed pages */
+	for (index = 0; index < cb->nr_pages; index++) {
+		page = cb->compressed_pages[index];
+		page->mapping = NULL;
+		put_page(page);
+	}
+
+	/* do io completion on the original bio */
+	if (cb->errors) {
+		bio_io_error(cb->orig_bio);
+	} else {
+		struct bio_vec *bvec;
+		struct bvec_iter_all iter_all;
+
+		ASSERT(bio);
+		ASSERT(!bio->bi_status);
+		/*
+		 * we have verified the checksum already, set page
+		 * checked so the end_io handlers know about it
+		 */
+		ASSERT(!bio_flagged(bio, BIO_CLONED));
+		bio_for_each_segment_all(bvec, cb->orig_bio, iter_all)
+			SetPageChecked(bvec->bv_page);
+
+		bio_endio(cb->orig_bio);
+	}
+
+	/* finally free the cb struct */
+	kfree(cb->compressed_pages);
+	kfree(cb);
+}
+
 /* when we finish reading compressed pages from the disk, we
  * decompress them and then run the bio end_io routines on the
  * decompressed pages (in the inode address space).
@@ -235,8 +274,6 @@ static void end_compressed_bio_read(struct bio *bio)
 {
 	struct compressed_bio *cb = bio->bi_private;
 	struct inode *inode;
-	struct page *page;
-	unsigned long index;
 	unsigned int mirror = btrfs_io_bio(bio)->mirror_num;
 	int ret = 0;
 
@@ -272,35 +309,7 @@ static void end_compressed_bio_read(struct bio *bio)
 	if (ret)
 		cb->errors = 1;
 
-	/* release the compressed pages */
-	index = 0;
-	for (index = 0; index < cb->nr_pages; index++) {
-		page = cb->compressed_pages[index];
-		page->mapping = NULL;
-		put_page(page);
-	}
-
-	/* do io completion on the original bio */
-	if (cb->errors) {
-		bio_io_error(cb->orig_bio);
-	} else {
-		struct bio_vec *bvec;
-		struct bvec_iter_all iter_all;
-
-		/*
-		 * we have verified the checksum already, set page
-		 * checked so the end_io handlers know about it
-		 */
-		ASSERT(!bio_flagged(bio, BIO_CLONED));
-		bio_for_each_segment_all(bvec, cb->orig_bio, iter_all)
-			SetPageChecked(bvec->bv_page);
-
-		bio_endio(cb->orig_bio);
-	}
-
-	/* finally free the cb struct */
-	kfree(cb->compressed_pages);
-	kfree(cb);
+	finish_compressed_bio_read(cb, bio);
 out:
 	bio_put(bio);
 }
@@ -426,6 +435,7 @@ blk_status_t btrfs_submit_compressed_write(struct btrfs_inode *inode, u64 start,
 		return BLK_STS_RESOURCE;
 	atomic_set(&cb->pending_bios, 0);
 	atomic_set(&cb->io_sectors, compressed_len >> fs_info->sectorsize_bits);
+	init_waitqueue_head(&cb->pending_bio_wait);
 	cb->errors = 0;
 	cb->inode = &inode->vfs_inode;
 	cb->start = start;
@@ -702,6 +712,7 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
 
 	atomic_set(&cb->pending_bios, 0);
 	atomic_set(&cb->io_sectors, compressed_len >> fs_info->sectorsize_bits);
+	init_waitqueue_head(&cb->pending_bio_wait);
 	cb->errors = 0;
 	cb->inode = inode;
 	cb->mirror_num = mirror_num;
@@ -777,20 +788,20 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
 			atomic_inc(&cb->pending_bios);
 			ret = btrfs_bio_wq_end_io(fs_info, comp_bio,
 						  BTRFS_WQ_ENDIO_DATA);
-			BUG_ON(ret); /* -ENOMEM */
+			if (ret)
+				goto finish_cb;
 
 			ret = btrfs_lookup_bio_sums(inode, comp_bio, sums);
-			BUG_ON(ret); /* -ENOMEM */
+			if (ret)
+				goto finish_cb;
 
 			nr_sectors = DIV_ROUND_UP(comp_bio->bi_iter.bi_size,
 						  fs_info->sectorsize);
 			sums += fs_info->csum_size * nr_sectors;
 
 			ret = btrfs_map_bio(fs_info, comp_bio, mirror_num);
-			if (ret) {
-				comp_bio->bi_status = ret;
-				bio_endio(comp_bio);
-			}
+			if (ret)
+				goto finish_cb;
 
 			comp_bio = btrfs_bio_alloc(cur_disk_byte);
 			comp_bio->bi_opf = REQ_OP_READ;
@@ -804,16 +815,16 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
 
 	atomic_inc(&cb->pending_bios);
 	ret = btrfs_bio_wq_end_io(fs_info, comp_bio, BTRFS_WQ_ENDIO_DATA);
-	BUG_ON(ret); /* -ENOMEM */
+	if (ret)
+		goto last_bio;
 
 	ret = btrfs_lookup_bio_sums(inode, comp_bio, sums);
-	BUG_ON(ret); /* -ENOMEM */
+	if (ret)
+		goto last_bio;
 
 	ret = btrfs_map_bio(fs_info, comp_bio, mirror_num);
-	if (ret) {
-		comp_bio->bi_status = ret;
-		bio_endio(comp_bio);
-	}
+	if (ret)
+		goto last_bio;
 
 	return 0;
 
@@ -829,6 +840,28 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
 out:
 	free_extent_map(em);
 	return ret;
+last_bio:
+	cb->errors = 1;
+	comp_bio->bi_status = ret;
+	/* This is the last bio, endio function will finish the compressed_bio */
+	bio_endio(comp_bio);
+	return ret;
+finish_cb:
+	cb->errors = 1;
+	if (comp_bio) {
+		comp_bio->bi_status = ret;
+		bio_endio(comp_bio);
+	}
+
+	/*
+	 * Even with previous bio ended, we should still have io not yet
+	 * submitted, thus need to finish manually.
+	 */
+	wait_event(cb->pending_bio_wait, atomic_read(&cb->pending_bios) == 0);
+	ASSERT(atomic_read(&cb->io_sectors));
+	/* Now we are the only one referring @cb, can finish it safely. */
+	finish_compressed_bio_read(cb, NULL);
+	return ret;
 }
 
 /*
diff --git a/fs/btrfs/compression.h b/fs/btrfs/compression.h
index 3df3262fedcd..597033019bb2 100644
--- a/fs/btrfs/compression.h
+++ b/fs/btrfs/compression.h
@@ -39,6 +39,9 @@ struct compressed_bio {
 	 */
 	atomic_t io_sectors;
 
+	/* To wait for any submitted bio, used in error handling */
+	wait_queue_head_t pending_bio_wait;
+
 	/* the pages with the compressed data on them */
 	struct page **compressed_pages;
 
-- 
2.32.0

[PATCH 4/9] btrfs: hunt down the BUG_ON()s inside btrfs_submit_compressed_write()

[Qu Wenruo]

Just like btrfs_submit_compressed_read(), there are quite some BUG_ON()s
inside btrfs_submit_compressed_write() for the bio submission path.

Fix them using the same method:

- For last bio, just endio the bio
  As in that case, one of the endio function of all these submitted bio
  will be able to free the comprssed_bio

- For half-submitted bio, wait and finish the compressed_bio manually
  In this case, as long as all other bio finishes, we're the only one
  referring the compressed_bio, and can manually finish it.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/compression.c | 112 ++++++++++++++++++++++++++++-------------
 1 file changed, 78 insertions(+), 34 deletions(-)

diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
index f1b9c5fd7779..4bc03309b7b6 100644
--- a/fs/btrfs/compression.c
+++ b/fs/btrfs/compression.c
@@ -352,50 +352,58 @@ static noinline void end_compressed_writeback(struct inode *inode,
 	/* the inode may be gone now */
 }
 
-/*
- * do the cleanup once all the compressed pages hit the disk.
- * This will clear writeback on the file pages and free the compressed
- * pages.
- *
- * This also calls the writeback end hooks for the file pages so that
- * metadata and checksums can be updated in the file.
- */
-static void end_compressed_bio_write(struct bio *bio)
+static void finish_compressed_bio_write(struct compressed_bio *cb)
 {
-	struct compressed_bio *cb = bio->bi_private;
-	struct inode *inode;
-	struct page *page;
+	struct inode *inode = cb->inode;
 	unsigned long index;
 
-	if (!dec_and_test_compressed_bio(cb, bio))
-		goto out;
-
-	/* ok, we're the last bio for this extent, step one is to
+	/*
+	 * Ok, we're the last bio for this extent, step one is to
 	 * call back into the FS and do all the end_io operations
 	 */
 	inode = cb->inode;
-	btrfs_record_physical_zoned(inode, cb->start, bio);
 	btrfs_writepage_endio_finish_ordered(BTRFS_I(inode), NULL,
 			cb->start, cb->start + cb->len - 1,
-			bio->bi_status == BLK_STS_OK);
+			!cb->errors);
 
 	end_compressed_writeback(inode, cb);
 	/* note, our inode could be gone now */
 
 	/*
-	 * release the compressed pages, these came from alloc_page and
+	 * Release the compressed pages, these came from alloc_page and
 	 * are not attached to the inode at all
 	 */
-	index = 0;
 	for (index = 0; index < cb->nr_pages; index++) {
-		page = cb->compressed_pages[index];
+		struct page *page = cb->compressed_pages[index];
+
 		page->mapping = NULL;
 		put_page(page);
 	}
 
-	/* finally free the cb struct */
+	/* Finally free the cb struct */
 	kfree(cb->compressed_pages);
 	kfree(cb);
+}
+/*
+ * do the cleanup once all the compressed pages hit the disk.
+ * This will clear writeback on the file pages and free the compressed
+ * pages.
+ *
+ * This also calls the writeback end hooks for the file pages so that
+ * metadata and checksums can be updated in the file.
+ */
+static void end_compressed_bio_write(struct bio *bio)
+{
+	struct compressed_bio *cb = bio->bi_private;
+
+	if (!dec_and_test_compressed_bio(cb, bio))
+		goto out;
+
+	btrfs_record_physical_zoned(cb->inode, cb->start, bio);
+
+	if (bio->bi_status)
+		cb->errors = 1;
+	finish_compressed_bio_write(cb);
 out:
 	bio_put(bio);
 }
@@ -491,18 +499,18 @@ blk_status_t btrfs_submit_compressed_write(struct btrfs_inode *inode, u64 start,
 			atomic_inc(&cb->pending_bios);
 			ret = btrfs_bio_wq_end_io(fs_info, bio,
 						  BTRFS_WQ_ENDIO_DATA);
-			BUG_ON(ret); /* -ENOMEM */
+			if (ret)
+				goto finish_cb;
 
 			if (!skip_sum) {
 				ret = btrfs_csum_one_bio(inode, bio, start, 1);
-				BUG_ON(ret); /* -ENOMEM */
+				if (ret)
+					goto finish_cb;
 			}
 
 			ret = btrfs_map_bio(fs_info, bio, 0);
-			if (ret) {
-				bio->bi_status = ret;
-				bio_endio(bio);
-			}
+			if (ret)
+				goto finish_cb;
 
 			bio = btrfs_bio_alloc(first_byte);
 			bio->bi_opf = bio_op | write_flags;
@@ -528,23 +536,59 @@ blk_status_t btrfs_submit_compressed_write(struct btrfs_inode *inode, u64 start,
 
 	atomic_inc(&cb->pending_bios);
 	ret = btrfs_bio_wq_end_io(fs_info, bio, BTRFS_WQ_ENDIO_DATA);
-	BUG_ON(ret); /* -ENOMEM */
+	if (ret)
+		goto last_bio;
 
 	if (!skip_sum) {
 		ret = btrfs_csum_one_bio(inode, bio, start, 1);
-		BUG_ON(ret); /* -ENOMEM */
+		if (ret)
+			goto last_bio;
 	}
 
 	ret = btrfs_map_bio(fs_info, bio, 0);
-	if (ret) {
-		bio->bi_status = ret;
-		bio_endio(bio);
-	}
+	if (ret)
+		goto last_bio;
 
 	if (blkcg_css)
 		kthread_associate_blkcg(NULL);
 
 	return 0;
+
+last_bio:
+	cb->errors = 1;
+	bio->bi_status = ret;
+	/*
+	 * This bio is the last bio, as long as it finishes, it will free the
+	 * compressed_bio, thus we don't need to bother finishing the
+	 * compressed_bio.
+	 */
+	bio_endio(bio);
+	return ret;
+
+finish_cb:
+	cb->errors = 1;
+	if (bio) {
+		bio->bi_status = ret;
+
+		/*
+		 * For write bio we may have zoned info, manually handle it
+		 * first
+		 */
+		btrfs_record_physical_zoned(cb->inode, cb->start, bio);
+
+		/* Now we can safely end the bio */
+		bio_endio(bio);
+	}
+
+	/*
+	 * Even with previous bio ended, we should still have io not yet
+	 * submitted, thus need to finish manually.
+	 */
+	ASSERT(atomic_read(&cb->io_sectors));
+	wait_event(cb->pending_bio_wait, atomic_read(&cb->pending_bios) == 0);
+	/* Now we are the only one referring @cb, can finish it safely. */
+	finish_compressed_bio_write(cb);
+	return ret;
 }
 
 static u64 bio_end_offset(struct bio *bio)
-- 
2.32.0

Re: [PATCH 4/9] btrfs: hunt down the BUG_ON()s inside btrfs_submit_compressed_write()

[Johannes Thumshirn]

On 11/06/2021 03:32, Qu Wenruo wrote:
> Just like btrfs_submit_compressed_read(), there are quite some BUG_ON()s
> inside btrfs_submit_compressed_write() for the bio submission path.
> 
> Fix them using the same method:
> 
> - For last bio, just endio the bio
>   As in that case, one of the endio function of all these submitted bio
>   will be able to free the comprssed_bio
> 
> - For half-submitted bio, wait and finish the compressed_bio manually
>   In this case, as long as all other bio finishes, we're the only one
>   referring the compressed_bio, and can manually finish it.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
And that one doesn't apply cleanly as well, which base did you work on?

Re: [PATCH 4/9] btrfs: hunt down the BUG_ON()s inside btrfs_submit_compressed_write()

[Qu Wenruo]

On 2021/6/11 下午3:36, Johannes Thumshirn wrote:
> On 11/06/2021 03:32, Qu Wenruo wrote:
>> Just like btrfs_submit_compressed_read(), there are quite some BUG_ON()s
>> inside btrfs_submit_compressed_write() for the bio submission path.
>>
>> Fix them using the same method:
>>
>> - For last bio, just endio the bio
>>    As in that case, one of the endio function of all these submitted bio
>>    will be able to free the comprssed_bio
>>
>> - For half-submitted bio, wait and finish the compressed_bio manually
>>    In this case, as long as all other bio finishes, we're the only one
>>    referring the compressed_bio, and can manually finish it.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
> And that one doesn't apply cleanly as well, which base did you work on?
> 
An old misc-next branch, with some subpage patches, which shouldn't 
change them.

I'll update the branch to solve the conflicts.

Thanks,
Qu

[PATCH 5/9] btrfs: introduce submit_compressed_bio() for compression

[Qu Wenruo]

The new helper, submit_compressed_bio(), will aggregate the following
work:

- Increase compressed_bio::pending_bios
- Remap the endio function
- Map and submit the bio

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/compression.c | 44 +++++++++++++++++-------------------------
 1 file changed, 18 insertions(+), 26 deletions(-)

diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
index 4bc03309b7b6..e3f61ab38a99 100644
--- a/fs/btrfs/compression.c
+++ b/fs/btrfs/compression.c
@@ -408,6 +408,20 @@ static void end_compressed_bio_write(struct bio *bio)
 	bio_put(bio);
 }
 
+static blk_status_t submit_compressed_bio(struct btrfs_fs_info *fs_info,
+					  struct compressed_bio *cb,
+					  struct bio *bio, int mirror_num)
+{
+	blk_status_t ret;
+
+	atomic_inc(&cb->pending_bios);
+	ret = btrfs_bio_wq_end_io(fs_info, bio, BTRFS_WQ_ENDIO_DATA);
+	if (ret)
+		return ret;
+	ret = btrfs_map_bio(fs_info, bio, mirror_num);
+	return ret;
+}
+
 /*
  * worker function to build and submit bios for previously compressed pages.
  * The corresponding pages in the inode should be marked for writeback
@@ -496,19 +510,13 @@ blk_status_t btrfs_submit_compressed_write(struct btrfs_inode *inode, u64 start,
 
 		page->mapping = NULL;
 		if (submit || len < PAGE_SIZE) {
-			atomic_inc(&cb->pending_bios);
-			ret = btrfs_bio_wq_end_io(fs_info, bio,
-						  BTRFS_WQ_ENDIO_DATA);
-			if (ret)
-				goto finish_cb;
-
 			if (!skip_sum) {
 				ret = btrfs_csum_one_bio(inode, bio, start, 1);
 				if (ret)
 					goto finish_cb;
 			}
 
-			ret = btrfs_map_bio(fs_info, bio, 0);
+			ret = submit_compressed_bio(fs_info, cb, bio, 0);
 			if (ret)
 				goto finish_cb;
 
@@ -534,18 +542,13 @@ blk_status_t btrfs_submit_compressed_write(struct btrfs_inode *inode, u64 start,
 		cond_resched();
 	}
 
-	atomic_inc(&cb->pending_bios);
-	ret = btrfs_bio_wq_end_io(fs_info, bio, BTRFS_WQ_ENDIO_DATA);
-	if (ret)
-		goto last_bio;
-
 	if (!skip_sum) {
 		ret = btrfs_csum_one_bio(inode, bio, start, 1);
 		if (ret)
 			goto last_bio;
 	}
 
-	ret = btrfs_map_bio(fs_info, bio, 0);
+	ret = submit_compressed_bio(fs_info, cb, bio, 0);
 	if (ret)
 		goto last_bio;
 
@@ -829,12 +832,6 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
 		if (submit || bio_add_page(comp_bio, page, pg_len, 0) < pg_len) {
 			unsigned int nr_sectors;
 
-			atomic_inc(&cb->pending_bios);
-			ret = btrfs_bio_wq_end_io(fs_info, comp_bio,
-						  BTRFS_WQ_ENDIO_DATA);
-			if (ret)
-				goto finish_cb;
-
 			ret = btrfs_lookup_bio_sums(inode, comp_bio, sums);
 			if (ret)
 				goto finish_cb;
@@ -843,7 +840,7 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
 						  fs_info->sectorsize);
 			sums += fs_info->csum_size * nr_sectors;
 
-			ret = btrfs_map_bio(fs_info, comp_bio, mirror_num);
+			ret = submit_compressed_bio(fs_info, cb, comp_bio, mirror_num);
 			if (ret)
 				goto finish_cb;
 
@@ -857,16 +854,11 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
 		cur_disk_byte += pg_len;
 	}
 
-	atomic_inc(&cb->pending_bios);
-	ret = btrfs_bio_wq_end_io(fs_info, comp_bio, BTRFS_WQ_ENDIO_DATA);
-	if (ret)
-		goto last_bio;
-
 	ret = btrfs_lookup_bio_sums(inode, comp_bio, sums);
 	if (ret)
 		goto last_bio;
 
-	ret = btrfs_map_bio(fs_info, comp_bio, mirror_num);
+	ret = submit_compressed_bio(fs_info, cb, comp_bio, mirror_num);
 	if (ret)
 		goto last_bio;
 
-- 
2.32.0

[PATCH 6/9] btrfs: introduce alloc_submit_compressed_bio() for compression

[Qu Wenruo]

Just aggregate the bio allocation code into one helper, so that we can
replace 4 call sites.

There is one special note for zoned write.

Currently btrfs_submit_compressed_write() will only allocate the first
bio using ZONE_APPEND.
If we have to submit current bio due to stripe boundary, the new bio
allocated will not use ZONE_APPEND.

In theory this should be a bug, but considering zoned mode currently
only support SINGLE profile, which doesn't have any stripe boundary
limit, it should never be a problem.

This function will provide a good entrance for any work which needs to be
done at bio allocation time. Like determining the stripe boundary.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/compression.c | 91 +++++++++++++++++++++++++++++-------------
 1 file changed, 63 insertions(+), 28 deletions(-)

diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
index e3f61ab38a99..19ec0f8e7170 100644
--- a/fs/btrfs/compression.c
+++ b/fs/btrfs/compression.c
@@ -422,6 +422,41 @@ static blk_status_t submit_compressed_bio(struct btrfs_fs_info *fs_info,
 	return ret;
 }
 
+/*
+ * To allocate a compressed_bio, which will be used to read/write on-disk data.
+ *
+ * @disk_bytenr:	Disk bytenr of the IO
+ * @opf:		Bio opf
+ * @endio_func:		Endio funcion
+ */
+static struct bio *alloc_compressed_bio(struct compressed_bio *cb, u64 disk_bytenr,
+					unsigned int opf, bio_end_io_t endio_func)
+{
+	struct bio *bio;
+
+	bio = btrfs_bio_alloc(disk_bytenr);
+	/* bioset allocation should not fail */
+	ASSERT(bio);
+
+	bio->bi_opf = opf;
+	bio->bi_private = cb;
+	bio->bi_end_io = endio_func;
+
+	if (bio_op(bio) == REQ_OP_ZONE_APPEND) {
+		struct btrfs_fs_info *fs_info = btrfs_sb(cb->inode->i_sb);
+		struct btrfs_device *device;
+
+		device = btrfs_zoned_get_device(fs_info, disk_bytenr,
+						fs_info->sectorsize);
+		if (IS_ERR(device)) {
+			bio_put(bio);
+			return ERR_CAST(device);
+		}
+		bio_set_dev(bio, device->bdev);
+	}
+	return bio;
+}
+
 /*
  * worker function to build and submit bios for previously compressed pages.
  * The corresponding pages in the inode should be marked for writeback
@@ -468,22 +503,11 @@ blk_status_t btrfs_submit_compressed_write(struct btrfs_inode *inode, u64 start,
 	cb->orig_bio = NULL;
 	cb->nr_pages = nr_pages;
 
-	bio = btrfs_bio_alloc(first_byte);
-	bio->bi_opf = bio_op | write_flags;
-	bio->bi_private = cb;
-	bio->bi_end_io = end_compressed_bio_write;
-
-	if (use_append) {
-		struct btrfs_device *device;
-
-		device = btrfs_zoned_get_device(fs_info, disk_start, PAGE_SIZE);
-		if (IS_ERR(device)) {
-			kfree(cb);
-			bio_put(bio);
-			return BLK_STS_NOTSUPP;
-		}
-
-		bio_set_dev(bio, device->bdev);
+	bio = alloc_compressed_bio(cb, first_byte, bio_op | write_flags,
+				   end_compressed_bio_write);
+	if (IS_ERR(bio)) {
+		kfree(cb);
+		return errno_to_blk_status(PTR_ERR(bio));
 	}
 
 	if (blkcg_css) {
@@ -520,10 +544,14 @@ blk_status_t btrfs_submit_compressed_write(struct btrfs_inode *inode, u64 start,
 			if (ret)
 				goto finish_cb;
 
-			bio = btrfs_bio_alloc(first_byte);
-			bio->bi_opf = bio_op | write_flags;
-			bio->bi_private = cb;
-			bio->bi_end_io = end_compressed_bio_write;
+			bio = alloc_compressed_bio(cb, first_byte,
+					bio_op | write_flags,
+					end_compressed_bio_write);
+			if (IS_ERR(bio)) {
+				ret = errno_to_blk_status(PTR_ERR(bio));
+				bio = NULL;
+				goto finish_cb;
+			}
 			if (blkcg_css)
 				bio->bi_opf |= REQ_CGROUP_PUNT;
 			/*
@@ -800,10 +828,13 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
 	/* include any pages we added in add_ra-bio_pages */
 	cb->len = bio->bi_iter.bi_size;
 
-	comp_bio = btrfs_bio_alloc(cur_disk_byte);
-	comp_bio->bi_opf = REQ_OP_READ;
-	comp_bio->bi_private = cb;
-	comp_bio->bi_end_io = end_compressed_bio_read;
+	comp_bio = alloc_compressed_bio(cb, cur_disk_byte, REQ_OP_READ,
+					end_compressed_bio_read);
+	if (IS_ERR(comp_bio)) {
+		ret = errno_to_blk_status(PTR_ERR(comp_bio));
+		comp_bio = NULL;
+		goto fail2;
+	}
 
 	for (pg_index = 0; pg_index < nr_pages; pg_index++) {
 		u32 pg_len = PAGE_SIZE;
@@ -844,10 +875,14 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
 			if (ret)
 				goto finish_cb;
 
-			comp_bio = btrfs_bio_alloc(cur_disk_byte);
-			comp_bio->bi_opf = REQ_OP_READ;
-			comp_bio->bi_private = cb;
-			comp_bio->bi_end_io = end_compressed_bio_read;
+			comp_bio = alloc_compressed_bio(cb, cur_disk_byte,
+					REQ_OP_READ,
+					end_compressed_bio_read);
+			if (IS_ERR(comp_bio)) {
+				ret = errno_to_blk_status(PTR_ERR(comp_bio));
+				comp_bio = NULL;
+				goto finish_cb;
+			}
 
 			bio_add_page(comp_bio, page, pg_len, 0);
 		}
-- 
2.32.0

[PATCH 7/9] btrfs: make btrfs_submit_compressed_read() to determine stripe boundary at bio allocation time

[Qu Wenruo]

Currently btrfs_submit_compressed_read() will check
btrfs_bio_fits_in_stripe() each time a new page is going to be added.

Even compressed extent is small, we don't really need to do that for
every page.

This patch will align the behavior to extent_io.c, by determining the
stripe boundary when allocating a bio.

Unlike extent_io.c, in compressed.c we don't need to bother things like
different bio flags, thus no need to re-use bio_ctrl.

Here we just manually introduce new local variable, next_stripe_start,
and teach alloc_compressed_bio() to calculate the stripe boundary.

Then each time we add some page range into the bio, we check if we
reached the boundary.
And if reached, submit it.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/compression.c | 140 +++++++++++++++++++++++++----------------
 1 file changed, 86 insertions(+), 54 deletions(-)

diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
index 19ec0f8e7170..71d7b1b9b4bf 100644
--- a/fs/btrfs/compression.c
+++ b/fs/btrfs/compression.c
@@ -428,11 +428,17 @@ static blk_status_t submit_compressed_bio(struct btrfs_fs_info *fs_info,
  * @disk_bytenr:	Disk bytenr of the IO
  * @opf:		Bio opf
  * @endio_func:		Endio funcion
+ * @next_stripe_start:	Disk bytenr of next stripe start.
  */
 static struct bio *alloc_compressed_bio(struct compressed_bio *cb, u64 disk_bytenr,
-					unsigned int opf, bio_end_io_t endio_func)
+					unsigned int opf, bio_end_io_t endio_func,
+					u64 *next_stripe_start)
 {
+	struct btrfs_fs_info *fs_info = btrfs_sb(cb->inode->i_sb);
+	struct btrfs_io_geometry geom;
+	struct extent_map *em;
 	struct bio *bio;
+	int ret;
 
 	bio = btrfs_bio_alloc(disk_bytenr);
 	/* bioset allocation should not fail */
@@ -443,7 +449,6 @@ static struct bio *alloc_compressed_bio(struct compressed_bio *cb, u64 disk_byte
 	bio->bi_end_io = endio_func;
 
 	if (bio_op(bio) == REQ_OP_ZONE_APPEND) {
-		struct btrfs_fs_info *fs_info = btrfs_sb(cb->inode->i_sb);
 		struct btrfs_device *device;
 
 		device = btrfs_zoned_get_device(fs_info, disk_bytenr,
@@ -454,6 +459,15 @@ static struct bio *alloc_compressed_bio(struct compressed_bio *cb, u64 disk_byte
 		}
 		bio_set_dev(bio, device->bdev);
 	}
+	em = btrfs_get_chunk_map(fs_info, disk_bytenr, fs_info->sectorsize);
+	if (IS_ERR(em))
+		return ERR_CAST(em);
+	ret = btrfs_get_io_geometry(fs_info, em, btrfs_op(bio), disk_bytenr,
+				    &geom);
+	free_extent_map(em);
+	if (ret < 0)
+		return ERR_PTR(ret);
+	*next_stripe_start = disk_bytenr + geom.len;
 	return bio;
 }
 
@@ -481,6 +495,7 @@ blk_status_t btrfs_submit_compressed_write(struct btrfs_inode *inode, u64 start,
 	int pg_index = 0;
 	struct page *page;
 	u64 first_byte = disk_start;
+	u64 next_stripe_start;
 	blk_status_t ret;
 	int skip_sum = inode->flags & BTRFS_INODE_NODATASUM;
 	const bool use_append = btrfs_use_zone_append(inode, disk_start);
@@ -504,7 +519,8 @@ blk_status_t btrfs_submit_compressed_write(struct btrfs_inode *inode, u64 start,
 	cb->nr_pages = nr_pages;
 
 	bio = alloc_compressed_bio(cb, first_byte, bio_op | write_flags,
-				   end_compressed_bio_write);
+				   end_compressed_bio_write,
+				   &next_stripe_start);
 	if (IS_ERR(bio)) {
 		kfree(cb);
 		return errno_to_blk_status(PTR_ERR(bio));
@@ -546,7 +562,8 @@ blk_status_t btrfs_submit_compressed_write(struct btrfs_inode *inode, u64 start,
 
 			bio = alloc_compressed_bio(cb, first_byte,
 					bio_op | write_flags,
-					end_compressed_bio_write);
+					end_compressed_bio_write,
+					&next_stripe_start);
 			if (IS_ERR(bio)) {
 				ret = errno_to_blk_status(PTR_ERR(bio));
 				bio = NULL;
@@ -759,9 +776,10 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
 	unsigned long compressed_len;
 	unsigned long nr_pages;
 	unsigned long pg_index;
-	struct page *page;
-	struct bio *comp_bio;
-	u64 cur_disk_byte = bio->bi_iter.bi_sector << 9;
+	struct bio *comp_bio = NULL;
+	const u64 disk_bytenr = bio->bi_iter.bi_sector << SECTOR_SHIFT;
+	u64 cur_disk_byte = disk_bytenr;
+	u64 next_stripe_start;
 	u64 em_len;
 	u64 em_start;
 	struct extent_map *em;
@@ -828,41 +846,63 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
 	/* include any pages we added in add_ra-bio_pages */
 	cb->len = bio->bi_iter.bi_size;
 
-	comp_bio = alloc_compressed_bio(cb, cur_disk_byte, REQ_OP_READ,
-					end_compressed_bio_read);
-	if (IS_ERR(comp_bio)) {
-		ret = errno_to_blk_status(PTR_ERR(comp_bio));
-		comp_bio = NULL;
-		goto fail2;
-	}
+	while (cur_disk_byte < disk_bytenr + compressed_len) {
+		u64 offset = cur_disk_byte - disk_bytenr;
+		int index = offset >> PAGE_SHIFT;
+		unsigned int real_size;
+		unsigned int added;
+		struct page *page;
 
-	for (pg_index = 0; pg_index < nr_pages; pg_index++) {
-		u32 pg_len = PAGE_SIZE;
-		int submit = 0;
+		/* Allocate new bio if not allocated or already submitted */
+		if (!comp_bio) {
+			comp_bio = alloc_compressed_bio(cb, cur_disk_byte,
+					REQ_OP_READ,
+					end_compressed_bio_read,
+					&next_stripe_start);
+			if (IS_ERR(comp_bio)) {
+				ret = errno_to_blk_status(PTR_ERR(comp_bio));
+				comp_bio = NULL;
+				goto finish_cb;
+			}
+		}
 
 		/*
-		 * To handle subpage case, we need to make sure the bio only
-		 * covers the range we need.
-		 *
-		 * If we're at the last page, truncate the length to only cover
-		 * the remaining part.
+		 * We should never reach next_stripe_start, as if we reach the
+		 * boundary we will submit the bio immediately.
 		 */
-		if (pg_index == nr_pages - 1)
-			pg_len = min_t(u32, PAGE_SIZE,
-					compressed_len - pg_index * PAGE_SIZE);
+		ASSERT(cur_disk_byte != next_stripe_start);
+		page = cb->compressed_pages[index];
 
-		page = cb->compressed_pages[pg_index];
-		page->mapping = inode->i_mapping;
-		page->index = em_start >> PAGE_SHIFT;
+		/*
+		 * We have various limit on the real read size:
+		 * - stripe boundary
+		 * - page boundary
+		 * - compressed length boundary
+		 */
+		real_size = min_t(u64, U32_MAX,
+				  next_stripe_start - cur_disk_byte);
+		real_size = min_t(u64, real_size,
+				  PAGE_SIZE - offset_in_page(offset));
+		real_size = min_t(u64, real_size,
+				  compressed_len - offset);
+		ASSERT(IS_ALIGNED(real_size, fs_info->sectorsize));
 
-		if (comp_bio->bi_iter.bi_size)
-			submit = btrfs_bio_fits_in_stripe(page, pg_len,
-							  comp_bio, 0);
 
-		page->mapping = NULL;
-		if (submit || bio_add_page(comp_bio, page, pg_len, 0) < pg_len) {
-			unsigned int nr_sectors;
+		added = bio_add_page(comp_bio, page, real_size,
+				     offset_in_page(offset));
+		/*
+		 * Maximum compressed extent size is 128K, we should never
+		 * reach bio size limit.
+		 */
+		ASSERT(added == real_size);
 
+		cur_disk_byte += added;
+
+		/* Reached boundary stripe, need to submit */
+		if (cur_disk_byte == next_stripe_start) {
+			int nr_sectors;
+
+			ASSERT(comp_bio->bi_iter.bi_size);
 			ret = btrfs_lookup_bio_sums(inode, comp_bio, sums);
 			if (ret)
 				goto finish_cb;
@@ -871,31 +911,23 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
 						  fs_info->sectorsize);
 			sums += fs_info->csum_size * nr_sectors;
 
-			ret = submit_compressed_bio(fs_info, cb, comp_bio, mirror_num);
+			ret = submit_compressed_bio(fs_info, cb, comp_bio,
+						    mirror_num);
 			if (ret)
 				goto finish_cb;
-
-			comp_bio = alloc_compressed_bio(cb, cur_disk_byte,
-					REQ_OP_READ,
-					end_compressed_bio_read);
-			if (IS_ERR(comp_bio)) {
-				ret = errno_to_blk_status(PTR_ERR(comp_bio));
-				comp_bio = NULL;
-				goto finish_cb;
-			}
-
-			bio_add_page(comp_bio, page, pg_len, 0);
+			comp_bio = NULL;
 		}
-		cur_disk_byte += pg_len;
 	}
+	/* Submit the last bio */
+	if (comp_bio) {
+		ret = btrfs_lookup_bio_sums(inode, comp_bio, sums);
+		if (ret)
+			goto last_bio;
 
-	ret = btrfs_lookup_bio_sums(inode, comp_bio, sums);
-	if (ret)
-		goto last_bio;
-
-	ret = submit_compressed_bio(fs_info, cb, comp_bio, mirror_num);
-	if (ret)
-		goto last_bio;
+		ret = submit_compressed_bio(fs_info, cb, comp_bio, mirror_num);
+		if (ret)
+			goto last_bio;
+	}
 
 	return 0;
 
-- 
2.32.0

Re: [PATCH 7/9] btrfs: make btrfs_submit_compressed_read() to determine stripe boundary at bio allocation time

[Johannes Thumshirn]

On 11/06/2021 03:32, Qu Wenruo wrote:
> @@ -443,7 +449,6 @@ static struct bio *alloc_compressed_bio(struct compressed_bio *cb, u64 disk_byte
>  	bio->bi_end_io = endio_func;
>  
>  	if (bio_op(bio) == REQ_OP_ZONE_APPEND) {
> -		struct btrfs_fs_info *fs_info = btrfs_sb(cb->inode->i_sb);
>  		struct btrfs_device *device;
>  
>  		device = btrfs_zoned_get_device(fs_info, disk_bytenr,
> @@ -454,6 +459,15 @@ static struct bio *alloc_compressed_bio(struct compressed_bio *cb, u64 disk_byte
>  		}
>  		bio_set_dev(bio, device->bdev);
>  	}
> +	em = btrfs_get_chunk_map(fs_info, disk_bytenr, fs_info->sectorsize);
> +	if (IS_ERR(em))
> +		return ERR_CAST(em);
> +	ret = btrfs_get_io_geometry(fs_info, em, btrfs_op(bio), disk_bytenr,
> +				    &geom);
> +	free_extent_map(em);
> +	if (ret < 0)
> +		return ERR_PTR(ret);
> +	*next_stripe_start = disk_bytenr + geom.len;
>  	return bio;
>  }
>  


If you're doing a chunk map lookup on allocation you can kill above btrfs_zoned_get_device()
call and grab the block device for bio_set_dev() directly from the 1st stripe. Otherwise we're
doing two chunk map lookups for zoned btrfs after another, which is inefficient.

[PATCH 8/9] btrfs: make btrfs_submit_compressed_read() to determine stripe boundary at bio allocation time

[Qu Wenruo]

Currently btrfs_submit_compressed_write() will check
btrfs_bio_fits_in_stripe() each time a new page is going to be added.

Even compressed extent is small, we don't really need to do that for
every page.

This patch will align the behavior to extent_io.c, by determining the
stripe boundary when allocating a bio.

Unlike extent_io.c, in compressed.c we don't need to bother things like
different bio flags, thus no need to re-use bio_ctrl.

Here we just manually introduce new local variable, next_stripe_start,
and use that value returned from alloc_compressed_bio() to calculate
the stripe boundary.

Then each time we add some page range into the bio, we check if we
reached the boundary.
And if reached, submit it.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/compression.c | 122 ++++++++++++++++++++---------------------
 1 file changed, 59 insertions(+), 63 deletions(-)

diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
index 71d7b1b9b4bf..de91489b92c0 100644
--- a/fs/btrfs/compression.c
+++ b/fs/btrfs/compression.c
@@ -491,10 +491,7 @@ blk_status_t btrfs_submit_compressed_write(struct btrfs_inode *inode, u64 start,
 	struct btrfs_fs_info *fs_info = inode->root->fs_info;
 	struct bio *bio = NULL;
 	struct compressed_bio *cb;
-	unsigned long bytes_left;
-	int pg_index = 0;
-	struct page *page;
-	u64 first_byte = disk_start;
+	u64 cur_disk_bytenr = disk_start;
 	u64 next_stripe_start;
 	blk_status_t ret;
 	int skip_sum = inode->flags & BTRFS_INODE_NODATASUM;
@@ -518,38 +515,58 @@ blk_status_t btrfs_submit_compressed_write(struct btrfs_inode *inode, u64 start,
 	cb->orig_bio = NULL;
 	cb->nr_pages = nr_pages;
 
-	bio = alloc_compressed_bio(cb, first_byte, bio_op | write_flags,
-				   end_compressed_bio_write,
-				   &next_stripe_start);
-	if (IS_ERR(bio)) {
-		kfree(cb);
-		return errno_to_blk_status(PTR_ERR(bio));
-	}
+	while (cur_disk_bytenr < disk_start + compressed_len) {
+		u64 offset = cur_disk_bytenr - disk_start;
+		int index = offset >> PAGE_SHIFT;
+		unsigned int real_size;
+		unsigned int added;
+		struct page *page = compressed_pages[index];
 
-	if (blkcg_css) {
-		bio->bi_opf |= REQ_CGROUP_PUNT;
-		kthread_associate_blkcg(blkcg_css);
-	}
+		/* Allocate new bio if not allocated or already submitted */
+		if (!bio) {
+			bio = alloc_compressed_bio(cb, cur_disk_bytenr,
+				bio_op | write_flags,
+				end_compressed_bio_write,
+				&next_stripe_start);
+			if (IS_ERR(bio)) {
+				ret = errno_to_blk_status(PTR_ERR(bio));
+				bio = NULL;
+				goto finish_cb;
+			}
+		}
+		/*
+		 * We should never reach next_stripe_start, as if we reach the
+		 * boundary we will submit the bio immediately.
+		 */
+		ASSERT(cur_disk_bytenr != next_stripe_start);
+
+		/*
+		 * We have various limit on the real read size:
+		 * - stripe boundary
+		 * - page boundary
+		 * - compressed length boundary
+		 */
+		real_size = min_t(u64, U32_MAX,
+				  next_stripe_start - cur_disk_bytenr);
+		real_size = min_t(u64, real_size,
+				  PAGE_SIZE - offset_in_page(offset));
+		real_size = min_t(u64, real_size,
+				  compressed_len - offset);
+		ASSERT(IS_ALIGNED(real_size, fs_info->sectorsize));
 
-	/* create and submit bios for the compressed pages */
-	bytes_left = compressed_len;
-	for (pg_index = 0; pg_index < cb->nr_pages; pg_index++) {
-		int submit = 0;
-		int len;
+		added = bio_add_page(bio, page, real_size,
+				     offset_in_page(offset));
+		/*
+		 * Maximum compressed extent size is 128K, we should never
+		 * reach bio size limit.
+		 */
+		ASSERT(added == real_size);
 
-		page = compressed_pages[pg_index];
-		page->mapping = inode->vfs_inode.i_mapping;
-		if (bio->bi_iter.bi_size)
-			submit = btrfs_bio_fits_in_stripe(page, PAGE_SIZE, bio,
-							  0);
+		cur_disk_bytenr += added;
 
-		if (pg_index == 0 && use_append)
-			len = bio_add_zone_append_page(bio, page, PAGE_SIZE, 0);
-		else
-			len = bio_add_page(bio, page, PAGE_SIZE, 0);
+		/* Reached boundary stripe, need to submit */
+		if (cur_disk_bytenr == next_stripe_start) {
 
-		page->mapping = NULL;
-		if (submit || len < PAGE_SIZE) {
 			if (!skip_sum) {
 				ret = btrfs_csum_one_bio(inode, bio, start, 1);
 				if (ret)
@@ -559,47 +576,26 @@ blk_status_t btrfs_submit_compressed_write(struct btrfs_inode *inode, u64 start,
 			ret = submit_compressed_bio(fs_info, cb, bio, 0);
 			if (ret)
 				goto finish_cb;
-
-			bio = alloc_compressed_bio(cb, first_byte,
-					bio_op | write_flags,
-					end_compressed_bio_write,
-					&next_stripe_start);
-			if (IS_ERR(bio)) {
-				ret = errno_to_blk_status(PTR_ERR(bio));
-				bio = NULL;
-				goto finish_cb;
-			}
-			if (blkcg_css)
-				bio->bi_opf |= REQ_CGROUP_PUNT;
-			/*
-			 * Use bio_add_page() to ensure the bio has at least one
-			 * page.
-			 */
-			bio_add_page(bio, page, PAGE_SIZE, 0);
-		}
-		if (bytes_left < PAGE_SIZE) {
-			btrfs_info(fs_info,
-					"bytes left %lu compress len %lu nr %lu",
-			       bytes_left, cb->compressed_len, cb->nr_pages);
+			bio = NULL;
 		}
-		bytes_left -= PAGE_SIZE;
-		first_byte += PAGE_SIZE;
 		cond_resched();
 	}
 
-	if (!skip_sum) {
-		ret = btrfs_csum_one_bio(inode, bio, start, 1);
+	/* Submit the last bio */
+	if (bio) {
+		if (!skip_sum) {
+			ret = btrfs_csum_one_bio(inode, bio, start, 1);
+			if (ret)
+				goto last_bio;
+		}
+
+		ret = submit_compressed_bio(fs_info, cb, bio, 0);
 		if (ret)
 			goto last_bio;
-	}
-
-	ret = submit_compressed_bio(fs_info, cb, bio, 0);
-	if (ret)
-		goto last_bio;
 
+	}
 	if (blkcg_css)
 		kthread_associate_blkcg(NULL);
-
 	return 0;
 
 last_bio:
-- 
2.32.0

Re: [PATCH 8/9] btrfs: make btrfs_submit_compressed_read() to determine stripe boundary at bio allocation time

[Johannes Thumshirn]

On 11/06/2021 03:32, Qu Wenruo wrote:
> +		/* Allocate new bio if not allocated or already submitted */
> +		if (!bio) {
> +			bio = alloc_compressed_bio(cb, cur_disk_bytenr,
> +				bio_op | write_flags,
> +				end_compressed_bio_write,
> +				&next_stripe_start);
> +			if (IS_ERR(bio)) {
> +				ret = errno_to_blk_status(PTR_ERR(bio));
> +				bio = NULL;
> +				goto finish_cb;
> +			}
> +		}
> +		/*
> +		 * We should never reach next_stripe_start, as if we reach the
> +		 * boundary we will submit the bio immediately.
> +		 */
> +		ASSERT(cur_disk_bytenr != next_stripe_start);
> +
> +		/*
> +		 * We have various limit on the real read size:
> +		 * - stripe boundary
> +		 * - page boundary
> +		 * - compressed length boundary
> +		 */
> +		real_size = min_t(u64, U32_MAX,
> +				  next_stripe_start - cur_disk_bytenr);
> +		real_size = min_t(u64, real_size,
> +				  PAGE_SIZE - offset_in_page(offset));
> +		real_size = min_t(u64, real_size,
> +				  compressed_len - offset);
> +		ASSERT(IS_ALIGNED(real_size, fs_info->sectorsize));
>  
> -	/* create and submit bios for the compressed pages */
> -	bytes_left = compressed_len;
> -	for (pg_index = 0; pg_index < cb->nr_pages; pg_index++) {
> -		int submit = 0;
> -		int len;
> +		added = bio_add_page(bio, page, real_size,
> +				     offset_in_page(offset));
> +		/*
> +		 * Maximum compressed extent size is 128K, we should never
> +		 * reach bio size limit.
> +		 */
> +		ASSERT(added == real_size);
>  
> -		page = compressed_pages[pg_index];
> -		page->mapping = inode->vfs_inode.i_mapping;
> -		if (bio->bi_iter.bi_size)
> -			submit = btrfs_bio_fits_in_stripe(page, PAGE_SIZE, bio,
> -							  0);
> +		cur_disk_bytenr += added;
>  
> -		if (pg_index == 0 && use_append)
> -			len = bio_add_zone_append_page(bio, page, PAGE_SIZE, 0);
> -		else
> -			len = bio_add_page(bio, page, PAGE_SIZE, 0);

I think you still need to distinguish between normal write and zone append here,
as you adding pages to an already created bio. Adding one page to an empty bio
will always succeed but when adding more than one page to a zone append bio, you
have to take the device's maximum zone append limit into account, as zone append
bios can't be split. This is also the reason why we do the device 
lookup/bio_set_dev() for the zone append bios, so bio_add_zone_append_page() can
look at the device's limitations when adding the pages.

Re: [PATCH 8/9] btrfs: make btrfs_submit_compressed_read() to determine stripe boundary at bio allocation time

[Qu Wenruo]

On 2021/6/11 下午3:49, Johannes Thumshirn wrote:
> On 11/06/2021 03:32, Qu Wenruo wrote:
>> +		/* Allocate new bio if not allocated or already submitted */
>> +		if (!bio) {
>> +			bio = alloc_compressed_bio(cb, cur_disk_bytenr,
>> +				bio_op | write_flags,
>> +				end_compressed_bio_write,
>> +				&next_stripe_start);
>> +			if (IS_ERR(bio)) {
>> +				ret = errno_to_blk_status(PTR_ERR(bio));
>> +				bio = NULL;
>> +				goto finish_cb;
>> +			}
>> +		}
>> +		/*
>> +		 * We should never reach next_stripe_start, as if we reach the
>> +		 * boundary we will submit the bio immediately.
>> +		 */
>> +		ASSERT(cur_disk_bytenr != next_stripe_start);
>> +
>> +		/*
>> +		 * We have various limit on the real read size:
>> +		 * - stripe boundary
>> +		 * - page boundary
>> +		 * - compressed length boundary
>> +		 */
>> +		real_size = min_t(u64, U32_MAX,
>> +				  next_stripe_start - cur_disk_bytenr);
>> +		real_size = min_t(u64, real_size,
>> +				  PAGE_SIZE - offset_in_page(offset));
>> +		real_size = min_t(u64, real_size,
>> +				  compressed_len - offset);
>> +		ASSERT(IS_ALIGNED(real_size, fs_info->sectorsize));
>>
>> -	/* create and submit bios for the compressed pages */
>> -	bytes_left = compressed_len;
>> -	for (pg_index = 0; pg_index < cb->nr_pages; pg_index++) {
>> -		int submit = 0;
>> -		int len;
>> +		added = bio_add_page(bio, page, real_size,
>> +				     offset_in_page(offset));
>> +		/*
>> +		 * Maximum compressed extent size is 128K, we should never
>> +		 * reach bio size limit.
>> +		 */
>> +		ASSERT(added == real_size);
>>
>> -		page = compressed_pages[pg_index];
>> -		page->mapping = inode->vfs_inode.i_mapping;
>> -		if (bio->bi_iter.bi_size)
>> -			submit = btrfs_bio_fits_in_stripe(page, PAGE_SIZE, bio,
>> -							  0);
>> +		cur_disk_bytenr += added;
>>
>> -		if (pg_index == 0 && use_append)
>> -			len = bio_add_zone_append_page(bio, page, PAGE_SIZE, 0);
>> -		else
>> -			len = bio_add_page(bio, page, PAGE_SIZE, 0);
>
> I think you still need to distinguish between normal write and zone append here,
> as you adding pages to an already created bio.

Oh, my bad, forgot to handle the zoned append differently.

> Adding one page to an empty bio
> will always succeed but when adding more than one page to a zone append bio, you
> have to take the device's maximum zone append limit into account, as zone append
> bios can't be split. This is also the reason why we do the device
> lookup/bio_set_dev() for the zone append bios, so bio_add_zone_append_page() can
> look at the device's limitations when adding the pages.

Did you mean that for the bio_add_zone_append_page(), it may return less
bytes than we expected?
Even if our compressed write is ensured to be smaller than 128K?

Thanks,
Qu

Re: [PATCH 8/9] btrfs: make btrfs_submit_compressed_read() to determine stripe boundary at bio allocation time

[Johannes Thumshirn]

On 11/06/2021 10:16, Qu Wenruo wrote:
> Did you mean that for the bio_add_zone_append_page(), it may return less
> bytes than we expected?
> Even if our compressed write is ensured to be smaller than 128K?

No it either adds the number of requested pages or it fails (I think there's
and effort going on to make bio_add_page() and friends return bool so this is
less confusing). 

Re: [PATCH 8/9] btrfs: make btrfs_submit_compressed_read() to determine stripe boundary at bio allocation time

[Qu Wenruo]

On 2021/6/11 下午4:19, Johannes Thumshirn wrote:
> On 11/06/2021 10:16, Qu Wenruo wrote:
>> Did you mean that for the bio_add_zone_append_page(), it may return less
>> bytes than we expected?
>> Even if our compressed write is ensured to be smaller than 128K?
> 
> No it either adds the number of requested pages or it fails (I think there's
> and effort going on to make bio_add_page() and friends return bool so this is
> less confusing).
> 

Got it, it means we still need to check the return value and submit if 
needed.

Only for regular bio_add_page() it would never fail as our write size is 
smaller than bio size limit.
But for bio_add_zone_append_page() it still has extra limit so that add 
can fail.

Will fix it in next version.

Thanks,
Qu

Re: [PATCH 8/9] btrfs: make btrfs_submit_compressed_read() to determine stripe boundary at bio allocation time

[Qu Wenruo]

On 2021/6/11 下午4:19, Johannes Thumshirn wrote:
> On 11/06/2021 10:16, Qu Wenruo wrote:
>> Did you mean that for the bio_add_zone_append_page(), it may return less
>> bytes than we expected?
>> Even if our compressed write is ensured to be smaller than 128K?
>
> No it either adds the number of requested pages or it fails (I think there's
> and effort going on to make bio_add_page() and friends return bool so this is
> less confusing).
>
BTW, I have a question about the add or not at all behavior.

One of my concern is, if we're at the zone boundary (or what's the
proper term?), and we want to add 4 sectors, but the zone boundary can
only add one sector, and if we just submit current bio, wouldn't it
cause some problem?

E.g. we leave a one sector gap for the current zone.
Will that be a problem?

Thanks,
Qu

Re: [PATCH 8/9] btrfs: make btrfs_submit_compressed_read() to determine stripe boundary at bio allocation time

[Johannes Thumshirn]

On 11/06/2021 14:40, Qu Wenruo wrote:
> 
> 
> On 2021/6/11 下午4:19, Johannes Thumshirn wrote:
>> On 11/06/2021 10:16, Qu Wenruo wrote:
>>> Did you mean that for the bio_add_zone_append_page(), it may return less
>>> bytes than we expected?
>>> Even if our compressed write is ensured to be smaller than 128K?
>>
>> No it either adds the number of requested pages or it fails (I think there's
>> and effort going on to make bio_add_page() and friends return bool so this is
>> less confusing).
>>
> BTW, I have a question about the add or not at all behavior.
> 
> One of my concern is, if we're at the zone boundary (or what's the
> proper term?), and we want to add 4 sectors, but the zone boundary can
> only add one sector, and if we just submit current bio, wouldn't it
> cause some problem?
> 
> E.g. we leave a one sector gap for the current zone.
> Will that be a problem?
> 

The zoned allocator will take care of it. It'll fill the zone as good as possible
or move the allocation to a new block group on a new zone.

[PATCH 9/9] btrfs: remove unused function btrfs_bio_fits_in_stripe()

[Qu Wenruo]

As the last caller in compression.c is removed, we don't need that
function anymore.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/ctree.h |  2 --
 fs/btrfs/inode.c | 42 ------------------------------------------
 2 files changed, 44 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 918df8877b45..4364733f4464 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -3144,8 +3144,6 @@ void btrfs_merge_delalloc_extent(struct inode *inode, struct extent_state *new,
 				 struct extent_state *other);
 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);
 void btrfs_set_range_writeback(struct btrfs_inode *inode, 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/inode.c b/fs/btrfs/inode.c
index 27d56a77aa5f..abbee108b298 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -2232,48 +2232,6 @@ void btrfs_clear_delalloc_extent(struct inode *vfs_inode,
 	}
 }
 
-/*
- * btrfs_bio_fits_in_stripe - Checks whether the size of the given bio will fit
- * in a chunk's stripe. This function ensures that bios do not span a
- * stripe/chunk
- *
- * @page - The page we are about to add to the bio
- * @size - size we want to add to the bio
- * @bio - bio we want to ensure is smaller than a stripe
- * @bio_flags - flags of the bio
- *
- * return 1 if page cannot be added to the bio
- * return 0 if page can be added to the bio
- * return error otherwise
- */
-int btrfs_bio_fits_in_stripe(struct page *page, size_t size, struct bio *bio,
-			     unsigned long bio_flags)
-{
-	struct inode *inode = page->mapping->host;
-	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
-	u64 logical = bio->bi_iter.bi_sector << 9;
-	u32 bio_len = bio->bi_iter.bi_size;
-	struct extent_map *em;
-	int ret = 0;
-	struct btrfs_io_geometry geom;
-
-	if (bio_flags & EXTENT_BIO_COMPRESSED)
-		return 0;
-
-	em = btrfs_get_chunk_map(fs_info, logical, fs_info->sectorsize);
-	if (IS_ERR(em))
-		return PTR_ERR(em);
-	ret = btrfs_get_io_geometry(fs_info, em, btrfs_op(bio), logical, &geom);
-	if (ret < 0)
-		goto out;
-
-	if (geom.len < bio_len + size)
-		ret = 1;
-out:
-	free_extent_map(em);
-	return ret;
-}
-
 /*
  * in order to insert checksums into the metadata in large chunks,
  * we wait until bio submission time.   All the pages in the bio are
-- 
2.32.0