[PATCH] block: Fix page refcounts for unaligned buffers in __bio_release_pages()

[PATCH] block: Fix page refcounts for unaligned buffers in __bio_release_pages()

[Tony Battersby]

Fix an incorrect number of pages being released for buffers that do not
start at the beginning of a page.

Fixes: 1b151e2435fc ("block: Remove special-casing of compound pages")
Cc: stable@vger.kernel.org
Signed-off-by: Tony Battersby <tonyb@cybernetics.com>
---

Tested with 6.1.79.  The 6.1 backport can just use
folio_put_refs(fi.folio, nr_pages) instead of do {...} while.

 block/bio.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/block/bio.c b/block/bio.c
index b9642a41f286..b52b56067e79 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -1152,7 +1152,7 @@ void __bio_release_pages(struct bio *bio, bool mark_dirty)
 
 	bio_for_each_folio_all(fi, bio) {
 		struct page *page;
-		size_t done = 0;
+		size_t nr_pages;
 
 		if (mark_dirty) {
 			folio_lock(fi.folio);
@@ -1160,10 +1160,11 @@ void __bio_release_pages(struct bio *bio, bool mark_dirty)
 			folio_unlock(fi.folio);
 		}
 		page = folio_page(fi.folio, fi.offset / PAGE_SIZE);
+		nr_pages = (fi.offset + fi.length - 1) / PAGE_SIZE -
+			   fi.offset / PAGE_SIZE + 1;
 		do {
 			bio_release_page(bio, page++);
-			done += PAGE_SIZE;
-		} while (done < fi.length);
+		} while (--nr_pages != 0);
 	}
 }
 EXPORT_SYMBOL_GPL(__bio_release_pages);

base-commit: d206a76d7d2726f3b096037f2079ce0bd3ba329b
-- 
2.25.1

Re: [PATCH] block: Fix page refcounts for unaligned buffers in __bio_release_pages()

[Matthew Wilcox]

On Thu, Feb 29, 2024 at 01:08:09PM -0500, Tony Battersby wrote:
> Fix an incorrect number of pages being released for buffers that do not
> start at the beginning of a page.

Oh, I see what I did.  Wouldn't a simpler fix be to just set "done" to
offset_in_page(fi.offset)?

> @@ -1152,7 +1152,7 @@ void __bio_release_pages(struct bio *bio, bool mark_dirty)
>  
>  	bio_for_each_folio_all(fi, bio) {
>  		struct page *page;
> -		size_t done = 0;
> +		size_t nr_pages;
>  
>  		if (mark_dirty) {
>  			folio_lock(fi.folio);
> @@ -1160,10 +1160,11 @@ void __bio_release_pages(struct bio *bio, bool mark_dirty)
>  			folio_unlock(fi.folio);
>  		}
>  		page = folio_page(fi.folio, fi.offset / PAGE_SIZE);
> +		nr_pages = (fi.offset + fi.length - 1) / PAGE_SIZE -
> +			   fi.offset / PAGE_SIZE + 1;
>  		do {
>  			bio_release_page(bio, page++);
> -			done += PAGE_SIZE;
> -		} while (done < fi.length);
> +		} while (--nr_pages != 0);
>  	}
>  }
>  EXPORT_SYMBOL_GPL(__bio_release_pages);

The long-term path here, I think, is to replace this bio_release_page()
with a bio_release_folio(folio, offset, length) which calls into
a new unpin_user_folio(folio, nr) which calls gup_put_folio().

Re: [PATCH] block: Fix page refcounts for unaligned buffers in __bio_release_pages()

[Tony Battersby]

On 2/29/24 14:53, Matthew Wilcox wrote:
> On Thu, Feb 29, 2024 at 01:08:09PM -0500, Tony Battersby wrote:
>> Fix an incorrect number of pages being released for buffers that do not
>> start at the beginning of a page.
> Oh, I see what I did.  Wouldn't a simpler fix be to just set "done" to
> offset_in_page(fi.offset)?

Actually it would be:

ssize_t done = -offset_in_page(offset);

But then you have signed vs. unsigned comparison in the while(), or you
could rearrange the loop to avoid the negative, but then it gets clunky.

