optimise split bio
cover optimise split bio
With the no-copy bvec patches and nullb eats blk_bio_segment_split() eats >7%. It adds yet another fast path for it.
8K 16K 32K 64K before: 932 904 868 788 after: 934 919 902 862
Would appreciate if anyone knows off the bat typical queue_max_segments, etc. numbers for NVMe.
Pavel Begunkov (2):
block: add a function for *segment_split fast path
block: add a fast path for seg split of large bio
block/blk-merge.c | 107 +++++++++++++++++++++++++++++----------------- 1 file changed, 68 insertions(+), 39 deletions(-)
Signature · 1 line
2.24.0
RFC 1/2 block: add a function for *segment_split fast path
Don't keep blk_bio_segment_split()'s fast path hand coded in __blk_queue_split(), extract it into a function. It's inlined perfectly well.
--- block/blk-merge.c | 84 ++++++++++++++++++++++++++--------------------- 1 file changed, 46 insertions(+), 38 deletions(-)
block/blk-merge.c +46 -38
@@ -223,29 +223,10 @@ static bool bvec_split_segs(const struct request_queue *q,return len >0 || bv->bv_len > max_len; } -/** -* blk_bio_segment_split - split a bio in two bios -* @q: [in] request queue pointer -* @bio: [in] bio to be split -* @bs: [in] bio set to allocate the clone from -* @segs: [out] number of segments in the bio with the first half of the sectors -* -* Clone @bio, update the bi_iter of the clone to represent the first sectors -* of @bio and update @bio->bi_iter to represent the remaining sectors. The -* following is guaranteed for the cloned bio: -* - That it has at most get_max_io_size(@q, @bio) sectors. -* - That it has at most queue_max_segments(@q) segments. -* -* Except for discard requests the cloned bio will point at the bi_io_vec of -* the original bio. It is the responsibility of the caller to ensure that the -* original bio is not freed before the cloned bio. The caller is also -* 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) +static 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;
@@ -290,6 +271,48 @@ static struct bio *blk_bio_segment_split(struct request_queue *q,return bio_split (bio, sectors,GFP_NOIO , bs); } +/** +* blk_bio_segment_split - split a bio in two bios +* @q: [in] request queue pointer +* @bio: [in] bio to be split +* @bs: [in] bio set to allocate the clone from +* @segs: [out] number of segments in the bio with the first half of the sectors +* +* Clone @bio, update the bi_iter of the clone to represent the first sectors +* of @bio and update @bio->bi_iter to represent the remaining sectors. The +* following is guaranteed for the cloned bio: +* - That it has at most get_max_io_size(@q, @bio) sectors. +* - That it has at most queue_max_segments(@q) segments. +* +* Except for discard requests the cloned bio will point at the bi_io_vec of +* the original bio. It is the responsibility of the caller to ensure that the +* original bio is not freed before the cloned bio. The caller is also +* responsible for ensuring that @bs is only destroyed after processing of the +* split bio has finished. +*/ +static inline struct bio *blk_bio_segment_split (struct request_queue *q, +struct bio *bio, +struct bio_set *bs, +unsigned *nr_segs) +{ +/* +* 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 ; +return NULL ; + } + +return __blk_bio_segment_split (q, bio, bs, nr_segs); +} +/** * __blk_queue_split - split a bio and submit the second half * @bio: [in, out] bio to be split
@@ -322,21 +345,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 ; }
Signature · 1 line
2.24.0
RFC 2/2 block: add a fast path for seg split of large bio
In reply to Pavel Begunkovblk_bio_segment_split() is very heavy, but the current fast path covers only one-segment under PAGE_SIZE bios. Add another one by estimating an upper bound of sectors a bio can contain.
One restricting factor here is queue_max_segment_size(), which it compare against full iter size to not dig into bvecs. By default it's 64KB, and so for requests under 64KB, but for those falling under the conditions it's much faster.
--- block/blk-merge.c | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-)
block/blk-merge.c +25 -4
@@ -226,12 +226,12 @@ static bool bvec_split_segs(const struct request_queue *q,static struct bio *__blk_bio_segment_split (struct request_queue *q,struct bio *bio,struct bio_set *bs, -unsigned *segs) +unsigned *segs, +const unsigned max_sectors) {struct bio_vec bv, bvprv, *bvprvp =NULL ;struct bvec_iter iter;unsigned nsegs =0 , sectors =0 ; -const unsigned max_sectors =get_max_io_size (q, bio);const unsigned max_segs =queue_max_segments (q);bio_for_each_bvec (bv, bio, iter) {
@@ -295,6 +295,9 @@ static inline struct bio *blk_bio_segment_split(struct request_queue *q,struct bio_set *bs, unsigned *nr_segs) { + unsigned int max_sectors, q_max_sectors; +unsigned int bio_segs = bio->bi_vcnt; +/* * All drivers must accept single-segments bios that are <= * PAGE_SIZE. This is a quick and dirty check that relies on
@@ -303,14 +306,32 @@ static inline struct bio *blk_bio_segment_split(struct request_queue *q,* 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 && +if (!q->limits.chunk_sectors && bio_segs ==1 && (bio->bi_io_vec[0 ].bv_len + bio->bi_io_vec[0 ].bv_offset) <=PAGE_SIZE ) { *nr_segs =1 ;return NULL ; } -return __blk_bio_segment_split (q, bio, bs, nr_segs); + q_max_sectors =get_max_io_size (q, bio); +if (!queue_virt_boundary (q) && bio_segs <queue_max_segments (q) && + bio->bi_iter.bi_size <=queue_max_segment_size (q)) { +/* +* Segments are contiguous, so only their ends may be not full. +* An upper bound for them would to assume that each takes 1B +* but adds a sector, and all left are just full sectors. +* Note: it's ok to round size down because all not full +* sectors are accounted by the first term. +*/ + max_sectors = bio_segs *2 ; + max_sectors += bio->bi_iter.bi_size >>9 ; + +if (max_sectors < q_max_sectors) { + *nr_segs = bio_segs; +return NULL ; + } + } +return __blk_bio_segment_split (q, bio, bs, nr_segs, q_max_sectors); }/**
Signature · 1 line
2.24.0
Re: [RFC 2/2] block: add a fast path for seg split of large bio
Quoted 14 lines · Pavel Begunkov
blk_bio_segment_split()is very heavy, but the current fast path covers only one-segment underPAGE_SIZEbios. Add another one by estimating an upper bound of sectors a bio can contain.One restricting factor here is
queue_max_segment_size(), which it compare against full iter size to not dig into bvecs. By default it's 64KB, and so for requests under 64KB, but for those falling under the conditions it's much faster.Signed-off-by Pavel Begunkov <[email protected]>--- block/blk-merge.c | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-)
[...]
block/blk-merge.c static inline struct bio *blk_bio_segment_split(struct request_queue *q, · line 318
Quoted 5 lines
block/blk-merge.c +3
-1-return __blk_bio_segment_split (q, bio, bs, nr_segs); + q_max_sectors =get_max_io_size (q, bio); +if (!queue_virt_boundary (q) && bio_segs <queue_max_segments (q) && + bio->bi_iter.bi_size <=queue_max_segment_size (q)) {
I think I miss a seg_boundary_mask check here. Any insights how to skip it? Or why it's 2^31-1 by default, but not say ((unsigned long)-1)?
Signature · 1 line
Pavel Begunkov
Re: [RFC 2/2] block: add a fast path for seg split of large bio
In reply to Pavel BegunkovQuoted 8 lines · Pavel Begunkov
blk_bio_segment_split()is very heavy, but the current fast path covers only one-segment underPAGE_SIZEbios. Add another one by estimating an upper bound of sectors a bio can contain.One restricting factor here is
queue_max_segment_size(), which it compare against full iter size to not dig into bvecs. By default it's 64KB, and so for requests under 64KB, but for those falling under the conditions it's much faster.
I think this works, but it is a pretty gross heuristic, which also doesn't help us with NVMe, which is the I/O fast path of choice for most people. What is your use/test case?
block/blk-merge.c static inline struct bio *blk_bio_segment_split(struct request_queue *q, · line 329
Quoted 11 lines
block/blk-merge.c +11
-0+/* +* Segments are contiguous, so only their ends may be not full. +* An upper bound for them would to assume that each takes 1B +* but adds a sector, and all left are just full sectors. +* Note: it's ok to round size down because all not full +* sectors are accounted by the first term. +*/ + max_sectors = bio_segs *2 ; + max_sectors += bio->bi_iter.bi_size >>9 ; + +if (max_sectors < q_max_sectors) {
I don't think we need the max_sectors variable here.
Re: [RFC 2/2] block: add a fast path for seg split of large bio
Quoted 11 lines · Christoph Hellwig
Quoted 8 lines · Pavel Begunkov
blk_bio_segment_split()is very heavy, but the current fast path covers only one-segment underPAGE_SIZEbios. Add another one by estimating an upper bound of sectors a bio can contain.One restricting factor here is
queue_max_segment_size(), which it compare against full iter size to not dig into bvecs. By default it's 64KB, and so for requests under 64KB, but for those falling under the conditions it's much faster.I think this works, but it is a pretty gross heuristic, which also
bio->bi_iter.bi_size <= queue_max_segment_size(q)
Do you mean this, right? I wouldn't say it's gross, but _very_ loose.
Quoted 2 lines
doesn't help us with NVMe, which is the I/O fast path of choice for most people. What is your use/test case?
Yeah, the idea to make it work for NVMe. Don't remember it restricting segment size or whatever, maybe only atomicity. Which condition do you see problematic? I can't recall without opening the spec.
Quoted 14 lines
block/blk-merge.c static inline struct bio *blk_bio_segment_split(struct request_queue *q, · line 329
Quoted 12 lines
block/blk-merge.c +11
-0+/* +* Segments are contiguous, so only their ends may be not full. +* An upper bound for them would to assume that each takes 1B +* but adds a sector, and all left are just full sectors. +* Note: it's ok to round size down because all not full +* sectors are accounted by the first term. +*/ + max_sectors = bio_segs *2 ; + max_sectors += bio->bi_iter.bi_size >>9 ; + +if (max_sectors < q_max_sectors) {I don't think we need the
max_sectorsvariable here.
Even more, considering that it's already sector aligned we can kill off all this section and just use (bi_iter.bi_size >> 9).
Signature · 1 line
Pavel Begunkov
Re: [RFC 2/2] block: add a fast path for seg split of large bio
In reply to Pavel BegunkovQuoted 49 lines · Pavel Begunkov
blk_bio_segment_split()is very heavy, but the current fast path covers only one-segment underPAGE_SIZEbios. Add another one by estimating an upper bound of sectors a bio can contain.One restricting factor here is
queue_max_segment_size(), which it compare against full iter size to not dig into bvecs. By default it's 64KB, and so for requests under 64KB, but for those falling under the conditions it's much faster.Signed-off-by Pavel Begunkov <[email protected]>--- block/blk-merge.c | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-)block/blk-merge.c +6
-3@@ -226,12 +226,12 @@ static bool bvec_split_segs(const struct request_queue *q,static struct bio *__blk_bio_segment_split (struct request_queue *q,struct bio *bio,struct bio_set *bs, -unsigned *segs) +unsigned *segs, +const unsigned max_sectors) {struct bio_vec bv, bvprv, *bvprvp =NULL ;struct bvec_iter iter;unsigned nsegs =0 , sectors =0 ; -const unsigned max_sectors =get_max_io_size (q, bio);const unsigned max_segs =queue_max_segments (q);bio_for_each_bvec (bv, bio, iter) {@@ -295,6 +295,9 @@ static inline struct bio *blk_bio_segment_split(struct request_queue *q,struct bio_set *bs, unsigned *nr_segs) { + unsigned int max_sectors, q_max_sectors; +unsigned int bio_segs = bio->bi_vcnt; +/* * All drivers must accept single-segments bios that are <= * PAGE_SIZE. This is a quick and dirty check that relies on @@ -303,14 +306,32 @@ static inline struct bio *blk_bio_segment_split(struct request_queue *q,* 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 && + if (!q->limits.chunk_sectors && bio_segs == 1 &&
block/blk-merge.c static inline struct bio *blk_bio_segment_split(struct request_queue *q, · line 318
Quoted 10 lines
block/blk-merge.c +3
-1(bio->bi_io_vec[0 ].bv_len + bio->bi_io_vec[0].bv_offset) <= PAGE_SIZE) { *nr_segs =1 ;return NULL ; } -return __blk_bio_segment_split (q, bio, bs, nr_segs); + q_max_sectors =get_max_io_size (q, bio); +if (!queue_virt_boundary (q) && bio_segs <queue_max_segments (q) && + bio->bi_iter.bi_size <=queue_max_segment_size (q)) {
.bi_vcnt is 0 for fast cloned bio, so the above check may become true when real nr_segment is > queue_max_segments(), especially in case that max segments limit is small and segment size is big.
Signature · 1 line
Ming
Re: [RFC 2/2] block: add a fast path for seg split of large bio
Quoted 56 lines · Ming Lei
Quoted 55 lines · Pavel Begunkov
blk_bio_segment_split()is very heavy, but the current fast path covers only one-segment underPAGE_SIZEbios. Add another one by estimating an upper bound of sectors a bio can contain.One restricting factor here is
queue_max_segment_size(), which it compare against full iter size to not dig into bvecs. By default it's 64KB, and so for requests under 64KB, but for those falling under the conditions it's much faster.Signed-off-by Pavel Begunkov <[email protected]>--- block/blk-merge.c | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-)block/blk-merge.c +6
-3@@ -226,12 +226,12 @@ static bool bvec_split_segs(const struct request_queue *q,static struct bio *__blk_bio_segment_split (struct request_queue *q,struct bio *bio,struct bio_set *bs, -unsigned *segs) +unsigned *segs, +const unsigned max_sectors) {struct bio_vec bv, bvprv, *bvprvp =NULL ;struct bvec_iter iter;unsigned nsegs =0 , sectors =0 ; -const unsigned max_sectors =get_max_io_size (q, bio);const unsigned max_segs =queue_max_segments (q);bio_for_each_bvec (bv, bio, iter) {@@ -295,6 +295,9 @@ static inline struct bio *blk_bio_segment_split(struct request_queue *q,struct bio_set *bs, unsigned *nr_segs) { + unsigned int max_sectors, q_max_sectors; +unsigned int bio_segs = bio->bi_vcnt; +/* * All drivers must accept single-segments bios that are <= * PAGE_SIZE. This is a quick and dirty check that relies on @@ -303,14 +306,32 @@ static inline struct bio *blk_bio_segment_split(struct request_queue *q,* 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 && +if (!q->limits.chunk_sectors && bio_segs ==1 && (bio->bi_io_vec[0 ].bv_len + bio->bi_io_vec[0 ].bv_offset) <=PAGE_SIZE ) { *nr_segs =1 ;return NULL ; }
Quoted 8 lines
block/blk-merge.c static inline struct bio *blk_bio_segment_split(struct request_queue *q, · line 318
Quoted 4 lines
block/blk-merge.c +3
-1-return __blk_bio_segment_split (q, bio, bs, nr_segs); + q_max_sectors =get_max_io_size (q, bio); +if (!queue_virt_boundary (q) && bio_segs <queue_max_segments (q) && + bio->bi_iter.bi_size <=queue_max_segment_size (q)) {.bi_vcnt is 0 for fast cloned bio, so the above check may become true when real
nr_segmentis >queue_max_segments(), especially in case that max segments limit is small and segment size is big.
I guess we can skip the fast path for them (i.e. bi_vcnt == 0)
I'm curious, why it's 0 but not the real number?
Signature · 1 line
Pavel Begunkov
Re: [RFC 2/2] block: add a fast path for seg split of large bio
Quoted 66 lines · Pavel Begunkov
Quoted 56 lines · Ming Lei
Quoted 55 lines · Pavel Begunkov
blk_bio_segment_split()is very heavy, but the current fast path covers only one-segment underPAGE_SIZEbios. Add another one by estimating an upper bound of sectors a bio can contain.One restricting factor here is
queue_max_segment_size(), which it compare against full iter size to not dig into bvecs. By default it's 64KB, and so for requests under 64KB, but for those falling under the conditions it's much faster.Signed-off-by Pavel Begunkov <[email protected]>--- block/blk-merge.c | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-)block/blk-merge.c +6
-3@@ -226,12 +226,12 @@ static bool bvec_split_segs(const struct request_queue *q,static struct bio *__blk_bio_segment_split (struct request_queue *q,struct bio *bio,struct bio_set *bs, -unsigned *segs) +unsigned *segs, +const unsigned max_sectors) {struct bio_vec bv, bvprv, *bvprvp =NULL ;struct bvec_iter iter;unsigned nsegs =0 , sectors =0 ; -const unsigned max_sectors =get_max_io_size (q, bio);const unsigned max_segs =queue_max_segments (q);bio_for_each_bvec (bv, bio, iter) {@@ -295,6 +295,9 @@ static inline struct bio *blk_bio_segment_split(struct request_queue *q,struct bio_set *bs, unsigned *nr_segs) { + unsigned int max_sectors, q_max_sectors; +unsigned int bio_segs = bio->bi_vcnt; +/* * All drivers must accept single-segments bios that are <= * PAGE_SIZE. This is a quick and dirty check that relies on @@ -303,14 +306,32 @@ static inline struct bio *blk_bio_segment_split(struct request_queue *q,* 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 && +if (!q->limits.chunk_sectors && bio_segs ==1 && (bio->bi_io_vec[0 ].bv_len + bio->bi_io_vec[0 ].bv_offset) <=PAGE_SIZE ) { *nr_segs =1 ;return NULL ; }Quoted 8 lines
block/blk-merge.c static inline struct bio *blk_bio_segment_split(struct request_queue *q, · line 318
Quoted 4 lines
block/blk-merge.c +3
-1-return __blk_bio_segment_split (q, bio, bs, nr_segs); + q_max_sectors =get_max_io_size (q, bio); +if (!queue_virt_boundary (q) && bio_segs <queue_max_segments (q) && + bio->bi_iter.bi_size <=queue_max_segment_size (q)) {.bi_vcnt is 0 for fast cloned bio, so the above check may become true when real
nr_segmentis >queue_max_segments(), especially in case that max segments limit is small and segment size is big.
Quoted 1 line
I guess we can skip the fast path for them (i.e.
bi_vcnt== 0)
But bi_vcnt can't represent real segment number, which can be bigger or less than .bi_vcnt.
Quoted 1 line
I'm curious, why it's 0 but not the real number?
fast-cloned bio shares bvec table of original bio, so it doesn't have .bi_vcnt.
Signature · 1 line
Ming
Re: [RFC 2/2] block: add a fast path for seg split of large bio
Quoted 11 lines · Ming Lei
Quoted 7 lines · Pavel Begunkov
Quoted 4 lines · Ming Lei
On Tue, Jan 05, 2021 at 07:43:38PM +0000, Pavel Begunkov wrote:
.bi_vcnt is 0 for fast cloned bio, so the above check may become true when realnr_segmentis >queue_max_segments(), especially in case that max segments limit is small and segment size is big.I guess we can skip the fast path for them (i.e.
bi_vcnt== 0)But
bi_vcntcan't represent real segment number, which can be bigger or less than .bi_vcnt.
Sounds like "go slow path on bi_vcnt==0" won't work. Ok, I need to go look what is the real purpose of .bi_vcnt
Quoted 5 lines
Quoted 2 lines
I'm curious, why it's 0 but not the real number?
fast-cloned bio shares bvec table of original bio, so it doesn't have .bi_vcnt.
Got it, thanks
Signature · 1 line
Pavel Begunkov