Re: [PATCH] khugepaged: hold invalidate_lock across collapse_file() readahead

Re: [PATCH] khugepaged: hold invalidate_lock across collapse_file() readahead

[Lance Yang]

Hi Nguyen,

Good catch, thanks!

On Sun, Sep 13, 2026 at 05:11:42PM +0700, Nguyen Ngoc Thang wrote:
>collapse_file() calls page_cache_sync_readahead() to fault in missing
>pages before collapsing them into a THP. That helper takes
>mapping->invalidate_lock itself for the duration of the call, then
>drops it -- but truncate (e.g. ext4_setattr() -> truncate_pagecache())
>takes invalidate_lock and then waits on each page's folio lock while
>holding it. If collapse_file() has already locked one of those folios
>by the time truncate reaches it, and then tries to acquire
>invalidate_lock again (e.g. on the next iteration, or via a nested
>readahead call), the two paths can deadlock/hang on each other's lock:
>truncate blocked on the folio lock collapse holds, and collapse
>blocked waiting for invalidate_lock that truncate holds.

That can happen on the first readahead call.

>
>Reproducing this over ~150,000 collapse iterations with truncate
>racing concurrently reliably hits hung_task: blocked tasks within
>about 20 seconds on an unpatched kernel.
>
>Fix it by taking invalidate_lock_shared once for the whole scan, before
>locking any folio, and using page_cache_ra_unbounded() directly in the
>readahead call site instead of page_cache_sync_readahead(), since the
>latter would try to retake the lock we already hold.
>page_cache_ra_unbounded() does not clamp to EOF like the helper it
>replaces, so clamp the requested range explicitly.
>
>Reported-by: syzbot+16bf7cd0ebeb1de93aa5@syzkaller.appspotmail.com
>Closes: https://syzkaller.appspot.com/bug?extid=16bf7cd0ebeb1de93aa5

Ouch, we should add a Fixes tag and Cc stable:

Fixes: 730633f0b7f9 ("mm: Protect operations adding pages to page cache with invalidate_lock")
Cc: stable@vger.kernel.org

730633f0b7f9 added invalidate_lock acquisition to readahead but missed
collapse_file(), which already called it with page locks held. The
subsequent filesystem conversions made the deadlock possible by taking
invalidate_lock before locking pages :( right?

>Signed-off-by: Nguyen Ngoc Thang <ngocthang2710.1999@gmail.com>
>---
> mm/khugepaged.c | 25 ++++++++++++++++++++++---
> 1 file changed, 22 insertions(+), 3 deletions(-)
>
>diff --git a/mm/khugepaged.c b/mm/khugepaged.c
>index 11ff98d55c76..690ccbcdf593 100644
>--- a/mm/khugepaged.c
>+++ b/mm/khugepaged.c
>@@ -2267,6 +2267,13 @@ static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr,
> 	VM_WARN_ON_ONCE(!is_shmem && !mapping_pmd_folio_support(mapping));
> 	VM_WARN_ON_ONCE(start & (HPAGE_PMD_NR - 1));
> 
>+	/*
>+	 * Take invalidate_lock before any folio lock: the readahead below
>+	 * needs it, and truncate holds it while waiting on folio locks.
>+	 */
>+	if (!is_shmem)
>+		filemap_invalidate_lock_shared(mapping);

Could we take the lock after alloc_charge_folio() succeeds, before
locking any folio? That would keep allocation and charging outside the
critical section. And if allocation fails, we should skip the unlock :)

Cheers, Lance

