Re: [PATCH] mm/filemap: let sequential faults restart mmap readahead

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] mm/filemap: do_sync_mmap_readahead() fails when
  mapping_min_folio_order() > 0
--

commit 6cc011b67fa27fb6f1bce2ab0a11aad698934f50
Author: Alireza Haddadi <alireza.haddadi@it.uu.se>

mm/filemap: let sequential faults restart mmap readahead

When mmap_miss exceeds MMAP_LOTSAMISS and the page preceding the faulting
offset is cached, this commit reads ahead one additional page and marks it
with PG_readahead so sequential access can restart async readahead.

> diff --git a/mm/filemap.c b/mm/filemap.c
> index 93db8fc60ae0a..0930aacee9fea 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -3403,8 +3403,33 @@ static struct file *do_sync_mmap_readahead(struct vm_fault *vmf)
>  		 * Do we miss much more than hit in this file? If so,
>  		 * stop bothering with read-ahead. It will only hurt.
>  		 */
> -		if (mmap_miss > MMAP_LOTSAMISS)
> +		if (mmap_miss > MMAP_LOTSAMISS) {
> +			struct folio *prev = NULL;
> +
> +			/*
> +			 * Nothing is read ahead now, so mmap_miss stays high
> +			 * forever.  If the previous page is cached, this may be
> +			 * a sequential read: read the next page as well and
> +			 * mark it, so that async readahead, which does not
> +			 * check mmap_miss, restarts when the mark is hit.  The
> +			 * pages it reads then count as hits and lower
> +			 * mmap_miss.  A random reader rarely has the previous
> +			 * page cached and keeps reading one page per fault.
> +			 */
> +			if (!force_thp_readahead && vmf->pgoff)
> +				prev = xa_load(&mapping->i_pages,
> +					       vmf->pgoff - 1);
> +			if (prev && !xa_is_value(prev)) {
> +				fpin = maybe_unlock_mmap_for_io(vmf, fpin);
> +				ra->start = vmf->pgoff;
> +				ra->size = 2;
> +				ra->async_size = 1;
> +				ra->order = 0;
> +				ractl._index = ra->start;
> +				page_cache_ra_order(&ractl, ra);
> +			}
>  			return fpin;
> +		}
>  	}

[Severity: Medium]
Does hardcoding ra->size = 2, ra->async_size = 1, and ra->order = 0 work on
mappings where mapping_min_folio_order(mapping) > 0, such as large block size
filesystems?

In page_cache_ra_order(), limit and mark are computed in base pages before
new_order is clamped up to min_order:

mm/readahead.c:page_cache_ra_order() {
	...
	if (limit > index + ra->size - 1) {
		limit = index + ra->size - 1;
		mark = index + ra->size - ra->async_size;
	}
	...
	new_order = max(new_order, min_order);
}

When min_order >= 1, limit and mark both become index + 1.  The allocation
loop then allocates only a single min_order folio at index, and
ra_alloc_folio() rounds mark down to 1UL << order, placing PG_readahead on
the faulting folio itself instead of a second lookahead folio.

On the FAULT_FLAG_TRIED retry after maybe_unlock_mmap_for_io(),
do_async_mmap_readahead() is skipped and finish_fault() maps all pages of
that single folio at once, so async readahead never starts and mmap_miss
stays above MMAP_LOTSAMISS.  Scaling ra->size and ra->async_size by
mapping_min_folio_nrpages(mapping) and setting ra->order to
mapping_min_folio_order(mapping) would read two minimum-order folios and
mark the second one.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109-mmap-miss-v1-1-8b44b33f3358@it.uu.se?part=1