| Message ID | 20210915071718.59418-3-wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: btrfs_bio and btrfs_io_bio rename | expand |
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. 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>
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. 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> >
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> >> >
On Fri, Sep 17, 2021 at 03:27:44PM +0300, Nikolay Borisov wrote: > > -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. I had thought about that before and cocluded that passing BIO_MAX_VECS everywhere would be the wrong way as it's a detail about how many bio vecs are allocated. So 0 is a default is ok as any other number means that it's the exact count. > 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. So should we add another helper that takes the exact number and drop the parameter everwhere is 0 so it's just btrfs_io_bio_alloc() with the fallback?
On 17.09.21 г. 15:43, David Sterba wrote: > So should we add another helper that takes the exact number and drop the > parameter everwhere is 0 so it's just btrfs_io_bio_alloc() with the > fallback? But by adding another helper we just introduce more indirection. Actually I'd argue that if 0 is a sane default then BIO_MAX_VECS cannot be any worse because: a) It's a number which is as good as 0 b) It's even named. So this is technically better than a plain 0
On 2021/9/17 20:49, Nikolay Borisov wrote: > > > On 17.09.21 г. 15:43, David Sterba wrote: >> So should we add another helper that takes the exact number and drop the >> parameter everwhere is 0 so it's just btrfs_io_bio_alloc() with the >> fallback? > > But by adding another helper we just introduce more indirection. > > Actually I'd argue that if 0 is a sane default then BIO_MAX_VECS cannot > be any worse because: > > a) It's a number which is as good as 0 > b) It's even named. So this is technically better than a plain 0 > Any final call on this? I hope this could be an example for future optional parameters. We have some existing codes using two different inline functions, and both of them call a internal but exported function with "__" prefix. We also have call sites passing all needed parameters just like Nikolay suggested. Despite that, I have some further optimization for the btrfs_logical_bio structure, thus I hope this helper situation can be solved soon. Thanks, Qu
On Mon, Sep 20, 2021 at 06:33:14PM +0800, Qu Wenruo wrote: > > > On 2021/9/17 20:49, Nikolay Borisov wrote: > > > > > > On 17.09.21 г. 15:43, David Sterba wrote: > >> So should we add another helper that takes the exact number and drop the > >> parameter everwhere is 0 so it's just btrfs_io_bio_alloc() with the > >> fallback? > > > > But by adding another helper we just introduce more indirection. > > > > Actually I'd argue that if 0 is a sane default then BIO_MAX_VECS cannot > > be any worse because: > > > > a) It's a number which is as good as 0 > > b) It's even named. So this is technically better than a plain 0 > > > > Any final call on this? > > I hope this could be an example for future optional parameters. > > We have some existing codes using two different inline functions, and > both of them call a internal but exported function with "__" prefix. > > We also have call sites passing all needed parameters just like Nikolay > suggested. I'm fine with explicitly using BIO_MAX_VECS instead of 0. I'll update it in the patch, no need to resend.
On 2021/9/20 20:41, David Sterba wrote: > On Mon, Sep 20, 2021 at 06:33:14PM +0800, Qu Wenruo wrote: >> >> >> On 2021/9/17 20:49, Nikolay Borisov wrote: >>> >>> >>> On 17.09.21 г. 15:43, David Sterba wrote: >>>> So should we add another helper that takes the exact number and drop the >>>> parameter everwhere is 0 so it's just btrfs_io_bio_alloc() with the >>>> fallback? >>> >>> But by adding another helper we just introduce more indirection. >>> >>> Actually I'd argue that if 0 is a sane default then BIO_MAX_VECS cannot >>> be any worse because: >>> >>> a) It's a number which is as good as 0 >>> b) It's even named. So this is technically better than a plain 0 >>> >> >> Any final call on this? >> >> I hope this could be an example for future optional parameters. >> >> We have some existing codes using two different inline functions, and >> both of them call a internal but exported function with "__" prefix. >> >> We also have call sites passing all needed parameters just like Nikolay >> suggested. > > I'm fine with explicitly using BIO_MAX_VECS instead of 0. I'll update it > in the patch, no need to resend. > Thank you very much. I'll no longer use this tricky way any more. Thanks, Qu
On 2021/9/15 15: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(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; I'm very surprised that, bbio->logical is not initialized here. This means, except two call sites which manually initialize bbio->logical, all other sites don't have bbio->logical set from the very beginning. I guess I need another patch to set the logical bytenr for all btrfs_bio allocator. Thankfully it doesn't cause any new regression, but any unitizlied member can always cause unexpected behavior when new use cases are added. In fact this uninitialized @logical is already cause my RFC patchset ("btrfs: refactor how we handle btrfs_io_context and slightly reduce memory usage for both btrfs_bio and btrfs_io_context") to crash. As in that patchset, we require bbio->logical to lookup the mirror device. I'll merge the proper initializer into the next version of that patchset. Thanks, Qu > + ASSERT(nr_iovecs <= BIO_MAX_VECS); > + if (nr_iovecs == 0) > + nr_iovecs = BIO_MAX_VECS; > + bio = bio_alloc_bioset(GFP_NOFS, nr_iovecs, &btrfs_bioset); > btrfs_io_bio_init(btrfs_io_bio(bio)); > return bio; > } > @@ -3148,16 +3154,6 @@ struct bio *btrfs_bio_clone(struct bio *bio) > return new; > } > > -struct bio *btrfs_io_bio_alloc(unsigned int nr_iovecs) > -{ > - struct bio *bio; > - > - /* Bio allocation backed by a bioset does not fail */ > - bio = bio_alloc_bioset(GFP_NOFS, nr_iovecs, &btrfs_bioset); > - btrfs_io_bio_init(btrfs_io_bio(bio)); > - return bio; > -} > - > struct bio *btrfs_bio_clone_partial(struct bio *orig, u64 offset, u64 size) > { > struct bio *bio; > @@ -3307,14 +3303,15 @@ static int alloc_new_bio(struct btrfs_inode *inode, > struct bio *bio; > int ret; > > + bio = btrfs_io_bio_alloc(0); > /* > * For compressed page range, its disk_bytenr is always @disk_bytenr > * passed in, no matter if we have added any range into previous bio. > */ > if (bio_flags & EXTENT_BIO_COMPRESSED) > - bio = btrfs_bio_alloc(disk_bytenr); > + bio->bi_iter.bi_sector = disk_bytenr >> SECTOR_SHIFT; > else > - bio = btrfs_bio_alloc(disk_bytenr + offset); > + bio->bi_iter.bi_sector = (disk_bytenr + offset) >> SECTOR_SHIFT; > bio_ctrl->bio = bio; > bio_ctrl->bio_flags = bio_flags; > bio->bi_end_io = end_io_func; > diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h > index ba471f2063a7..81fa68eaa699 100644 > --- a/fs/btrfs/extent_io.h > +++ b/fs/btrfs/extent_io.h > @@ -278,7 +278,6 @@ void extent_range_redirty_for_io(struct inode *inode, u64 start, u64 end); > void extent_clear_unlock_delalloc(struct btrfs_inode *inode, u64 start, u64 end, > struct page *locked_page, > u32 bits_to_clear, unsigned long page_ops); > -struct bio *btrfs_bio_alloc(u64 first_byte); > struct bio *btrfs_io_bio_alloc(unsigned int nr_iovecs); > struct bio *btrfs_bio_clone(struct bio *bio); > struct bio *btrfs_bio_clone_partial(struct bio *orig, u64 offset, u64 size); >
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(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; + bio = bio_alloc_bioset(GFP_NOFS, nr_iovecs, &btrfs_bioset); btrfs_io_bio_init(btrfs_io_bio(bio)); return bio; } @@ -3148,16 +3154,6 @@ struct bio *btrfs_bio_clone(struct bio *bio) return new; } -struct bio *btrfs_io_bio_alloc(unsigned int nr_iovecs) -{ - struct bio *bio; - - /* Bio allocation backed by a bioset does not fail */ - bio = bio_alloc_bioset(GFP_NOFS, nr_iovecs, &btrfs_bioset); - btrfs_io_bio_init(btrfs_io_bio(bio)); - return bio; -} - struct bio *btrfs_bio_clone_partial(struct bio *orig, u64 offset, u64 size) { struct bio *bio; @@ -3307,14 +3303,15 @@ static int alloc_new_bio(struct btrfs_inode *inode, struct bio *bio; int ret; + bio = btrfs_io_bio_alloc(0); /* * For compressed page range, its disk_bytenr is always @disk_bytenr * passed in, no matter if we have added any range into previous bio. */ if (bio_flags & EXTENT_BIO_COMPRESSED) - bio = btrfs_bio_alloc(disk_bytenr); + bio->bi_iter.bi_sector = disk_bytenr >> SECTOR_SHIFT; else - bio = btrfs_bio_alloc(disk_bytenr + offset); + bio->bi_iter.bi_sector = (disk_bytenr + offset) >> SECTOR_SHIFT; bio_ctrl->bio = bio; bio_ctrl->bio_flags = bio_flags; bio->bi_end_io = end_io_func; diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h index ba471f2063a7..81fa68eaa699 100644 --- a/fs/btrfs/extent_io.h +++ b/fs/btrfs/extent_io.h @@ -278,7 +278,6 @@ void extent_range_redirty_for_io(struct inode *inode, u64 start, u64 end); void extent_clear_unlock_delalloc(struct btrfs_inode *inode, u64 start, u64 end, struct page *locked_page, u32 bits_to_clear, unsigned long page_ops); -struct bio *btrfs_bio_alloc(u64 first_byte); struct bio *btrfs_io_bio_alloc(unsigned int nr_iovecs); struct bio *btrfs_bio_clone(struct bio *bio); struct bio *btrfs_bio_clone_partial(struct bio *orig, u64 offset, u64 size);
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(-)