| Message ID | 20230124170108.1070389-7-dhowells@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iov_iter: Improve page extraction (pin or just list) | expand |
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.
diff --git a/block/bio.c b/block/bio.c index 851c23641a0d..fc45aaa97696 100644 --- a/block/bio.c +++ b/block/bio.c @@ -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); diff --git a/block/blk.h b/block/blk.h index 4c3b3325219a..32b252903f9a 100644 --- a/block/blk.h +++ b/block/blk.h @@ -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); diff --git a/include/linux/bio.h b/include/linux/bio.h index 805957c99147..b2c09997d79c 100644 --- a/include/linux/bio.h +++ b/include/linux/bio.h @@ -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); } diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h index 7daa261f4f98..a0e339ff3d09 100644 --- a/include/linux/blk_types.h +++ b/include/linux/blk_types.h @@ -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 */
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(-)