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

[Jan Kara]

On Mon 11-05-26 14:15:41, Tal Zussman wrote:
> 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.

Frankly, whatever you pick :) I'm fine also with the patch as you've
originally written it. I was mostly suggesting this as a variant that also
looks sensible to me and maybe it would look better for Vishal.

								Honza
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR