On 2021/6/11 下午12:18, Anand Jain wrote:

On 11/6/21 9:31 am, Qu Wenruo wrote:

quoted

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

quoted

   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 <redacted>
---
  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;