Re: [PATCH v3 2/3] btrfs: remove btrfs_bio_alloc() helper

[Nikolay Borisov <nborisov@suse.com>]

On 17.09.21 г. 15:33, Qu Wenruo wrote:
> 
> 
> On 2021/9/17 20:27, Nikolay Borisov wrote:
>>
>>
>> On 15.09.21 г. 10:17, Qu Wenruo wrote:
>>> 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 <wqu@suse.com>
>>> ---
>>>   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.
> 
> 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
>>
>> 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>
>>
>