[PATCH 1/2] block: remove bio_add_pc_page
Sagi Grimberg
sagi at grimberg.me
Sat Jan 4 14:11:50 PST 2025
Reviewed-by: Sagi Grimberg <sagi at grimberg.me>
On 03/01/2025 9:33, Christoph Hellwig wrote:
> Lift bio_split_rw_at into blk_rq_append_bio so that it validates the
> hardware limits. With this all passthrough callers can simply add
> bio_add_page to build the bio and delay checking for exceeding of limits
> to this point instead of doing it for each page.
>
> While this looks like adding a new expensive loop over all bio_vecs,
> blk_rq_append_bio is already doing that just to counter the number of
> segments.
>
> Signed-off-by: Christoph Hellwig <hch at lst.de>
> ---
> block/bio.c | 107 ++------------------------
> block/blk-map.c | 118 +++++++----------------------
> block/blk.h | 8 --
> drivers/nvme/target/passthru.c | 18 +++--
> drivers/nvme/target/zns.c | 3 +-
> drivers/target/target_core_pscsi.c | 6 +-
> include/linux/bio.h | 2 -
> 7 files changed, 48 insertions(+), 214 deletions(-)
>
> diff --git a/block/bio.c b/block/bio.c
> index d5bdc31d88d3..4e1a27d312c9 100644
> --- a/block/bio.c
> +++ b/block/bio.c
> @@ -946,8 +946,11 @@ static bool bvec_try_merge_page(struct bio_vec *bv, struct page *page,
>
> /*
> * Try to merge a page into a segment, while obeying the hardware segment
> - * size limit. This is not for normal read/write bios, but for passthrough
> - * or Zone Append operations that we can't split.
> + * size limit.
> + *
> + * This is kept around for the integrity metadata, which is still tries
> + * to build the initial bio to the hardware limit and doesn't have proper
> + * helpers to split. Hopefully this will go away soon.
> */
> bool bvec_try_merge_hw_page(struct request_queue *q, struct bio_vec *bv,
> struct page *page, unsigned len, unsigned offset,
> @@ -964,106 +967,6 @@ bool bvec_try_merge_hw_page(struct request_queue *q, struct bio_vec *bv,
> return bvec_try_merge_page(bv, page, len, offset, same_page);
> }
>
> -/**
> - * bio_add_hw_page - attempt to add a page to a bio with hw constraints
> - * @q: the target queue
> - * @bio: destination bio
> - * @page: page to add
> - * @len: vec entry length
> - * @offset: vec entry offset
> - * @max_sectors: maximum number of sectors that can be added
> - * @same_page: return if the segment has been merged inside the same page
> - *
> - * Add a page to a bio while respecting the hardware max_sectors, max_segment
> - * and gap limitations.
> - */
> -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)
> -{
> - unsigned int max_size = max_sectors << SECTOR_SHIFT;
> -
> - if (WARN_ON_ONCE(bio_flagged(bio, BIO_CLONED)))
> - return 0;
> -
> - len = min3(len, max_size, queue_max_segment_size(q));
> - if (len > max_size - bio->bi_iter.bi_size)
> - return 0;
> -
> - if (bio->bi_vcnt > 0) {
> - struct bio_vec *bv = &bio->bi_io_vec[bio->bi_vcnt - 1];
> -
> - if (bvec_try_merge_hw_page(q, bv, page, len, offset,
> - same_page)) {
> - bio->bi_iter.bi_size += len;
> - return len;
> - }
> -
> - if (bio->bi_vcnt >=
> - min(bio->bi_max_vecs, queue_max_segments(q)))
> - return 0;
> -
> - /*
> - * If the queue doesn't support SG gaps and adding this segment
> - * would create a gap, disallow it.
> - */
> - if (bvec_gap_to_prev(&q->limits, bv, offset))
> - return 0;
> - }
> -
> - bvec_set_page(&bio->bi_io_vec[bio->bi_vcnt], page, len, offset);
> - bio->bi_vcnt++;
> - bio->bi_iter.bi_size += len;
> - return len;
> -}
> -
> -/**
> - * bio_add_hw_folio - attempt to add a folio to a bio with hw constraints
> - * @q: the target queue
> - * @bio: destination bio
> - * @folio: folio to add
> - * @len: vec entry length
> - * @offset: vec entry offset in the folio
> - * @max_sectors: maximum number of sectors that can be added
> - * @same_page: return if the segment has been merged inside the same folio
> - *
> - * Add a folio to a bio while respecting the hardware max_sectors, max_segment
> - * and gap limitations.
> - */
> -int bio_add_hw_folio(struct request_queue *q, struct bio *bio,
> - struct folio *folio, size_t len, size_t offset,
> - unsigned int max_sectors, bool *same_page)
> -{
> - if (len > UINT_MAX || offset > UINT_MAX)
> - return 0;
> - return bio_add_hw_page(q, bio, folio_page(folio, 0), len, offset,
> - max_sectors, same_page);
> -}
> -
> -/**
> - * bio_add_pc_page - attempt to add page to passthrough bio
> - * @q: the target queue
> - * @bio: destination bio
> - * @page: page to add
> - * @len: vec entry length
> - * @offset: vec entry offset
> - *
> - * Attempt to add a page to the bio_vec maplist. This can fail for a
> - * number of reasons, such as the bio being full or target block device
> - * limitations. The target block device must allow bio's up to PAGE_SIZE,
> - * so it is always possible to add a single page to an empty bio.
> - *
> - * This should only be used by passthrough bios.
> - */
> -int bio_add_pc_page(struct request_queue *q, struct bio *bio,
> - struct page *page, unsigned int len, unsigned int offset)
> -{
> - bool same_page = false;
> - return bio_add_hw_page(q, bio, page, len, offset,
> - queue_max_hw_sectors(q), &same_page);
> -}
> -EXPORT_SYMBOL(bio_add_pc_page);
> -
> /**
> * __bio_add_page - add page(s) to a bio in a new segment
> * @bio: destination bio
> diff --git a/block/blk-map.c b/block/blk-map.c
> index 894009b2d881..67a2da3b7ed9 100644
> --- a/block/blk-map.c
> +++ b/block/blk-map.c
> @@ -189,7 +189,7 @@ static int bio_copy_user_iov(struct request *rq, struct rq_map_data *map_data,
> }
> }
>
> - if (bio_add_pc_page(rq->q, bio, page, bytes, offset) < bytes) {
> + if (bio_add_page(bio, page, bytes, offset) < bytes) {
> if (!map_data)
> __free_page(page);
> break;
> @@ -272,86 +272,27 @@ static struct bio *blk_rq_map_bio_alloc(struct request *rq,
> static int bio_map_user_iov(struct request *rq, struct iov_iter *iter,
> gfp_t gfp_mask)
> {
> - iov_iter_extraction_t extraction_flags = 0;
> - unsigned int max_sectors = queue_max_hw_sectors(rq->q);
> unsigned int nr_vecs = iov_iter_npages(iter, BIO_MAX_VECS);
> struct bio *bio;
> int ret;
> - int j;
>
> if (!iov_iter_count(iter))
> return -EINVAL;
>
> bio = blk_rq_map_bio_alloc(rq, nr_vecs, gfp_mask);
> - if (bio == NULL)
> + if (!bio)
> return -ENOMEM;
> -
> - if (blk_queue_pci_p2pdma(rq->q))
> - extraction_flags |= ITER_ALLOW_P2PDMA;
> - if (iov_iter_extract_will_pin(iter))
> - bio_set_flag(bio, BIO_PAGE_PINNED);
> -
> - while (iov_iter_count(iter)) {
> - struct page *stack_pages[UIO_FASTIOV];
> - struct page **pages = stack_pages;
> - ssize_t bytes;
> - size_t offs;
> - int npages;
> -
> - if (nr_vecs > ARRAY_SIZE(stack_pages))
> - pages = NULL;
> -
> - bytes = iov_iter_extract_pages(iter, &pages, LONG_MAX,
> - nr_vecs, extraction_flags, &offs);
> - if (unlikely(bytes <= 0)) {
> - ret = bytes ? bytes : -EFAULT;
> - goto out_unmap;
> - }
> -
> - npages = DIV_ROUND_UP(offs + bytes, PAGE_SIZE);
> -
> - if (unlikely(offs & queue_dma_alignment(rq->q)))
> - j = 0;
> - else {
> - for (j = 0; j < npages; j++) {
> - struct page *page = pages[j];
> - unsigned int n = PAGE_SIZE - offs;
> - bool same_page = false;
> -
> - if (n > bytes)
> - n = bytes;
> -
> - if (!bio_add_hw_page(rq->q, bio, page, n, offs,
> - max_sectors, &same_page))
> - break;
> -
> - if (same_page)
> - bio_release_page(bio, page);
> - bytes -= n;
> - offs = 0;
> - }
> - }
> - /*
> - * release the pages we didn't map into the bio, if any
> - */
> - while (j < npages)
> - bio_release_page(bio, pages[j++]);
> - if (pages != stack_pages)
> - kvfree(pages);
> - /* couldn't stuff something into bio? */
> - if (bytes) {
> - iov_iter_revert(iter, bytes);
> - break;
> - }
> - }
> -
> + ret = bio_iov_iter_get_pages(bio, iter);
> + if (ret)
> + goto out_put;
This looks like a more involved change than converting bio_add_hw_page.
Perhaps should go into a separate patch? Other than that, looks good
Reviewed-by: Sagi Grimberg <sagi at grimberg.me>
More information about the Linux-nvme
mailing list