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

[Christoph Hellwig <hch@xxxxxx>]

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?

> 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?

> +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..

Also not for this patch, but we should really consider moving this
function out of line given how big it is.


Here is how I'd do this with my slight changes:


diff --git a/include/linux/bio.h b/include/linux/bio.h
index bb915591557b..fd7629ac9f11 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -120,19 +120,42 @@ static inline bool bio_full(struct bio *bio)
 	return bio->bi_vcnt >= bio->bi_max_vecs;
 }
 
-#define mp_bvec_for_each_segment(bv, bvl, i, iter_all)			\
-	for (bv = bvec_init_iter_all(&iter_all);			\
-		(iter_all.done < (bvl)->bv_len) &&			\
-		(mp_bvec_next_segment((bvl), &iter_all), 1);		\
-		iter_all.done += bv->bv_len, i += 1)
+static inline bool __bio_next_segment_all(struct bio *bio,
+		struct bvec_iter_all *iter_all)
+{
+	struct bio_vec *bvec = &bio->bi_io_vec[iter_all->idx];
+	struct bio_vec *bv = &iter_all->bv;
+
+	if (iter_all->idx >= bio->bi_vcnt)
+		return false;
+
+	if (iter_all->done) {
+		bv->bv_page = nth_page(bv->bv_page, 1);
+		bv->bv_offset = 0;
+	} else {
+		bv->bv_page = bvec->bv_page;
+		bv->bv_offset = bvec->bv_offset;
+	}
+	bv->bv_len = min_t(unsigned int, PAGE_SIZE - bv->bv_offset,
+			   bvec->bv_len - iter_all->done);
+	iter_all->done += bv->bv_len;
+
+	if (iter_all->done == bvec->bv_len) {
+		iter_all->idx++;
+		iter_all->done = 0;
+	}
+
+	return true;
+}
 
 /*
  * drivers should _never_ use the all version - the bio may have been split
  * before it got to the driver and the driver won't own all of it
  */
-#define bio_for_each_segment_all(bvl, bio, i, iter_all)		\
-	for (i = 0, iter_all.idx = 0; iter_all.idx < (bio)->bi_vcnt; iter_all.idx++)	\
-		mp_bvec_for_each_segment(bvl, &((bio)->bi_io_vec[iter_all.idx]), i, iter_all)
+#define bio_for_each_segment_all(bvl, bio, i, iter_all)			\
+	for (i = 0, bvl = bvec_init_iter_all(&iter_all);		\
+	     __bio_next_segment_all(bio, &iter_all);			\
+	     i++)
 
 static inline void bio_advance_iter(struct bio *bio, struct bvec_iter *iter,
 				    unsigned bytes)
diff --git a/include/linux/bvec.h b/include/linux/bvec.h
index f6275c4da13a..bcebfc522498 100644
--- a/include/linux/bvec.h
+++ b/include/linux/bvec.h
@@ -145,28 +145,12 @@ static inline bool bvec_iter_advance(const struct bio_vec *bv,
 
 static inline struct bio_vec *bvec_init_iter_all(struct bvec_iter_all *iter_all)
 {
-	iter_all->bv.bv_page = NULL;
 	iter_all->done = 0;
+	iter_all->idx = 0;
 
 	return &iter_all->bv;
 }
 
-static inline void mp_bvec_next_segment(const struct bio_vec *bvec,
-					struct bvec_iter_all *iter_all)
-{
-	struct bio_vec *bv = &iter_all->bv;
-
-	if (bv->bv_page) {
-		bv->bv_page = nth_page(bv->bv_page, 1);
-		bv->bv_offset = 0;
-	} else {
-		bv->bv_page = bvec->bv_page;
-		bv->bv_offset = bvec->bv_offset;
-	}
-	bv->bv_len = min_t(unsigned int, PAGE_SIZE - bv->bv_offset,
-			   bvec->bv_len - iter_all->done);
-}
-
 /*
  * Get the last single-page segment from the multi-page bvec and store it
  * in @seg