On 2025/9/4 06:50, Andrew Morton wrote:
On Tue, 12 Aug 2025 17:08:53 +0800 Youling Tang <youling.tang@xxxxxxxxx> wrote:
Hi, Jan
On 2025/7/14 17:33, Jan Kara wrote:
On Fri 11-07-25 13:55:09, Youling Tang wrote:
From: Youling Tang <tangyouling@xxxxxxxxxx>
...
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -2584,8 +2584,9 @@ static int filemap_get_pages(struct kiocb *iocb, size_t count,
unsigned int flags;
int err = 0;
- /* "last_index" is the index of the page beyond the end of the read */
- last_index = DIV_ROUND_UP(iocb->ki_pos + count, PAGE_SIZE);
+ /* "last_index" is the index of the folio beyond the end of the read */
+ last_index = round_up(iocb->ki_pos + count, mapping_min_folio_nrbytes(mapping));
+ last_index >>= PAGE_SHIFT;
I think that filemap_get_pages() shouldn't be really trying to guess what
readahead code needs and round last_index based on min folio order. After
all the situation isn't special for LBS filesystems. It can also happen
that the readahead mark ends up in the middle of large folio for other
reasons. In fact, we already do have code in page_cache_ra_order() ->
ra_alloc_folio() that handles rounding of index where mark should be placed
so your changes essentially try to outsmart that code which is not good. I
think the solution should really be placed in page_cache_ra_order() +
ra_alloc_folio() instead.
In fact the problem you are trying to solve was kind of introduced (or at
least made more visible) by my commit ab4443fe3ca62 ("readahead: avoid
multiple marked readahead pages"). There I've changed the code to round the
index down because I've convinced myself it doesn't matter and rounding
down is easier to handle in that place. But your example shows there are
cases where rounding down has weird consequences and rounding up would have
been better. So I think we need to come up with a method how to round up
the index of marked folio to fix your case without reintroducing problems
mentioned in commit ab4443fe3ca62.
Yes, I simply replaced round_up() in ra_alloc_folio() with round_down()
to avoid this phenomenon before submitting this patch.
But at present, I haven't found a suitable way to solve both of these
problems
simultaneously. Do you have a better solution on your side?
fyi, this patch remains stuck in mm.git awaiting resolution.
Do we have any information regarding its importance? Which means do we
have any measurement of its effect upon any real-world workload?
No measurements were taken regarding the impact on real-world
workloads, this was merely a line of thinking triggered by unusual
readahead behavior.
Thanks,
Youling.
Thanks.
