On 2021/10/5 03:42, David Sterba wrote:

On Mon, Sep 27, 2021 at 03:21:52PM +0800, Qu Wenruo wrote:

quoted

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 <redacted>
---
  fs/btrfs/compression.c | 45 ++++++++++++++++++------------------------
  1 file changed, 19 insertions(+), 26 deletions(-)

diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
index b6ed5933caf5..3f0be97d17f3 100644
--- a/fs/btrfs/compression.c
+++ b/fs/btrfs/compression.c

@@ -422,6 +422,21 @@ 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;
+
+	ASSERT(bio->bi_iter.bi_size);
+	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

@@ -518,19 +533,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);

Can you please send me an explanation why it's still OK to aggregate the
calls as it changes the order. Originally there's

	atomic_inc
	btrfs_bio_wq_end_io
	btrfs_csum_one_bio
	btrfs_map_bio

While in the new code:

	btrfs_csum_one_bio
	from submit_compressed_bio:
		atomic_inc
		btrfs_bio_wq_end_io
		btrfs_map_bio

So in particular the order of

atomic_inc+btrfs_bio_wq_end_io is in reverse order with
btrfs_csum_one_bio

The point is, btrfs_csum_one_bio() does nothing related to bio submission.

It really only fulfill btrfs_bio::csum.

The only important order is atomic_inc() -> btrfs_bio_wq_end_io() ->
btrfs_map_bio().

Thanks,
Qu

quoted

@@ -889,7 +887,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;

@@ -904,16 +902,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);

Same for btrfs_lookup_bio_sums instead of btrfs_csum_one_bio.