brd cleanups

brd cleanups

[Christoph Hellwig]

Hi all,

this series has various brd cleanups mostly to get rid kmap_atomic and
poking into the bvec fields.  I'm posting it now because some of the
work might be useful for the discard fixes from Yu Kuai, but I'll happily
rebase it on top of any pending urgent fixes.

Diffstat:
 brd.c |  165 ++++++++++++++----------------------------------------------------
 1 file changed, 37 insertions(+), 128 deletions(-)

[PATCH 1/5] brd; pass a bvec pointer to brd_do_bvec

[Christoph Hellwig]

Pass the bvec to brd_do_bvec instead of marshalling the information into
individual arguments.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/block/brd.c | 35 +++++++++++++----------------------
 1 file changed, 13 insertions(+), 22 deletions(-)

diff --git a/drivers/block/brd.c b/drivers/block/brd.c
index 292f127cae0a..c8974bc545fb 100644
--- a/drivers/block/brd.c
+++ b/drivers/block/brd.c
@@ -189,12 +189,10 @@ static void copy_from_brd(void *dst, struct brd_device *brd,
 /*
  * Process a single bvec of a bio.
  */
-static int brd_do_bvec(struct brd_device *brd, struct page *page,
-			unsigned int len, unsigned int off, blk_opf_t opf,
-			sector_t sector)
+static int brd_rw_bvec(struct brd_device *brd, struct bio_vec *bv,
+		blk_opf_t opf, sector_t sector)
 {
 	void *mem;
-	int err = 0;
 
 	if (op_is_write(opf)) {
 		/*
@@ -202,24 +200,23 @@ static int brd_do_bvec(struct brd_device *brd, struct page *page,
 		 * 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, len, gfp);
+		err = copy_to_brd_setup(brd, sector, bv->bv_len, gfp);
 		if (err)
-			goto out;
+			return err;
 	}
 
-	mem = kmap_atomic(page);
+	mem = kmap_atomic(bv->bv_page);
 	if (!op_is_write(opf)) {
-		copy_from_brd(mem + off, brd, sector, len);
-		flush_dcache_page(page);
+		copy_from_brd(mem + bv->bv_offset, brd, sector, bv->bv_len);
+		flush_dcache_page(bv->bv_page);
 	} else {
-		flush_dcache_page(page);
-		copy_to_brd(brd, mem + off, sector, len);
+		flush_dcache_page(bv->bv_page);
+		copy_to_brd(brd, mem + bv->bv_offset, sector, bv->bv_len);
 	}
 	kunmap_atomic(mem);
-
-out:
-	return err;
+	return 0;
 }
 
 static void brd_do_discard(struct brd_device *brd, sector_t sector, u32 size)
@@ -255,15 +252,9 @@ static void brd_submit_bio(struct bio *bio)
 	}
 
 	bio_for_each_segment(bvec, bio, iter) {
-		unsigned int len = bvec.bv_len;
 		int err;
 
-		/* Don't support un-aligned buffer */
-		WARN_ON_ONCE((bvec.bv_offset & (SECTOR_SIZE - 1)) ||
-				(len & (SECTOR_SIZE - 1)));
-
-		err = brd_do_bvec(brd, bvec.bv_page, len, bvec.bv_offset,
-				  bio->bi_opf, sector);
+		err = brd_rw_bvec(brd, &bvec, bio->bi_opf, sector);
 		if (err) {
 			if (err == -ENOMEM && bio->bi_opf & REQ_NOWAIT) {
 				bio_wouldblock_error(bio);
@@ -272,7 +263,7 @@ static void brd_submit_bio(struct bio *bio)
 			bio_io_error(bio);
 			return;
 		}
-		sector += len >> SECTOR_SHIFT;
+		sector += bvec.bv_len >> SECTOR_SHIFT;
 	}
 
 	bio_endio(bio);
-- 
2.47.2

Re: [PATCH 1/5] brd; pass a bvec pointer to brd_do_bvec

[Yu Kuai]

在 2025/04/21 15:26, Christoph Hellwig 写道:
> Pass the bvec to brd_do_bvec instead of marshalling the information into
> individual arguments.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   drivers/block/brd.c | 35 +++++++++++++----------------------
>   1 file changed, 13 insertions(+), 22 deletions(-)
> 
LGTM

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

> diff --git a/drivers/block/brd.c b/drivers/block/brd.c
> index 292f127cae0a..c8974bc545fb 100644
> --- a/drivers/block/brd.c
> +++ b/drivers/block/brd.c
> @@ -189,12 +189,10 @@ static void copy_from_brd(void *dst, struct brd_device *brd,
>   /*
>    * Process a single bvec of a bio.
>    */
> -static int brd_do_bvec(struct brd_device *brd, struct page *page,
> -			unsigned int len, unsigned int off, blk_opf_t opf,
> -			sector_t sector)
> +static int brd_rw_bvec(struct brd_device *brd, struct bio_vec *bv,
> +		blk_opf_t opf, sector_t sector)
>   {
>   	void *mem;
> -	int err = 0;
>   
>   	if (op_is_write(opf)) {
>   		/*
> @@ -202,24 +200,23 @@ static int brd_do_bvec(struct brd_device *brd, struct page *page,
>   		 * 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, len, gfp);
> +		err = copy_to_brd_setup(brd, sector, bv->bv_len, gfp);
>   		if (err)
> -			goto out;
> +			return err;
>   	}
>   
> -	mem = kmap_atomic(page);
> +	mem = kmap_atomic(bv->bv_page);
>   	if (!op_is_write(opf)) {
> -		copy_from_brd(mem + off, brd, sector, len);
> -		flush_dcache_page(page);
> +		copy_from_brd(mem + bv->bv_offset, brd, sector, bv->bv_len);
> +		flush_dcache_page(bv->bv_page);
>   	} else {
> -		flush_dcache_page(page);
> -		copy_to_brd(brd, mem + off, sector, len);
> +		flush_dcache_page(bv->bv_page);
> +		copy_to_brd(brd, mem + bv->bv_offset, sector, bv->bv_len);
>   	}
>   	kunmap_atomic(mem);
> -
> -out:
> -	return err;
> +	return 0;
>   }
>   
>   static void brd_do_discard(struct brd_device *brd, sector_t sector, u32 size)
> @@ -255,15 +252,9 @@ static void brd_submit_bio(struct bio *bio)
>   	}
>   
>   	bio_for_each_segment(bvec, bio, iter) {
> -		unsigned int len = bvec.bv_len;
>   		int err;
>   
> -		/* Don't support un-aligned buffer */
> -		WARN_ON_ONCE((bvec.bv_offset & (SECTOR_SIZE - 1)) ||
> -				(len & (SECTOR_SIZE - 1)));
> -
> -		err = brd_do_bvec(brd, bvec.bv_page, len, bvec.bv_offset,
> -				  bio->bi_opf, sector);
> +		err = brd_rw_bvec(brd, &bvec, bio->bi_opf, sector);
>   		if (err) {
>   			if (err == -ENOMEM && bio->bi_opf & REQ_NOWAIT) {
>   				bio_wouldblock_error(bio);
> @@ -272,7 +263,7 @@ static void brd_submit_bio(struct bio *bio)
>   			bio_io_error(bio);
>   			return;
>   		}
> -		sector += len >> SECTOR_SHIFT;
> +		sector += bvec.bv_len >> SECTOR_SHIFT;
>   	}
>   
>   	bio_endio(bio);
> 

Re: [PATCH 1/5] brd; pass a bvec pointer to brd_do_bvec

[Hannes Reinecke]

On 4/21/25 09:26, Christoph Hellwig wrote:
> Pass the bvec to brd_do_bvec instead of marshalling the information into
> individual arguments.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   drivers/block/brd.c | 35 +++++++++++++----------------------
>   1 file changed, 13 insertions(+), 22 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

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

Re: [PATCH 1/5] brd; pass a bvec pointer to brd_do_bvec

[Johannes Thumshirn]

Nit:
s/;/:/ in Subject

[PATCH 2/5] brd: remove the sector variable in brd_submit_bio

[Christoph Hellwig]

The bvec iter iterates over the sector already, no need to duplicate the
work.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/block/brd.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/block/brd.c b/drivers/block/brd.c
index c8974bc545fb..91eb50126355 100644
--- a/drivers/block/brd.c
+++ b/drivers/block/brd.c
@@ -241,12 +241,12 @@ 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;
-	sector_t sector = bio->bi_iter.bi_sector;
 	struct bio_vec bvec;
 	struct bvec_iter iter;
 
 	if (unlikely(op_is_discard(bio->bi_opf))) {
-		brd_do_discard(brd, sector, bio->bi_iter.bi_size);
+		brd_do_discard(brd, bio->bi_iter.bi_sector,
+				bio->bi_iter.bi_size);
 		bio_endio(bio);
 		return;
 	}
@@ -254,7 +254,7 @@ static void brd_submit_bio(struct bio *bio)
 	bio_for_each_segment(bvec, bio, iter) {
 		int err;
 
-		err = brd_rw_bvec(brd, &bvec, bio->bi_opf, sector);
+		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);
@@ -263,7 +263,6 @@ static void brd_submit_bio(struct bio *bio)
 			bio_io_error(bio);
 			return;
 		}
-		sector += bvec.bv_len >> SECTOR_SHIFT;
 	}
 
 	bio_endio(bio);
-- 
2.47.2

Re: [PATCH 2/5] brd: remove the sector variable in brd_submit_bio

[Yu Kuai]

在 2025/04/21 15:26, Christoph Hellwig 写道:
> The bvec iter iterates over the sector already, no need to duplicate the
> work.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   drivers/block/brd.c | 7 +++----
>   1 file changed, 3 insertions(+), 4 deletions(-)
> 
LGTM

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

> diff --git a/drivers/block/brd.c b/drivers/block/brd.c
> index c8974bc545fb..91eb50126355 100644
> --- a/drivers/block/brd.c
> +++ b/drivers/block/brd.c
> @@ -241,12 +241,12 @@ 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;
> -	sector_t sector = bio->bi_iter.bi_sector;
>   	struct bio_vec bvec;
>   	struct bvec_iter iter;
>   
>   	if (unlikely(op_is_discard(bio->bi_opf))) {
> -		brd_do_discard(brd, sector, bio->bi_iter.bi_size);
> +		brd_do_discard(brd, bio->bi_iter.bi_sector,
> +				bio->bi_iter.bi_size);
>   		bio_endio(bio);
>   		return;
>   	}
> @@ -254,7 +254,7 @@ static void brd_submit_bio(struct bio *bio)
>   	bio_for_each_segment(bvec, bio, iter) {
>   		int err;
>   
> -		err = brd_rw_bvec(brd, &bvec, bio->bi_opf, sector);
> +		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);
> @@ -263,7 +263,6 @@ static void brd_submit_bio(struct bio *bio)
>   			bio_io_error(bio);
>   			return;
>   		}
> -		sector += bvec.bv_len >> SECTOR_SHIFT;
>   	}
>   
>   	bio_endio(bio);
> 

Re: [PATCH 2/5] brd: remove the sector variable in brd_submit_bio

[Hannes Reinecke]

On 4/21/25 09:26, Christoph Hellwig wrote:
> The bvec iter iterates over the sector already, no need to duplicate the
> work.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   drivers/block/brd.c | 7 +++----
>   1 file changed, 3 insertions(+), 4 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

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

[PATCH 3/5] brd: use bvec_kmap_local in brd_do_bvec

[Christoph Hellwig]

Use the proper helper to kmap a bvec in brd_do_bvec instead of directly
accessing the bvec fields and use the deprecated kmap_atomic API.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/block/brd.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/block/brd.c b/drivers/block/brd.c
index 91eb50126355..0c70d29379f1 100644
--- a/drivers/block/brd.c
+++ b/drivers/block/brd.c
@@ -207,15 +207,15 @@ static int brd_rw_bvec(struct brd_device *brd, struct bio_vec *bv,
 			return err;
 	}
 
-	mem = kmap_atomic(bv->bv_page);
+	mem = bvec_kmap_local(bv);
 	if (!op_is_write(opf)) {
-		copy_from_brd(mem + bv->bv_offset, brd, sector, bv->bv_len);
+		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 + bv->bv_offset, sector, bv->bv_len);
+		copy_to_brd(brd, mem, sector, bv->bv_len);
 	}
-	kunmap_atomic(mem);
+	kunmap_local(mem);
 	return 0;
 }
 
