Re: [PATCH v4 7/7] iov_iter, block: Make bio structs pin pages rather than ref'ing if appropriate

From: David Howells
Date: Tue Jan 10 2023 - 09:38:59 EST

Jens Axboe <axboe@xxxxxxxxx> wrote:

> I think it makes more sense to have NO_REF check, to be honest, as that
> means the general path doesn't have to set that flag. But I don't feel
> too strongly about that part.

It's just that the logic seems weird with BIO_NO_PAGE_REF and BIO_PAGE_PINNED
being kind of opposite polarity.

Anyway, see attached.

David
---
iov_iter, block: Make bio structs pin pages rather than ref'ing if appropriate

Convert the block layer's bio code to use iov_iter_extract_pages() instead
of iov_iter_get_pages(). This will pin pages or leave them unaltered
rather than getting a ref on them as appropriate to the source iterator.

The pages need to be pinned for DIO-read rather than having refs taken on
them to prevent VM copy-on-write from malfunctioning during a concurrent
fork() (the result of the I/O would otherwise end up only visible to the
child process and not the parent).

To implement this:

(1) The BIO_NO_PAGE_REF flag, if unset, indicates the page needs putting
or unpinning.

(2) A BIO_PAGE_PINNED flag is added. If set, this causes attached pages
to be passed to unpin_user_page() during cleanup instead of
put_page().

(3) BIO_NO_PAGE_REF and BIO_PAGE_PINNED are both cleared by default when
the bio is (re-)initialised.

(4) If iov_iter_extract_pages() indicates FOLL_PIN, then BIO_PAGE_PINNED
is set; if it indicates 0, BIO_NO_PAGE_REF is set; and if it indicates
FOLL_GET, then neither flag is set. If it indicates anything else, a
WARN_ON_ONCE will be triggered and BIO_NO_PAGE_REF will be set.

Mixed sets are not supported - all the pages must be handled in the
same way.

(5) Cloned bio structs have BIO_NO_PAGE_REF as they don't own their own
pages.

(6) bio_release_pages() will do the release if BIO_NO_PAGE_REF flag is
not set.

[!] Note that this is tested a bit with ext4, but nothing else.

Signed-off-by: David Howells <dhowells@xxxxxxxxxx>
cc: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
cc: Jens Axboe <axboe@xxxxxxxxx>
cc: Christoph Hellwig <hch@xxxxxx>
cc: Matthew Wilcox <willy@xxxxxxxxxxxxx>
cc: Logan Gunthorpe <logang@xxxxxxxxxxxx>
---
block/bio.c | 66 ++++++++++++++++++++++++++++++++++++----------
include/linux/blk_types.h | 1
2 files changed, 53 insertions(+), 14 deletions(-)

diff --git a/block/bio.c b/block/bio.c
index 5f96fcae3f75..88dfa0e34e81 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -243,6 +243,12 @@ 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 neither BIO_NO_PAGE_REF nor
+ * BIO_PAGE_PINNED; BIO_PAGE_PINNED should be set if the page should be
+ * unpinned instead and BIO_NO_PAGE_REF should be set if the pages should not
+ * be put or unpinned.
*/
void bio_init(struct bio *bio, struct block_device *bdev, struct bio_vec *table,
unsigned short max_vecs, blk_opf_t opf)
@@ -814,6 +820,8 @@ static int __bio_clone(struct bio *bio, struct bio *bio_src, gfp_t gfp)
bio_set_flag(bio, BIO_CLONED);
bio->bi_ioprio = bio_src->bi_ioprio;
bio->bi_iter = bio_src->bi_iter;
+ bio_set_flag(bio, BIO_NO_PAGE_REF);
+ bio_clear_flag(bio, BIO_PAGE_PINNED);

