| Message ID | 86e592a9-98d4-4cff-a646-0c0084328356@cybernetics.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: Fix page refcounts for unaligned buffers in __bio_release_pages() | expand |
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().
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
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/
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
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.
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,
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);
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(-) base-commit: d206a76d7d2726f3b096037f2079ce0bd3ba329b