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

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

Posted by Tony Battersby 1 year, 11 months ago

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

Posted by Jens Axboe 1 year, 11 months ago

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

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

Posted by Greg Edwards 1 year, 11 months ago

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

Posted by Tony Battersby 1 year, 11 months ago

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

Posted by Jens Axboe 1 year, 11 months ago

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

Posted by Matthew Wilcox 1 year, 11 months ago

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

Posted by Tony Battersby 1 year, 11 months ago

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