Re: [PATCH v3 1/1] khugepaged: fix readahead lock inversion in collapse_file()

[Pedro Falcato <pfalcato@suse.de>]

On Fri, Oct 16, 2026 at 05:57:48PM +0800, Baolin Wang wrote:
> 
> 
> On 10/16/26 2:59 PM, Lance Yang wrote:
> > collapse_file() can end up calling page_cache_sync_readahead() with
> > source folios from earlier in the range still locked. If truncate already
> > holds mapping->invalidate_lock exclusively and is waiting for one of
> > those folios, readahead blocks on the same invalidate_lock and neither
> > side can make progress.
> > 
> > Do readahead in a separate pass before we start holding source folios
> > locked for collapse. Keep page_cache_sync_readahead() and its existing
> > policy, and drop each folio's lock and reference after checking that it
> > is uptodate. If truncate or reclaim removes a folio between the two
> > passes, roll back instead of doing readahead again with source folios
> > locked.
> > 
> > Shmem handles swapin and fallocated folios through shmem_get_folio()
> > without taking mapping->invalidate_lock, so it isn't affected by this
> > lock inversion.
> > 
> > Reported-by: syzbot+16bf7cd0ebeb1de93aa5@syzkaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=16bf7cd0ebeb1de93aa5
> > Fixes: 730633f0b7f9 ("mm: Protect operations adding pages to page cache with invalidate_lock")
> > Signed-off-by: Lance Yang <lance.yang@linux.dev>
> > ---
> 
> Thanks for the fix.
> 
> > Thanks to Nguyen Ngoc Thang for v2. David asked for volunteers, so I'm
> > happy to take this over.
> > 
> > Some older stable trees lack the folio infrastructure used here, and the
> > code will differ for each stable tree, so each backport will need quite a
> > bit of rework and its own review. I'm not sure whether to Cc stable here.
> > Once we agree on the fix, I'll send a separate backport for each maintained
> > stable tree except 5.10, which isn't affected.
> > 
> > v2 -> v3:
> >   - Replace the scan-wide invalidate_lock and page_cache_ra_unbounded() with
> >     a preparation pass using page_cache_sync_readahead().
> >   - Roll back on a new page cache miss during collapse.
> >   - https://lore.kernel.org/all/20260913163644.122133-1-ngocthang2710.1999@gmail.com/
> > 
> >   mm/khugepaged.c | 42 +++++++++++++++++++++++++++++++-----------
> >   1 file changed, 31 insertions(+), 11 deletions(-)
> > 
> > diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> > index ef5cd6fda34f..b3f67617caf0 100644
> > --- a/mm/khugepaged.c
> > +++ b/mm/khugepaged.c
> > @@ -2262,6 +2262,34 @@ static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr,
> >   	new_folio->index = start;
> >   	new_folio->mapping = mapping;
> > +	if (!is_shmem) {
> > +		/*
> > +		 * Populate the range before accumulating locked source folios:
> > +		 * readahead may block on invalidate_lock.
> > +		 */
> > +		for (index = start; index < end;) {
> 
> why drop xas_unlock_irq()?
> 
> > +			folio = filemap_lock_folio(mapping, index);
> > +			if (IS_ERR(folio)) {
> > +				page_cache_sync_readahead(mapping, &file->f_ra,
> > +							  file, index, end - index);
> > +				folio = filemap_lock_folio(mapping, index);
> > +				if (IS_ERR(folio)) {
> > +					result = SCAN_FAIL;
> > +					goto rollback;
> > +				}
> > +			}
> > +			if (!folio_test_uptodate(folio))
> > +				result = SCAN_FAIL;
> 
> Do we need to check PG_uptodate after locking the folio? If we successfully
> lock the folio, doesn't that mean the read I/O has completed? The following

Yes, you do; a read can "complete" without the IO having completed successfully,
and in those cases PG_uptodate is not set, but the folio is unlocked.

Either way, why are we checking for uptodate here? We only care for starting readahead.
Anyway, I would possibly recommend we do this as post-processing, not pre-processing.
The current loop is a little overhead-happy.

-- 
Pedro