[v6,09/34] bio: Rename BIO_NO_PAGE_REF to BIO_PAGE_REFFED and invert the meaning

Commit Message

David Howells Jan. 16, 2023, 11:09 p.m. UTC

Rename BIO_NO_PAGE_REF to BIO_PAGE_REFFED and invert the meaning.  In a
following patch I intend to add a BIO_PAGE_PINNED flag to indicate that the
page needs unpinning and this way both flags have the same logic.

Changes
=======
ver #5)
 - Split from patch that uses iov_iter_extract_pages().

Signed-off-by: David Howells <[e-mail]>
cc: Al Viro <[e-mail]>
cc: Jens Axboe <[e-mail]>
cc: Jan Kara <[e-mail]>
cc: Christoph Hellwig <[e-mail]>
cc: Matthew Wilcox <[e-mail]>
cc: Logan Gunthorpe <[e-mail]>
cc: [e-mail]

Link: https://lore.kernel.org/r/[e-mail]/ # v4
Link: https://lore.kernel.org/r/[e-mail]/ # v5
---

 block/bio.c               |    9 ++++++++-
 include/linux/bio.h       |    2 +-
 include/linux/blk_types.h |    2 +-
 3 files changed, 10 insertions(+), 3 deletions(-)

Comments

Christoph Hellwig Jan. 17, 2023, 8:02 a.m. UTC | #1

Looks good:

Reviewed-by: Christoph Hellwig <[e-mail]>

David Howells Jan. 18, 2023, 2 p.m. UTC | #2

Actually, should I make it so that the bottom two bits of bi_flags are a
four-state variable and make it such that bio_release_page() gives a warning
if the state is 0 - ie. unset?

The states would then be, say:

	0	WARN(), do no cleanup
	1	FOLL_GET
	2	FOLL_PUT
	3	do no cleanup

This should help debug any places, such as iomap_dio_zero() that I just found,
that add pages with refs without calling iov_iter_extract_pages().

David

Christoph Hellwig Jan. 18, 2023, 2:09 p.m. UTC | #3

On Wed, Jan 18, 2023 at 02:00:54PM +0000, David Howells wrote:
> Actually, should I make it so that the bottom two bits of bi_flags are a
> four-state variable and make it such that bio_release_page() gives a warning
> if the state is 0 - ie. unset?
> 
> The states would then be, say:
> 
> 	0	WARN(), do no cleanup
> 	1	FOLL_GET
> 	2	FOLL_PUT
> 	3	do no cleanup
> 
> This should help debug any places, such as iomap_dio_zero() that I just found,
> that add pages with refs without calling iov_iter_extract_pages().

I don't really see a point.  The fundamental use case of the bio itself
isn't really to this at all.  So we're stealing one, or in the future
two bits mostly to optimize some direct I/O use cases.  In fact I
wonder if instead we should just drop this micro-optimization entirely
an just add a member for the foll flags to the direct I/O container
structures (struct blkdev_dio, strut iomap_dio, struct dio, or just on
stack for __blkdev_direct_IO_simple and zonefs_file_dio_append) and
pass that to bio_release_pages.

Patch

diff --git a/block/bio.c b/block/bio.c
index 867cf4db87ea..5b6a76c3e620 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -243,6 +243,10 @@  static void bio_free(struct bio *bio)
  * Users of this function have their own bio allocation. Subsequently,
  * they must remember to pair any call to bio_init() with bio_uninit()
  * when IO has completed, or when the bio is released.
+ *
+ * We set the initial assumption that pages attached to the bio will be
+ * released with put_page() by setting BIO_PAGE_REFFED; if the pages
+ * should not be put, this flag should be cleared.
  */
 void bio_init(struct bio *bio, struct block_device *bdev, struct bio_vec *table,
 	      unsigned short max_vecs, blk_opf_t opf)
@@ -274,6 +278,7 @@  void bio_init(struct bio *bio, struct block_device *bdev, struct bio_vec *table,
 #ifdef CONFIG_BLK_DEV_INTEGRITY
 	bio->bi_integrity = NULL;
 #endif
+	bio_set_flag(bio, BIO_PAGE_REFFED);
 	bio->bi_vcnt = 0;
 
 	atomic_set(&bio->__bi_remaining, 1);
@@ -302,6 +307,7 @@  void bio_reset(struct bio *bio, struct block_device *bdev, blk_opf_t opf)
 {
 	bio_uninit(bio);
 	memset(bio, 0, BIO_RESET_BYTES);
+	bio_set_flag(bio, BIO_PAGE_REFFED);
 	atomic_set(&bio->__bi_remaining, 1);
 	bio->bi_bdev = bdev;
 	if (bio->bi_bdev)
@@ -812,6 +818,7 @@  EXPORT_SYMBOL(bio_put);
 static int __bio_clone(struct bio *bio, struct bio *bio_src, gfp_t gfp)
 {
 	bio_set_flag(bio, BIO_CLONED);
+	bio_clear_flag(bio, BIO_PAGE_REFFED);
 	bio->bi_ioprio = bio_src->bi_ioprio;
 	bio->bi_iter = bio_src->bi_iter;
 
@@ -1198,7 +1205,7 @@  void bio_iov_bvec_set(struct bio *bio, struct iov_iter *iter)
 	bio->bi_io_vec = (struct bio_vec *)iter->bvec;
 	bio->bi_iter.bi_bvec_done = iter->iov_offset;
 	bio->bi_iter.bi_size = size;
-	bio_set_flag(bio, BIO_NO_PAGE_REF);
+	bio_clear_flag(bio, BIO_PAGE_REFFED);
 	bio_set_flag(bio, BIO_CLONED);
 }
 
diff --git a/include/linux/bio.h b/include/linux/bio.h
index 3f7ba7fe48ac..69b32c5532f6 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -496,7 +496,7 @@  void zero_fill_bio(struct bio *bio);
 
 static inline void bio_release_pages(struct bio *bio, bool mark_dirty)
 {
-	if (!bio_flagged(bio, BIO_NO_PAGE_REF))
+	if (bio_flagged(bio, BIO_PAGE_REFFED))
 		__bio_release_pages(bio, mark_dirty);
 }
 
diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
index 99be590f952f..86711fb0534a 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -318,7 +318,7 @@  struct bio {
  * bio flags
  */
 enum {
-	BIO_NO_PAGE_REF,	/* don't put release vec pages */
+	BIO_PAGE_REFFED,	/* Pages need refs putting (equivalent to FOLL_GET) */
 	BIO_CLONED,		/* doesn't own data */
 	BIO_BOUNCED,		/* bio is a bounce bio */
 	BIO_QUIET,		/* Make BIO Quiet */