On 11/10/2025 5:26 PM, Lorenzo Stoakes wrote:
> Please, please, please send a cover letter when there's > 1 patch :)
>
> This 2/2 replying to 1/2 is a pain (not your fault that perhaps you're not
> aware
> of typical mm series style but FYI :P)
>
Sure, will do this in V2 (posting today).
> Also there is some tiny conflict on khugepaged.c in mm-new, but it's
> literally 1
> #include so probably nothing to worry about.
> > On Mon, Nov 10, 2025 at 11:32:55AM +0000, Shivank Garg wrote:
>> When MADV_COLLAPSE encounters dirty file-backed pages, it currently
>> returns -EINVAL, this is misleading as EINVAL suggests invalid arguments,
>> whereas dirty pages are a transient condition that may resolve on retry.
>>
>> Introduce SCAN_PAGE_DIRTY and map it to -EAGAIN. For khugepaged, this
>> is harmless as it will revisit the range after async writeback completes.
>>
>> Signed-off-by: Shivank Garg <[email protected]>
>
> With comments below addressed, LGTM so:
>
> Reviewed-by: Lorenzo Stoakes <[email protected]>
Thank you for the review.
>
>> ---
>> include/trace/events/huge_memory.h | 3 ++-
>> mm/khugepaged.c | 4 +++-
>> 2 files changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/trace/events/huge_memory.h
>> b/include/trace/events/huge_memory.h
>> index dd94d14a2427..9014a9bbe64c 100644
>> --- a/include/trace/events/huge_memory.h
>> +++ b/include/trace/events/huge_memory.h
>> @@ -38,7 +38,8 @@
>> EM( SCAN_PAGE_HAS_PRIVATE, "page_has_private") \
>> EM( SCAN_STORE_FAILED, "store_failed") \
>> EM( SCAN_COPY_MC, "copy_poisoned_page") \
>> - EMe(SCAN_PAGE_FILLED, "page_filled")
>> + EM(SCAN_PAGE_FILLED, "page_filled") \
>> + EMe(SCAN_PAGE_DIRTY, "page_dirty")
>>
>> #undef EM
>> #undef EMe
>> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
>> index d08ed6eb9ce1..7df329c9c87d 100644
>> --- a/mm/khugepaged.c
>> +++ b/mm/khugepaged.c
>> @@ -60,6 +60,7 @@ enum scan_result {
>> SCAN_STORE_FAILED,
>> SCAN_COPY_MC,
>> SCAN_PAGE_FILLED,
>> + SCAN_PAGE_DIRTY,
>
> it feels like a lot to add a scan result for this, but I mean... probably
> actually valid.
>
>> };
>>
>> #define CREATE_TRACE_POINTS
>> @@ -1967,7 +1968,7 @@ static int collapse_file(struct mm_struct *mm,
>> unsigned long addr,
>> */
>> xas_unlock_irq(&xas);
>> filemap_flush(mapping);
>> - result = SCAN_FAIL;
>> + result = SCAN_PAGE_DIRTY;
>> goto xa_unlocked;
>
> Hmmm shmem dirty is going to be weird but we also have:
>
> 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
> * since first write.
> */
> result = SCAN_FAIL;
> goto out_unlock;
> }
>
> It's weird though, why would we have writeback, surely handled by swap, and
> won't it be like anon, i.e. pretty well always dirty? This comment seems
> copy/pasta wrong.
>
> We do need to at least mention in commit message that shmem is explicitly
> excluded.
>
Looking at the code, the dirty/writeback checks where I'm making changes
are all in the !is_shmem branch, so it only affects regular files, not
shmem.
Should I mention in the commit message that these changes are limited
to regular files and don't affect shmem?
I'm not sure I fully understood your concern on shmem. Could you please
elaborate?
Thanks,
Shivank
>
>> } else if (folio_test_writeback(folio)) {
>> xas_unlock_irq(&xas);
>> @@ -2747,6 +2748,7 @@ static int madvise_collapse_errno(enum scan_result r)
>> case SCAN_PAGE_LRU:
>> case SCAN_DEL_PAGE_LRU:
>> case SCAN_PAGE_FILLED:
>> + case SCAN_PAGE_DIRTY:
>> return -EAGAIN;
>> /*
>> * Other: Trying again likely not to succeed / error intrinsic to
>> --
>> 2.43.0
>>
