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

[Shakeel Butt <shakeel.butt@linux.dev>]

On Thu, Sep 18, 2025 at 05:36:17PM +0800, Baolin Wang wrote:
> 
> 
> On 2025/9/18 14:00, David Hildenbrand wrote:
> > On 18.09.25 05:46, Baolin Wang wrote:
> > > The folio_test_private() check in pageout() was introduced by commit
> > > ce91b575332b ("orphaned pagecache memleak fix") in 2005 (checked from
> > > a history tree[1]). As the commit message mentioned, it was to address
> > > the issue where reiserfs pagecache may be truncated while still pinned.
> > > To further explain, the truncation removes the page->mapping, but the
> > > page is still listed in the VM queues because it still has buffers.
> > > 
> > > 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 it 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 commit a2b345642f530, we forcefully clear the page's dirty flag
> > > during truncation (in truncate_complete_page()).
> > > 
> > > Now it seems this was just a peculiar usage specific to reiserfs. Maybe
> > > reiserfs had some extra refcount on these pages, which caused them
> > > to pass
> > > the is_page_cache_freeable() check. With the fix provided by commit
> > > a2b345642f530
> > > and reiserfs being removed in 2024 by commit fb6f20ecb121 ("reiserfs: The
> > > last commit"), such a case is unlikely to occur again. So let's
> > > remove the
> > > redundant folio_test_private() checks and related buffer_head
> > > release logic,
> > > and just leave a warning here to catch such a bug.
> > > 
> > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git
> > > Acked-by: David Hildenbrand <david@redhat.com>
> > > Acked-by: Shakeel Butt <shakeel.butt@linux.dev>
> > > Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
> > > ---
> > >   mm/vmscan.c | 12 +++---------
> > >   1 file changed, 3 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > > index f1fc36729ddd..930add6d90ab 100644
> > > --- a/mm/vmscan.c
> > > +++ b/mm/vmscan.c
> > > @@ -701,16 +701,10 @@ static pageout_t pageout(struct folio *folio,
> > > struct address_space *mapping,
> > >           return PAGE_KEEP;
> > >       if (!mapping) {
> > >           /*
> > > -         * Some data journaling orphaned folios can have
> > > -         * folio->mapping == NULL while being dirty with clean buffers.
> > > +         * Is it still possible to have a dirty folio with
> > > +         * a NULL mapping? I think not.
> > >            */
> > 
> > I would rephrase slightly (removing the "I think not"):
> > 
> > /*
> >   * We should no longer have dirty folios with clean buffers and a NULL
> >   * mapping. However, let's be careful for now.
> >   */
> 
> LGTM.
> 
> Andrew, could you help squash these comments into this patch? Thanks.
> 
> > > -        if (folio_test_private(folio)) {
> > > -            if (try_to_free_buffers(folio)) {
> > > -                folio_clear_dirty(folio);
> > > -                pr_info("%s: orphaned folio\n", __func__);
> > > -                return PAGE_CLEAN;
> > > -            }
> > > -        }
> > > +        VM_WARN_ON_FOLIO(true, folio);

Unexpected but better to use VM_WARN_ON_ONCE_FOLIO here.