On Sun, Apr 7, 2019 at 2:53 PM Christoph Hellwig <hch@xxxxxx> wrote:
>
> The change itself looks fine to be, but a few comments on semingly
> unrelated changes:
>
> > +#define bio_for_each_segment_all(bvl, bio, i, iter_all) \
> > + for (i = 0, bvl = bvec_init_iter_all(&iter_all); \
> > + iter_all.idx < (bio)->bi_vcnt && \
> > + (mp_bvec_advance(&((bio)->bi_io_vec[iter_all.idx]), \
> > + &iter_all), 1); i++)
>
> Instead of the complicated expression inside the for loop test
> expression can't we move the index check into mp_bvec_advance and let
> it return a bool?
OK, will move index check into mp_bvec_advance.
>
> > diff --git a/include/linux/bvec.h b/include/linux/bvec.h
> > index f6275c4da13a..6e4996dfc847 100644
> > --- a/include/linux/bvec.h
> > +++ b/include/linux/bvec.h
> > @@ -48,7 +48,7 @@ struct bvec_iter {
> > struct bvec_iter_all {
> > struct bio_vec bv;
> > int idx;
> > - unsigned done;
> > + unsigned bv_done;
>
> Why the rename here?
'done' may be a bit misleading given we know this field is for recording how
many bytes we have done on the current bvec. Or .bvec_done?
>
> > +static inline void mp_bvec_advance(const struct bio_vec *bvec,
> > + struct bvec_iter_all *iter_all)
>
> If we rename this we should probably drop the mp_ prefix..
OK.
>
> Also not for this patch, but we should really consider moving this
> function out of line given how big it is.
It is fine for me, but I think they shouldn't belong to this fix.
Thanks,
Ming Lei
