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

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

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@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.
> 
> 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@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()?
> 
> 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.