Message-ID: <122ec2b9-8f0f-4184-a15b-8f3ccbd336ea@linux.alibaba.com>
Date: Thu, 18 Sep 2025 10:22:22 +0800
From: Baolin Wang <baolin.wang@...ux.alibaba.com>
To: Hugh Dickins <hughd@...gle.com>
Cc: Shakeel Butt <shakeel.butt@...ux.dev>, akpm@...ux-foundation.org,
 hannes@...xchg.org, david@...hat.com, mhocko@...nel.org,
 zhengqi.arch@...edance.com, lorenzo.stoakes@...cle.com, willy@...radead.org,
 linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] mm: vmscan: remove folio_test_private() check in
 pageout()



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@king.bitgnome.net/
> 
>  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.

Powered by blists - more mailing lists