fix bio_add_hw_page for larger folios / compound pages

fix bio_add_hw_page for larger folios / compound pages

[Christoph Hellwig]

Hi Jens,

bio_add_hw_page currently fails miserably when trying to add larger
contiguous ranges than support by the underlying hardware, a it
always adds everything or nothing.  That isn't really a problem yet
as there are no callers that actually pass anything where off + len
doesn't fit in a single page, but I've been working on code that
will, which immediately tripped it.

Diffstat:
 bio.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

[PATCH 1/2] block: prevent an integer overflow in bvec_try_merge_hw_page

[Christoph Hellwig]

Reordered a check to avoid a possible overflow when adding len to bv_len.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/bio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/bio.c b/block/bio.c
index 816d412c06e9b4..cef830adbc06e0 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -944,7 +944,7 @@ bool bvec_try_merge_hw_page(struct request_queue *q, struct bio_vec *bv,
 
 	if ((addr1 | mask) != (addr2 | mask))
 		return false;
-	if (bv->bv_len + len > queue_max_segment_size(q))
+	if (len > queue_max_segment_size(q) - bv->bv_len)
 		return false;
 	return bvec_try_merge_page(bv, page, len, offset, same_page);
 }
-- 
2.39.2

[PATCH 2/2] block: support adding less than len in bio_add_hw_page

[Christoph Hellwig]

bio_add_hw_page currently always fails or succeeds.  This is fine for
the existing callers that always add PAGE_SIZE worth given that the
max_segment_size and max_sectors must always allow at least a page
worth of data.  But when we want to add it for bigger amounts of data
this means it can also fail when adding the data to a bio, and creating
a fallback for that becomes really annoying in the callers.

Make use of the existing API design that allows to return a smaller
length than the one passed in and add up to max_segment_size worth
of data from a larger input.  All the existing callers are fine with
this - not because they handle this return correctly, but because they
never pass more than a page in.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/bio.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/block/bio.c b/block/bio.c
index cef830adbc06e0..335d81398991b3 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -966,10 +966,13 @@ 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;
 
-	if (((bio->bi_iter.bi_size + len) >> SECTOR_SHIFT) > max_sectors)
+	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) {
-- 
2.39.2

Re: [PATCH 2/2] block: support adding less than len in bio_add_hw_page

[Johannes Thumshirn]

On 04.12.23 18:35, Christoph Hellwig wrote:
> [...] All the existing callers are fine with
> this - not because they handle this return correctly, but because they
> never pass more than a page in.


Wouldn't it also be beneficial to do proper return checking in the 
current callers on top of this series?

Re: [PATCH 2/2] block: support adding less than len in bio_add_hw_page

[Christoph Hellwig]

On Tue, Dec 05, 2023 at 04:34:02PM +0000, Johannes Thumshirn wrote:
> On 04.12.23 18:35, Christoph Hellwig wrote:
> > [...] All the existing callers are fine with
> > this - not because they handle this return correctly, but because they
> > never pass more than a page in.
> 
> 
> Wouldn't it also be beneficial to do proper return checking in the 
> current callers on top of this series?

I did look into that - but given how they are structured it would
create an even bigger mess.  Except for the nvmet zns backend they
all either allocate the added page right next to them or take the
output from a pin_user_pages variant.

Re: [PATCH 2/2] block: support adding less than len in bio_add_hw_page

[Keith Busch]

On Mon, Dec 04, 2023 at 06:34:19PM +0100, Christoph Hellwig wrote:
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/block/bio.c b/block/bio.c
> index cef830adbc06e0..335d81398991b3 100644
> --- a/block/bio.c
> +++ b/block/bio.c
> @@ -966,10 +966,13 @@ 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;
>  
> -	if (((bio->bi_iter.bi_size + len) >> SECTOR_SHIFT) > max_sectors)
> +	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) {

Not related to your patch, but noticed while reviewing: would it not be
beneficial to truncate 'len' down to what can fit in the current-segment
instead of assuming the max segment size?

Re: [PATCH 2/2] block: support adding less than len in bio_add_hw_page

[Christoph Hellwig]

On Tue, Dec 05, 2023 at 09:37:47AM -0700, Keith Busch wrote:
> > -	if (((bio->bi_iter.bi_size + len) >> SECTOR_SHIFT) > max_sectors)
> > +	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) {
> 
> Not related to your patch, but noticed while reviewing: would it not be
> beneficial to truncate 'len' down to what can fit in the current-segment
> instead of assuming the max segment size?

That would be pretty intrusive to the code shared with the normal not
hw limited bio add code, so I decided to keep it simple.

Re: fix bio_add_hw_page for larger folios / compound pages

[Johannes Thumshirn]

On 04.12.23 18:35, Christoph Hellwig wrote:
> Hi Jens,
> 
> bio_add_hw_page currently fails miserably when trying to add larger
> contiguous ranges than support by the underlying hardware, a it
> always adds everything or nothing.  That isn't really a problem yet
> as there are no callers that actually pass anything where off + len
> doesn't fit in a single page, but I've been working on code that
> will, which immediately tripped it.
> 
> Diffstat:
>   bio.c |    7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 
> 

For the series,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: fix bio_add_hw_page for larger folios / compound pages

[Christoph Hellwig]

Jens,

does this looks good to you, or do you want any tweaks to it.

Re: fix bio_add_hw_page for larger folios / compound pages

[Jens Axboe]

On Mon, 04 Dec 2023 18:34:17 +0100, Christoph Hellwig wrote:
> bio_add_hw_page currently fails miserably when trying to add larger
> contiguous ranges than support by the underlying hardware, a it
> always adds everything or nothing.  That isn't really a problem yet
> as there are no callers that actually pass anything where off + len
> doesn't fit in a single page, but I've been working on code that
> will, which immediately tripped it.
> 
> [...]

Applied, thanks!

[1/2] block: prevent an integer overflow in bvec_try_merge_hw_page
      commit: 3f034c374ad55773c12dd8f3c1607328e17c0072
[2/2] block: support adding less than len in bio_add_hw_page
      commit: 6ef02df154a245a4a7c0a66daa5a353daa788dba

Best regards,
-- 
Jens Axboe