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

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

[Lance Yang]

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.

Keep page_cache_sync_readahead() and wait for read I/O with
filemap_lock_folio(), releasing each folio before moving to the next. The
existing uptodate check during collapse still handles read errors.

Shmem uses shmem_get_folio() for swapin and fallocated folios without
taking mapping->invalidate_lock, so it is unaffected.

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")
Cc: stable@vger.kernel.org
Signed-off-by: Lance Yang <lance.yang@linux.dev>
---
v3 -> v4:
 - Move readahead after rollback and retry collapse once (per Pedro).
 - Drop the preparation pass and its redundant uptodate check (per
   Baolin and Pedro). Thanks!
 - https://lore.kernel.org/all/20261016065920.4153-1-lance.yang@linux.dev/

 mm/khugepaged.c | 44 +++++++++++++++++++++++++++++++++-----------
 1 file changed, 33 insertions(+), 11 deletions(-)

diff --git a/mm/khugepaged.c b/mm/khugepaged.c
index da91762f6358..0d3bd9e23613 100644
--- a/mm/khugepaged.c
+++ b/mm/khugepaged.c
@@ -2239,6 +2239,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
@@ -2256,6 +2258,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);
@@ -2320,17 +2326,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
@@ -2657,6 +2655,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));
-- 
2.49.0

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

[Pedro Falcato]

On Mon, Oct 26, 2026 at 11:20:13PM +0800, 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.
> 
> Keep page_cache_sync_readahead() and wait for read I/O with
> filemap_lock_folio(), releasing each folio before moving to the next. The
> existing uptodate check during collapse still handles read errors.
> 
> Shmem uses shmem_get_folio() for swapin and fallocated folios without
> taking mapping->invalidate_lock, so it is unaffected.
> 
> 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")
> Cc: stable@vger.kernel.org
> Signed-off-by: Lance Yang <lance.yang@linux.dev>

Thanks for the patch! This looks almost perfect!

> ---
> v3 -> v4:
>  - Move readahead after rollback and retry collapse once (per Pedro).
>  - Drop the preparation pass and its redundant uptodate check (per
>    Baolin and Pedro). Thanks!
>  - https://lore.kernel.org/all/20261016065920.4153-1-lance.yang@linux.dev/
> 
>  mm/khugepaged.c | 44 +++++++++++++++++++++++++++++++++-----------
>  1 file changed, 33 insertions(+), 11 deletions(-)
> 
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index da91762f6358..0d3bd9e23613 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -2239,6 +2239,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
> @@ -2256,6 +2258,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);
> @@ -2320,17 +2326,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
> @@ -2657,6 +2655,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);

I'm not sure if there's a point in locking the folio. If we are retrying,
then the logic above will lock it anyway and check uptodate. So I don't
think there's much of a point doing it here. Right?

So I think we could get away with a filemap_get_folio() here, and no
filemap_lock_folio() after RA. Unless you're intent on waiting for RA
right before failing, but I don't see a point in that.

With that addressed:

Reviewed-by: Pedro Falcato <pfalcato@suse.de>

> +			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));
> -- 
> 2.49.0
> 

-- 
Pedro

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

[Lance Yang]

On Mon, Oct 26, 2026 at 04:33:08PM +0000, Pedro Falcato wrote:
>On Mon, Oct 26, 2026 at 11:20:13PM +0800, 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.
>> 
>> Keep page_cache_sync_readahead() and wait for read I/O with
>> filemap_lock_folio(), releasing each folio before moving to the next. The
>> existing uptodate check during collapse still handles read errors.
>> 
>> Shmem uses shmem_get_folio() for swapin and fallocated folios without
>> taking mapping->invalidate_lock, so it is unaffected.
>> 
>> 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")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Lance Yang <lance.yang@linux.dev>
>
>Thanks for the patch! This looks almost perfect!

Thanks Pedro!

>
>> ---
>> v3 -> v4:
>>  - Move readahead after rollback and retry collapse once (per Pedro).
>>  - Drop the preparation pass and its redundant uptodate check (per
>>    Baolin and Pedro). Thanks!
>>  - https://lore.kernel.org/all/20261016065920.4153-1-lance.yang@linux.dev/
>> 
>>  mm/khugepaged.c | 44 +++++++++++++++++++++++++++++++++-----------
>>  1 file changed, 33 insertions(+), 11 deletions(-)
>> 
>> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
>> index da91762f6358..0d3bd9e23613 100644
>> --- a/mm/khugepaged.c
>> +++ b/mm/khugepaged.c
>> @@ -2239,6 +2239,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
>> @@ -2256,6 +2258,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);
>> @@ -2320,17 +2326,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
>> @@ -2657,6 +2655,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);
>
>I'm not sure if there's a point in locking the folio. If we are retrying,
>then the logic above will lock it anyway and check uptodate. So I don't
>think there's much of a point doing it here. Right?
>
>So I think we could get away with a filemap_get_folio() here, and no
>filemap_lock_folio() after RA. Unless you're intent on waiting for RA
>right before failing, but I don't see a point in that.

