Re: [PATCH v3 2/3] btrfs: remove btrfs_bio_alloc() helper
From: Nikolay Borisov <hidden>
Date: 2021-09-17 12:27:50
On 15.09.21 г. 10:17, Qu Wenruo wrote:
quoted hunk ↗ jump to hunk
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)
}
/*
- * 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. 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>