On 5/3/24 17:26, Matthew Wilcox wrote:
On Thu, May 02, 2024 at 02:53:40PM +0200, Christoph Hellwig wrote:
On Thu, May 02, 2024 at 08:45:33AM +0200, Hannes Reinecke wrote:
- nr_pages = (fi.offset + fi.length - 1) / PAGE_SIZE -
- fi.offset / PAGE_SIZE + 1;
- do {
- bio_release_page(bio, page++);
- } while (--nr_pages != 0);
+ bio_release_page(bio, page);
Errm. I guess you need to call 'folio_put()' here, otherwise the page
reference counting will be messed up.
It shouldn't. See the rfc patch and explanation that Keith sent in reply
to the previous version. But as I wrote earlier it should be a separate
prep patch including a commit log clearly explaining the reason for it
and how it works.
I think this is wandering into a minefield. I'm pretty sure
it's considered valid to split the bio, and complete the two halves
independently. Each one will put the refcounts for the pages it touches,
and if we do this early putting of references, that's going to fail.
Precisesly my worries. Something I want to talk to you about at LSF;
refcounting of folios vs refcounting of pages.
When one takes a refcount on a folio we are actually taking a refcount
on the first page, which is okay if we stick with using the folio
throughout the call chain. But if we start mixing between pages and
folios (as we do here) we will be getting the refcount wrong.
Do you have plans how we could improve the situation?
Like a warning 'Hey, you've used the folio for taking the reference, but
now you are releasing the references for the page'?
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@xxxxxxx +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich
