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

[Pedro Falcato]

On Fri, Oct 16, 2026 at 12:00:24PM +0100, Pedro Falcato wrote:
> 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@xxxxxxxxxxxxxxxxxxxxxxxxx
> > > 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@xxxxxxxxx>
> > > ---
> > 
> > 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@xxxxxxxxx/
> > > 
> > >   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.

To clarify: I would do this kind of RA as an error path if isolation failed.
Namely, at the end of rollback, after unlocking folios. It doesn't make sense
to me to do this kind of checking upfront. I don't think it's particularly
realistic that, starting RA upfront, you'd see uptodate completed folio reads
when doing the actual isolation.

-- 
Pedro