| Message ID | 30045b11-0064-1849-5b10-f8fa05c6958d@kernel.dk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: handle fast path of bio splitting inline | expand |
On Wed, Oct 13, 2021 at 02:44:14PM -0600, Jens Axboe wrote: > The fast path is no splitting needed. Separate the handling into a > check part we can inline, and an out-of-line handling path if we do > need to split. > > Signed-off-by: Jens Axboe <axboe@kernel.dk> What about something like this version instead? diff --git a/block/blk-merge.c b/block/blk-merge.c index 9a55b50708293..f333afb45eb15 100644 --- a/block/blk-merge.c +++ b/block/blk-merge.c @@ -353,21 +353,6 @@ void __blk_queue_split(struct bio **bio, unsigned int *nr_segs) nr_segs); break; default: - /* - * All drivers must accept single-segments bios that are <= - * PAGE_SIZE. This is a quick and dirty check that relies on - * the fact that bi_io_vec[0] is always valid if a bio has data. - * The check might lead to occasional false negatives when bios - * are cloned, but compared to the performance impact of cloned - * bios themselves the loop below doesn't matter anyway. - */ - if (!q->limits.chunk_sectors && - (*bio)->bi_vcnt == 1 && - ((*bio)->bi_io_vec[0].bv_len + - (*bio)->bi_io_vec[0].bv_offset) <= PAGE_SIZE) { - *nr_segs = 1; - break; - } split = blk_bio_segment_split(q, *bio, &q->bio_split, nr_segs); break; } @@ -399,7 +384,8 @@ void blk_queue_split(struct bio **bio) { unsigned int nr_segs; - __blk_queue_split(bio, &nr_segs); + if (blk_may_split(*bio)) + __blk_queue_split(bio, &nr_segs); } EXPORT_SYMBOL(blk_queue_split); diff --git a/block/blk-mq.c b/block/blk-mq.c index 608b270a7f6b8..7c82e052ca83f 100644 --- a/block/blk-mq.c +++ b/block/blk-mq.c @@ -2251,11 +2251,12 @@ void blk_mq_submit_bio(struct bio *bio) const int is_flush_fua = op_is_flush(bio->bi_opf); struct request *rq; struct blk_plug *plug; - unsigned int nr_segs; + unsigned int nr_segs = 1; blk_status_t ret; blk_queue_bounce(q, &bio); - __blk_queue_split(&bio, &nr_segs); + if (blk_may_split(bio)) + __blk_queue_split(&bio, &nr_segs); if (!bio_integrity_prep(bio)) goto queue_exit; diff --git a/block/blk.h b/block/blk.h index 0afee3e6a7c1e..34b31baf51324 100644 --- a/block/blk.h +++ b/block/blk.h @@ -262,6 +262,31 @@ ssize_t part_timeout_show(struct device *, struct device_attribute *, char *); ssize_t part_timeout_store(struct device *, struct device_attribute *, const char *, size_t); +static inline bool blk_may_split(struct bio *bio) +{ + switch (bio_op(bio)) { + case REQ_OP_DISCARD: + case REQ_OP_SECURE_ERASE: + case REQ_OP_WRITE_ZEROES: + case REQ_OP_WRITE_SAME: + return true; /* non-trivial splitting decisions */ + default: + break; + } + + /* + * All drivers must accept single-segments bios that are <= PAGE_SIZE. + * This is a quick and dirty check that relies on the fact that + * bi_io_vec[0] is always valid if a bio has data. The check might + * lead to occasional false negatives when bios are cloned, but compared + * to the performance impact of cloned bios themselves the loop below + * doesn't matter anyway. + */ + return bio->bi_bdev->bd_disk->queue->limits.chunk_sectors || + bio->bi_vcnt != 1 || + bio->bi_io_vec->bv_len + bio->bi_io_vec->bv_offset >PAGE_SIZE; +} + void __blk_queue_split(struct bio **bio, unsigned int *nr_segs); int ll_back_merge_fn(struct request *req, struct bio *bio, unsigned int nr_segs);
On 10/13/21 11:38 PM, Christoph Hellwig wrote: > On Wed, Oct 13, 2021 at 02:44:14PM -0600, Jens Axboe wrote: >> The fast path is no splitting needed. Separate the handling into a >> check part we can inline, and an out-of-line handling path if we do >> need to split. >> >> Signed-off-by: Jens Axboe <axboe@kernel.dk> > > What about something like this version instead? A bit of a combo, I think this will do fine. commit d997c5f4001031863de1c8c437bd2fcc6a4f79a2 Author: Jens Axboe <axboe@kernel.dk> Date: Wed Oct 13 12:43:41 2021 -0600 block: handle fast path of bio splitting inline The fast path is no splitting needed. Separate the handling into a check part we can inline, and an out-of-line handling path if we do need to split. Signed-off-by: Jens Axboe <axboe@kernel.dk> diff --git a/block/blk-merge.c b/block/blk-merge.c index 4da2bf18fa4d..f390a8753268 100644 --- a/block/blk-merge.c +++ b/block/blk-merge.c @@ -324,6 +324,7 @@ static struct bio *blk_bio_segment_split(struct request_queue *q, /** * __blk_queue_split - split a bio and submit the second half + * @q: [in] request_queue new bio is being queued at * @bio: [in, out] bio to be split * @nr_segs: [out] number of segments in the first bio * @@ -334,9 +335,9 @@ static struct bio *blk_bio_segment_split(struct request_queue *q, * of the caller to ensure that q->bio_split is only released after processing * of the split bio has finished. */ -void __blk_queue_split(struct bio **bio, unsigned int *nr_segs) +void __blk_queue_split(struct request_queue *q, struct bio **bio, + unsigned int *nr_segs) { - struct request_queue *q = (*bio)->bi_bdev->bd_disk->queue; struct bio *split = NULL; switch (bio_op(*bio)) { @@ -353,21 +354,6 @@ void __blk_queue_split(struct bio **bio, unsigned int *nr_segs) nr_segs); break; default: - /* - * All drivers must accept single-segments bios that are <= - * PAGE_SIZE. This is a quick and dirty check that relies on - * the fact that bi_io_vec[0] is always valid if a bio has data. - * The check might lead to occasional false negatives when bios - * are cloned, but compared to the performance impact of cloned - * bios themselves the loop below doesn't matter anyway. - */ - if (!q->limits.chunk_sectors && - (*bio)->bi_vcnt == 1 && - ((*bio)->bi_io_vec[0].bv_len + - (*bio)->bi_io_vec[0].bv_offset) <= PAGE_SIZE) { - *nr_segs = 1; - break; - } split = blk_bio_segment_split(q, *bio, &q->bio_split, nr_segs); break; } @@ -397,9 +383,11 @@ void __blk_queue_split(struct bio **bio, unsigned int *nr_segs) */ void blk_queue_split(struct bio **bio) { + struct request_queue *q = (*bio)->bi_bdev->bd_disk->queue; unsigned int nr_segs; - __blk_queue_split(bio, &nr_segs); + if (blk_may_split(q, *bio)) + __blk_queue_split(q, bio, &nr_segs); } EXPORT_SYMBOL(blk_queue_split); diff --git a/block/blk-mq.c b/block/blk-mq.c index dd4121dcd3ce..0cca4b7a4d16 100644 --- a/block/blk-mq.c +++ b/block/blk-mq.c @@ -2521,11 +2521,12 @@ void blk_mq_submit_bio(struct bio *bio) struct request *rq; struct blk_plug *plug; struct request *same_queue_rq = NULL; - unsigned int nr_segs; + unsigned int nr_segs = 1; blk_status_t ret; blk_queue_bounce(q, &bio); - __blk_queue_split(&bio, &nr_segs); + if (blk_may_split(q, bio)) + __blk_queue_split(q, &bio, &nr_segs); if (!bio_integrity_prep(bio)) goto queue_exit; diff --git a/block/blk.h b/block/blk.h index fa23338449ed..f6e61cebd6ae 100644 --- a/block/blk.h +++ b/block/blk.h @@ -264,7 +264,32 @@ ssize_t part_timeout_show(struct device *, struct device_attribute *, char *); ssize_t part_timeout_store(struct device *, struct device_attribute *, const char *, size_t); -void __blk_queue_split(struct bio **bio, unsigned int *nr_segs); +static inline bool blk_may_split(struct request_queue *q, struct bio *bio) +{ + switch (bio_op(bio)) { + case REQ_OP_DISCARD: + case REQ_OP_SECURE_ERASE: + case REQ_OP_WRITE_ZEROES: + case REQ_OP_WRITE_SAME: + return true; /* non-trivial splitting decisions */ + default: + break; + } + + /* + * All drivers must accept single-segments bios that are <= PAGE_SIZE. + * This is a quick and dirty check that relies on the fact that + * bi_io_vec[0] is always valid if a bio has data. The check might + * lead to occasional false negatives when bios are cloned, but compared + * to the performance impact of cloned bios themselves the loop below + * doesn't matter anyway. + */ + return q->limits.chunk_sectors || bio->bi_vcnt != 1 || + bio->bi_io_vec->bv_len + bio->bi_io_vec->bv_offset > PAGE_SIZE; +} + +void __blk_queue_split(struct request_queue *q, struct bio **bio, + unsigned int *nr_segs); int ll_back_merge_fn(struct request *req, struct bio *bio, unsigned int nr_segs); bool blk_attempt_req_merge(struct request_queue *q, struct request *rq,
On Thu, Oct 14, 2021 at 07:16:50AM -0600, Jens Axboe wrote: > On 10/13/21 11:38 PM, Christoph Hellwig wrote: > > On Wed, Oct 13, 2021 at 02:44:14PM -0600, Jens Axboe wrote: > >> The fast path is no splitting needed. Separate the handling into a > >> check part we can inline, and an out-of-line handling path if we do > >> need to split. > >> > >> Signed-off-by: Jens Axboe <axboe@kernel.dk> > > > > What about something like this version instead? > > A bit of a combo, I think this will do fine. Fine with me: Reviewed-by: Christoph Hellwig <hch@lst.de>
diff --git a/block/blk-merge.c b/block/blk-merge.c index 9a55b5070829..20ec9f00801a 100644 --- a/block/blk-merge.c +++ b/block/blk-merge.c @@ -14,6 +14,7 @@ #include "blk.h" #include "blk-rq-qos.h" #include "blk-throttle.h" +#include "blk-merge.h" static inline void bio_get_first_bvec(struct bio *bio, struct bio_vec *bv) { @@ -92,10 +93,8 @@ static inline bool req_gap_front_merge(struct request *req, struct bio *bio) return bio_will_gap(req->q, NULL, bio, req->bio); } -static struct bio *blk_bio_discard_split(struct request_queue *q, - struct bio *bio, - struct bio_set *bs, - unsigned *nsegs) +struct bio *blk_bio_discard_split(struct request_queue *q, struct bio *bio, + struct bio_set *bs, unsigned *nsegs) { unsigned int max_discard_sectors, granularity; int alignment; @@ -136,8 +135,8 @@ static struct bio *blk_bio_discard_split(struct request_queue *q, return bio_split(bio, split_sectors, GFP_NOIO, bs); } -static struct bio *blk_bio_write_zeroes_split(struct request_queue *q, - struct bio *bio, struct bio_set *bs, unsigned *nsegs) +struct bio *blk_bio_write_zeroes_split(struct request_queue *q, struct bio *bio, + struct bio_set *bs, unsigned *nsegs) { *nsegs = 0; @@ -150,10 +149,8 @@ static struct bio *blk_bio_write_zeroes_split(struct request_queue *q, return bio_split(bio, q->limits.max_write_zeroes_sectors, GFP_NOIO, bs); } -static struct bio *blk_bio_write_same_split(struct request_queue *q, - struct bio *bio, - struct bio_set *bs, - unsigned *nsegs) +struct bio *blk_bio_write_same_split(struct request_queue *q, struct bio *bio, + struct bio_set *bs, unsigned *nsegs) { *nsegs = 1; @@ -275,10 +272,8 @@ static bool bvec_split_segs(const struct request_queue *q, * responsible for ensuring that @bs is only destroyed after processing of the * split bio has finished. */ -static struct bio *blk_bio_segment_split(struct request_queue *q, - struct bio *bio, - struct bio_set *bs, - unsigned *segs) +struct bio *blk_bio_segment_split(struct request_queue *q, struct bio *bio, + struct bio_set *bs, unsigned *segs) { struct bio_vec bv, bvprv, *bvprvp = NULL; struct bvec_iter iter; @@ -322,67 +317,17 @@ static struct bio *blk_bio_segment_split(struct request_queue *q, return bio_split(bio, sectors, GFP_NOIO, bs); } -/** - * __blk_queue_split - split a bio and submit the second half - * @bio: [in, out] bio to be split - * @nr_segs: [out] number of segments in the first bio - * - * Split a bio into two bios, chain the two bios, submit the second half and - * store a pointer to the first half in *@bio. If the second bio is still too - * big it will be split by a recursive call to this function. Since this - * function may allocate a new bio from q->bio_split, it is the responsibility - * of the caller to ensure that q->bio_split is only released after processing - * of the split bio has finished. - */ -void __blk_queue_split(struct bio **bio, unsigned int *nr_segs) +void blk_bio_handle_split(struct bio **bio, struct bio *split) { - struct request_queue *q = (*bio)->bi_bdev->bd_disk->queue; - struct bio *split = NULL; - - switch (bio_op(*bio)) { - case REQ_OP_DISCARD: - case REQ_OP_SECURE_ERASE: - split = blk_bio_discard_split(q, *bio, &q->bio_split, nr_segs); - break; - case REQ_OP_WRITE_ZEROES: - split = blk_bio_write_zeroes_split(q, *bio, &q->bio_split, - nr_segs); - break; - case REQ_OP_WRITE_SAME: - split = blk_bio_write_same_split(q, *bio, &q->bio_split, - nr_segs); - break; - default: - /* - * All drivers must accept single-segments bios that are <= - * PAGE_SIZE. This is a quick and dirty check that relies on - * the fact that bi_io_vec[0] is always valid if a bio has data. - * The check might lead to occasional false negatives when bios - * are cloned, but compared to the performance impact of cloned - * bios themselves the loop below doesn't matter anyway. - */ - if (!q->limits.chunk_sectors && - (*bio)->bi_vcnt == 1 && - ((*bio)->bi_io_vec[0].bv_len + - (*bio)->bi_io_vec[0].bv_offset) <= PAGE_SIZE) { - *nr_segs = 1; - break; - } - split = blk_bio_segment_split(q, *bio, &q->bio_split, nr_segs); - break; - } - - if (split) { - /* there isn't chance to merge the splitted bio */ - split->bi_opf |= REQ_NOMERGE; + /* there isn't chance to merge the splitted bio */ + split->bi_opf |= REQ_NOMERGE; - bio_chain(split, *bio); - trace_block_split(split, (*bio)->bi_iter.bi_sector); - submit_bio_noacct(*bio); - *bio = split; + bio_chain(split, *bio); + trace_block_split(split, (*bio)->bi_iter.bi_sector); + submit_bio_noacct(*bio); + *bio = split; - blk_throtl_charge_bio_split(*bio); - } + blk_throtl_charge_bio_split(*bio); } /** @@ -397,9 +342,10 @@ void __blk_queue_split(struct bio **bio, unsigned int *nr_segs) */ void blk_queue_split(struct bio **bio) { + struct request_queue *q = (*bio)->bi_bdev->bd_disk->queue; unsigned int nr_segs; - __blk_queue_split(bio, &nr_segs); + __blk_queue_split(q, bio, &nr_segs); } EXPORT_SYMBOL(blk_queue_split); diff --git a/block/blk-merge.h b/block/blk-merge.h new file mode 100644 index 000000000000..8f0b7dec2dd2 --- /dev/null +++ b/block/blk-merge.h @@ -0,0 +1,71 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef BLK_MERGE_H +#define BLK_MERGE_H + +#include "blk-mq.h" + +struct bio *blk_bio_discard_split(struct request_queue *q, struct bio *bio, + struct bio_set *bs, unsigned *nsegs); +struct bio *blk_bio_write_zeroes_split(struct request_queue *q, struct bio *bio, + struct bio_set *bs, unsigned *nsegs); +struct bio *blk_bio_write_same_split(struct request_queue *q, struct bio *bio, + struct bio_set *bs, unsigned *nsegs); +struct bio *blk_bio_segment_split(struct request_queue *q, struct bio *bio, + struct bio_set *bs, unsigned *segs); +void blk_bio_handle_split(struct bio **bio, struct bio *split); + +/** + * blk_queue_split - split a bio and submit the second half + * @bio: [in, out] bio to be split + * @nr_segs: [out] number of segments in the first bio + * + * Split a bio into two bios, chain the two bios, submit the second half and + * store a pointer to the first half in *@bio. If the second bio is still too + * big it will be split by a recursive call to this function. Since this + * function may allocate a new bio from q->bio_split, it is the responsibility + * of the caller to ensure that q->bio_split is only released after processing + * of the split bio has finished. + */ +static inline void __blk_queue_split(struct request_queue *q, struct bio **bio, + unsigned int *nr_segs) +{ + struct bio *split = NULL; + + switch (bio_op(*bio)) { + case REQ_OP_DISCARD: + case REQ_OP_SECURE_ERASE: + split = blk_bio_discard_split(q, *bio, &q->bio_split, nr_segs); + break; + case REQ_OP_WRITE_ZEROES: + split = blk_bio_write_zeroes_split(q, *bio, &q->bio_split, + nr_segs); + break; + case REQ_OP_WRITE_SAME: + split = blk_bio_write_same_split(q, *bio, &q->bio_split, + nr_segs); + break; + default: + /* + * All drivers must accept single-segments bios that are <= + * PAGE_SIZE. This is a quick and dirty check that relies on + * the fact that bi_io_vec[0] is always valid if a bio has data. + * The check might lead to occasional false negatives when bios + * are cloned, but compared to the performance impact of cloned + * bios themselves the loop below doesn't matter anyway. + */ + if (!q->limits.chunk_sectors && + (*bio)->bi_vcnt == 1 && + ((*bio)->bi_io_vec[0].bv_len + + (*bio)->bi_io_vec[0].bv_offset) <= PAGE_SIZE) { + *nr_segs = 1; + break; + } + split = blk_bio_segment_split(q, *bio, &q->bio_split, nr_segs); + break; + } + + if (split) + blk_bio_handle_split(bio, split); +} + +#endif diff --git a/block/blk-mq.c b/block/blk-mq.c index 7dadfac5bad0..54c510c0f3ee 100644 --- a/block/blk-mq.c +++ b/block/blk-mq.c @@ -42,6 +42,7 @@ #include "blk-stat.h" #include "blk-mq-sched.h" #include "blk-rq-qos.h" +#include "blk-merge.h" static DEFINE_PER_CPU(struct llist_head, blk_cpu_done); @@ -2514,7 +2515,7 @@ void blk_mq_submit_bio(struct bio *bio) blk_status_t ret; blk_queue_bounce(q, &bio); - __blk_queue_split(&bio, &nr_segs); + __blk_queue_split(q, &bio, &nr_segs); if (!bio_integrity_prep(bio)) goto queue_exit; diff --git a/block/blk.h b/block/blk.h index 0afee3e6a7c1..c0b59bc38a2e 100644 --- a/block/blk.h +++ b/block/blk.h @@ -262,7 +262,6 @@ ssize_t part_timeout_show(struct device *, struct device_attribute *, char *); ssize_t part_timeout_store(struct device *, struct device_attribute *, const char *, size_t); -void __blk_queue_split(struct bio **bio, unsigned int *nr_segs); int ll_back_merge_fn(struct request *req, struct bio *bio, unsigned int nr_segs); bool blk_attempt_req_merge(struct request_queue *q, struct request *rq,
The fast path is no splitting needed. Separate the handling into a check part we can inline, and an out-of-line handling path if we do need to split. Signed-off-by: Jens Axboe <axboe@kernel.dk> ---