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

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

On 10/16/26 9:18 PM, Pedro Falcato wrote:
> 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@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.

OK.

>> 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.

Sounds good.

> 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.

But why not add RA logic in collapse_pmd(), as we do for 
SCAN_PAGE_DIRTY_OR_WRITEBACK? There's already retry logic there, and 
this would also simplify the long collapse_file().