Right, waiting for RA is what I intended here. Without that wait, the
retry can hit folio_trylock() while the read is still in flight and bail
out with SCAN_PAGE_LOCK, before it even gets to the uptodate check.

If the read completes successfully, we can continue with collapse.
That's why I kept the wait before retrying. Do you think waiting isn't
worth it here, and we'd be better off leaving collapse to a later
attempt?

>
>With that addressed:
>
>Reviewed-by: Pedro Falcato <pfalcato@suse.de>

Really appreciated!

>> +			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));
>> -- 
>> 2.49.0
>> 
>
>-- 
>Pedro
>

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

[Pedro Falcato]

On Tue, Oct 27, 2026 at 10:55:07AM +0800, Lance Yang wrote:
> 
> On Mon, Oct 26, 2026 at 04:33:08PM +0000, Pedro Falcato wrote:
> >On Mon, Oct 26, 2026 at 11:20:13PM +0800, 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.
> >> 
> >> Keep page_cache_sync_readahead() and wait for read I/O with
> >> filemap_lock_folio(), releasing each folio before moving to the next. The
> >> existing uptodate check during collapse still handles read errors.
> >> 
> >> Shmem uses shmem_get_folio() for swapin and fallocated folios without
> >> taking mapping->invalidate_lock, so it is unaffected.
> >> 
> >> 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")
> >> Cc: stable@vger.kernel.org
> >> Signed-off-by: Lance Yang <lance.yang@linux.dev>
> >
> >Thanks for the patch! This looks almost perfect!
> 
> Thanks Pedro!
> 
> >
> >> ---
> >> v3 -> v4:
> >>  - Move readahead after rollback and retry collapse once (per Pedro).
> >>  - Drop the preparation pass and its redundant uptodate check (per
> >>    Baolin and Pedro). Thanks!
> >>  - https://lore.kernel.org/all/20261016065920.4153-1-lance.yang@linux.dev/
> >> 
> >>  mm/khugepaged.c | 44 +++++++++++++++++++++++++++++++++-----------
> >>  1 file changed, 33 insertions(+), 11 deletions(-)
> >> 
> >> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> >> index da91762f6358..0d3bd9e23613 100644
> >> --- a/mm/khugepaged.c
> >> +++ b/mm/khugepaged.c
> >> @@ -2239,6 +2239,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
> >> @@ -2256,6 +2258,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);
> >> @@ -2320,17 +2326,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
> >> @@ -2657,6 +2655,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);
> >
> >I'm not sure if there's a point in locking the folio. If we are retrying,
> >then the logic above will lock it anyway and check uptodate. So I don't
> >think there's much of a point doing it here. Right?
> >
> >So I think we could get away with a filemap_get_folio() here, and no
> >filemap_lock_folio() after RA. Unless you're intent on waiting for RA
> >right before failing, but I don't see a point in that.
> 
> Right, waiting for RA is what I intended here. Without that wait, the
> retry can hit folio_trylock() while the read is still in flight and bail
> out with SCAN_PAGE_LOCK, before it even gets to the uptodate check.
> 
> If the read completes successfully, we can continue with collapse.
> That's why I kept the wait before retrying. Do you think waiting isn't
> worth it here, and we'd be better off leaving collapse to a later
> attempt?

Hmm, yes, thanks for the explanation. I had a fresh look at the code and your
patch preserves that bit of behavior. Isolating folios is simply trylocking,
and only does a hard folio_lock after RA. So, LGTM!

-- 
Pedro

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

[David Hildenbrand (Arm)]

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?

Then we don't need this fallback logic.

-- 
Cheers,

David

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

[Pedro Falcato]

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.

FWIW, I plan on rebasing my refactoring patches for 7.5, and hopefully knock
some sense into this code. That will hopefully make this cleaner.

-- 
Pedro

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

[David Hildenbrand (Arm)]

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

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

[Lance Yang]

On 2026/10/27 21:58, David Hildenbrand (Arm) wrote:
> 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)

IIUC, we're good with the fix, so let's keep v4 as is and leave the cleanup
for the rework :)

Cheers, Lance