bio_add_* cleanups

bio_add_* cleanups

[Christoph Hellwig]

Hi Jens,

this removes a bit of dead code and then further consolidates the bio_add_*
helpers.

Diffstat:
 bio.c |   89 ++++++++++++++++++++++--------------------------------------------
 blk.h |    4 --
 2 files changed, 30 insertions(+), 63 deletions(-)

[PATCH 1/2] block: remove bio_add_hw_folio

[Christoph Hellwig]

The only user of bio_add_hw_folio was removed in commit cafd00d0e909
("block: remove zone append special casing from the direct I/O path"),
so remove it as well.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/bio.c | 23 -----------------------
 block/blk.h |  4 ----
 2 files changed, 27 deletions(-)

diff --git a/block/bio.c b/block/bio.c
index daceb0a5c1d7..1f6ac44b4881 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -1017,29 +1017,6 @@ int bio_add_hw_page(struct request_queue *q, struct bio *bio,
 	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
diff --git a/block/blk.h b/block/blk.h
index 63d5df0dc29c..837a57ed1911 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -569,10 +569,6 @@ 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);
 
-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);
-
 /*
  * Clean up a page appropriately, where the page may be pinned, may have a
  * ref taken on it or neither.
-- 
2.45.2

Re: [PATCH 1/2] block: remove bio_add_hw_folio

[Kundan Kumar]

[-- Attachment #1: Type: text/plain, Size: 314 bytes --]

On 05/11/24 04:52PM, Christoph Hellwig wrote:
>The only user of bio_add_hw_folio was removed in commit cafd00d0e909
>("block: remove zone append special casing from the direct I/O path"),
>so remove it as well.
>
>Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Kundan Kumar <kundan.kumar@samsung.com>

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

[PATCH 2/2] block: share more code for bio addition helpers

[Christoph Hellwig]

__bio_iov_iter_get_pages currently open codes adding pages to the bio,
which duplicates a lot of code from bio_add_page.  Add a lower level
bio_do_add_page helpers that passes down the same_page output argument
so that __bio_iov_iter_get_pages can reuse the code and also check
for the error return from it - while no error should happen due to
how the bvec space is used for the pin_user_space result I'd rather
be safe than sorry and actually check for these impossible errors.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/bio.c | 66 ++++++++++++++++++++++++-----------------------------
 1 file changed, 30 insertions(+), 36 deletions(-)

diff --git a/block/bio.c b/block/bio.c
index 1f6ac44b4881..bc3bca5f0686 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -1063,20 +1063,9 @@ void __bio_add_page(struct bio *bio, struct page *page,
 }
 EXPORT_SYMBOL_GPL(__bio_add_page);
 
-/**
- *	bio_add_page	-	attempt to add page(s) to bio
- *	@bio: destination bio
- *	@page: start page to add
- *	@len: vec entry length, may cross pages
- *	@offset: vec entry offset relative to @page, may cross pages
- *
- *	Attempt to add page(s) to the bio_vec maplist. This will only fail
- *	if either bio->bi_vcnt == bio->bi_max_vecs or it's a cloned bio.
- */
-int bio_add_page(struct bio *bio, struct page *page,
-		 unsigned int len, unsigned int offset)
+static int bio_do_add_page(struct bio *bio, struct page *page,
+		 unsigned int len, unsigned int offset, bool *same_page)
 {
-	bool same_page = false;
 
 	if (WARN_ON_ONCE(bio_flagged(bio, BIO_CLONED)))
 		return 0;
@@ -1085,7 +1074,7 @@ int bio_add_page(struct bio *bio, struct page *page,
 
 	if (bio->bi_vcnt > 0 &&
 	    bvec_try_merge_page(&bio->bi_io_vec[bio->bi_vcnt - 1],
-				page, len, offset, &same_page)) {
+				page, len, offset, same_page)) {
 		bio->bi_iter.bi_size += len;
 		return len;
 	}
@@ -1095,6 +1084,24 @@ int bio_add_page(struct bio *bio, struct page *page,
 	__bio_add_page(bio, page, len, offset);
 	return len;
 }
+
+/**
+ * bio_add_page	- attempt to add page(s) to bio
+ * @bio: destination bio
+ * @page: start page to add
+ * @len: vec entry length, may cross pages
+ * @offset: vec entry offset relative to @page, may cross pages
+ *
+ * Attempt to add page(s) to the bio_vec maplist.  Will only fail if the
+ * bio is full, or it is incorrectly used on a cloned bio.
+ */
+int bio_add_page(struct bio *bio, struct page *page,
+		 unsigned int len, unsigned int offset)
+{
+	bool same_page = false;
+
+	return bio_do_add_page(bio, page, len, offset, &same_page);
+}
 EXPORT_SYMBOL(bio_add_page);
 
 void bio_add_folio_nofail(struct bio *bio, struct folio *folio, size_t len,
@@ -1159,27 +1166,6 @@ void bio_iov_bvec_set(struct bio *bio, struct iov_iter *iter)
 	bio_set_flag(bio, BIO_CLONED);
 }
 
-static int bio_iov_add_folio(struct bio *bio, struct folio *folio, size_t len,
-			     size_t offset)
-{
-	bool same_page = false;
-
-	if (WARN_ON_ONCE(bio->bi_iter.bi_size > UINT_MAX - len))
-		return -EIO;
-
-	if (bio->bi_vcnt > 0 &&
-	    bvec_try_merge_page(&bio->bi_io_vec[bio->bi_vcnt - 1],
-				folio_page(folio, 0), len, offset,
-				&same_page)) {
-		bio->bi_iter.bi_size += len;
-		if (same_page && bio_flagged(bio, BIO_PAGE_PINNED))
-			unpin_user_folio(folio, 1);
-		return 0;
-	}
-	bio_add_folio_nofail(bio, folio, len, offset);
-	return 0;
-}
-
 static unsigned int get_contig_folio_len(unsigned int *num_pages,
 					 struct page **pages, unsigned int i,
 					 struct folio *folio, size_t left,
@@ -1274,6 +1260,7 @@ static int __bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter)
 	for (left = size, i = 0; left > 0; left -= len, i += num_pages) {
 		struct page *page = pages[i];
 		struct folio *folio = page_folio(page);
+		bool same_page = false;
 
 		folio_offset = ((size_t)folio_page_idx(folio, page) <<
 			       PAGE_SHIFT) + offset;
@@ -1286,7 +1273,14 @@ static int __bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter)
 			len = get_contig_folio_len(&num_pages, pages, i,
 						   folio, left, offset);
 
-		bio_iov_add_folio(bio, folio, len, folio_offset);
+		if (bio_do_add_page(bio, folio_page(folio, 0), len,
+				folio_offset, &same_page) != len) {
+			ret = -EINVAL;
+			break;
+		}
+
+		if (same_page && bio_flagged(bio, BIO_PAGE_PINNED))
+			unpin_user_folio(folio, 1);
 		offset = 0;
 	}
 
