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

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

On Wed, Nov 19, 2025 at 03:55:29PM +0530, Garg, Shivank wrote:
>
>
> 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.

Yeah sorry I think I was being somehow blind to the fact that each
dirty/writeback test has !is_shmem, esp. given I literally quote it there :)

So err ignore me I think here haha

>
> Should I mention in the commit message that these changes are limited
> to regular files and don't affect shmem?

No that's fine.

>
> I'm not sure I fully understood your concern on shmem. Could you please elaborate?

Yeah I think I just misread the code after a long day :P

>
> Thanks,
> Shivank

Cheers, Lorenzo

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