| Message ID | e1acd31d91a1e9501a5420d6ac1488a4412a0353.1606240077.git.asml.silence@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | optimise bvec/bio iteration | expand |
On Tue, Nov 24, 2020 at 05:58:13PM +0000, Pavel Begunkov wrote: > __bio_for_each_bvec(), __bio_for_each_segment() and bio_copy_data_iter() > fall under conditions of bvec_iter_advance_single(), which is a faster > and slimmer version of bvec_iter_advance(). Add > bio_advance_iter_single() and convert them. Are you sure about bio_advance_iter()? That API looks like it might not always be limited to a single segment, and might at least need a WARN_ON_ONCE to make sure it is not abused.
On 26/11/2020 10:02, Christoph Hellwig wrote: > On Tue, Nov 24, 2020 at 05:58:13PM +0000, Pavel Begunkov wrote: >> __bio_for_each_bvec(), __bio_for_each_segment() and bio_copy_data_iter() >> fall under conditions of bvec_iter_advance_single(), which is a faster >> and slimmer version of bvec_iter_advance(). Add >> bio_advance_iter_single() and convert them. > > Are you sure about bio_advance_iter()? That API looks like it might Both those listed bio_for_each*() pass bvl.bv_len, which is truncated to current segment by bio_iter_iovec() (i.e. bvec_iter_bvec) or mp_bvec_iter_bvec(). And just to note that I didn't change bio_advance_iter() but added a new function. There is always space for stupid mistakes, but I'm sure. What makes you to think opposite? I may have missed it. > not always be limited to a single segment, and might at least need a > WARN_ON_ONCE to make sure it is not abused. I thought twice about converting other places as you commented before, and it looks saner to not do that exactly for that reason. I prefer to leave *_single() versions for rare but high impact cases like for_each()s. And as it's contained I decided to not add overhead on WARN_ONCE(), e.g. with inlining and w/o string dedup it grows .data section much.
On 26/11/2020 12:32, Pavel Begunkov wrote: > On 26/11/2020 10:02, Christoph Hellwig wrote: >> On Tue, Nov 24, 2020 at 05:58:13PM +0000, Pavel Begunkov wrote: >>> __bio_for_each_bvec(), __bio_for_each_segment() and bio_copy_data_iter() >>> fall under conditions of bvec_iter_advance_single(), which is a faster >>> and slimmer version of bvec_iter_advance(). Add >>> bio_advance_iter_single() and convert them. >> >> Are you sure about bio_advance_iter()? That API looks like it might > > Both those listed bio_for_each*() pass bvl.bv_len, which is truncated to > current segment by bio_iter_iovec() (i.e. bvec_iter_bvec) or > mp_bvec_iter_bvec(). > > And just to note that I didn't change bio_advance_iter() but added a > new function. > There is always space for stupid mistakes, but I'm sure. What makes you > to think opposite? I may have missed it. Christoph, any doubts left? >> not always be limited to a single segment, and might at least need a >> WARN_ON_ONCE to make sure it is not abused. > > I thought twice about converting other places as you commented before, > and it looks saner to not do that exactly for that reason. I prefer > to leave *_single() versions for rare but high impact cases like > for_each()s. > And as it's contained I decided to not add overhead on WARN_ONCE(), > e.g. with inlining and w/o string dedup it grows .data section much. >
On Tue, Nov 24, 2020 at 05:58:13PM +0000, Pavel Begunkov wrote: > __bio_for_each_bvec(), __bio_for_each_segment() and bio_copy_data_iter() > fall under conditions of bvec_iter_advance_single(), which is a faster > and slimmer version of bvec_iter_advance(). Add > bio_advance_iter_single() and convert them. > > Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> Looks good, Reviewed-by: Christoph Hellwig <hch@lst.de>
diff --git a/block/bio.c b/block/bio.c index fa01bef35bb1..8e718920457a 100644 --- a/block/bio.c +++ b/block/bio.c @@ -1212,8 +1212,8 @@ void bio_copy_data_iter(struct bio *dst, struct bvec_iter *dst_iter, flush_dcache_page(dst_bv.bv_page); - bio_advance_iter(src, src_iter, bytes); - bio_advance_iter(dst, dst_iter, bytes); + bio_advance_iter_single(src, src_iter, bytes); + bio_advance_iter_single(dst, dst_iter, bytes); } } EXPORT_SYMBOL(bio_copy_data_iter); diff --git a/include/linux/bio.h b/include/linux/bio.h index c6d765382926..d55d53c49ae4 100644 --- a/include/linux/bio.h +++ b/include/linux/bio.h @@ -148,11 +148,24 @@ static inline void bio_advance_iter(const struct bio *bio, /* TODO: It is reasonable to complete bio with error here. */ } +/* @bytes should be less or equal to bvec[i->bi_idx].bv_len */ +static inline void bio_advance_iter_single(const struct bio *bio, + struct bvec_iter *iter, + unsigned int bytes) +{ + iter->bi_sector += bytes >> 9; + + if (bio_no_advance_iter(bio)) + iter->bi_size -= bytes; + else + bvec_iter_advance_single(bio->bi_io_vec, iter, bytes); +} + #define __bio_for_each_segment(bvl, bio, iter, start) \ for (iter = (start); \ (iter).bi_size && \ ((bvl = bio_iter_iovec((bio), (iter))), 1); \ - bio_advance_iter((bio), &(iter), (bvl).bv_len)) + bio_advance_iter_single((bio), &(iter), (bvl).bv_len)) #define bio_for_each_segment(bvl, bio, iter) \ __bio_for_each_segment(bvl, bio, iter, (bio)->bi_iter) @@ -161,7 +174,7 @@ static inline void bio_advance_iter(const struct bio *bio, for (iter = (start); \ (iter).bi_size && \ ((bvl = mp_bvec_iter_bvec((bio)->bi_io_vec, (iter))), 1); \ - bio_advance_iter((bio), &(iter), (bvl).bv_len)) + bio_advance_iter_single((bio), &(iter), (bvl).bv_len)) /* iterate over multi-page bvec */ #define bio_for_each_bvec(bvl, bio, iter) \
__bio_for_each_bvec(), __bio_for_each_segment() and bio_copy_data_iter() fall under conditions of bvec_iter_advance_single(), which is a faster and slimmer version of bvec_iter_advance(). Add bio_advance_iter_single() and convert them. Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> --- block/bio.c | 4 ++-- include/linux/bio.h | 17 +++++++++++++++-- 2 files changed, 17 insertions(+), 4 deletions(-)