>
>> @@ -1152,7 +1152,7 @@ void __bio_release_pages(struct bio *bio, bool mark_dirty)
>>  
>>  	bio_for_each_folio_all(fi, bio) {
>>  		struct page *page;
>> -		size_t done = 0;
>> +		size_t nr_pages;
>>  
>>  		if (mark_dirty) {
>>  			folio_lock(fi.folio);
>> @@ -1160,10 +1160,11 @@ void __bio_release_pages(struct bio *bio, bool mark_dirty)
>>  			folio_unlock(fi.folio);
>>  		}
>>  		page = folio_page(fi.folio, fi.offset / PAGE_SIZE);
>> +		nr_pages = (fi.offset + fi.length - 1) / PAGE_SIZE -
>> +			   fi.offset / PAGE_SIZE + 1;
>>  		do {
>>  			bio_release_page(bio, page++);
>> -			done += PAGE_SIZE;
>> -		} while (done < fi.length);
>> +		} while (--nr_pages != 0);
>>  	}
>>  }
>>  EXPORT_SYMBOL_GPL(__bio_release_pages);
> The long-term path here, I think, is to replace this bio_release_page()
> with a bio_release_folio(folio, offset, length) which calls into
> a new unpin_user_folio(folio, nr) which calls gup_put_folio().

I developed the patch with the 6.1 stable series, which just has:

nr_pages = (fi.offset + fi.length - 1) / PAGE_SIZE -
	   fi.offset / PAGE_SIZE + 1;
folio_put_refs(fi.folio, nr_pages);

Which is another reason that I went for the direct nr_pages
calculation.  Would you still prefer the negative offset_in_page() approach?

Tony

Re: [PATCH] block: Fix page refcounts for unaligned buffers in __bio_release_pages()

[Greg Edwards]

On Thu, Feb 29, 2024 at 01:08:09PM -0500, Tony Battersby wrote:
> Fix an incorrect number of pages being released for buffers that do not
> start at the beginning of a page.
>
> Fixes: 1b151e2435fc ("block: Remove special-casing of compound pages")
> Cc: stable@vger.kernel.org
> Signed-off-by: Tony Battersby <tonyb@cybernetics.com>
> ---

This resolves the QEMU hugetlb issue I noted earlier today here [1].
I tested it on 6.1.79, 6.8-rc6 and linux-next-20240229.  Thank you!

Feel free to add a:

Tested-by: Greg Edwards <gedwards@ddn.com>

[1] https://lore.kernel.org/linux-block/20240229182513.GA17355@bobdog.home.arpa/

Re: [PATCH] block: Fix page refcounts for unaligned buffers in __bio_release_pages()

[Tony Battersby]

On 2/29/24 17:56, Greg Edwards wrote:
> On Thu, Feb 29, 2024 at 01:08:09PM -0500, Tony Battersby wrote:
>> Fix an incorrect number of pages being released for buffers that do not
>> start at the beginning of a page.
>>
>> Fixes: 1b151e2435fc ("block: Remove special-casing of compound pages")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Tony Battersby <tonyb@cybernetics.com>
>> ---
> This resolves the QEMU hugetlb issue I noted earlier today here [1].
> I tested it on 6.1.79, 6.8-rc6 and linux-next-20240229.  Thank you!
>
> Feel free to add a:
>
> Tested-by: Greg Edwards <gedwards@ddn.com>
>
> [1] https://lore.kernel.org/linux-block/20240229182513.GA17355@bobdog.home.arpa/

Jens, can I get this added to 6.8 (or 6.9 if it is too late)?

Thanks,
Tony

Re: [PATCH] block: Fix page refcounts for unaligned buffers in __bio_release_pages()

[Jens Axboe]

