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@xxxxxxxx>
SUSE Labs, CR