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

[Lance Yang <lance.yang@linux.dev>]

On 2026/10/17 13:43, Baolin Wang wrote:
> 
> 
> 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().

That would be cleaner, yeah. Some older stable trees don't have that
outer retry loop, though ... I'd prefer to get the fix backported first,
then move the RA/retry logic out in a follow-up, wdyt?

Cheers, Lance