Re: [RFC PATCH v4 1/5] mm/readahead: Honour new_order in page_cache_ra_order()

Ryan Roberts
2025-05-09T15:04:04Z

Hi Pankaj,

Thanks for the review! ...

On 08/05/2025 13:55, Pankaj Raghav (Samsung) wrote:
> Hey Ryan,
>
> On Wed, Apr 30, 2025 at 03:59:14PM +0100, Ryan Roberts wrote:
>> FOLIO 0x0001a000 0x0001b000 4096 26 27 1 0
>> FOLIO 0x0001b000 0x0001c000 4096 27 28 1 0
>> FOLIO 0x0001c000 0x0001d000 4096 28 29 1 0
>> FOLIO 0x0001d000 0x0001e000 4096 29 30 1 0
>> FOLIO 0x0001e000 0x0001f000 4096 30 31 1 0
>> FOLIO 0x0001f000 0x00020000 4096 31 32 1 0
>> FOLIO 0x00020000 0x00024000 16384 32 36 4 2
>> FOLIO 0x00024000 0x00028000 16384 36 40 4 2
>> FOLIO 0x00028000 0x0002c000 16384 40 44 4 2
>> FOLIO 0x0002c000 0x00030000 16384 44 48 4 2
>> ...
>>
>> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
>> ---
>> mm/readahead.c | 4 +---
>> 1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/mm/readahead.c b/mm/readahead.c
>> index 6a4e96b69702..8bb316f5a842 100644
>> --- a/mm/readahead.c
>> +++ b/mm/readahead.c
>> @@ -479,9 +479,6 @@ void page_cache_ra_order(struct readahead_control *ractl,
>>
>
> So we always had a fallback to do_page_cache_ra() if the size of the
> readahead is less than 4 pages (16k). I think this was there because we
> were adding `2` to the new_order:

If this is the reason for the magic number 4, then it's a bug in itself IMHO. 4
pages is only 16K when the page size is 4K; arm64 supports other page sizes. But
additionally, it's not just ra->size that dictates the final order of the folio;
it also depends on alignment in the file, EOF, etc.

If we remove the fallback condition completely, things will still work out. So
unless someone can explain the reason for that condition (Matthew?), my vote
would be to remove it entirely.

>
> unsigned int min_ra_size = max(4, mapping_min_folio_nrpages(mapping));
>
> /*
> * Fallback when size < min_nrpages as each folio should be
> * at least min_nrpages anyway.
> */
> if (!mapping_large_folio_support(mapping) || ra->size < min_ra_size)
> goto fallback;
>
>> limit = min(limit, index + ra->size - 1);
>>
>> - if (new_order < mapping_max_folio_order(mapping))
>> - new_order += 2;
>
> Now that you have moved this, we could make the lhs of the max to be 2
> (8k) instead of 4(16k).

I don't really understand why magic number 2 would now be correct?

>
> - unsigned int min_ra_size = max(4, mapping_min_folio_nrpages(mapping));
> + unsigned int min_ra_size = max(2, mapping_min_folio_nrpages(mapping));
>
> I think if we do that, we might ramp up to 8k sooner rather than jumping
> from 4k to 16k directly?

In practice I don't think so; This would only give us order-1 where we didn't
have it before if new_order >= 1 and ra->size is 3 or 4 pages.

But as I said, my vote would be to remove this fallback condition entirely. What
do you think?

Thanks,
Ryan

>
>> -
>> new_order = min(mapping_max_folio_order(mapping), new_order);
>> new_order = min_t(unsigned int, new_order, ilog2(ra->size));
>> new_order = max(new_order, min_order);
>> @@ -683,6 +680,7 @@ void page_cache_async_ra(struct readahead_control *ractl,
>> ra->size = get_next_ra_size(ra, max_pages);
>> ra->async_size = ra->size;
>> readit:
>> + order += 2;
>> ractl->_index = ra->start;
>> page_cache_ra_order(ractl, ra, order);
>> }
>> --
>> 2.43.0
>>
>
> --
> Pankaj