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

[Lance Yang]

On Fri, Oct 16, 2026 at 02:18:43PM +0100, 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.
>> 
>> 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.
>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.

Good call! I should have thought of that :P We can do this on demand after
rollback rather than walk the range every time :)

I played around with it a bit, something like:

---8<---
diff --git a/mm/khugepaged.c b/mm/khugepaged.c
index 85ea095906fb..ce5d89ff47b3 100644
--- a/mm/khugepaged.c
+++ b/mm/khugepaged.c
@@ -2290,6 +2290,8 @@ static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr,
 	enum scan_result result = SCAN_SUCCEED;
 	int nr_none = 0;
 	bool is_shmem = shmem_file(file);
+	bool do_readahead = false;
+	bool readahead_done = false;

 	/*
 	 * MADV_COLLAPSE ignores shmem huge config, so do not check shmem
@@ -2307,6 +2309,10 @@ static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr,

 	mapping_set_update(&xas, mapping);

+retry:
+	result = SCAN_SUCCEED;
+	do_readahead = false;
+	xas_set_order(&xas, start, HPAGE_PMD_ORDER);
 	__folio_set_locked(new_folio);
 	if (is_shmem)
 		__folio_set_swapbacked(new_folio);
@@ -2371,17 +2377,9 @@ static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr,
 			}
 		} else {	/* !is_shmem */
 			if (!folio || xa_is_value(folio)) {
-				xas_unlock_irq(&xas);
-				page_cache_sync_readahead(mapping, &file->f_ra,
-							  file, index,
-							  end - index);
-				/* drain lru cache to help folio_isolate_lru() */
-				lru_add_drain();
-				folio = filemap_lock_folio(mapping, index);
-				if (IS_ERR(folio)) {
-					result = SCAN_FAIL;
-					goto xa_unlocked;
-				}
+				do_readahead = !readahead_done;
+				result = SCAN_FAIL;
+				goto xa_locked;
 			} else if (folio_test_dirty(folio)) {
 				/*
 				 * This page is dirty because it hasn't
@@ -2704,6 +2702,30 @@ static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr,
 	new_folio->mapping = NULL;

 	folio_unlock(new_folio);
+
+	/* Readahead may take invalidate_lock; source folios are unlocked. */
+	if (do_readahead) {
+		for (index = start; index < end;) {
+			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 out_put;
+				}
+			}
+			index = folio_next_index(folio);
+			folio_unlock(folio);
+			folio_put(folio);
+		}
+		/* Drain the local LRU batch before retrying isolation. */
+		lru_add_drain();
+		readahead_done = true;
+		goto retry;
+	}
+out_put:
 	folio_put(new_folio);
 out:
 	VM_BUG_ON(!list_empty(&pagelist));
--

Cheers, Lance