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
