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

[Lorenzo Stoakes]

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)

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

With comments below addressed, LGTM so:

Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@xxxxxxxxxx>

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


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