diff mbox series

block: fix adding folio to bio

Message ID 20250312113805.2868986-1-ming.lei@redhat.com (mailing list archive)
State New
Headers show
Series block: fix adding folio to bio | expand

Commit Message

Ming Lei March 12, 2025, 11:38 a.m. UTC 
>4GB folio is possible on some ARCHs, such as aarch64, 16GB hugepage
is supported, then 'offset' of folio can't be held in 'unsigned int',
cause warning in bio_add_folio_nofail() and IO failure.

Fix it by adjusting 'page' & 'offset' so that 'offset' can be stored
in 'unsigned int'.

Fixes: ed9832bc08db ("block: introduce folio awareness and add a bigger size from folio")
Cc: Kundan Kumar <kundan.kumar@samsung.com>
Cc: Matthew Wilcox (Oracle) <willy@infradead.org>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Luis Chamberlain <mcgrof@kernel.org>
Cc: Gavin Shan <gshan@redhat.com>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/bio.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

Comments

Matthew Wilcox March 12, 2025, 12:19 p.m. UTC | #1 
On Wed, Mar 12, 2025 at 07:38:05PM +0800, Ming Lei wrote:
> +++ b/block/bio.c
> @@ -1026,9 +1026,17 @@ EXPORT_SYMBOL(bio_add_page);
>  void bio_add_folio_nofail(struct bio *bio, struct folio *folio, size_t len,
>  			  size_t off)
>  {
> +	struct page *page = &folio->page;
> +
>  	WARN_ON_ONCE(len > UINT_MAX);
> -	WARN_ON_ONCE(off > UINT_MAX);
> -	__bio_add_page(bio, &folio->page, len, off);
> +	if (unlikely(off > UINT_MAX)) {

I think we should probably make this:

	if (unlikely(off + len > UINT_MAX))

because I'm not sure that everything will cope well with an I/O that
crosses the 4GB boundary.

Actually, why bother with the conditional?  Let's just do it always.

 {
+	unsigned long nr = off / PAGE_SIZE;
 	WARN_ON_ONCE(len > UINT_MAX);
-	WARN_ON_ONCE(off > UINT_MAX);
-	__bio_add_page(bio, &folio->page, len, off);
+	off = off % PAGE_SIZE;
+	__bio_add_page(bio, folio_page(folio, nr), len, off);
 }

Also you need to do bio_add_folio(), not just the _nofail variant.
Ming Lei March 12, 2025, 2:02 p.m. UTC | #2 
On Wed, Mar 12, 2025 at 12:19:12PM +0000, Matthew Wilcox wrote:
> On Wed, Mar 12, 2025 at 07:38:05PM +0800, Ming Lei wrote:
> > +++ b/block/bio.c
> > @@ -1026,9 +1026,17 @@ EXPORT_SYMBOL(bio_add_page);
> >  void bio_add_folio_nofail(struct bio *bio, struct folio *folio, size_t len,
> >  			  size_t off)
> >  {
> > +	struct page *page = &folio->page;
> > +
> >  	WARN_ON_ONCE(len > UINT_MAX);
> > -	WARN_ON_ONCE(off > UINT_MAX);
> > -	__bio_add_page(bio, &folio->page, len, off);
> > +	if (unlikely(off > UINT_MAX)) {
> 
> I think we should probably make this:
> 
> 	if (unlikely(off + len > UINT_MAX))
> 
> because I'm not sure that everything will cope well with an I/O that
> crosses the 4GB boundary.

If hardware doesn't support it, the bio will be splitted before submitting
to the disk, so I think the check isn't needed here.

> 
> Actually, why bother with the conditional?  Let's just do it always.
> 
>  {
> +	unsigned long nr = off / PAGE_SIZE;
>  	WARN_ON_ONCE(len > UINT_MAX);
> -	WARN_ON_ONCE(off > UINT_MAX);
> -	__bio_add_page(bio, &folio->page, len, off);
> +	off = off % PAGE_SIZE;
> +	__bio_add_page(bio, folio_page(folio, nr), len, off);
>  }
> 
> Also you need to do bio_add_folio(), not just the _nofail variant.
 
OK, will cover bio_add_folio() in V2 by the unconditional way.


Thanks,
Ming
diff mbox series

Patch

diff --git a/block/bio.c b/block/bio.c
index f0c416e5931d..dd61d783717f 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -1026,9 +1026,17 @@  EXPORT_SYMBOL(bio_add_page);
 void bio_add_folio_nofail(struct bio *bio, struct folio *folio, size_t len,
 			  size_t off)
 {
+	struct page *page = &folio->page;
+
 	WARN_ON_ONCE(len > UINT_MAX);
-	WARN_ON_ONCE(off > UINT_MAX);
-	__bio_add_page(bio, &folio->page, len, off);
+	if (unlikely(off > UINT_MAX)) {
+		unsigned long nr = off / PAGE_SIZE;
+
+		page = folio_page(folio, nr);
+		off -= nr * PAGE_SIZE;
+	}
+
+	__bio_add_page(bio, page, len, off);
 }
 EXPORT_SYMBOL_GPL(bio_add_folio_nofail);