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@xxxxxxxxxx> 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);
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 ...
--
Cheers,
David / dhildenb
