Re: [PATCH 2/2] mm/khugepaged: return EAGAIN for transient dirty pages in MADV_COLLAPSE

["Garg, Shivank" <shivankg@amd.com>]

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 <shivankg@amd.com>
> 
> With comments below addressed, LGTM so:
> 
> Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>

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
>>