On 17.09.21 г. 15:33, Qu Wenruo wrote:

On 2021/9/17 20:27, Nikolay Borisov wrote:

quoted

On 15.09.21 г. 10:17, Qu Wenruo wrote:

quoted

The helper btrfs_bio_alloc() is almost the same as btrfs_io_bio_alloc(),
except it's allocating using BIO_MAX_VECS as @nr_iovecs, and initialize
bio->bi_iter.bi_sector.

However the naming itself is not using "btrfs_io_bio" to indicate its
parameter is "strcut btrfs_io_bio" and can be easily confused with
"struct btrfs_bio".

Considering assigned bio->bi_iter.bi_sector is such a simple work and
there are already tons of call sites doing that manually, there is no
need to do that in a helper.

Remove btrfs_bio_alloc() helper, and enhance btrfs_io_bio_alloc()
function to provide a fail-safe value for its @nr_iovecs.

And then replace all btrfs_bio_alloc() callers with
btrfs_io_bio_alloc().

Signed-off-by: Qu Wenruo <redacted>
---
  fs/btrfs/compression.c | 12 ++++++++----
  fs/btrfs/extent_io.c   | 33 +++++++++++++++------------------
  fs/btrfs/extent_io.h   |  1 -
  3 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
index 7869ad12bc6e..2475dc0b1c22 100644
--- a/fs/btrfs/compression.c
+++ b/fs/btrfs/compression.c

@@ -418,7 +418,8 @@ 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 = btrfs_io_bio_alloc(0);
+    bio->bi_iter.bi_sector = first_byte >> SECTOR_SHIFT;
      bio->bi_opf = bio_op | write_flags;
      bio->bi_private = cb;
      bio->bi_end_io = end_compressed_bio_write;

@@ -490,7 +491,8 @@ blk_status_t btrfs_submit_compressed_write(struct

btrfs_inode *inode, u64 start,
                  bio_endio(bio);
              }

-            bio = btrfs_bio_alloc(first_byte);
+            bio = btrfs_io_bio_alloc(0);
+            bio->bi_iter.bi_sector = first_byte >> SECTOR_SHIFT;
              bio->bi_opf = bio_op | write_flags;
              bio->bi_private = cb;
              bio->bi_end_io = end_compressed_bio_write;

@@ -748,7 +750,8 @@ 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 = btrfs_io_bio_alloc(0);
+    comp_bio->bi_iter.bi_sector = cur_disk_byte >> SECTOR_SHIFT;
      comp_bio->bi_opf = REQ_OP_READ;
      comp_bio->bi_private = cb;
      comp_bio->bi_end_io = end_compressed_bio_read;

@@ -806,7 +809,8 @@ blk_status_t btrfs_submit_compressed_read(struct

inode *inode, struct bio *bio,
                  bio_endio(comp_bio);
              }

-            comp_bio = btrfs_bio_alloc(cur_disk_byte);
+            comp_bio = btrfs_io_bio_alloc(0);
+            comp_bio->bi_iter.bi_sector = cur_disk_byte >>
SECTOR_SHIFT;
              comp_bio->bi_opf = REQ_OP_READ;
              comp_bio->bi_private = cb;
              comp_bio->bi_end_io = end_compressed_bio_read;

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 1aed03ef5f49..d3fcf7e8dc48 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c

@@ -3121,16 +3121,22 @@ static inline void btrfs_io_bio_init

After reading through the whole patch I agree with the naming, though
yeah it's a bit long, but we've been using this wordy naming. For
identifiers it's fine to use lbio and it's now clear from the context
that it's about the btrfs-specific features.(struct btrfs_io_bio
*btrfs_bio)

quoted

  }

  /*
- * The following helpers allocate a bio. As it's backed by a bioset,
it'll
- * never fail.  We're returning a bio right now but you can call
btrfs_io_bio
- * for the appropriate container_of magic
+ * Allocate a btrfs_io_bio, with @nr_iovecs as maximum iovecs.
+ *
+ * If @nr_iovecs is 0, it will use BIO_MAX_VECS as @nr_iovces instead.
+ * This behavior is to provide a fail-safe default value.
+ *
+ * This helper uses bioset to allocate the bio, thus it's backed by
mempool,
+ * and should not fail from process contexts.
   */
-struct bio *btrfs_bio_alloc(u64 first_byte)
+struct bio *btrfs_io_bio_alloc(unsigned int nr_iovecs)
  {
      struct bio *bio;

-    bio = bio_alloc_bioset(GFP_NOFS, BIO_MAX_VECS, &btrfs_bioset);
-    bio->bi_iter.bi_sector = first_byte >> 9;
+    ASSERT(nr_iovecs <= BIO_MAX_VECS);
+    if (nr_iovecs == 0)
+        nr_iovecs = BIO_MAX_VECS;

hell no! How come passing 0 actually means BIO_MAX_VEC. Instead of
having 0 everywhere and have the function translate this to
BIO_MAX_VECS, simply pass BIO_MAX_VECS in every call site where it's
needed.

That's part of the feedback I want.

I'm not yet determined on which should be the proper way.

Yes, we can pass BIO_MAX_VEC for call sites which doesn't care about the
vector size.

But I also think letting callers to bother less is a good idea.
(one of the few moments I think function overriding can be very useful
here)

If you have objection, I'm pretty happy to change the behavior, and just
do an ASSERT() to catch any values larger than BIO_MAX_VECS.

This sounds much better, in the worst case it should be -1 which treated
as the "max" and definitely not 0. I don't think 0 has been used as a
special value to mean "max" anywhere.

Thanks,
Qu

quoted

David, please either fix the patch in the tree or retract it. Let's try
and refrain from adding such "gems" to the code base.

<snip>