Re: [PATCH] block: don't use for-inside-for in bio_for_each_segment_all

[Ming Lei <tom.leiming@gmail.com>]

On Sun, Apr 7, 2019 at 2:53 PM Christoph Hellwig <hch@lst.de> 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