Re: [PATCH] mm/filemap: Align last_index to folio size

[Andrew Morton]

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?

Thanks.