Messages in this thread | | | Date | Mon, 19 Nov 2018 11:31:11 +0800 | From | Ming Lei <> | Subject | Re: [PATCH V10 02/19] block: introduce bio_for_each_bvec() |
| |
On Fri, Nov 16, 2018 at 02:30:28PM +0100, Christoph Hellwig wrote: > > +static inline void __bio_advance_iter(struct bio *bio, struct bvec_iter *iter, > > + unsigned bytes, bool mp) > > I think these magic 'bool np' arguments and wrappers over wrapper > don't help anyone to actually understand the code. I'd vote for > removing as many wrappers as we really don't need, and passing the > actual segment limit instead of the magic bool flag. Something like > this untested patch:
I think this way is fine, just a little comment.
> > diff --git a/include/linux/bio.h b/include/linux/bio.h > index 277921ad42e7..dcad0b69f57a 100644 > --- a/include/linux/bio.h > +++ b/include/linux/bio.h > @@ -138,30 +138,21 @@ static inline bool bio_full(struct bio *bio) > bvec_for_each_segment(bvl, &((bio)->bi_io_vec[iter_all.idx]), i, iter_all) > > static inline void __bio_advance_iter(struct bio *bio, struct bvec_iter *iter, > - unsigned bytes, bool mp) > + unsigned bytes, unsigned max_segment)
The new parameter should have been named as 'max_segment_len' or 'max_seg_len'.
> { > iter->bi_sector += bytes >> 9; > > if (bio_no_advance_iter(bio)) > iter->bi_size -= bytes; > else > - if (!mp) > - bvec_iter_advance(bio->bi_io_vec, iter, bytes); > - else > - mp_bvec_iter_advance(bio->bi_io_vec, iter, bytes); > + __bvec_iter_advance(bio->bi_io_vec, iter, bytes, max_segment); > /* TODO: It is reasonable to complete bio with error here. */ > } > > static inline void bio_advance_iter(struct bio *bio, struct bvec_iter *iter, > unsigned bytes) > { > - __bio_advance_iter(bio, iter, bytes, false); > -} > - > -static inline void bio_advance_mp_iter(struct bio *bio, struct bvec_iter *iter, > - unsigned bytes) > -{ > - __bio_advance_iter(bio, iter, bytes, true); > + __bio_advance_iter(bio, iter, bytes, PAGE_SIZE); > } > > #define __bio_for_each_segment(bvl, bio, iter, start) \ > @@ -177,7 +168,7 @@ static inline void bio_advance_mp_iter(struct bio *bio, struct bvec_iter *iter, > for (iter = (start); \ > (iter).bi_size && \ > ((bvl = bio_iter_mp_iovec((bio), (iter))), 1); \ > - bio_advance_mp_iter((bio), &(iter), (bvl).bv_len)) > + __bio_advance_iter((bio), &(iter), (bvl).bv_len, 0))
Even we might pass '-1' for multi-page segment.
> > /* returns one real segment(multipage bvec) each time */ > #define bio_for_each_bvec(bvl, bio, iter) \ > diff --git a/include/linux/bvec.h b/include/linux/bvec.h > index 02f26d2b59ad..5e2ed46c1c88 100644 > --- a/include/linux/bvec.h > +++ b/include/linux/bvec.h > @@ -138,8 +138,7 @@ struct bvec_iter_all { > }) > > static inline bool __bvec_iter_advance(const struct bio_vec *bv, > - struct bvec_iter *iter, > - unsigned bytes, bool mp) > + struct bvec_iter *iter, unsigned bytes, unsigned max_segment) > { > if (WARN_ONCE(bytes > iter->bi_size, > "Attempted to advance past end of bvec iter\n")) { > @@ -148,18 +147,18 @@ static inline bool __bvec_iter_advance(const struct bio_vec *bv, > } > > while (bytes) { > - unsigned len; > + unsigned segment_len = mp_bvec_iter_len(bv, *iter); > > - if (mp) > - len = mp_bvec_iter_len(bv, *iter); > - else > - len = bvec_iter_len(bv, *iter); > + if (max_segment) { > + max_segment -= bvec_iter_offset(bv, *iter); > + segment_len = min(segment_len, max_segment);
Looks 'max_segment' needs to be constant, shouldn't be updated.
If '-1' is passed for multipage case, the above change may become:
segment_len = min_t(segment_len, max_seg_len - bvec_iter_offset(bv, *iter));
This way is more clean, but with extra cost of the above line for multipage case.
Thanks, Ming
| |