Re: [PATCH 4/5] brd: split I/O at page boundaries

Yu Kuai
2025-04-22T11:10:21Z

在 2025/04/21 15:26, Christoph Hellwig 写道:
> A lot of complexity in brd stems from the fact that it tries to handle
> I/O spanning two backing pages. Instead limit the size of a single
> bvec iteration so that it never crosses a page boundary and remove all
> the now unneeded code.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> drivers/block/brd.c | 116 +++++++++++++-------------------------------
> 1 file changed, 34 insertions(+), 82 deletions(-)
>
LGTM

Reviewed-by: Yu Kuai <yukuai3@huawei.com>

> diff --git a/drivers/block/brd.c b/drivers/block/brd.c
> index 0c70d29379f1..580b2d8ce99c 100644
> --- a/drivers/block/brd.c
> +++ b/drivers/block/brd.c
> @@ -99,27 +99,6 @@ static void brd_free_pages(struct brd_device *brd)
> xa_destroy(&brd->brd_pages);
> }
>
> -/*
> - * copy_to_brd_setup must be called before copy_to_brd. It may sleep.
> - */
> -static int copy_to_brd_setup(struct brd_device *brd, sector_t sector, size_t n,
> - gfp_t gfp)
> -{
> - unsigned int offset = (sector & (PAGE_SECTORS-1)) << SECTOR_SHIFT;
> - size_t copy;
> - int ret;
> -
> - copy = min_t(size_t, n, PAGE_SIZE - offset);
> - ret = brd_insert_page(brd, sector, gfp);
> - if (ret)
> - return ret;
> - if (copy < n) {
> - sector += copy >> SECTOR_SHIFT;
> - ret = brd_insert_page(brd, sector, gfp);
> - }
> - return ret;
> -}
> -
> /*
> * Copy n bytes from src to the brd starting at sector. Does not sleep.
> */
> @@ -129,27 +108,13 @@ static void copy_to_brd(struct brd_device *brd, const void *src,
> struct page *page;
> void *dst;
> unsigned int offset = (sector & (PAGE_SECTORS-1)) << SECTOR_SHIFT;
> - size_t copy;
>
> - copy = min_t(size_t, n, PAGE_SIZE - offset);
> page = brd_lookup_page(brd, sector);
> BUG_ON(!page);
>
> dst = kmap_atomic(page);
> - memcpy(dst + offset, src, copy);
> + memcpy(dst + offset, src, n);
> kunmap_atomic(dst);
> -
> - if (copy < n) {
> - src += copy;
> - sector += copy >> SECTOR_SHIFT;
> - copy = n - copy;
> - page = brd_lookup_page(brd, sector);
> - BUG_ON(!page);
> -
> - dst = kmap_atomic(page);
> - memcpy(dst, src, copy);
> - kunmap_atomic(dst);
> - }
> }
>
> /*
> @@ -161,62 +126,60 @@ static void copy_from_brd(void *dst, struct brd_device *brd,
> struct page *page;
> void *src;
> unsigned int offset = (sector & (PAGE_SECTORS-1)) << SECTOR_SHIFT;
> - size_t copy;
>
> - copy = min_t(size_t, n, PAGE_SIZE - offset);
> page = brd_lookup_page(brd, sector);
> if (page) {
> src = kmap_atomic(page);
> - memcpy(dst, src + offset, copy);
> + memcpy(dst, src + offset, n);
> kunmap_atomic(src);
> } else
> - memset(dst, 0, copy);
> -
> - if (copy < n) {
> - dst += copy;
> - sector += copy >> SECTOR_SHIFT;
> - copy = n - copy;
> - page = brd_lookup_page(brd, sector);
> - if (page) {
> - src = kmap_atomic(page);
> - memcpy(dst, src, copy);
> - kunmap_atomic(src);
> - } else
> - memset(dst, 0, copy);
> - }
> + memset(dst, 0, n);
> }
>
> /*
> - * Process a single bvec of a bio.
> + * Process a single segment. The segment is capped to not cross page boundaries
> + * in both the bio and the brd backing memory.
> */
> -static int brd_rw_bvec(struct brd_device *brd, struct bio_vec *bv,
> - blk_opf_t opf, sector_t sector)
> +static bool brd_rw_bvec(struct brd_device *brd, struct bio *bio)
> {
> + struct bio_vec bv = bio_iter_iovec(bio, bio->bi_iter);
> + sector_t sector = bio->bi_iter.bi_sector;
> + u32 offset = (sector & (PAGE_SECTORS - 1)) << SECTOR_SHIFT;
> + blk_opf_t opf = bio->bi_opf;
> void *mem;
>
> + bv.bv_len = min_t(u32, bv.bv_len, PAGE_SIZE - offset);
> +
> if (op_is_write(opf)) {
> + int err;
> +
> /*
> * Must use NOIO because we don't want to recurse back into the
> * block or filesystem layers from page reclaim.
> */
> - gfp_t gfp = opf & REQ_NOWAIT ? GFP_NOWAIT : GFP_NOIO;
> - int err;
> -
> - err = copy_to_brd_setup(brd, sector, bv->bv_len, gfp);
> - if (err)
> - return err;
> + err = brd_insert_page(brd, sector,
> + (opf & REQ_NOWAIT) ? GFP_NOWAIT : GFP_NOIO);
> + if (err) {
> + if (err == -ENOMEM && (opf & REQ_NOWAIT))
> + bio_wouldblock_error(bio);
> + else
> + bio_io_error(bio);
> + return false;
> + }
> }
>
> - mem = bvec_kmap_local(bv);
> + mem = bvec_kmap_local(&bv);
> if (!op_is_write(opf)) {
> - copy_from_brd(mem, brd, sector, bv->bv_len);
> - flush_dcache_page(bv->bv_page);
> + copy_from_brd(mem, brd, sector, bv.bv_len);
> + flush_dcache_page(bv.bv_page);
> } else {
> - flush_dcache_page(bv->bv_page);
> - copy_to_brd(brd, mem, sector, bv->bv_len);
> + flush_dcache_page(bv.bv_page);
> + copy_to_brd(brd, mem, sector, bv.bv_len);
> }
> kunmap_local(mem);
> - return 0;
> +
> + bio_advance_iter_single(bio, &bio->bi_iter, bv.bv_len);
> + return true;
> }
>
> static void brd_do_discard(struct brd_device *brd, sector_t sector, u32 size)
> @@ -241,8 +204,6 @@ static void brd_do_discard(struct brd_device *brd, sector_t sector, u32 size)
> static void brd_submit_bio(struct bio *bio)
> {
> struct brd_device *brd = bio->bi_bdev->bd_disk->private_data;
> - struct bio_vec bvec;
> - struct bvec_iter iter;
>
> if (unlikely(op_is_discard(bio->bi_opf))) {
> brd_do_discard(brd, bio->bi_iter.bi_sector,
> @@ -251,19 +212,10 @@ static void brd_submit_bio(struct bio *bio)
> return;
> }
>
> - bio_for_each_segment(bvec, bio, iter) {
> - int err;
> -
> - err = brd_rw_bvec(brd, &bvec, bio->bi_opf, iter.bi_sector);
> - if (err) {
> - if (err == -ENOMEM && bio->bi_opf & REQ_NOWAIT) {
> - bio_wouldblock_error(bio);
> - return;
> - }
> - bio_io_error(bio);
> + do {
> + if (!brd_rw_bvec(brd, bio))
> return;
> - }
> - }
> + } while (bio->bi_iter.bi_size);
>
> bio_endio(bio);
> }
>