Re: [PATCH v2] mm/readahead: Fix large folio support in async readahead

[Yafang Shao <laoar.shao@gmail.com>]

On Mon, Nov 11, 2024 at 11:26 PM David Hildenbrand <david@redhat.com> wrote:
>
> On 11.11.24 16:05, David Hildenbrand wrote:
> > On 11.11.24 15:28, Yafang Shao wrote:
> >> On Mon, Nov 11, 2024 at 6:33 PM David Hildenbrand <david@redhat.com> wrote:
> >>>
> >>> On 08.11.24 15:17, Yafang Shao wrote:
> >>>> When testing large folio support with XFS on our servers, we observed that
> >>>> only a few large folios are mapped when reading large files via mmap.
> >>>> After a thorough analysis, I identified it was caused by the
> >>>> `/sys/block/*/queue/read_ahead_kb` setting. On our test servers, this
> >>>> parameter is set to 128KB. After I tune it to 2MB, the large folio can
> >>>> work as expected. However, I believe the large folio behavior should not be
> >>>> dependent on the value of read_ahead_kb. It would be more robust if the
> >>>> kernel can automatically adopt to it.
> >>>
> >>> Now I am extremely confused.
> >>>
> >>> Documentation/ABI/stable/sysfs-block:
> >>>
> >>> "[RW] Maximum number of kilobytes to read-ahead for filesystems on this
> >>> block device."
> >>>
> >>>
> >>> So, with your patch, will we also be changing the readahead size to
> >>> exceed that, or simply allocate larger folios and not exceeding the
> >>> readahead size (e.g., leaving them partially non-filled)?
> >>
> >> Exceeding the readahead size for the MADV_HUGEPAGE case is
> >> straightforward; this is what the current patch accomplishes.
> >>
> >
> > Okay, so this only applies with MADV_HUGEPAGE I assume. Likely we should
> > also make that clearer in the subject.
> >
> > mm/readahead: allow exceeding configured read_ahead_kb with MADV_HUGEPAGE
> >
> >
> > If this is really a fix, especially one that deserves CC-stable, I
> > cannot tell. Willy is the obvious expert :)
> >
> >>>
> >>> If you're also changing the readahead behavior to exceed the
> >>> configuration parameter it would sound to me like "I am pushing the
> >>> brake pedal and my care brakes; fix the brakes to adopt whether to brake
> >>> automatically" :)
> >>>
> >>> Likely I am missing something here, and how the read_ahead_kb parameter
> >>> is used after your patch.
> >>
> >> The read_ahead_kb parameter continues to function for
> >> non-MADV_HUGEPAGE scenarios, whereas special handling is required for
> >> the MADV_HUGEPAGE case. It appears that we ought to update the
> >> Documentation/ABI/stable/sysfs-block to reflect the changes related to
> >> large folios, correct?
> >
> > Yes, how it related to MADV_HUGEPAGE. I would assume that it would get
> > ignored, but ...
> >
> > ... staring at get_next_ra_size(), it's not quite ignored, because we
> > still us it as a baseline to detect how much we want to bump up the
> > limit when the requested size is small? (*2 vs *4 etc) :/
> >
> > So the semantics are really starting to get weird, unless I am missing
> > something important.
> Likely what I am missing is that the value of get_next_ra_size() will never be relevant
> in that case. I assume the following would end up doing the same:
>
> iff --git a/mm/readahead.c b/mm/readahead.c
> index 475d2940a1edb..cc7f883f83d86 100644
> --- a/mm/readahead.c
> +++ b/mm/readahead.c
> @@ -668,7 +668,12 @@ void page_cache_async_ra(struct readahead_control *ractl,
>          ra->start = start;
>          ra->size = start - index;       /* old async_size */
>          ra->size += req_count;
> -       ra->size = get_next_ra_size(ra, max_pages);
> +       /*
> +        * Allow the actual size to exceed the readahead window for
> +        * MADV_HUGEPAGE.
> +        */
> +       if (ra->size < max_pages)
> +               ra->size = get_next_ra_size(ra, max_pages);

This change doesn’t apply to MADV_HUGEPAGE because, in cases where
`ra->size > max_pages`, ra->size has already been set to max_pages.
This can be easily verified with the example provided in the previous
version[1].

[1]. https://lore.kernel.org/linux-mm/20241106092114.8408-1-laoar.shao@gmail.com/

>          ra->async_size = ra->size;
>   readit:
>          ractl->_index = ra->start;
>
>
> So maybe it should just be in get_next_ra_size() where we clarify what "max_pages"
> means and why we simply decide to ignore the value ...

-- 
Regards
Yafang