if (bio->bi_bdev) {
if (bio->bi_bdev == bio_src->bi_bdev &&
@@ -1168,6 +1176,20 @@ bool bio_add_folio(struct bio *bio, struct folio *folio, size_t len,
return bio_add_page(bio, &folio->page, len, off) > 0;
}

+/*
+ * Clean up a page according to the mode indicated by iov_iter_extract_pages(),
+ * where the page is may be pinned or may have a ref taken on it.
+ */
+static void bio_release_page(struct bio *bio, struct page *page)
+{
+ if (bio_flagged(bio, BIO_NO_PAGE_REF))
+ return;
+ if (bio_flagged(bio, BIO_PAGE_PINNED))
+ unpin_user_page(page);
+ else
+ put_page(page);
+}
+
void __bio_release_pages(struct bio *bio, bool mark_dirty)
{
struct bvec_iter_all iter_all;
@@ -1176,7 +1198,7 @@ void __bio_release_pages(struct bio *bio, bool mark_dirty)
bio_for_each_segment_all(bvec, bio, iter_all) {
if (mark_dirty && !PageCompound(bvec->bv_page))
set_page_dirty_lock(bvec->bv_page);
- put_page(bvec->bv_page);
+ bio_release_page(bio, bvec->bv_page);
}
}
EXPORT_SYMBOL_GPL(__bio_release_pages);
@@ -1213,7 +1235,7 @@ static int bio_iov_add_page(struct bio *bio, struct page *page,
}

if (same_page)
- put_page(page);
+ bio_release_page(bio, page);
return 0;
}

@@ -1227,7 +1249,7 @@ static int bio_iov_add_zone_append_page(struct bio *bio, struct page *page,
queue_max_zone_append_sectors(q), &same_page) != len)
return -EINVAL;
if (same_page)
- put_page(page);
+ bio_release_page(bio, page);
return 0;
}

@@ -1238,10 +1260,11 @@ static int bio_iov_add_zone_append_page(struct bio *bio, struct page *page,
* @bio: bio to add pages to
* @iter: iov iterator describing the region to be mapped
*
- * Pins pages from *iter and appends them to @bio's bvec array. The
- * pages will have to be released using put_page() when done.
- * For multi-segment *iter, this function only adds pages from the
- * next non-empty segment of the iov iterator.
+ * Pins pages from *iter and appends them to @bio's bvec array. The pages will
+ * have to be released using put_page() or unpin_user_page() when done as
+ * according to BIO_NO_PAGE_REF and BIO_PAGE_PINNED. For multi-segment *iter,
+ * this function only adds pages from the next non-empty segment of the iov
+ * iterator.
*/
static int __bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter)
{
@@ -1249,7 +1272,7 @@ static int __bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter)
unsigned short entries_left = bio->bi_max_vecs - bio->bi_vcnt;
struct bio_vec *bv = bio->bi_io_vec + bio->bi_vcnt;
struct page **pages = (struct page **)bv;
- unsigned int gup_flags = 0;
+ unsigned int gup_flags = 0, cleanup_mode;
ssize_t size, left;
unsigned len, i = 0;
size_t offset, trim;
@@ -1273,12 +1296,27 @@ static int __bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter)
* result to ensure the bio's total size is correct. The remainder of
* the iov data will be picked up in the next bio iteration.
*/
- size = iov_iter_get_pages(iter, pages,
- UINT_MAX - bio->bi_iter.bi_size,
- nr_pages, &offset, gup_flags);
+ size = iov_iter_extract_pages(iter, &pages,
+ UINT_MAX - bio->bi_iter.bi_size,
+ nr_pages, gup_flags,
+ &offset, &cleanup_mode);
if (unlikely(size <= 0))
return size ? size : -EFAULT;

+ switch (cleanup_mode) {
+ case FOLL_GET:
+ break;
+ case FOLL_PIN:
+ bio_set_flag(bio, BIO_PAGE_PINNED);
+ break;
+ default:
+ WARN_ON_ONCE(1);
+ fallthrough;
+ case 0:
+ bio_set_flag(bio, BIO_NO_PAGE_REF);
+ break;
+ }
+
nr_pages = DIV_ROUND_UP(offset + size, PAGE_SIZE);

trim = size & (bdev_logical_block_size(bio->bi_bdev) - 1);
@@ -1308,7 +1346,7 @@ static int __bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter)
iov_iter_revert(iter, left);
out:
while (i < nr_pages)
- put_page(pages[i++]);
+ bio_release_page(bio, pages[i++]);

return ret;
}
@@ -1489,8 +1527,8 @@ void bio_set_pages_dirty(struct bio *bio)
* the BIO and re-dirty the pages in process context.
*
* It is expected that bio_check_pages_dirty() will wholly own the BIO from
- * here on. It will run one put_page() against each page and will run one
- * bio_put() against the BIO.
+ * here on. It will run one put_page() or unpin_user_page() against each page
+ * and will run one bio_put() against the BIO.
*/

static void bio_dirty_fn(struct work_struct *work);
diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
index 99be590f952f..38e22a27d029 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -319,6 +319,7 @@ struct bio {
*/
enum {
BIO_NO_PAGE_REF, /* don't put release vec pages */
+ BIO_PAGE_PINNED, /* Pages need unpinning rather than putting */
BIO_CLONED, /* doesn't own data */
BIO_BOUNCED, /* bio is a bounce bio */
BIO_QUIET, /* Make BIO Quiet */