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

[Tal Zussman <tz2294@columbia.edu>]

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;
>> >  }