-- 
2.47.2

Re: [PATCH 3/5] brd: use bvec_kmap_local in brd_do_bvec

[Yu Kuai]

在 2025/04/21 15:26, Christoph Hellwig 写道:
> Use the proper helper to kmap a bvec in brd_do_bvec instead of directly
> accessing the bvec fields and use the deprecated kmap_atomic API.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   drivers/block/brd.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
LGTM

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

> diff --git a/drivers/block/brd.c b/drivers/block/brd.c
> index 91eb50126355..0c70d29379f1 100644
> --- a/drivers/block/brd.c
> +++ b/drivers/block/brd.c
> @@ -207,15 +207,15 @@ static int brd_rw_bvec(struct brd_device *brd, struct bio_vec *bv,
>   			return err;
>   	}
>   
> -	mem = kmap_atomic(bv->bv_page);
> +	mem = bvec_kmap_local(bv);
>   	if (!op_is_write(opf)) {
> -		copy_from_brd(mem + bv->bv_offset, brd, sector, bv->bv_len);
> +		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 + bv->bv_offset, sector, bv->bv_len);
> +		copy_to_brd(brd, mem, sector, bv->bv_len);
>   	}
> -	kunmap_atomic(mem);
> +	kunmap_local(mem);
>   	return 0;
>   }
>   
> 