>+
> 	result = alloc_charge_folio(&new_folio, mm, cc, HPAGE_PMD_ORDER);
> 	if (result != SCAN_SUCCEED)
> 		goto out;
>@@ -2337,10 +2344,20 @@ static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr,
> 			}
> 		} else {	/* !is_shmem */
> 			if (!folio || xa_is_value(folio)) {
>+				DEFINE_READAHEAD(ractl, file, &file->f_ra,
>+						  mapping, index);
>+				pgoff_t eof = DIV_ROUND_UP(i_size_read(mapping->host),
>+							    PAGE_SIZE);
>+
> 				xas_unlock_irq(&xas);
>-				page_cache_sync_readahead(mapping, &file->f_ra,
>-							  file, index,
>-							  end - index);
>+				/*
>+				 * invalidate_lock held above; don't retake it.
>+				 * page_cache_ra_unbounded(), unlike the readahead
>+				 * helper this replaces, does not clamp to EOF.
>+				 */
>+				if (index < eof)
>+					page_cache_ra_unbounded(&ractl,
>+						min(end, eof) - index, 0);
> 				/* drain lru cache to help folio_isolate_lru() */
> 				lru_add_drain();
> 				folio = filemap_lock_folio(mapping, index);
>@@ -2672,6 +2689,8 @@ static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr,
> 	folio_unlock(new_folio);
> 	folio_put(new_folio);
> out:
>+	if (!is_shmem)
>+		filemap_invalidate_unlock_shared(mapping);
> 	VM_BUG_ON(!list_empty(&pagelist));
> 	trace_mm_khugepaged_collapse_file(mm, new_folio, index, addr, is_shmem, file, HPAGE_PMD_NR, result);
> 	return result;
>-- 
>2.43.0
>
>

Re: [PATCH] khugepaged: hold invalidate_lock across collapse_file() readahead

[Lance Yang]

On 2026/9/14 00:12, Lance Yang wrote:
> Hi Nguyen,
> 
> Good catch, thanks!
> 
> On Sun, Sep 13, 2026 at 05:11:42PM +0700, Nguyen Ngoc Thang wrote:
>> collapse_file() calls page_cache_sync_readahead() to fault in missing
>> pages before collapsing them into a THP. That helper takes
>> mapping->invalidate_lock itself for the duration of the call, then
>> drops it -- but truncate (e.g. ext4_setattr() -> truncate_pagecache())
>> takes invalidate_lock and then waits on each page's folio lock while
>> holding it. If collapse_file() has already locked one of those folios
>> by the time truncate reaches it, and then tries to acquire
>> invalidate_lock again (e.g. on the next iteration, or via a nested
>> readahead call), the two paths can deadlock/hang on each other's lock:
>> truncate blocked on the folio lock collapse holds, and collapse
>> blocked waiting for invalidate_lock that truncate holds.
> 
> That can happen on the first readahead call.
> 
>>
>> Reproducing this over ~150,000 collapse iterations with truncate
>> racing concurrently reliably hits hung_task: blocked tasks within
>> about 20 seconds on an unpatched kernel.
>>
>> Fix it by taking invalidate_lock_shared once for the whole scan, before
>> locking any folio, and using page_cache_ra_unbounded() directly in the
>> readahead call site instead of page_cache_sync_readahead(), since the
>> latter would try to retake the lock we already hold.
>> page_cache_ra_unbounded() does not clamp to EOF like the helper it
>> replaces, so clamp the requested range explicitly.
>>
>> Reported-by: syzbot+16bf7cd0ebeb1de93aa5@syzkaller.appspotmail.com
>> Closes: https://syzkaller.appspot.com/bug?extid=16bf7cd0ebeb1de93aa5
> 
> Ouch, we should add a Fixes tag and Cc stable:
> 
> Fixes: 730633f0b7f9 ("mm: Protect operations adding pages to page cache with invalidate_lock")
> Cc: stable@vger.kernel.org
> 
> 730633f0b7f9 added invalidate_lock acquisition to readahead but missed
> collapse_file(), which already called it with page locks held. The
> subsequent filesystem conversions made the deadlock possible by taking
> invalidate_lock before locking pages :( right?
> 
>> Signed-off-by: Nguyen Ngoc Thang <ngocthang2710.1999@gmail.com>
>> ---

Forgot to mention:

I reproduced the hang too, and it goes away with this patch applied :)

Tested-by: Lance Yang <lance.yang@linux.dev>

>> mm/khugepaged.c | 25 ++++++++++++++++++++++---
>> 1 file changed, 22 insertions(+), 3 deletions(-)
>>
>> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
>> index 11ff98d55c76..690ccbcdf593 100644
>> --- a/mm/khugepaged.c
>> +++ b/mm/khugepaged.c
>> @@ -2267,6 +2267,13 @@ static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr,
>> 	VM_WARN_ON_ONCE(!is_shmem && !mapping_pmd_folio_support(mapping));
>> 	VM_WARN_ON_ONCE(start & (HPAGE_PMD_NR - 1));
>>
>> +	/*
>> +	 * Take invalidate_lock before any folio lock: the readahead below
>> +	 * needs it, and truncate holds it while waiting on folio locks.
>> +	 */
>> +	if (!is_shmem)
>> +		filemap_invalidate_lock_shared(mapping);
> 
> Could we take the lock after alloc_charge_folio() succeeds, before
> locking any folio? That would keep allocation and charging outside the
> critical section. And if allocation fails, we should skip the unlock :)
> 
> Cheers, Lance
> 
>> +
>> 	result = alloc_charge_folio(&new_folio, mm, cc, HPAGE_PMD_ORDER);
>> 	if (result != SCAN_SUCCEED)
>> 		goto out;
>> @@ -2337,10 +2344,20 @@ static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr,
>> 			}
>> 		} else {	/* !is_shmem */
>> 			if (!folio || xa_is_value(folio)) {
>> +				DEFINE_READAHEAD(ractl, file, &file->f_ra,
>> +						  mapping, index);
>> +				pgoff_t eof = DIV_ROUND_UP(i_size_read(mapping->host),
>> +							    PAGE_SIZE);
>> +
>> 				xas_unlock_irq(&xas);
>> -				page_cache_sync_readahead(mapping, &file->f_ra,
>> -							  file, index,
>> -							  end - index);
>> +				/*
>> +				 * invalidate_lock held above; don't retake it.
>> +				 * page_cache_ra_unbounded(), unlike the readahead
>> +				 * helper this replaces, does not clamp to EOF.
>> +				 */
>> +				if (index < eof)
>> +					page_cache_ra_unbounded(&ractl,
>> +						min(end, eof) - index, 0);
>> 				/* drain lru cache to help folio_isolate_lru() */
>> 				lru_add_drain();
>> 				folio = filemap_lock_folio(mapping, index);
>> @@ -2672,6 +2689,8 @@ static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr,
>> 	folio_unlock(new_folio);
>> 	folio_put(new_folio);
>> out:
>> +	if (!is_shmem)
>> +		filemap_invalidate_unlock_shared(mapping);
>> 	VM_BUG_ON(!list_empty(&pagelist));
>> 	trace_mm_khugepaged_collapse_file(mm, new_folio, index, addr, is_shmem, file, HPAGE_PMD_NR, result);
>> 	return result;
>> -- 
>> 2.43.0
>>
>>