Re: [PATCH v3 1/1] khugepaged: fix readahead lock inversion in collapse_file()
From: Baolin Wang
Date: Sat Oct 17 2026 - 01:43:46 EST
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@xxxxxxxxxxxxxxxxxxxxxxxxx
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@xxxxxxxxx>
---
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@xxxxxxxxx/
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().