Re: [PATCH 3/5] brd: use bvec_kmap_local in brd_do_bvec

[Hannes Reinecke]

On 4/21/25 09:26, Christoph Hellwig wrote:
> Use the proper helper to kmap a bvec in brd_do_bvec instead of directly
> accessing the bvec fields and use the deprecated kmap_atomic API.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   drivers/block/brd.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

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

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

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

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

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

[Hannes Reinecke]

On 4/21/25 09:26, Christoph Hellwig wrote:
> 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(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

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

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

[Yu Kuai]

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

[PATCH 5/5] brd: use memcpy_{to,from]_page in brd_rw_bvec

[Christoph Hellwig]

Use the proper helpers to copy to/from potential highmem pages, which
do a local instead of atomic kmap underneath, and perform
flush_dcache_page where needed.  This also simplifies the code so much
that the separate read write helpers are not required any more.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/block/brd.c | 57 ++++++++++-----------------------------------
 1 file changed, 12 insertions(+), 45 deletions(-)

diff --git a/drivers/block/brd.c b/drivers/block/brd.c
index 580b2d8ce99c..79e96221f887 100644
--- a/drivers/block/brd.c
+++ b/drivers/block/brd.c
@@ -99,43 +99,6 @@ static void brd_free_pages(struct brd_device *brd)
 	xa_destroy(&brd->brd_pages);
 }
 