On 3/6/24 8:03 AM, Tony Battersby wrote:
> On 2/29/24 17:56, Greg Edwards wrote:
>> On Thu, Feb 29, 2024 at 01:08:09PM -0500, Tony Battersby wrote:
>>> Fix an incorrect number of pages being released for buffers that do not
>>> start at the beginning of a page.
>>>
>>> Fixes: 1b151e2435fc ("block: Remove special-casing of compound pages")
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Tony Battersby <tonyb@cybernetics.com>
>>> ---
>> This resolves the QEMU hugetlb issue I noted earlier today here [1].
>> I tested it on 6.1.79, 6.8-rc6 and linux-next-20240229.  Thank you!
>>
>> Feel free to add a:
>>
>> Tested-by: Greg Edwards <gedwards@ddn.com>
>>
>> [1] https://lore.kernel.org/linux-block/20240229182513.GA17355@bobdog.home.arpa/
> 
> Jens, can I get this added to 6.8 (or 6.9 if it is too late)?

Let's just go for 6.9 at this pount, we're almost there. I'll queue it
up.

-- 
Jens Axboe

Re: [PATCH] block: Fix page refcounts for unaligned buffers in __bio_release_pages()

[Jens Axboe]

On Thu, 29 Feb 2024 13:08:09 -0500, Tony Battersby wrote:
> Fix an incorrect number of pages being released for buffers that do not
> start at the beginning of a page.
> 
> 

Applied, thanks!

[1/1] block: Fix page refcounts for unaligned buffers in __bio_release_pages()
      commit: 38b43539d64b2fa020b3b9a752a986769f87f7a6

Best regards,
-- 
Jens Axboe

[PATCH] block: Fix page refcounts for unaligned buffers in __bio_release_pages()

[Tony Battersby]

commit 38b43539d64b2fa020b3b9a752a986769f87f7a6 upstream.

Fix an incorrect number of pages being released for buffers that do not
start at the beginning of a page.

  [ Tony: backport to v6.1 by replacing bio_release_page() loop with
    folio_put_refs() as commits fd363244e883 and e4cc64657bec are not
    present. ]

Fixes: 1b151e2435fc ("block: Remove special-casing of compound pages")
Cc: stable@vger.kernel.org
Signed-off-by: Tony Battersby <tonyb@cybernetics.com>
Tested-by: Greg Edwards <gedwards@ddn.com>
Link: https://lore.kernel.org/r/86e592a9-98d4-4cff-a646-0c0084328356@cybernetics.com
Signed-off-by: Jens Axboe <axboe@kernel.dk>
---

This is the backport for 6.1.

The upstream patch should apply cleanly to 6.6, 6.7, and 6.8.

This patch does not need to be backported to 5.15, 5.10, 5.4, or 4.19,
since the backport of 1b151e2435fc to those kernels did not include
the bug fixed by this patch.

 block/bio.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/block/bio.c b/block/bio.c
index 74c2818c7ec9..3318e0022fdf 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -1112,19 +1112,16 @@ void __bio_release_pages(struct bio *bio, bool mark_dirty)
 	struct folio_iter fi;
 
 	bio_for_each_folio_all(fi, bio) {
-		struct page *page;
-		size_t done = 0;
+		size_t nr_pages;
 
 		if (mark_dirty) {
 			folio_lock(fi.folio);
 			folio_mark_dirty(fi.folio);
 			folio_unlock(fi.folio);
 		}
-		page = folio_page(fi.folio, fi.offset / PAGE_SIZE);
-		do {
-			folio_put(fi.folio);
-			done += PAGE_SIZE;
-		} while (done < fi.length);
+		nr_pages = (fi.offset + fi.length - 1) / PAGE_SIZE -
+			   fi.offset / PAGE_SIZE + 1;
+		folio_put_refs(fi.folio, nr_pages);
 	}
 }
 EXPORT_SYMBOL_GPL(__bio_release_pages);

base-commit: 61adba85cc40287232a539e607164f273260e0fe
-- 
2.25.1

Re: [PATCH] block: Fix page refcounts for unaligned buffers in __bio_release_pages()

[Greg KH]

On Wed, Mar 13, 2024 at 10:02:23AM -0400, Tony Battersby wrote:
> commit 38b43539d64b2fa020b3b9a752a986769f87f7a6 upstream.
> 
> Fix an incorrect number of pages being released for buffers that do not
> start at the beginning of a page.
> 
>   [ Tony: backport to v6.1 by replacing bio_release_page() loop with
>     folio_put_refs() as commits fd363244e883 and e4cc64657bec are not
>     present. ]

Now queued up, thanks.

greg k-h