[V2] block: don't use for-inside-for in bio_for_each_segment_all
diff mbox series

Message ID 20190407085325.9380-1-ming.lei@redhat.com
State New
Headers show
Series
  • [V2] block: don't use for-inside-for in bio_for_each_segment_all
Related show

Commit Message

Ming Lei April 7, 2019, 8:53 a.m. UTC
Commit 6dc4f100c175 ("block: allow bio_for_each_segment_all() to
iterate over multi-page bvec") changes bio_for_each_segment_all()
to use for-inside-for.

This way breaks all bio_for_each_segment_all() call with error out
branch via 'break', since now 'break' can only break from the inner
loop.

Fixes this issue by implementing bio_for_each_segment_all() via
single 'for' loop, and now the logic is very similar with normal
bvec iterator.

Cc: Qu Wenruo <quwenruo.btrfs@gmx.com>
Cc: linux-btrfs@vger.kernel.org
Cc: linux-fsdevel@vger.kernel.org
Cc: Omar Sandoval <osandov@fb.com>
Cc: Christoph Hellwig <hch@lst.de>
Reported-and-Tested-by: Qu Wenruo <quwenruo.btrfs@gmx.com>
Fixes: 6dc4f100c175 ("block: allow bio_for_each_segment_all() to iterate over multi-page bvec")
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 include/linux/bio.h  | 20 ++++++++++++--------
 include/linux/bvec.h | 23 +++++++++++++++--------
 2 files changed, 27 insertions(+), 16 deletions(-)

Comments

Christoph Hellwig April 8, 2019, 6:16 a.m. UTC | #1
Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>
Christoph Hellwig April 8, 2019, 8:33 a.m. UTC | #2
On Mon, Apr 08, 2019 at 08:16:04AM +0200, Christoph Hellwig wrote:
> Looks good:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Actually..  I just noticed the gratious done rename creates a (trivial)
conflict in a series I was about to submit for 5.2.  Can you respin
it without the rename to make everyones life a littler easier?
Ming Lei April 8, 2019, 8:46 a.m. UTC | #3
On Mon, Apr 8, 2019 at 4:34 PM Christoph Hellwig <hch@lst.de> wrote:
>
> On Mon, Apr 08, 2019 at 08:16:04AM +0200, Christoph Hellwig wrote:
> > Looks good:
> >
> > Reviewed-by: Christoph Hellwig <hch@lst.de>
>
> Actually..  I just noticed the gratious done rename creates a (trivial)
> conflict in a series I was about to submit for 5.2.  Can you respin
> it without the rename to make everyones life a littler easier?

Yeah, no problem.

You mean the 'done->bvec_done' rename?


Thanks,
Ming Lei
Christoph Hellwig April 8, 2019, 10:47 a.m. UTC | #4
On Mon, Apr 08, 2019 at 04:46:33PM +0800, Ming Lei wrote:
> > Actually..  I just noticed the gratious done rename creates a (trivial)
> > conflict in a series I was about to submit for 5.2.  Can you respin
> > it without the rename to make everyones life a littler easier?
> 
> Yeah, no problem.
> 
> You mean the 'done->bvec_done' rename?

Yes.

Patch
diff mbox series

diff --git a/include/linux/bio.h b/include/linux/bio.h
index bb6090aa165d..9c583bf98ff9 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -120,19 +120,23 @@  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(const struct bio *bio,
+					 struct bvec_iter_all *iter)
+{
+	if (iter->idx >= bio->bi_vcnt)
+		return false;
+
+	bvec_advance(&bio->bi_io_vec[iter->idx], iter);
+	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)			\
+	for (i = 0, bvl = bvec_init_iter_all(&iter);			\
+	     bio_next_segment((bio), &iter); 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..67eda310b643 100644
--- a/include/linux/bvec.h
+++ b/include/linux/bvec.h
@@ -47,8 +47,9 @@  struct bvec_iter {
 
 struct bvec_iter_all {
 	struct bio_vec	bv;
-	int		idx;
-	unsigned	done;
+	int		idx;		/* current index into bvec table */
+	unsigned	bvec_done;	/* number of bytes completed in
+					   currnet bvec	*/
 };
 
 static inline struct page *bvec_nth_page(struct page *page, int idx)
@@ -145,18 +146,18 @@  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->bvec_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)
+static inline void bvec_advance(const struct bio_vec *bvec,
+		struct bvec_iter_all *iter_all)
 {
 	struct bio_vec *bv = &iter_all->bv;
 
-	if (bv->bv_page) {
+	if (iter_all->bvec_done) {
 		bv->bv_page = nth_page(bv->bv_page, 1);
 		bv->bv_offset = 0;
 	} else {
@@ -164,7 +165,13 @@  static inline void mp_bvec_next_segment(const struct bio_vec *bvec,
 		bv->bv_offset = bvec->bv_offset;
 	}
 	bv->bv_len = min_t(unsigned int, PAGE_SIZE - bv->bv_offset,
-			   bvec->bv_len - iter_all->done);
+			   bvec->bv_len - iter_all->bvec_done);
+	iter_all->bvec_done += bv->bv_len;
+
+	if (iter_all->bvec_done == bvec->bv_len) {
+		iter_all->idx++;
+		iter_all->bvec_done = 0;
+	}
 }
 
 /*