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

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

On 10/17/26 3:41 PM, Lance Yang wrote:
> 
> 
> 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?

IIRC, the approach to bug fixes is to fix bugs properly and cleanly for 
upstream, without being constrained by backporting requirements.

Anyway, I don't have a strong opinion on this. Let's also see what David 
prefers.