-/*
- * Copy n bytes from src to the brd starting at sector. Does not sleep.
- */
-static void copy_to_brd(struct brd_device *brd, const void *src,
-			sector_t sector, size_t n)
-{
-	struct page *page;
-	void *dst;
-	unsigned int offset = (sector & (PAGE_SECTORS-1)) << SECTOR_SHIFT;
-
-	page = brd_lookup_page(brd, sector);
-	BUG_ON(!page);
-
-	dst = kmap_atomic(page);
-	memcpy(dst + offset, src, n);
-	kunmap_atomic(dst);
-}
-
-/*
- * Copy n bytes to dst from the brd starting at sector. Does not sleep.
- */
-static void copy_from_brd(void *dst, struct brd_device *brd,
-			sector_t sector, size_t n)
-{
-	struct page *page;
-	void *src;
-	unsigned int offset = (sector & (PAGE_SECTORS-1)) << SECTOR_SHIFT;
-
-	page = brd_lookup_page(brd, sector);
-	if (page) {
-		src = kmap_atomic(page);
-		memcpy(dst, src + offset, n);
-		kunmap_atomic(src);
-	} else
-		memset(dst, 0, n);
-}
-
 /*
  * Process a single segment.  The segment is capped to not cross page boundaries
  * in both the bio and the brd backing memory.
@@ -146,7 +109,8 @@ static bool brd_rw_bvec(struct brd_device *brd, struct bio *bio)
 	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;
+	struct page *page;
+	void *kaddr;
 
 	bv.bv_len = min_t(u32, bv.bv_len, PAGE_SIZE - offset);
 
@@ -168,15 +132,18 @@ static bool brd_rw_bvec(struct brd_device *brd, struct bio *bio)
 		}
 	}
 
-	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);
+	page = brd_lookup_page(brd, sector);
+
+	kaddr = bvec_kmap_local(&bv);
+	if (op_is_write(opf)) {
+		BUG_ON(!page);
+		memcpy_to_page(page, offset, kaddr, bv.bv_len);
+	} else if (page) {
+		memcpy_from_page(kaddr, page, offset, bv.bv_len);
 	} else {
-		flush_dcache_page(bv.bv_page);
-		copy_to_brd(brd, mem, sector, bv.bv_len);
+		memset(kaddr, 0, bv.bv_len);
 	}
-	kunmap_local(mem);
+	kunmap_local(kaddr);
 
 	bio_advance_iter_single(bio, &bio->bi_iter, bv.bv_len);
 	return true;
-- 
2.47.2

Re: [PATCH 5/5] brd: use memcpy_{to,from]_page in brd_rw_bvec

[Hannes Reinecke]

On 4/21/25 09:26, Christoph Hellwig wrote:
> Use the proper helpers to copy to/from potential highmem pages, which
> do a local instead of atomic kmap underneath, and perform
> flush_dcache_page where needed.  This also simplifies the code so much
> that the separate read write helpers are not required any more.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   drivers/block/brd.c | 57 ++++++++++-----------------------------------
>   1 file changed, 12 insertions(+), 45 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

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

Re: [PATCH 5/5] brd: use memcpy_{to,from]_page in brd_rw_bvec

[Yu Kuai]

在 2025/04/21 15:26, Christoph Hellwig 写道:
> Use the proper helpers to copy to/from potential highmem pages, which
> do a local instead of atomic kmap underneath, and perform
> flush_dcache_page where needed.  This also simplifies the code so much
> that the separate read write helpers are not required any more.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   drivers/block/brd.c | 57 ++++++++++-----------------------------------
>   1 file changed, 12 insertions(+), 45 deletions(-)
> 
LGTM

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

> diff --git a/drivers/block/brd.c b/drivers/block/brd.c
> index 580b2d8ce99c..79e96221f887 100644
> --- a/drivers/block/brd.c
> +++ b/drivers/block/brd.c
> @@ -99,43 +99,6 @@ static void brd_free_pages(struct brd_device *brd)
>   	xa_destroy(&brd->brd_pages);
>   }
>   
> -/*
> - * Copy n bytes from src to the brd starting at sector. Does not sleep.
> - */
> -static void copy_to_brd(struct brd_device *brd, const void *src,
> -			sector_t sector, size_t n)
> -{
> -	struct page *page;
> -	void *dst;
> -	unsigned int offset = (sector & (PAGE_SECTORS-1)) << SECTOR_SHIFT;
> -
> -	page = brd_lookup_page(brd, sector);
> -	BUG_ON(!page);
> -
> -	dst = kmap_atomic(page);
> -	memcpy(dst + offset, src, n);
> -	kunmap_atomic(dst);
> -}
> -
> -/*
> - * Copy n bytes to dst from the brd starting at sector. Does not sleep.
> - */
> -static void copy_from_brd(void *dst, struct brd_device *brd,
> -			sector_t sector, size_t n)
> -{
> -	struct page *page;
> -	void *src;
> -	unsigned int offset = (sector & (PAGE_SECTORS-1)) << SECTOR_SHIFT;
> -
> -	page = brd_lookup_page(brd, sector);
> -	if (page) {
> -		src = kmap_atomic(page);
> -		memcpy(dst, src + offset, n);
> -		kunmap_atomic(src);
> -	} else
> -		memset(dst, 0, n);
> -}
> -
>   /*
>    * Process a single segment.  The segment is capped to not cross page boundaries
>    * in both the bio and the brd backing memory.
> @@ -146,7 +109,8 @@ static bool brd_rw_bvec(struct brd_device *brd, struct bio *bio)
>   	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;
> +	struct page *page;
> +	void *kaddr;
>   
>   	bv.bv_len = min_t(u32, bv.bv_len, PAGE_SIZE - offset);
>   
> @@ -168,15 +132,18 @@ static bool brd_rw_bvec(struct brd_device *brd, struct bio *bio)
>   		}
>   	}
>   
> -	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);
> +	page = brd_lookup_page(brd, sector);
> +
> +	kaddr = bvec_kmap_local(&bv);
> +	if (op_is_write(opf)) {
> +		BUG_ON(!page);
> +		memcpy_to_page(page, offset, kaddr, bv.bv_len);
> +	} else if (page) {
> +		memcpy_from_page(kaddr, page, offset, bv.bv_len);
>   	} else {
> -		flush_dcache_page(bv.bv_page);
> -		copy_to_brd(brd, mem, sector, bv.bv_len);
> +		memset(kaddr, 0, bv.bv_len);
>   	}
> -	kunmap_local(mem);
> +	kunmap_local(kaddr);
>   
>   	bio_advance_iter_single(bio, &bio->bi_iter, bv.bv_len);
>   	return true;
> 

Re: [PATCH 5/5] brd: use memcpy_{to,from]_page in brd_rw_bvec

[Christoph Hellwig]

On Tue, Apr 22, 2025 at 07:18:05PM +0800, Yu Kuai wrote:
> Reviewed-by: Yu Kuai <yukuai3@huawei.com>

If you think some or all of this is useful for your discard fixes feel
free to pull some or all of it into that.  Otherwise I'll repost on top
of that once it is finalized.

Re: [PATCH 5/5] brd: use memcpy_{to,from]_page in brd_rw_bvec

[Yu Kuai]

Hi,

在 2025/04/22 21:50, Christoph Hellwig 写道:
> On Tue, Apr 22, 2025 at 07:18:05PM +0800, Yu Kuai wrote:
>> Reviewed-by: Yu Kuai <yukuai3@huawei.com>
> 
> If you think some or all of this is useful for your discard fixes feel
> free to pull some or all of it into that.  Otherwise I'll repost on top
> of that once it is finalized.

ok, I'll try to send a v2 soon.

Thanks,
Kuai

> 
> .
> 

brd cleanups v2

[Christoph Hellwig]

Hi all,

this series has various brd cleanups mostly to get rid kmap_atomic and
poking into the bvec fields.  It is used as the baseline for the discard
fixes from Yu Kuai.

Changes since v1:
 - fix a subject
 - minor tweaks to formating in brd_rw_bvec

Diffstat:
 brd.c |  166 +++++++++++++++---------------------------------------------------
 1 file changed, 38 insertions(+), 128 deletions(-)

[PATCH 5/5] brd: use memcpy_{to,from]_page in brd_rw_bvec

[Christoph Hellwig]

Use the proper helpers to copy to/from potential highmem pages, which
do a local instead of atomic kmap underneath, and perform
flush_dcache_page where needed.  This also simplifies the code so much
that the separate read write helpers are not required any more.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Hannes Reinecke <hare@suse.de>
Reviewed-by: Yu Kuai <yukuai3@huawei.com>
---
 drivers/block/brd.c | 58 ++++++++++-----------------------------------
 1 file changed, 13 insertions(+), 45 deletions(-)

diff --git a/drivers/block/brd.c b/drivers/block/brd.c
index 580b2d8ce99c..fa1290992a7f 100644
--- a/drivers/block/brd.c
+++ b/drivers/block/brd.c
@@ -99,43 +99,6 @@ static void brd_free_pages(struct brd_device *brd)
 	xa_destroy(&brd->brd_pages);
 }
 
-/*
- * Copy n bytes from src to the brd starting at sector. Does not sleep.
- */
-static void copy_to_brd(struct brd_device *brd, const void *src,
-			sector_t sector, size_t n)
-{
-	struct page *page;
-	void *dst;
-	unsigned int offset = (sector & (PAGE_SECTORS-1)) << SECTOR_SHIFT;
-
-	page = brd_lookup_page(brd, sector);
-	BUG_ON(!page);
-
-	dst = kmap_atomic(page);
-	memcpy(dst + offset, src, n);
-	kunmap_atomic(dst);
-}
-
-/*
- * Copy n bytes to dst from the brd starting at sector. Does not sleep.
- */
-static void copy_from_brd(void *dst, struct brd_device *brd,
-			sector_t sector, size_t n)
-{
-	struct page *page;
-	void *src;
-	unsigned int offset = (sector & (PAGE_SECTORS-1)) << SECTOR_SHIFT;
-
-	page = brd_lookup_page(brd, sector);
-	if (page) {
-		src = kmap_atomic(page);
-		memcpy(dst, src + offset, n);
-		kunmap_atomic(src);
-	} else
-		memset(dst, 0, n);
-}
-
 /*
  * Process a single segment.  The segment is capped to not cross page boundaries
  * in both the bio and the brd backing memory.
@@ -146,7 +109,8 @@ static bool brd_rw_bvec(struct brd_device *brd, struct bio *bio)
 	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;
+	struct page *page;
+	void *kaddr;
 
 	bv.bv_len = min_t(u32, bv.bv_len, PAGE_SIZE - offset);
 
@@ -168,15 +132,19 @@ static bool brd_rw_bvec(struct brd_device *brd, struct bio *bio)
 		}
 	}
 
-	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);
+	page = brd_lookup_page(brd, sector);
+
+	kaddr = bvec_kmap_local(&bv);
+	if (op_is_write(opf)) {
+		BUG_ON(!page);
+		memcpy_to_page(page, offset, kaddr, bv.bv_len);
 	} else {
-		flush_dcache_page(bv.bv_page);
-		copy_to_brd(brd, mem, sector, bv.bv_len);
+		if (page)
+			memcpy_from_page(kaddr, page, offset, bv.bv_len);
+		else
+			memset(kaddr, 0, bv.bv_len);
 	}
-	kunmap_local(mem);
+	kunmap_local(kaddr);
 
 	bio_advance_iter_single(bio, &bio->bi_iter, bv.bv_len);
 	return true;
-- 
2.47.2