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 Tue, Oct 15, 2024 at 06:33:11PM +0100, Matthew Wilcox wrote:
> On Tue, Oct 15, 2024 at 06:41:06PM +0200, Pankaj Raghav (Samsung) wrote:
> 
> v14?  Where are v1..13 of this patch?  It's the first time I've seen it.

Sorry for the confusion. My git send script messed up the version
number. It is v1 :)

> 
> > The readahead flag is set on a folio based on the lookahead_size and
> > nr_to_read. For example, when the readahead happens from index to index
> > + nr_to_read, then the readahead `mark` offset from index is set at
> > nr_to_read - lookahead_size.
> > 
> > There are some scenarios where the lookahead_size > nr_to_read. If this
> > happens, readahead flag is not set on any folio on the current
> > readahead window.
> 
> Please describe those scenarios, as that's the important bit.
> 

Yes. I will do that in the next version. do_page_cache_ra() can clamp
the nr_to_read if the readahead window extends beyond EOF.

I think this probably happens when readahead window was created and
the file was truncated before the readahead starts.

> > There are two problems at the moment in the way `mark` is calculated
> > when lookahead_size > nr_to_read:
> > 
> > - unsigned long `mark` will be assigned a negative value which can lead
> >   to unexpected results in extreme cases due to wrap around.
> 
> Can such an extreme case happen?
> 

I think this is highly unlikely. I will probably remove this reason
from the commit message. It was just a bit weird to me that we are
assigning a negative number to an unsigned value which is supposed to be
the offset.

> > - 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
we shouldn't have. Does that make sense?

> I think the worst case scenario is that we set the flag on the wrong
> folio, causing readahead to occur when it should not.  But maybe I'm
> wrong?

You are right. We might unnecessarily trigger a readahead where we
should not. I think it is good to mention this consequence as well in
the commit message to be clear.

--
Pankaj