Re: Regression in NFS probably due to very large amounts of readahead

[Anders Blomdell <anders.blomdell@gmail.com>]

On 2024-11-26 17:55, Matthew Wilcox wrote:
> On Tue, Nov 26, 2024 at 04:28:04PM +0100, Anders Blomdell wrote:
>> On 2024-11-26 16:06, Jan Kara wrote:
>>> Hum, checking the history the update of ra->size has been added by Neil two
>>> years ago in 9fd472af84ab ("mm: improve cleanup when ->readpages doesn't
>>> process all pages"). Neil, the changelog seems as there was some real
>>> motivation behind updating of ra->size in read_pages(). What was it? Now I
>>> somewhat disagree with reducing ra->size in read_pages() because it seems
>>> like a wrong place to do that and if we do need something like that,
>>> readahead window sizing logic should rather be changed to take that into
>>> account? But it all depends on what was the real rationale behind reducing
>>> ra->size in read_pages()...
>>
>> My (rather limited) understanding of the patch is that it was intended to read those pages
>> that didn't get read because the allocation of a bigger folio failed, while not redoing what
>> readpages already did; how it was actually going to accomplish that is still unclear to me,
>> but I even don't even quite understand the comment...
>>
>> 	/*
>> 	 * If there were already pages in the page cache, then we may have
>> 	 * left some gaps.  Let the regular readahead code take care of this
>> 	 * situation.
>> 	 */
>>
>> the reason for an unchanged async_size is also beyond my understanding.
> 
> This isn't because we couldn't allocate a folio, this is when we
> allocated folios, tried to read them and we failed to submit the I/O.
> This is a pretty rare occurrence under normal conditions.

I beg to differ, the code is reached when there is
no folio support or ra->size < 4 (not considered in
this discussion) or falling throug when !err, err
is set by:

         err = ra_alloc_folio(ractl, index, mark, order, gfp);
                 if (err)
                         break;

isn't the reading done by:

         read_pages(ractl);

which does not set err!

/Anders