On 06/28/23 12:43, Yin Fengwei wrote:
> The commit
> 9425c591e06a ("page cache: fix page_cache_next/prev_miss off by one")
> updated the page_cache_next_miss() to return the index beyond
> range.
>
> But it breaks the start/size of ra in ondemand_readahead() because
> the offset by one is accumulated to readahead_index. As a consequence,
> not best readahead order is picked.
>
> Tracing of the order parameter of filemap_alloc_folio() showed:
> page order : count distribution
> 0 : 892073 | |
> 1 : 0 | |
> 2 : 65120457 |****************************************|
> 3 : 32914005 |******************** |
> 4 : 33020991 |******************** |
> with 9425c591e06a9.
>
> With parent commit:
> page order : count distribution
> 0 : 3417288 |**** |
> 1 : 0 | |
> 2 : 877012 |* |
> 3 : 288 | |
> 4 : 5607522 |******* |
> 5 : 29974228 |****************************************|
>
> Fix the issue by removing the offset by one when page_cache_next_miss()
> returns no gaps in the range.
>
> After the fix:
> page order : count distribution
> 0 : 2598561 |*** |
> 1 : 0 | |
> 2 : 687739 | |
> 3 : 288 | |
> 4 : 207210 | |
> 5 : 32628260 |****************************************|
>
Thank you for your detailed analysis!
When the regression was initially discovered, I sent a patch to revert
commit 9425c591e06a. Andrew has picked up this change. And, Andrew has
also picked up this patch.
I have not verified yet, but I suspect that this patch is going to cause
a regression because it depends on the behavior of page_cache_next_miss
in 9425c591e06a which has been reverted.
Sorry for the delay in responding as I was traveling.
--
Mike Kravetz
> Reported-by: kernel test robot <oliver.sang@xxxxxxxxx>
> Closes: https://lore.kernel.org/oe-lkp/202306211346.1e9ff03e-oliver.sang@xxxxxxxxx
> Fixes: 9425c591e06a ("page cache: fix page_cache_next/prev_miss off by one")
> Signed-off-by: Yin Fengwei <fengwei.yin@xxxxxxxxx>
> ---
> Changes from v1:
> - only removing offset by one when there is no gaps found by
> page_cache_next_miss()
> - Update commit message to include the histogram of page order
> after fix
>
> mm/readahead.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/mm/readahead.c b/mm/readahead.c
> index 47afbca1d122..a93af773686f 100644
> --- a/mm/readahead.c
> +++ b/mm/readahead.c
> @@ -614,9 +614,17 @@ static void ondemand_readahead(struct readahead_control *ractl,
> max_pages);
> rcu_read_unlock();
>
> - if (!start || start - index > max_pages)
> + if (!start || start - index - 1 > max_pages)
> return;
>
> + /*
> + * If no gaps in the range, page_cache_next_miss() returns
> + * index beyond range. Adjust it back to make sure
> + * ractl->_index is updated correctly later.
> + */
> + if ((start - index - 1) == max_pages)
> + start--;
> +
> ra->start = start;
> ra->size = start - index; /* old async_size */
> ra->size += req_size;
> --
> 2.39.2
>
