Re: [PATCH 7/7] khugepaged: Use a folio throughout hpage_collapse_scan_file()

[Matthew Wilcox <willy@infradead.org>]

On Mon, Apr 08, 2024 at 10:07:01PM +0200, David Hildenbrand wrote:
> On 03.04.24 19:18, Matthew Wilcox (Oracle) wrote:
> > Replace the use of pages with folios.  Saves a few calls to
> > compound_head() and removes some uses of obsolete functions.
> > 
> > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> > ---
> 
> [...]
> 
> > -		if (page_count(page) !=
> > -		    1 + page_mapcount(page) + page_has_private(page)) {
> > +		if (folio_ref_count(folio) !=
> > +		    1 + folio_mapcount(folio) + folio_test_private(folio)) {
> >   			result = SCAN_PAGE_COUNT;
> 
> Just stumbled over that: page_has_private() would have checked PG_private
> and PG_private_2. The corresponding replacement would have been
> folio_has_private(). folio_test_private() only checks PG_private.
> 
> Are we sure that we no longer have to check PG_private_2 here? pagecache ...
> so I have no clue :)

Oh man.  Vishal just asked me about this in our meeting and I struggled
to explain the whole horrid history.  The short answer is that this
almost certainly fixes a bug.

We have a rule (which most filesystem developers don't know about) that
attaching private data to the folio should increase its refcount by one.
This is handled by folio_attach_private().  But there's no corresponding
rule that setting PG_private_2 should also increment the refcount.
So checking PG_private_2 is wrong here because a folio with PG_private_2
set and PG_private clear will not have its refcount increased.

There are longer versions of this answer, and if you keep asking, I'll
give them ;-P