Re: [PATCH] mm/filemap: restart mmap readahead on sequential faults after many misses

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] mm/filemap: do_sync_mmap_readahead() recovery fails when
  min_order > 0
- [Medium] mm/filemap: do_sync_mmap_readahead() ignores VM_RAND_READ and
  !ra_pages
- [Low] mm/filemap: prev_pos == -1 wraps to match pgoff 0 on 32-bit in
  mmap ra
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [Medium] mm/readahead: unsigned underflow of index - start in
  page_cache_ra_order() [1]
--

commit 607714545f8f33b9040290a1fe8d984f2caffeba
Author: Gopal Malaviya <gopalmalaviya53@gmail.com>

mm/filemap: restart mmap readahead on sequential faults after many misses

When mmap_miss exceeds MMAP_LOTSAMISS in do_sync_mmap_readahead(), this
commit records the fault offset in ra->prev_pos and calls
page_cache_sync_ra() if the next fault miss lands on the immediately
following page, while clamping ra->mmap_miss to MMAP_LOTSAMISS * 10.

> diff --git a/mm/filemap.c b/mm/filemap.c
> index 371a4bbd06876..1548021472729 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -3398,9 +3398,27 @@ 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.
> +		 *
> +		 * But without read-around no hits are left to bring
> +		 * mmap_miss down again, so a file that turns sequential
> +		 * would stay without read-ahead. Remember where we missed,
> +		 * and treat a miss on the next page like a sequential
> +		 * read() miss: page_cache_sync_ra() starts with a small
> +		 * window and only grows it while access stays sequential.
> +		 * Keep mmap_miss at its limit, so that hits on that window
> +		 * do not bring back full read-around for random access.
>  		 */
> -		if (mmap_miss > MMAP_LOTSAMISS)
> +		if (mmap_miss > MMAP_LOTSAMISS) {
> +			pgoff_t prev_index = (unsigned long long)ra->prev_pos >> PAGE_SHIFT;
> +
> +			ra->prev_pos = (loff_t)vmf->pgoff << PAGE_SHIFT;
> +			if (vmf->pgoff - prev_index != 1)
> +				return fpin;

[Severity: Low]
On 32-bit architectures, can a fault at vmf->pgoff == 0 match
vmf->pgoff - prev_index == 1 when ra->prev_pos is still -1?

file_ra_state_init() initializes ra->prev_pos to -1, and mmap faults while
mmap_miss <= MMAP_LOTSAMISS do not update it.  When the first fault with
mmap_miss > MMAP_LOTSAMISS lands on pgoff 0, truncating
(unsigned long long)ra->prev_pos >> PAGE_SHIFT to a 32-bit pgoff_t yields
prev_index = ULONG_MAX, so 0UL - ULONG_MAX == 1UL wraps around to true and
starts readahead on a non-sequential miss.

> +			WRITE_ONCE(ra->mmap_miss, MMAP_LOTSAMISS * 10);
> +			fpin = maybe_unlock_mmap_for_io(vmf, fpin);
> +			page_cache_sync_ra(&ractl, 1);
>  			return fpin;
> +		}
>  	}
>  
>  	if (force_thp_readahead) {

[Severity: Medium]
Does this readahead recovery work when mapping_min_folio_order(mapping) > 0?

When the first miss above MMAP_LOTSAMISS returns without readahead,
filemap_fault() -> __filemap_get_folio_mpol() allocates a folio of
min_nrpages = mapping_min_folio_nrpages(mapping) base pages (at least 2).
During a sequential scan starting at a folio-aligned offset, the next cache
miss occurs at prev_index + min_nrpages, so vmf->pgoff - prev_index != 1 is
always true and readahead never starts.

Even if the initial miss was on the last base page of the previous folio so
that vmf->pgoff - prev_index == 1 holds, passing req_count = 1 instead of
mapping_min_folio_nrpages(mapping) to page_cache_sync_ra() sets mark =
index + 1 in page_cache_ra_order().  In ra_alloc_folio(),

mm/readahead.c:ra_alloc_folio() {
    ...
	mark = round_down(mark, 1UL << order);
	if (index == mark)
		folio_set_readahead(folio);
    ...
}

rounding mark down by 1UL << order places PG_readahead on the first
(faulting) folio instead of the next folio.  Because
maybe_unlock_mmap_for_io() drops mmap_lock, the retried fault runs with
FAULT_FLAG_TRIED set, skips do_async_mmap_readahead(), and maps that folio
via finish_fault(), so page_cache_async_ra() is never called to grow the
window.

[Severity: Medium]
When force_thp_readahead is true, can this block run page_cache_sync_ra()
even when VM_RAND_READ is set or ra->ra_pages is 0?

do_sync_mmap_readahead() only checks VM_RAND_READ and !ra->ra_pages when
!force_thp_readahead.  Once mmap_miss exceeds MMAP_LOTSAMISS on a
VM_HUGEPAGE VMA backed by a mapping with large folio support, two adjacent
misses reach this path: with VM_RAND_READ, page_cache_sync_ra() only checks
FMODE_RANDOM and issues multi-page sequential readahead while setting
PG_readahead on a folio that do_async_mmap_readahead() refuses to clear;
with !ra->ra_pages, maybe_unlock_mmap_for_io() drops mmap_lock and forces a
VM_FAULT_RETRY cycle only for page_cache_sync_ra() to do a single-page
forced read.

[Severity: Medium]
This problem wasn't introduced by this patch, but in page_cache_ra_order()
[1], can index - start underflow when an unaligned readahead request
fails on its first folio allocation?

mm/readahead.c:page_cache_ra_order() {
	pgoff_t start = readahead_index(ractl);
	pgoff_t index = start;
    ...
	ractl->_index = mapping_align_index(mapping, index);
	index = readahead_index(ractl);

	while (index <= limit) {
    ...
		err = ra_alloc_folio(ractl, index, mark, order, gfp);
		if (err)
			break;
		index += 1UL << order;
	}
    ...
fallback:
	if (ra->size > index - start)
		do_page_cache_ra(ractl, ra->size - (index - start),
				 ra->async_size);
}

On a mapping with mapping_min_folio_order(mapping) > 0, start holds the
unaligned readahead_index(ractl) before mapping_align_index() rounds index
down to a min_order boundary, making index < start.  If the first
ra_alloc_folio() call fails (for example with -EEXIST when a folio is
already cached at the aligned index), the loop breaks before advancing
index, so index - start wraps around near ULONG_MAX and skips the
do_page_cache_ra() fallback for the entire window.

[1] https://sashiko.dev/#/bug/linux-da6e8836-f808-4b39-9f04-de52995a0162

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109091444.128521-1-gopalmalaviya53@gmail.com?part=1