| Message ID | 20220901074216.1849941-2-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/17] block: export bio_split_rw | expand |
Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
On 2022/9/1 15:42, Christoph Hellwig wrote: > bio_split_rw can be used by file systems to split and incoming write > bio into multiple bios fitting the hardware limit for use as ZONE_APPEND > bios. Export it for initial use in btrfs. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > block/blk-merge.c | 3 ++- > include/linux/bio.h | 4 ++++ > 2 files changed, 6 insertions(+), 1 deletion(-) > > diff --git a/block/blk-merge.c b/block/blk-merge.c > index ff04e9290715a..e68295462977b 100644 > --- a/block/blk-merge.c > +++ b/block/blk-merge.c > @@ -267,7 +267,7 @@ static bool bvec_split_segs(struct queue_limits *lim, const struct bio_vec *bv, > * responsible for ensuring that @bs is only destroyed after processing of the > * split bio has finished. > */ > -static struct bio *bio_split_rw(struct bio *bio, struct queue_limits *lim, > +struct bio *bio_split_rw(struct bio *bio, struct queue_limits *lim, I found the queue_limits structure pretty scary, while we only have very limited members used in this case: - lim->virt_boundary_mask Used in bvec_gap_to_prev() - lim->max_segments - lim->seg_boundary_mask - lim->max_segment_size Used in bvec_split_segs() - lim->logical_block_size Not familiar with block layer, thus I'm wondering do btrfs really need a full queue_limits structure to call bio_split_rw(). Or can we have a simplified wrapper? IIRC inside btrfs we only need two cases for bio split: - Split for stripe boundary - Split for OE/zoned boundary Thanks, Qu > unsigned *segs, struct bio_set *bs, unsigned max_bytes) > { > struct bio_vec bv, bvprv, *bvprvp = NULL; > @@ -317,6 +317,7 @@ static struct bio *bio_split_rw(struct bio *bio, struct queue_limits *lim, > bio_clear_polled(bio); > return bio_split(bio, bytes >> SECTOR_SHIFT, GFP_NOIO, bs); > } > +EXPORT_SYMBOL_GPL(bio_split_rw); > > /** > * __bio_split_to_limits - split a bio to fit the queue limits > diff --git a/include/linux/bio.h b/include/linux/bio.h > index ca22b06700a94..46890f8235401 100644 > --- a/include/linux/bio.h > +++ b/include/linux/bio.h > @@ -12,6 +12,8 @@ > > #define BIO_MAX_VECS 256U > > +struct queue_limits; > + > static inline unsigned int bio_max_segs(unsigned int nr_segs) > { > return min(nr_segs, BIO_MAX_VECS); > @@ -375,6 +377,8 @@ static inline void bip_set_seed(struct bio_integrity_payload *bip, > void bio_trim(struct bio *bio, sector_t offset, sector_t size); > extern struct bio *bio_split(struct bio *bio, int sectors, > gfp_t gfp, struct bio_set *bs); > +struct bio *bio_split_rw(struct bio *bio, struct queue_limits *lim, > + unsigned *segs, struct bio_set *bs, unsigned max_bytes); > > /** > * bio_next_split - get next @sectors from a bio, splitting if necessary
On Thu, Sep 01, 2022 at 04:54:32PM +0800, Qu Wenruo wrote: > I found the queue_limits structure pretty scary, while we only have very > limited members used in this case: > > - lim->virt_boundary_mask > Used in bvec_gap_to_prev() > > - lim->max_segments > > - lim->seg_boundary_mask > - lim->max_segment_size > Used in bvec_split_segs() > > - lim->logical_block_size > > Not familiar with block layer, thus I'm wondering do btrfs really need a > full queue_limits structure to call bio_split_rw(). Well, the queue limits is what the block layer uses for communicating the I/O size limitations, and thus both bio_split_rw and the stacking layer helpers operate on it. > Or can we have a simplified wrapper? I don't think we can simplify anything here. The alternative would be to open code the I/O path logic, which means a lot more code that needs to be maintained and has a high probability to get out of sync with the block layer logic. So I'd much rather share this code between everything that stacks block devices, be that to represent another block device on the top like dm/md or for a 'direct' stacking in the file system like btrfs does. > IIRC inside btrfs we only need two cases for bio split: > > - Split for stripe boundary > > - Split for OE/zoned boundary No. For zoned devices we all limitations for bio, basically all that you mentioned above.
On 2022/9/5 14:44, Christoph Hellwig wrote: > On Thu, Sep 01, 2022 at 04:54:32PM +0800, Qu Wenruo wrote: >> I found the queue_limits structure pretty scary, while we only have very >> limited members used in this case: >> >> - lim->virt_boundary_mask >> Used in bvec_gap_to_prev() >> >> - lim->max_segments >> >> - lim->seg_boundary_mask >> - lim->max_segment_size >> Used in bvec_split_segs() >> >> - lim->logical_block_size >> >> Not familiar with block layer, thus I'm wondering do btrfs really need a >> full queue_limits structure to call bio_split_rw(). > > Well, the queue limits is what the block layer uses for communicating > the I/O size limitations, and thus both bio_split_rw and the stacking > layer helpers operate on it. > >> Or can we have a simplified wrapper? > > I don't think we can simplify anything here. The alternative would > be to open code the I/O path logic, which means a lot more code that > needs to be maintained and has a high probability to get out of sync > with the block layer logic. So I'd much rather share this code > between everything that stacks block devices, be that to represent > another block device on the top like dm/md or for a 'direct' stacking > in the file system like btrfs does. > >> IIRC inside btrfs we only need two cases for bio split: >> >> - Split for stripe boundary >> >> - Split for OE/zoned boundary > > No. For zoned devices we all limitations for bio, basically all that > you mentioned above. OK, that explains the reason for the full queue_limits exported. Then it makes sense to me now. Thanks, Qu
On Thu, Sep 01, 2022 at 10:42:00AM +0300, Christoph Hellwig wrote: > bio_split_rw can be used by file systems to split and incoming write > bio into multiple bios fitting the hardware limit for use as ZONE_APPEND > bios. Export it for initial use in btrfs. > > Signed-off-by: Christoph Hellwig <hch@lst.de> Reviewed-by: Josef Bacik <josef@toxicpanda.com> Thanks, Josef
diff --git a/block/blk-merge.c b/block/blk-merge.c index ff04e9290715a..e68295462977b 100644 --- a/block/blk-merge.c +++ b/block/blk-merge.c @@ -267,7 +267,7 @@ static bool bvec_split_segs(struct queue_limits *lim, const struct bio_vec *bv, * responsible for ensuring that @bs is only destroyed after processing of the * split bio has finished. */ -static struct bio *bio_split_rw(struct bio *bio, struct queue_limits *lim, +struct bio *bio_split_rw(struct bio *bio, struct queue_limits *lim, unsigned *segs, struct bio_set *bs, unsigned max_bytes) { struct bio_vec bv, bvprv, *bvprvp = NULL; @@ -317,6 +317,7 @@ static struct bio *bio_split_rw(struct bio *bio, struct queue_limits *lim, bio_clear_polled(bio); return bio_split(bio, bytes >> SECTOR_SHIFT, GFP_NOIO, bs); } +EXPORT_SYMBOL_GPL(bio_split_rw); /** * __bio_split_to_limits - split a bio to fit the queue limits diff --git a/include/linux/bio.h b/include/linux/bio.h index ca22b06700a94..46890f8235401 100644 --- a/include/linux/bio.h +++ b/include/linux/bio.h @@ -12,6 +12,8 @@ #define BIO_MAX_VECS 256U +struct queue_limits; + static inline unsigned int bio_max_segs(unsigned int nr_segs) { return min(nr_segs, BIO_MAX_VECS); @@ -375,6 +377,8 @@ static inline void bip_set_seed(struct bio_integrity_payload *bip, void bio_trim(struct bio *bio, sector_t offset, sector_t size); extern struct bio *bio_split(struct bio *bio, int sectors, gfp_t gfp, struct bio_set *bs); +struct bio *bio_split_rw(struct bio *bio, struct queue_limits *lim, + unsigned *segs, struct bio_set *bs, unsigned max_bytes); /** * bio_next_split - get next @sectors from a bio, splitting if necessary
bio_split_rw can be used by file systems to split and incoming write bio into multiple bios fitting the hardware limit for use as ZONE_APPEND bios. Export it for initial use in btrfs. Signed-off-by: Christoph Hellwig <hch@lst.de> --- block/blk-merge.c | 3 ++- include/linux/bio.h | 4 ++++ 2 files changed, 6 insertions(+), 1 deletion(-)