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

[Baolin Wang <baolin.wang@linux.alibaba.com>]

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 code also checks the writeback flag, so the 
uptodate check seems unnecessary to me.

> +			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?

> +
>   	/*
>   	 * Ensure we have slots for all the pages in the range.  This is
>   	 * almost certainly a no-op because most of the pages must be present
> @@ -2320,17 +2348,9 @@ static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr,
>   			}
>   		} else {	/* !is_shmem */
>   			if (!folio || xa_is_value(folio)) {
> -				xas_unlock_irq(&xas);
> -				page_cache_sync_readahead(mapping, &file->f_ra,
> -							  file, index,
> -							  end - index);
> -				/* drain lru cache to help folio_isolate_lru() */
> -				lru_add_drain();
> -				folio = filemap_lock_folio(mapping, index);
> -				if (IS_ERR(folio)) {
> -					result = SCAN_FAIL;
> -					goto xa_unlocked;
> -				}
> +				/* The range changed after the preparation pass. */
> +				result = SCAN_PAGE_LOCK;
> +				goto xa_locked;
>   			} else if (folio_test_dirty(folio)) {
>   				/*
>   				 * This page is dirty because it hasn't