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

[Baolin Wang <baolin.wang@linux.alibaba.com>]

On 2025/9/22 13:32, Hugh Dickins wrote:
> On Fri, 19 Sep 2025, Baolin Wang wrote:
>> On 2025/9/19 09:06, Shakeel Butt wrote:
>>> 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:
> ...
>>>>>> 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.
> 
> (Myself, I would delete the comment entirely: it's justifying the change,
> which is good history to go into the commit message, but not so good in
> the final source. And we don't fully understand what to say here anyway.)
> 
>>>>
>>>>>> -        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.
>>
>> Um, I don't think it makes much difference, because we should no longer hit
>> this.
> 
> We do hit it.  Observe, there was no WARNING before in the !mapping
> case, just a pr_info in a particular instance of the !mapping case.
> We believe that instance went away with reiserfs, but that does not
> imply that there are no more !mapping cases left.
> 
> We do hit that WARNING: not often, and I've not seen it repeatedly
> (as ONCE-advisors fear), but a few cutdown examples from my testing
> appended below.

Thanks for reporting. That surprises me how that happened.

> I'm sure you'd like me to explain how it comes about: I did spend a
> while looking to do so, but then found better things to do.  By all
> means go in search of the explanation if it's worth your time: it
> might indicate a bug somewhere; but more likely it's just a race
> against code elsewhere cleaning up the folio.

Interesting. I'll try to reproduce this issue.

> The fact that it does not appear repeatedly suggests that the folio
> is successfully dealt with afterwards.  I didn't think to check at
> first, but in later runs did check back on such folios after, and
> verified that they had moved on to being freed and reused, not leaked.

No leaks, good.

> The examples I've seen have all been swapbacked, though that may
> just reflect my tmpfs swapping load.  With mapping NULLed, there's
> not much to go on: but index 0x7ff5ce103 is likely to have been
> anon, and index 0xcff more likely to have been tmpfs.
> 
> My vote is simply to delete the warning (and the comment).

Thanks for your examples and sound reasonable to me.

Andrew, could you help squash the following changes (if you need a new 
version, please let me know)? Thanks.


diff --git a/mm/vmscan.c b/mm/vmscan.c
index 4907e255857a..aadbee50a851 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -689,16 +689,8 @@ static pageout_t pageout(struct folio *folio, 
struct address_space *mapping,
          * A freeable shmem or swapcache folio is referenced only by the
          * caller that isolated the folio and the page cache.
          */
-       if (folio_ref_count(folio) != 1 + folio_nr_pages(folio))
+       if (folio_ref_count(folio) != 1 + folio_nr_pages(folio) || !mapping)
                 return PAGE_KEEP;
-       if (!mapping) {
-               /*
-                * We should no longer have dirty folios with clean 
buffers and
-                * a NULL mapping. However, let's be careful for now.
-                */
-               VM_WARN_ON_FOLIO(true, folio);
-               return PAGE_KEEP;
-       }

         if (!shmem_mapping(mapping) && !folio_test_anon(folio))
                 return PAGE_ACTIVATE;