Re: [PATCH v2] block : add larger order folio size instead of pages

Hannes Reinecke
2024-05-02T06:45:36Z

On 4/30/24 19:50, Kundan Kumar wrote:
> When mTHP is enabled, IO can contain larger folios instead of pages.
> In such cases add a larger size to the bio instead of looping through
> pages. This reduces the overhead of iterating through pages for larger
> block sizes. perf diff before and after this change:
>
> Perf diff for write I/O with 128K block size:
> 1.26% -1.04% [kernel.kallsyms] [k] bio_iov_iter_get_pages
> 1.74% [kernel.kallsyms] [k] bvec_try_merge_page
> Perf diff for read I/O with 128K block size:
> 4.40% -3.63% [kernel.kallsyms] [k] bio_iov_iter_get_pages
> 5.60% [kernel.kallsyms] [k] bvec_try_merge_page
>
> Signed-off-by: Kundan Kumar <kundan.kumar@samsung.com>
> ---
> Changes since v1:
> https://lore.kernel.org/all/20240419091721.1790-1-kundan.kumar@samsung.com/
> - Changed functions bio_iov_add_page() and bio_iov_add_zone_append_page() to
> accept a folio
> - Removed branching and now calculate folio_offset and len in same fashion for
> both 0 order and larger folios
> - Added change in NVMe driver to use nvme_setup_prp_simple() by ignoring
> multiples of NVME_CTRL_PAGE_SIZE in offset
> - Added change to unpin_user_pages which were added as folios. Also stopped
> the unpin of pages one by one from __bio_release_pages()(Suggested by
> Keith)
>
> block/bio.c | 48 +++++++++++++++++++++++++----------------
> drivers/nvme/host/pci.c | 3 ++-
> 2 files changed, 31 insertions(+), 20 deletions(-)
>
> diff --git a/block/bio.c b/block/bio.c
> index 38baedb39c6f..0ec453ad15b3 100644
> --- a/block/bio.c
> +++ b/block/bio.c
> @@ -1155,7 +1155,6 @@ void __bio_release_pages(struct bio *bio, bool mark_dirty)
>
> bio_for_each_folio_all(fi, bio) {
> struct page *page;
> - size_t nr_pages;
>
> if (mark_dirty) {
> folio_lock(fi.folio);
> @@ -1163,11 +1162,7 @@ void __bio_release_pages(struct bio *bio, bool mark_dirty)
> folio_unlock(fi.folio);
> }
> page = folio_page(fi.folio, fi.offset / PAGE_SIZE);
> - nr_pages = (fi.offset + fi.length - 1) / PAGE_SIZE -
> - fi.offset / PAGE_SIZE + 1;
> - do {
> - bio_release_page(bio, page++);
> - } while (--nr_pages != 0);
> + bio_release_page(bio, page);

Errm. I guess you need to call 'folio_put()' here, otherwise the page
reference counting will be messed up.

> }
> }
> EXPORT_SYMBOL_GPL(__bio_release_pages);
> @@ -1192,7 +1187,7 @@ void bio_iov_bvec_set(struct bio *bio, struct iov_iter *iter)
> bio_set_flag(bio, BIO_CLONED);
> }
>
> -static int bio_iov_add_page(struct bio *bio, struct page *page,
> +static int bio_iov_add_folio(struct bio *bio, struct folio *folio,
> unsigned int len, unsigned int offset)
> {
> bool same_page = false;
> @@ -1202,27 +1197,26 @@ static int bio_iov_add_page(struct bio *bio, struct page *page,
>
> if (bio->bi_vcnt > 0 &&
> bvec_try_merge_page(&bio->bi_io_vec[bio->bi_vcnt - 1],
> - page, len, offset, &same_page)) {
> + &folio->page, len, offset, &same_page)) {
> bio->bi_iter.bi_size += len;
> if (same_page)
> - bio_release_page(bio, page);
> + bio_release_page(bio, &folio->page);
> return 0;
> }
> - __bio_add_page(bio, page, len, offset);
> + bio_add_folio_nofail(bio, folio, len, offset);
> return 0;
> }

That is not a valid conversion.
bvec_try_to_merge_pages() will try to merge a page with an existing
bvec. If the logic is switch to folios you would need to iterate over
all pages in a folio, and call bvec_try_to_merge_pages() for each page
in the folio.
Or convert / add a function 'bvec_try_to_merge_folio()'.
But with the above patch it will only ever try to merge the first page
in the folio.

>
> -static int bio_iov_add_zone_append_page(struct bio *bio, struct page *page,
> +static int bio_iov_add_zone_append_folio(struct bio *bio, struct folio *folio,
> unsigned int len, unsigned int offset)
> {
> struct request_queue *q = bdev_get_queue(bio->bi_bdev);
> bool same_page = false;
> -
> - if (bio_add_hw_page(q, bio, page, len, offset,
> + if (bio_add_hw_page(q, bio, &folio->page, len, offset,
> queue_max_zone_append_sectors(q), &same_page) != len)

Wouldn't this just try to add the first page in a folio?

> return -EINVAL;
> if (same_page)
> - bio_release_page(bio, page);
> + bio_release_page(bio, &folio->page);
> return 0;
> }
>
> @@ -1247,8 +1241,10 @@ static int __bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter)
> struct page **pages = (struct page **)bv;
> ssize_t size, left;
> unsigned len, i = 0;
> - size_t offset;
> + size_t offset, folio_offset, size_folio;
> int ret = 0;
> + int num_pages;
> + struct folio *folio;
>
> /*
> * Move page array up in the allocated memory for the bio vecs as far as
> @@ -1289,16 +1285,30 @@ static int __bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter)
>
> for (left = size, i = 0; left > 0; left -= len, i++) {
> struct page *page = pages[i];
> + folio = page_folio(page);
> +
> + /* See the offset in folio and the size */
> + folio_offset = (folio_page_idx(folio, page)
> + << PAGE_SHIFT) + offset;
> + size_folio = folio_size(folio);
> +
> + /* Calculate the length of folio to be added */
> + len = min_t(size_t, (size_folio - folio_offset), left);
> +
> + num_pages = DIV_ROUND_UP(offset + len, PAGE_SIZE);
>
> - len = min_t(size_t, PAGE_SIZE - offset, left);
> if (bio_op(bio) == REQ_OP_ZONE_APPEND) {
> - ret = bio_iov_add_zone_append_page(bio, page, len,
> - offset);
> + ret = bio_iov_add_zone_append_folio(bio, folio, len,
> + folio_offset);
> if (ret)
> break;
> } else
> - bio_iov_add_page(bio, page, len, offset);
> + bio_iov_add_folio(bio, folio, len, folio_offset);
>
> + /* Skip the pages which got added */
> + if (bio_flagged(bio, BIO_PAGE_PINNED) && num_pages > 1)
> + unpin_user_pages(pages + i, num_pages - 1);
> + i = i + (num_pages - 1);
> offset = 0;
> }
>
I would rather use folio as an argument here ...

> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index 8e0bb9692685..7c07b0582cae 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -778,7 +778,8 @@ static blk_status_t nvme_map_data(struct nvme_dev *dev, struct request *req,
> struct bio_vec bv = req_bvec(req);
>
> if (!is_pci_p2pdma_page(bv.bv_page)) {
> - if (bv.bv_offset + bv.bv_len <= NVME_CTRL_PAGE_SIZE * 2)
> + if ((bv.bv_offset & (NVME_CTRL_PAGE_SIZE - 1))
> + + bv.bv_len <= NVME_CTRL_PAGE_SIZE * 2)
> return nvme_setup_prp_simple(dev, req,
> &cmnd->rw, &bv);
>

In general I'm in favour of moving the block layer over to folios, but
then we should do it by adding a folio-based version of the functions,
and the gradually convert the callers over to those functions.

Trying to slide it in via folio->page or page_folio() things is not
the way to do it as they are not equivalent transformations.

Cheers,

Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich