[v9,6/8] block: Switch to pinning pages.
Commit Message
Add BIO_PAGE_PINNED to indicate that the pages in a bio are pinned
(FOLL_PIN) and that the pin will need removing.
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Al Viro <viro@zeniv.linux.org.uk>
cc: Jens Axboe <axboe@kernel.dk>
cc: Jan Kara <jack@suse.cz>
cc: Christoph Hellwig <hch@lst.de>
cc: Matthew Wilcox <willy@infradead.org>
cc: Logan Gunthorpe <logang@deltatee.com>
cc: linux-block@vger.kernel.org
---
Notes:
ver #9)
- Only consider pinning in bio_set_cleanup_mode(). Ref'ing pages in
struct bio is going away.
- page_put_unpin() is removed; call unpin_user_page() and put_page()
directly.
- Use bio_release_page() in __bio_release_pages().
- BIO_PAGE_PINNED and BIO_PAGE_REFFED can't both be set, so use if-else
when testing both of them.
ver #8)
- Move the infrastructure to clean up pinned pages to this patch [hch].
- Put BIO_PAGE_PINNED before BIO_PAGE_REFFED as the latter should
probably be removed at some point. FOLL_PIN can then be renumbered
first.
block/bio.c | 6 +++---
block/blk.h | 21 +++++++++++++++++++++
include/linux/bio.h | 3 ++-
include/linux/blk_types.h | 1 +
4 files changed, 27 insertions(+), 4 deletions(-)
Comments
On Tue, Jan 24, 2023 at 05:01:06PM +0000, David Howells wrote:
> Add BIO_PAGE_PINNED to indicate that the pages in a bio are pinned
> (FOLL_PIN) and that the pin will need removing.
The subject is odd when this doesn't actually switch anything,
but just adds the infrastructure to unpin pages.
> +/*
> + * Set the cleanup mode for a bio from an iterator and the extraction flags.
> + */
> +static inline void bio_set_cleanup_mode(struct bio *bio, struct iov_iter *iter)
> +{
> + if (iov_iter_extract_will_pin(iter))
> + bio_set_flag(bio, BIO_PAGE_PINNED);
> +}
At this point I'd be tempted to just open code these two lines
instead of adding a helper, but I can live with the helper if you
prefer it.
Otherwise looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
On 1/24/23 09:01, David Howells wrote:
> Add BIO_PAGE_PINNED to indicate that the pages in a bio are pinned
> (FOLL_PIN) and that the pin will need removing.
>
> Signed-off-by: David Howells <dhowells@redhat.com>
> cc: Al Viro <viro@zeniv.linux.org.uk>
> cc: Jens Axboe <axboe@kernel.dk>
> cc: Jan Kara <jack@suse.cz>
> cc: Christoph Hellwig <hch@lst.de>
> cc: Matthew Wilcox <willy@infradead.org>
> cc: Logan Gunthorpe <logang@deltatee.com>
> cc: linux-block@vger.kernel.org
> ---
>
> Notes:
> ver #9)
> - Only consider pinning in bio_set_cleanup_mode(). Ref'ing pages in
> struct bio is going away.
> - page_put_unpin() is removed; call unpin_user_page() and put_page()
> directly.
> - Use bio_release_page() in __bio_release_pages().
> - BIO_PAGE_PINNED and BIO_PAGE_REFFED can't both be set, so use if-else
> when testing both of them.
>
> ver #8)
> - Move the infrastructure to clean up pinned pages to this patch [hch].
> - Put BIO_PAGE_PINNED before BIO_PAGE_REFFED as the latter should
> probably be removed at some point. FOLL_PIN can then be renumbered
> first.
>
> block/bio.c | 6 +++---
> block/blk.h | 21 +++++++++++++++++++++
> include/linux/bio.h | 3 ++-
> include/linux/blk_types.h | 1 +
> 4 files changed, 27 insertions(+), 4 deletions(-)
Neatly avoiding any use of FOLL_PIN or FOLL_GET, good. :)
Reviewed-by: John Hubbard <jhubbard@nvidia.com>
thanks,
Christoph Hellwig <hch@infradead.org> wrote:
> > Add BIO_PAGE_PINNED to indicate that the pages in a bio are pinned
> > (FOLL_PIN) and that the pin will need removing.
>
> The subject is odd when this doesn't actually switch anything,
> but just adds the infrastructure to unpin pages.
How about:
block: Add BIO_PAGE_PINNED and associated infrastructure
> > +static inline void bio_set_cleanup_mode(struct bio *bio, struct iov_iter *iter)
> ...
> At this point I'd be tempted to just open code these two lines
> instead of adding a helper, but I can live with the helper if you
> prefer it.
I can do that. It makes sense to put the call to that next to the call to
iov_iter_extract_pages().
David
On Tue, Jan 24, 2023 at 08:59:11PM +0000, David Howells wrote:
> Christoph Hellwig <hch@infradead.org> wrote:
>
> > > Add BIO_PAGE_PINNED to indicate that the pages in a bio are pinned
> > > (FOLL_PIN) and that the pin will need removing.
> >
> > The subject is odd when this doesn't actually switch anything,
> > but just adds the infrastructure to unpin pages.
>
> How about:
>
> block: Add BIO_PAGE_PINNED and associated infrastructure
sounds good.
@@ -1176,7 +1176,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);
@@ -1496,8 +1496,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 unpin each page and will run one bio_put() against the
+ * BIO.
*/
static void bio_dirty_fn(struct work_struct *work);
@@ -425,6 +425,27 @@ int bio_add_hw_page(struct request_queue *q, struct bio *bio,
struct page *page, unsigned int len, unsigned int offset,
unsigned int max_sectors, bool *same_page);
+/*
+ * Set the cleanup mode for a bio from an iterator and the extraction flags.
+ */
+static inline void bio_set_cleanup_mode(struct bio *bio, struct iov_iter *iter)
+{
+ if (iov_iter_extract_will_pin(iter))
+ bio_set_flag(bio, BIO_PAGE_PINNED);
+}
+
+/*
+ * Clean up a page appropriately, where the page may be pinned, may have a
+ * ref taken on it or neither.
+ */
+static inline void bio_release_page(struct bio *bio, struct page *page)
+{
+ if (bio_flagged(bio, BIO_PAGE_PINNED))
+ unpin_user_page(page);
+ else if (bio_flagged(bio, BIO_PAGE_REFFED))
+ put_page(page);
+}
+
struct request_queue *blk_alloc_queue(int node_id);
int disk_scan_partitions(struct gendisk *disk, fmode_t mode, void *owner);
@@ -484,7 +484,8 @@ void zero_fill_bio(struct bio *bio);
static inline void bio_release_pages(struct bio *bio, bool mark_dirty)
{
- if (bio_flagged(bio, BIO_PAGE_REFFED))
+ if (bio_flagged(bio, BIO_PAGE_REFFED) ||
+ bio_flagged(bio, BIO_PAGE_PINNED))
__bio_release_pages(bio, mark_dirty);
}
@@ -318,6 +318,7 @@ struct bio {
* bio flags
*/
enum {
+ BIO_PAGE_PINNED, /* Unpin pages in bio_release_pages() */
BIO_PAGE_REFFED, /* put pages in bio_release_pages() */
BIO_CLONED, /* doesn't own data */
BIO_BOUNCED, /* bio is a bounce bio */