[PATCH] mm/filemap: fix page_cache_prev_miss() when no hole is found

[PATCH] mm/filemap: fix page_cache_prev_miss() when no hole is found

[Tal Zussman]

page_cache_prev_miss() is documented to return a value outside the
searched range when no gap is found. However, the no-gap-found path
returns xas.xa_index, which after a successful loop is the first index
in the range. As such, that index is misreported as a gap.

The sole caller, page_cache_sync_ra(), uses the return value to estimate
the cached run preceding a sequential read. In some cases, the buggy
return value can undercount the contiguous range by one, shrinking the
readahead window or pushing borderline requests into the
small-random-read branch.

Mirror the fix in commit bbcaee20e03e ("readahead: fix return value of
page_cache_next_miss() when no hole is found"): preserve max_scan in a
separate variable across the loop and return `index - max_scan` from the
no-gap-found path.

Both helpers were previously fixed together in commit 9425c591e06a
("page cache: fix page_cache_next/prev_miss off by one"), but the fix
was reverted because it caused a hugetlb performance regression. hugetlb
no longer uses these functions and next_miss was subsequently refixed
in commit 901a269ff3d5 ("filemap: fix page_cache_next_miss() when no
hole found") and commit bbcaee20e03e ("readahead: fix return value of
page_cache_next_miss() when no hole is found"), but prev_miss was not
addressed.

This was found by pointing Claude Opus 4.7 at mm/filemap.c.

Fixes: 0d3f92966629 ("page cache: Convert hole search to XArray")
Assisted-by: Claude:claude-opus-4-7
Signed-off-by: Tal Zussman <tz2294@columbia.edu>
---
 mm/filemap.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/mm/filemap.c b/mm/filemap.c
index ab34cab2416a..545ccd59777d 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -1845,16 +1845,17 @@ pgoff_t page_cache_prev_miss(struct address_space *mapping,
 			     pgoff_t index, unsigned long max_scan)
 {
 	XA_STATE(xas, &mapping->i_pages, index);
+	unsigned long nr = max_scan;
 
-	while (max_scan--) {
+	while (nr--) {
 		void *entry = xas_prev(&xas);
 		if (!entry || xa_is_value(entry))
-			break;
+			return xas.xa_index;
 		if (xas.xa_index == ULONG_MAX)
-			break;
+			return ULONG_MAX;
 	}
 
-	return xas.xa_index;
+	return index - max_scan;
 }
 EXPORT_SYMBOL(page_cache_prev_miss);
 

---
base-commit: e9dd96806dbc2d50a66770b6a86962bd5d601153
change-id: 20260510-prev_miss_fix-fcb308472131

Best regards,
-- 
Tal Zussman <tz2294@columbia.edu>

Re: [PATCH] mm/filemap: fix page_cache_prev_miss() when no hole is found

[Vishal Moola]

On Sun, May 10, 2026 at 05:54:17PM -0400, Tal Zussman wrote:
> page_cache_prev_miss() is documented to return a value outside the
> searched range when no gap is found. However, the no-gap-found path
> returns xas.xa_index, which after a successful loop is the first index
> in the range. As such, that index is misreported as a gap.
> 
> The sole caller, page_cache_sync_ra(), uses the return value to estimate
> the cached run preceding a sequential read. In some cases, the buggy
> return value can undercount the contiguous range by one, shrinking the
> readahead window or pushing borderline requests into the
> small-random-read branch.
> 
> Mirror the fix in commit bbcaee20e03e ("readahead: fix return value of
> page_cache_next_miss() when no hole is found"): preserve max_scan in a
> separate variable across the loop and return `index - max_scan` from the
> no-gap-found path.

IMO, this way of fixing it hurts the readability. I'd prefer something
similar to the fix in the original commit. Or...

> -	while (max_scan--) {
> +	while (nr--) {
>  		void *entry = xas_prev(&xas);
>  		if (!entry || xa_is_value(entry))
> -			break;
> +			return xas.xa_index;
>  		if (xas.xa_index == ULONG_MAX)
> -			break;
> +			return ULONG_MAX;
>  	}

If I understand this correctly, couldn't we just do something like:
if (!max_scan)
	return xas.xa_index - 1;

> -	return xas.xa_index;
> +	return index - max_scan;
>  }
>  EXPORT_SYMBOL(page_cache_prev_miss);
>  
> 
> ---
> base-commit: e9dd96806dbc2d50a66770b6a86962bd5d601153
> change-id: 20260510-prev_miss_fix-fcb308472131
> 
> Best regards,
> -- 
> Tal Zussman <tz2294@columbia.edu>
> 

Re: [PATCH] mm/filemap: fix page_cache_prev_miss() when no hole is found

[Jan Kara]

On Mon 11-05-26 13:44:17, Vishal Moola wrote:
> On Sun, May 10, 2026 at 05:54:17PM -0400, Tal Zussman wrote:
> > page_cache_prev_miss() is documented to return a value outside the
> > searched range when no gap is found. However, the no-gap-found path
> > returns xas.xa_index, which after a successful loop is the first index
> > in the range. As such, that index is misreported as a gap.
> > 
> > The sole caller, page_cache_sync_ra(), uses the return value to estimate
> > the cached run preceding a sequential read. In some cases, the buggy
> > return value can undercount the contiguous range by one, shrinking the
> > readahead window or pushing borderline requests into the
> > small-random-read branch.
> > 
> > Mirror the fix in commit bbcaee20e03e ("readahead: fix return value of
> > page_cache_next_miss() when no hole is found"): preserve max_scan in a
> > separate variable across the loop and return `index - max_scan` from the
> > no-gap-found path.
> 
> IMO, this way of fixing it hurts the readability. I'd prefer something
> similar to the fix in the original commit. Or...
> 
> > -	while (max_scan--) {
> > +	while (nr--) {
> >  		void *entry = xas_prev(&xas);
> >  		if (!entry || xa_is_value(entry))
> > -			break;
> > +			return xas.xa_index;
> >  		if (xas.xa_index == ULONG_MAX)
> > -			break;
> > +			return ULONG_MAX;
> >  	}
> 
> If I understand this correctly, couldn't we just do something like:
> if (!max_scan)
> 	return xas.xa_index - 1;

I think the easiest to understand would be to do the above two explicit
returns instead of 'break' and change below to:

	/* Return start of the range - 1 when no hole is found */
	return xas.xa_index - 1;

> > -	return xas.xa_index;
> > +	return index - max_scan;
> >  }

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] mm/filemap: fix page_cache_prev_miss() when no hole is found

[Tal Zussman]

On 5/11/26 12:26 PM, Jan Kara wrote:
> On Mon 11-05-26 13:44:17, Vishal Moola wrote:
>> On Sun, May 10, 2026 at 05:54:17PM -0400, Tal Zussman wrote:
>> > page_cache_prev_miss() is documented to return a value outside the
>> > searched range when no gap is found. However, the no-gap-found path
>> > returns xas.xa_index, which after a successful loop is the first index
>> > in the range. As such, that index is misreported as a gap.
>> > 
>> > The sole caller, page_cache_sync_ra(), uses the return value to estimate
>> > the cached run preceding a sequential read. In some cases, the buggy
>> > return value can undercount the contiguous range by one, shrinking the
>> > readahead window or pushing borderline requests into the
>> > small-random-read branch.
>> > 
>> > Mirror the fix in commit bbcaee20e03e ("readahead: fix return value of
>> > page_cache_next_miss() when no hole is found"): preserve max_scan in a
>> > separate variable across the loop and return `index - max_scan` from the
>> > no-gap-found path.
>> 
>> IMO, this way of fixing it hurts the readability. I'd prefer something
>> similar to the fix in the original commit. Or...
>> 
>> > -	while (max_scan--) {
>> > +	while (nr--) {
>> >  		void *entry = xas_prev(&xas);
>> >  		if (!entry || xa_is_value(entry))
>> > -			break;
>> > +			return xas.xa_index;
>> >  		if (xas.xa_index == ULONG_MAX)
>> > -			break;
>> > +			return ULONG_MAX;
>> >  	}
>> 
>> If I understand this correctly, couldn't we just do something like:
>> if (!max_scan)
>> 	return xas.xa_index - 1;
> 
> I think the easiest to understand would be to do the above two explicit
> returns instead of 'break' and change below to:
> 
> 	/* Return start of the range - 1 when no hole is found */
> 	return xas.xa_index - 1;

I can do that, but I think it should be consistent with
page_cache_next_miss(), which does index + max_scan. If the xas.xa_index
approach is preferred, I'll change it in both functions, and also get rid of
nr.

The nice part of 'index - max_scan' is that the kdoc describes the range in
terms of that already.

Thoughts?

>> > -	return xas.xa_index;
>> > +	return index - max_scan;
>> >  }