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

Christoph Hellwig
2024-05-02T05:35:57Z

> - Changed functions bio_iov_add_page() and bio_iov_add_zone_append_page() to
> accept a folio

Those should be separate prep patches.

> - Added change in NVMe driver to use nvme_setup_prp_simple() by ignoring
> multiples of NVME_CTRL_PAGE_SIZE in offset

This should also be a prep patch.

> - 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)

and this as well.

> @@ -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);

Please keep an empty line after declarations. But I think you can also
just move the folio declaration here and combine the lines, i.e.

struct page *page = pages[i];
struct folio *folio = page_folio(page);

...

> + /* See the offset in folio and the size */
> + folio_offset = (folio_page_idx(folio, page)
> + << PAGE_SHIFT) + offset;

Kernel coding style keeps the operators on the previous line, i.e.

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);

size_folio is only used in this expression, so we can simplify the code
by just removing the variable:

/* Calculate how much of the folio we're going to add: */
len = min_t(size_t, folio_size(folio) - folio_offset, left);

> + /* Skip the pages which got added */
> + if (bio_flagged(bio, BIO_PAGE_PINNED) && num_pages > 1)
> + unpin_user_pages(pages + i, num_pages - 1);

The comment doesn't sound quite correct to me: we're not really skipping
the pages, but we are dropping the extra references early here.

> 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)

Sme comment about overator placement as above.