diff mbox series

[5/7] block: Rework bio_for_each_folio_all()

Message ID 20230525214822.2725616-6-kent.overstreet@linux.dev (mailing list archive)
State Under Review
Headers show
Series block layer patches for bcachefs | expand

Commit Message

Kent Overstreet May 25, 2023, 9:48 p.m. UTC
This reimplements bio_for_each_folio_all() on top of the newly-reworked
bvec_iter_all, and since it's now trivial we also provide
bio_for_each_folio.

Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: linux-block@vger.kernel.org
---
 fs/crypto/bio.c        |  9 +++--
 fs/iomap/buffered-io.c | 14 ++++---
 fs/verity/verify.c     |  9 +++--
 include/linux/bio.h    | 91 +++++++++++++++++++++---------------------
 include/linux/bvec.h   | 15 +++++--
 5 files changed, 75 insertions(+), 63 deletions(-)

Comments

Dave Chinner May 26, 2023, 12:36 a.m. UTC | #1
On Thu, May 25, 2023 at 05:48:20PM -0400, Kent Overstreet wrote:
> This reimplements bio_for_each_folio_all() on top of the newly-reworked
> bvec_iter_all, and since it's now trivial we also provide
> bio_for_each_folio.
> 
> Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>
> Cc: Matthew Wilcox <willy@infradead.org>
> Cc: linux-block@vger.kernel.org
> ---
>  fs/crypto/bio.c        |  9 +++--
>  fs/iomap/buffered-io.c | 14 ++++---
>  fs/verity/verify.c     |  9 +++--
>  include/linux/bio.h    | 91 +++++++++++++++++++++---------------------
>  include/linux/bvec.h   | 15 +++++--
>  5 files changed, 75 insertions(+), 63 deletions(-)
....
> diff --git a/include/linux/bio.h b/include/linux/bio.h
> index f86c7190c3..7ced281734 100644
> --- a/include/linux/bio.h
> +++ b/include/linux/bio.h
> @@ -169,6 +169,42 @@ static inline void bio_advance(struct bio *bio, unsigned int nbytes)
>  #define bio_for_each_segment(bvl, bio, iter)				\
>  	__bio_for_each_segment(bvl, bio, iter, (bio)->bi_iter)
>  
> +struct folio_vec {
> +	struct folio	*fv_folio;
> +	size_t		fv_offset;
> +	size_t		fv_len;
> +};

Can we drop the "fv_" variable prefix here? It's just unnecessary
verbosity when we know we have a folio_vec structure. i.e fv->folio
is easier to read and type than fv->fv_folio...

Hmmm, this is probably not a good name considering "struct pagevec" is
something completely different - the equivalent is "struct
folio_batch" but I can see this being confusing for people who
largely expect some symmetry between page<->folio naming
conventions...

Also, why is this in bio.h and not in a mm/folio related header
file?

Cheers,

Dave.
Kent Overstreet May 26, 2023, 12:50 a.m. UTC | #2
On Fri, May 26, 2023 at 10:36:03AM +1000, Dave Chinner wrote:
> On Thu, May 25, 2023 at 05:48:20PM -0400, Kent Overstreet wrote:
> > This reimplements bio_for_each_folio_all() on top of the newly-reworked
> > bvec_iter_all, and since it's now trivial we also provide
> > bio_for_each_folio.
> > 
> > Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>
> > Cc: Matthew Wilcox <willy@infradead.org>
> > Cc: linux-block@vger.kernel.org
> > ---
> >  fs/crypto/bio.c        |  9 +++--
> >  fs/iomap/buffered-io.c | 14 ++++---
> >  fs/verity/verify.c     |  9 +++--
> >  include/linux/bio.h    | 91 +++++++++++++++++++++---------------------
> >  include/linux/bvec.h   | 15 +++++--
> >  5 files changed, 75 insertions(+), 63 deletions(-)
> ....
> > diff --git a/include/linux/bio.h b/include/linux/bio.h
> > index f86c7190c3..7ced281734 100644
> > --- a/include/linux/bio.h
> > +++ b/include/linux/bio.h
> > @@ -169,6 +169,42 @@ static inline void bio_advance(struct bio *bio, unsigned int nbytes)
> >  #define bio_for_each_segment(bvl, bio, iter)				\
> >  	__bio_for_each_segment(bvl, bio, iter, (bio)->bi_iter)
> >  
> > +struct folio_vec {
> > +	struct folio	*fv_folio;
> > +	size_t		fv_offset;
> > +	size_t		fv_len;
> > +};
> 
> Can we drop the "fv_" variable prefix here? It's just unnecessary
> verbosity when we know we have a folio_vec structure. i.e fv->folio
> is easier to read and type than fv->fv_folio...

That's actually one of the things I like about bio/biovec, it's been
handy in the past for grepping and block layer refactorings...

(I would _kill_ for a tool that let me do that kind of type-aware grep.
ctags can in theory produce that kind of an index but I never figured
out how to get vim to use it properly. I believe the lsp-server stuff
that uses the compiler as a backend can do it; I've started using that
stuff for Rust coding and it works amazingly, don't think I've tried it
for struct members - I wonder if that stuff works at all on a codebase
the size of the kernel or just dies...)

> Hmmm, this is probably not a good name considering "struct pagevec" is
> something completely different - the equivalent is "struct
> folio_batch" but I can see this being confusing for people who
> largely expect some symmetry between page<->folio naming
> conventions...

Yeah, good point. folio_seg, perhaps?

(I think Matthew may have already made that suggestion...)

> Also, why is this in bio.h and not in a mm/folio related header
> file?

Is it worth moving it there considering it's only used in bio.h/bvec.h?
Perhaps we could keep it where it's used for now and move it if it gains
more users?
diff mbox series

Patch

diff --git a/fs/crypto/bio.c b/fs/crypto/bio.c
index d57d0a020f..6469861add 100644
--- a/fs/crypto/bio.c
+++ b/fs/crypto/bio.c
@@ -30,11 +30,12 @@ 
  */
 bool fscrypt_decrypt_bio(struct bio *bio)
 {
-	struct folio_iter fi;
+	struct bvec_iter_all iter;
+	struct folio_vec fv;
 
-	bio_for_each_folio_all(fi, bio) {
-		int err = fscrypt_decrypt_pagecache_blocks(fi.folio, fi.length,
-							   fi.offset);
+	bio_for_each_folio_all(fv, bio, iter) {
+		int err = fscrypt_decrypt_pagecache_blocks(fv.fv_folio, fv.fv_len,
+							   fv.fv_offset);
 
 		if (err) {
 			bio->bi_status = errno_to_blk_status(err);
diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index 6f4c97a6d7..60661c87d5 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -187,10 +187,11 @@  static void iomap_finish_folio_read(struct folio *folio, size_t offset,
 static void iomap_read_end_io(struct bio *bio)
 {
 	int error = blk_status_to_errno(bio->bi_status);
-	struct folio_iter fi;
+	struct bvec_iter_all iter;
+	struct folio_vec fv;
 
-	bio_for_each_folio_all(fi, bio)
-		iomap_finish_folio_read(fi.folio, fi.offset, fi.length, error);
+	bio_for_each_folio_all(fv, bio, iter)
+		iomap_finish_folio_read(fv.fv_folio, fv.fv_offset, fv.fv_len, error);
 	bio_put(bio);
 }
 
@@ -1328,7 +1329,8 @@  iomap_finish_ioend(struct iomap_ioend *ioend, int error)
 	u32 folio_count = 0;
 
 	for (bio = &ioend->io_inline_bio; bio; bio = next) {
-		struct folio_iter fi;
+		struct bvec_iter_all iter;
+		struct folio_vec fv;
 
 		/*
 		 * For the last bio, bi_private points to the ioend, so we
@@ -1340,8 +1342,8 @@  iomap_finish_ioend(struct iomap_ioend *ioend, int error)
 			next = bio->bi_private;
 
 		/* walk all folios in bio, ending page IO on them */
-		bio_for_each_folio_all(fi, bio) {
-			iomap_finish_folio_write(inode, fi.folio, fi.length,
+		bio_for_each_folio_all(fv, bio, iter) {
+			iomap_finish_folio_write(inode, fv.fv_folio, fv.fv_len,
 					error);
 			folio_count++;
 		}
diff --git a/fs/verity/verify.c b/fs/verity/verify.c
index e250822275..b111ab0102 100644
--- a/fs/verity/verify.c
+++ b/fs/verity/verify.c
@@ -340,7 +340,8 @@  void fsverity_verify_bio(struct bio *bio)
 	struct inode *inode = bio_first_page_all(bio)->mapping->host;
 	struct fsverity_info *vi = inode->i_verity_info;
 	struct ahash_request *req;
-	struct folio_iter fi;
+	struct bvec_iter_all iter;
+	struct folio_vec fv;
 	unsigned long max_ra_pages = 0;
 
 	/* This allocation never fails, since it's mempool-backed. */
@@ -359,9 +360,9 @@  void fsverity_verify_bio(struct bio *bio)
 		max_ra_pages = bio->bi_iter.bi_size >> (PAGE_SHIFT + 2);
 	}
 
-	bio_for_each_folio_all(fi, bio) {
-		if (!verify_data_blocks(inode, vi, req, fi.folio, fi.length,
-					fi.offset, max_ra_pages)) {
+	bio_for_each_folio_all(fv, bio, iter) {
+		if (!verify_data_blocks(inode, vi, req, fv.fv_folio, fv.fv_len,
+					fv.fv_offset, max_ra_pages)) {
 			bio->bi_status = BLK_STS_IOERR;
 			break;
 		}
diff --git a/include/linux/bio.h b/include/linux/bio.h
index f86c7190c3..7ced281734 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -169,6 +169,42 @@  static inline void bio_advance(struct bio *bio, unsigned int nbytes)
 #define bio_for_each_segment(bvl, bio, iter)				\
 	__bio_for_each_segment(bvl, bio, iter, (bio)->bi_iter)
 
+struct folio_vec {
+	struct folio	*fv_folio;
+	size_t		fv_offset;
+	size_t		fv_len;
+};
+
+static inline struct folio_vec biovec_to_foliovec(struct bio_vec bv)
+{
+
+	struct folio *folio	= page_folio(bv.bv_page);
+	size_t offset		= (folio_page_idx(folio, bv.bv_page) << PAGE_SHIFT) +
+		bv.bv_offset;
+	size_t len = min_t(size_t, folio_size(folio) - offset, bv.bv_len);
+
+	return (struct folio_vec) {
+		.fv_folio	= folio,
+		.fv_offset	= offset,
+		.fv_len		= len,
+	};
+}
+
+static inline struct folio_vec bio_iter_iovec_folio(struct bio *bio,
+						    struct bvec_iter iter)
+{
+	return biovec_to_foliovec(bio_iter_iovec(bio, iter));
+}
+
+#define __bio_for_each_folio(bvl, bio, iter, start)			\
+	for (iter = (start);						\
+	     (iter).bi_size &&						\
+		((bvl = bio_iter_iovec_folio((bio), (iter))), 1);	\
+	     bio_advance_iter_single((bio), &(iter), (bvl).fv_len))
+
+#define bio_for_each_folio(bvl, bio, iter)				\
+	__bio_for_each_folio(bvl, bio, iter, (bio)->bi_iter)
+
 #define __bio_for_each_bvec(bvl, bio, iter, start)		\
 	for (iter = (start);						\
 	     (iter).bi_size &&						\
@@ -277,59 +313,22 @@  static inline struct bio_vec *bio_last_bvec_all(struct bio *bio)
 	return &bio->bi_io_vec[bio->bi_vcnt - 1];
 }
 
-/**
- * struct folio_iter - State for iterating all folios in a bio.
- * @folio: The current folio we're iterating.  NULL after the last folio.
- * @offset: The byte offset within the current folio.
- * @length: The number of bytes in this iteration (will not cross folio
- *	boundary).
- */
-struct folio_iter {
-	struct folio *folio;
-	size_t offset;
-	size_t length;
-	/* private: for use by the iterator */
-	struct folio *_next;
-	size_t _seg_count;
-	int _i;
-};
-
-static inline void bio_first_folio(struct folio_iter *fi, struct bio *bio,
-				   int i)
-{
-	struct bio_vec *bvec = bio_first_bvec_all(bio) + i;
-
-	fi->folio = page_folio(bvec->bv_page);
-	fi->offset = bvec->bv_offset +
-			PAGE_SIZE * (bvec->bv_page - &fi->folio->page);
-	fi->_seg_count = bvec->bv_len;
-	fi->length = min(folio_size(fi->folio) - fi->offset, fi->_seg_count);
-	fi->_next = folio_next(fi->folio);
-	fi->_i = i;
-}
-
-static inline void bio_next_folio(struct folio_iter *fi, struct bio *bio)
+static inline struct folio_vec bio_folio_iter_all_peek(const struct bio *bio,
+						       const struct bvec_iter_all *iter)
 {
-	fi->_seg_count -= fi->length;
-	if (fi->_seg_count) {
-		fi->folio = fi->_next;
-		fi->offset = 0;
-		fi->length = min(folio_size(fi->folio), fi->_seg_count);
-		fi->_next = folio_next(fi->folio);
-	} else if (fi->_i + 1 < bio->bi_vcnt) {
-		bio_first_folio(fi, bio, fi->_i + 1);
-	} else {
-		fi->folio = NULL;
-	}
+	return biovec_to_foliovec(__bvec_iter_all_peek(bio->bi_io_vec, iter));
 }
 
 /**
  * bio_for_each_folio_all - Iterate over each folio in a bio.
- * @fi: struct folio_iter which is updated for each folio.
+ * @fi: struct bio_folio_iter_all which is updated for each folio.
  * @bio: struct bio to iterate over.
  */
-#define bio_for_each_folio_all(fi, bio)				\
-	for (bio_first_folio(&fi, bio, 0); fi.folio; bio_next_folio(&fi, bio))
+#define bio_for_each_folio_all(fv, bio, iter)				\
+	for (bvec_iter_all_init(&iter);					\
+	     iter.idx < bio->bi_vcnt &&					\
+		((fv = bio_folio_iter_all_peek(bio, &iter)), true);	\
+	     bio_iter_all_advance((bio), &iter, fv.fv_len))
 
 enum bip_flags {
 	BIP_BLOCK_INTEGRITY	= 1 << 0, /* block layer owns integrity data */
diff --git a/include/linux/bvec.h b/include/linux/bvec.h
index 635fb54143..d238f959e3 100644
--- a/include/linux/bvec.h
+++ b/include/linux/bvec.h
@@ -205,18 +205,27 @@  static inline void bvec_iter_all_init(struct bvec_iter_all *iter_all)
 	iter_all->idx = 0;
 }
 
-static inline struct bio_vec bvec_iter_all_peek(const struct bio_vec *bvec,
-						struct bvec_iter_all *iter)
+static inline struct bio_vec __bvec_iter_all_peek(const struct bio_vec *bvec,
+						  const struct bvec_iter_all *iter)
 {
 	struct bio_vec bv = bvec[iter->idx];
 
+	BUG_ON(iter->done >= bv.bv_len);
+
 	bv.bv_offset	+= iter->done;
 	bv.bv_len	-= iter->done;
 
 	bv.bv_page	+= bv.bv_offset >> PAGE_SHIFT;
 	bv.bv_offset	&= ~PAGE_MASK;
-	bv.bv_len	= min_t(unsigned, PAGE_SIZE - bv.bv_offset, bv.bv_len);
+	return bv;
+}
+
+static inline struct bio_vec bvec_iter_all_peek(const struct bio_vec *bvec,
+						const struct bvec_iter_all *iter)
+{
+	struct bio_vec bv = __bvec_iter_all_peek(bvec, iter);
 
+	bv.bv_len = min_t(unsigned, PAGE_SIZE - bv.bv_offset, bv.bv_len);
 	return bv;
 }