Re: [PATCH 7.2 v3 02/12] mm/khugepaged: add folio dirty check after try_to_unmap()

Zi Yan
2026-04-23T13:15:09Z

On 23 Apr 2026, at 4:30, Lance Yang wrote:

> On Fri, Apr 17, 2026 at 10:44:19PM -0400, Zi Yan wrote:
>> This check ensures the correctness of collapse read-only THPs for FSes
>> after READ_ONLY_THP_FOR_FS is enabled by default for all FSes supporting
>> PMD THP pagecache.
>>
>> READ_ONLY_THP_FOR_FS only supports read-only fd and uses mapping->nr_thps
>> and inode->i_writecount to prevent any write to read-only to-be-collapsed
>> folios. In upcoming commits, READ_ONLY_THP_FOR_FS will be removed and the
>> aforementioned mechanism will go away too. To ensure khugepaged functions
>> as expected after the changes, skip if any folio is dirty after
>> try_to_unmap(), since a dirty folio means this read-only folio
>> got some writes via mmap can happen between try_to_unmap() and
>> try_to_unmap_flush() via cached TLB entries and khugepaged does not support
>> writable pagecache folio collapse yet.
>>
>> Signed-off-by: Zi Yan <ziy@nvidia.com>
>> ---
>> mm/khugepaged.c | 25 +++++++++++++++++++++----
>> 1 file changed, 21 insertions(+), 4 deletions(-)
>>
>> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
>> index 3eb5d982d3d3..1c0fdc81d276 100644
>> --- a/mm/khugepaged.c
>> +++ b/mm/khugepaged.c
>> @@ -1979,8 +1979,7 @@ static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr,
>> }
>> } else if (folio_test_dirty(folio)) {
>> /*
>> - * khugepaged only works on read-only fd,
>> - * so this page is dirty because it hasn't
>> + * This page is dirty because it hasn't
>> * been flushed since first write. There
>> * won't be new dirty pages.
>> *
>> @@ -2038,8 +2037,8 @@ static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr,
>> if (!is_shmem && (folio_test_dirty(folio) ||
>> folio_test_writeback(folio))) {
>> /*
>> - * khugepaged only works on read-only fd, so this
>> - * folio is dirty because it hasn't been flushed
>> + * khugepaged only works on clean file-backed folios,
>> + * so this folio is dirty because it hasn't been flushed
>> * since first write.
>> */
>> result = SCAN_PAGE_DIRTY_OR_WRITEBACK;
>> @@ -2083,6 +2082,24 @@ static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr,
>> goto out_unlock;
>> }
>>
>> + /*
>> + * At this point, the folio is locked, unmapped. Make sure the
>> + * folio is clean, so that no one else is able to write to it,
>> + * since that would require taking the folio lock first.
>> + * Otherwise that means the folio was pointed by a dirty PTE and
>> + * some CPU might have a valid TLB entry with dirty bit set
>> + * still pointing to this folio and writes can happen without
>> + * causing a page table walk and folio lock acquisition before
>> + * the try_to_unmap_flush() below is done. After the collapse,
>> + * file-backed folio is not set as dirty and can be discarded
>> + * before any new write marks the folio dirty, causing data
>> + * corruption.
>> + */
>> + if (!is_shmem && folio_test_dirty(folio)) {
>> + result = SCAN_PAGE_DIRTY_OR_WRITEBACK;
>> + goto out_unlock;
>
> Looks buggy :)
>
> This runs after folio_isolate_lru() and after xas_lock_irq(&xas) ...
>
> If not missing something, "goto out_unlock" would leave the xarray lock
> held and the folio off the LRU :)
>
> Note that the block right above does call xas_unlock_irq(&xas), and it
> also does call folio_putback_lru(folio):
>
> ---8<---
> if (folio_ref_count(folio) != 2 + folio_nr_pages(folio)) {
> result = SCAN_PAGE_COUNT;
> xas_unlock_irq(&xas); <-
> folio_putback_lru(folio); <-
> goto out_unlock;
> }
> ---
>
> So we should follow the same cleanup as that block here, right?

You are right. Will fix it. Thank you.

Best Regards,
Yan, Zi