Re: [PATCH v4] khugepaged: fix readahead lock inversion in collapse_file()

["David Hildenbrand (Arm)" <david@kernel.org>]

On 10/27/26 14:05, Pedro Falcato wrote:
> On Tue, Oct 27, 2026 at 12:06:05PM +0100, David Hildenbrand (Arm) wrote:
>> On 10/26/26 16:20, Lance Yang wrote:
>>> collapse_file() can call page_cache_sync_readahead() while still holding
>>> source folios locked. If truncate holds mapping->invalidate_lock
>>> exclusively and waits for one of those folios, readahead waits for
>>> invalidate_lock and neither side can make progress.
>>>
>>> We need to release those folio locks before starting readahead. On a page
>>> cache miss, roll back first, populate the collapse range, and retry once.
>>> Doing readahead after rollback avoids an extra pass when all folios are
>>> already present. If another miss occurs on retry, give up rather than keep
>>> retrying while truncate or reclaim removes folios.
>>
>> Makes sense. So we simply try readahead on all folios.
>>
>> Any reason we don't simply add a second loop after the first loop where we don't
>> take the folio locks and do the swapin/readahead dance, then a second loop where
>> we do all the other stuff?
> 
> I'm not sure I understand. You want to nest two loops? Or do you want a loop
> upfront? I don't think we want a loop upfont. That's slow and involves
> starting/waiting for IO. Fallback logic makes more sense IMO.

I'd have an earlier loop where we focus on filling holes, and grabbing folios
then in the second loop only from the pagecache + trylocking them.

That would make the second easier to the eye.

collapse_scan() file could just indicate if there are any holes and whether
looping makes sense at all. It already does a scan.

So you could just skip the loop when there is no reason to readahead at all.

This function is pretty horrible and the retry logic doesn't quite improve the
situation.

But no strong opinion for this fix as long as we clean it up properly later.

(looking forward to reworks)

-- 
Cheers,

David