Re: [PATCH 1/2] mm: vmscan: remove folio_test_private() check in pageout()

[Baolin Wang]

On 2025/9/17 15:49, Hugh Dickins wrote:
> On Wed, 17 Sep 2025, Baolin Wang wrote:
> > On 2025/9/16 15:18, Baolin Wang wrote:
> ...
> > > Additionally, I'm still struggling to understand this case where a folio is
> > > dirty but has a NULL mapping, but I might understand that ext3 journaling
> > > might do this from the comments in truncate_cleanup_folio().
> > >
> > > But I still doubt whether this case exists because the refcount check in
> > > is_page_cache_freeable() considers the pagecache. This means if this dirty
> > > folio's mapping is NULL, the following check would return false. If it
> > > returns true, it means that even if we release the private data here, the
> > > orphaned folio's refcount still doesn't meet the requirements for being
> > > reclaimed. Please correct me if I missed anything.
> > >
> > > static inline int is_page_cache_freeable(struct folio *folio)
> > > {
> > >           /*
> > >            * A freeable page cache folio is referenced only by the caller
> > >            * that isolated the folio, the page cache and optional filesystem
> > >            * private data at folio->private.
> > >            */
> > >           return folio_ref_count(folio) - folio_test_private(folio) ==
> > >                   1 + folio_nr_pages(folio);
> > > }
>
> Good point, yes, it's surprising that that such a folio could pass
> that check and reach the code you're proposing to delete.
>
> (Though a racing scanner of physical memory could raise the refcount
> momentarily, causing the folio to look like a page cache freeable.)
>
> > I continued to dig into the historical commits, where the private check was
> > introduced in 2005 by commit ce91b575332b ("orphaned pagecache memleak fix"),
> > as the commit message mentioned, it was to address the issue where reiserfs
> > pagecache may be truncated while still pinned:
>
> Yes, I had been doing the same research, coming to that same 2.6.12 commit,
> one of the last to go in before the birth of git.
>
> > "
> > Chris found that with data journaling a reiserfs pagecache may be truncate
> > while still pinned.  The truncation removes the page->mapping, but the page is
> > still listed in the VM queues because it still has buffers.  Then during the
> > journaling process, a buffer is marked dirty and that sets the PG_dirty
> > bitflag as well (in mark_buffer_dirty). After that the page is leaked because
> > it's both dirty and without a mapping.
> >
> > So we must allow pages without mapping and dirty to reach the PagePrivate
> > check.  The page->mapping will be checked again right after the PagePrivate
> > check.
> > "
> >
> > In 2008, commit a2b345642f530 ("Fix dirty page accounting leak with ext3
> > data=journal") seems to be dealing with a similar issue, where the page
> > becomes dirty after truncation, and provides a very useful call stack:
> > truncate_complete_page()
> >        cancel_dirty_page() // PG_dirty cleared, decr. dirty pages
> >        do_invalidatepage()
> >          ext3_invalidatepage()
> >            journal_invalidatepage()
> >              journal_unmap_buffer()
> >                __dispose_buffer()
> >                  __journal_unfile_buffer()
> >                    __journal_temp_unlink_buffer()
> >                      mark_buffer_dirty(); // PG_dirty set, incr. dirty pages
> >
> > In this fix, we forcefully clear the page's dirty flag during truncation (in
> > truncate_complete_page()).
>
> But missed that one.
>
> > However, I am still unsure how the reiserfs case is checked through
> > is_page_cache_freeable() (if the pagecache is truncated, then the pagecache
> > refcount would be decreased). Fortunately, reiserfs was removed in 2024 by
> > commit fb6f20ecb121 ("reiserfs: The last commit").
>
> I did find a single report of the "pageout: orphaned page" message
> (where Andrew claims the message as his forgotten temporary debugging):
>
> https://lore.kernel.org/all/20061002170353.GA26816@xxxxxxxxxxxxxxxxx/
>
>  From 2006 on 2.6.18: and indeed it was on reiserfs - maybe reiserfs
> had some extra refcounting on these pages, which caused them to pass
> the is_page_cache_freeable() check (but would they actually be freeable,
> or leaked? TBH I haven't tried to work that out, nor care very much).
>
> Where does this leave us?  I think it says that your deletion of that
> block from pageout() is acceptable now, with reiserfs gone to history.
>
> Though somehow I would prefer, like that ext3 fix, that we would just
> clear dirty on such a folio (to avoid "Bad page state" later if it is
> freeable), not go to pageout(), but proceed to the folio_needs_release()
> block like for clean folios.
>
> But whatever: you've persuaded me! I withdraw my objection to your patch.

Thanks for confirming. I will update the commit message based on our 
discussion.