-- 
2.45.2

Re: [PATCH 2/2] block: share more code for bio addition helpers

[Kundan Kumar]

[-- Attachment #1: Type: text/plain, Size: 1826 bytes --]

On 05/11/24 04:52PM, Christoph Hellwig wrote:
>__bio_iov_iter_get_pages currently open codes adding pages to the bio,
>which duplicates a lot of code from bio_add_page.  Add a lower level
>bio_do_add_page helpers that passes down the same_page output argument
>so that __bio_iov_iter_get_pages can reuse the code and also check
>for the error return from it - while no error should happen due to
>how the bvec space is used for the pin_user_space result I'd rather
>be safe than sorry and actually check for these impossible errors.
>
>Signed-off-by: Christoph Hellwig <hch@lst.de>
>---
> block/bio.c | 66 ++++++++++++++++++++++++-----------------------------
> 1 file changed, 30 insertions(+), 36 deletions(-)
>
>diff --git a/block/bio.c b/block/bio.c
>index 1f6ac44b4881..bc3bca5f0686 100644
>--- a/block/bio.c
>+++ b/block/bio.c
>@@ -1063,20 +1063,9 @@ void __bio_add_page(struct bio *bio, struct page *page,
> }
> EXPORT_SYMBOL_GPL(__bio_add_page);
>
>-/**
>- *	bio_add_page	-	attempt to add page(s) to bio
>- *	@bio: destination bio
>- *	@page: start page to add
>- *	@len: vec entry length, may cross pages
>- *	@offset: vec entry offset relative to @page, may cross pages
>- *
>- *	Attempt to add page(s) to the bio_vec maplist. This will only fail
>- *	if either bio->bi_vcnt == bio->bi_max_vecs or it's a cloned bio.
>- */
>-int bio_add_page(struct bio *bio, struct page *page,
>-		 unsigned int len, unsigned int offset)
>+static int bio_do_add_page(struct bio *bio, struct page *page,
>+		 unsigned int len, unsigned int offset, bool *same_page)

As we are passing length within a folio, values will reach near UINT_MAX.
It will be better to make len and offset as size_t, also to add a check like :
if (len > UINT_MAX || offset > UINT_MAX)
		return 0; 

> {
>-	bool same_page = false;

nit: extra line got added


[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 2/2] block: share more code for bio addition helpers

[Christoph Hellwig]

On Wed, Nov 06, 2024 at 03:33:38PM +0530, Kundan Kumar wrote:
>> -int bio_add_page(struct bio *bio, struct page *page,
>> -		 unsigned int len, unsigned int offset)
>> +static int bio_do_add_page(struct bio *bio, struct page *page,
>> +		 unsigned int len, unsigned int offset, bool *same_page)
>
> As we are passing length within a folio, values will reach near UINT_MAX.
> It will be better to make len and offset as size_t, also to add a check like :
> if (len > UINT_MAX || offset > UINT_MAX)
> 		return 0;

Not sure what the point is.  IFF we get folio sizes overflowing an
unsigned int we'll have a massive problem as it will overflow the
bv_offset and bv_len fields.  So we'd need to address that first before
doing anything else.

>> {
>> -	bool same_page = false;
>
> nit: extra line got added

Yes, the previous code was missing the empty line after the variable
declaration, so this got fixed.

Re: [PATCH 2/2] block: share more code for bio addition helpers

[Kundan Kumar]

[-- Attachment #1: Type: text/plain, Size: 1343 bytes --]

On 07/11/24 08:20AM, Christoph Hellwig wrote:
>On Wed, Nov 06, 2024 at 03:33:38PM +0530, Kundan Kumar wrote:
>>> -int bio_add_page(struct bio *bio, struct page *page,
>>> -		 unsigned int len, unsigned int offset)
>>> +static int bio_do_add_page(struct bio *bio, struct page *page,
>>> +		 unsigned int len, unsigned int offset, bool *same_page)
>>
>> As we are passing length within a folio, values will reach near UINT_MAX.
>> It will be better to make len and offset as size_t, also to add a check like :
>> if (len > UINT_MAX || offset > UINT_MAX)
>> 		return 0;
>
>Not sure what the point is.  IFF we get folio sizes overflowing an
>unsigned int we'll have a massive problem as it will overflow the
>bv_offset and bv_len fields.  So we'd need to address that first before
>doing anything else.

In my opinion, there isn't a current use case where it overflows, but
its better to include a check for the future. WARN_ON is also an option
as used in bio_add_folio_nofail()

>
>>> {
>>> -	bool same_page = false;
>>
>> nit: extra line got added
>
>Yes, the previous code was missing the empty line after the variable
>declaration, so this got fixed.
>

Actually the extra line got introduced at the beginning of function
definition of bio_do_add_page().

Otherwise the code looks good. 

Reviewed-by: Kundan Kumar <kundan.kumar@samsung.com>

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]