Re: Re: Re: [PATCH v14] mm: don't set readahead flag on a folio when lookahead_size > nr_to_read

["Pankaj Raghav (Samsung)" <kernel@pankajraghav.com>]

On Wed, Oct 16, 2024 at 12:57:44PM +0100, Matthew Wilcox wrote:
> On Wed, Oct 16, 2024 at 03:35:27PM +0530, Pankaj Raghav (Samsung) wrote:
> > > > - The current calculation for `mark` with mapping_min_order > 0 gives
> > > >   incorrect results when lookahead_size > nr_to_read due to rounding
> > > >   up operation.
> > > > 
> > > > Explicitly initialize `mark` to be ULONG_MAX and only calculate it
> > > > when lookahead_size is within the readahead window.
> > > 
> > > You haven't really spelled out the consequences of this properly.
> > > Perhaps a worked example would help.
> > 
> > Got it. I saw this while running generic/476 on XFS with 64k block size.
> > 
> > Let's assume the following values:
> > index = 128
> > nr_to_read = 16
> > lookahead_size = 28
> > mapping_min_order = 4 (16 pages)
> > 
> > The lookahead_size is actually lying outside the current readahead
> > window. The calculation without this patch will result in incorrect mark
> > as follows:
> > 
> > ra_folio_index = round_up(128 + 16 - 28, 16) = 128;
> > mark = 128 - 128 = 0;
> > 
> > So we will be marking the folio on 0th index with RA flag, even though

Oops. I shouldn't have said 0th index. I meant at offset 0 from the
index.

> > we shouldn't have. Does that make sense?
> 
> But we don't go back and find the folio for index 0.  We only consider
> the folios we're actually reading for marking.  So if 'mark' lies
> outside the readahead window, we simply won't mark any of them.  So I

`mark` is the offset from index. So we compare `mark` with the 
iterator `i`, which starts at 0. So we will set the RA flag on index
128 in this example, which is not correct.

if (i == mark)
	folio_set_readahead(folio);

With this patch, mark will be ULONG_MAX and not 0.

> don't think your patch changes anything.  Or did I miss something?

Does that make sense?