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

[Baolin Wang]

On 10/16/26 11:43 PM, Lance Yang wrote:
> +Cc: Pedro
>
> 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.
>
> Cheers!
>
> > > 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()?
>
> Hmm ... we haven't taken the lock here yet, so there's just nothing to
> unlock, no?

Ah, yes, I misread the diff. Sorry for noise:) .

> > > +			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 code also checks the writeback flag, so the
> > uptodate check seems unnecessary to me.
>
> IIRC, the folio gets unlocked even if the read fails. I just wanted to
> catch read errors early. And yeah, we already check uptodate later,
> so this one can go, thanks!
>
> > > +			index = folio_next_index(folio);
> > > +			folio_unlock(folio);
> > > +			folio_put(folio);
> > > +			if (result != SCAN_SUCCEED)
> > > +				goto rollback;
> > > +		}
> > > +		/* drain lru cache to help folio_isolate_lru() */
> > > +		lru_add_drain();
> > > +	}
> >
> > Could we factor this code out into a small helper?
>
> Okay, let me play with it a bit :) Might pull it out if it reads cleaner.

Thanks. Because collapse_file() is already too long.