Re: [PATCH v3 04/11] readahead: rework loop in page_cache_ra_unbounded()

[Hannes Reinecke]

On 3/26/24 11:06, Pankaj Raghav wrote:
> On 26/03/2024 11:00, Hannes Reinecke wrote:
> > On 3/26/24 10:44, Pankaj Raghav wrote:
> > > Hi Hannes,
> > >
> > > On 26/03/2024 10:39, Hannes Reinecke wrote:
> > > > On 3/25/24 19:41, Matthew Wilcox wrote:
> > > > > On Wed, Mar 13, 2024 at 06:02:46PM +0100, Pankaj Raghav (Samsung) wrote:
> > > > > > @@ -239,8 +239,8 @@ void page_cache_ra_unbounded(struct readahead_control *ractl,
> > > > > >                  * not worth getting one just for that.
> > > > > >                  */
> > > > > >                 read_pages(ractl);
> > > > > > -            ractl->_index++;
> > > > > > -            i = ractl->_index + ractl->_nr_pages - index - 1;
> > > > > > +            ractl->_index += folio_nr_pages(folio);
> > > > > > +            i = ractl->_index + ractl->_nr_pages - index;
> > > > > >                 continue;
> > > > > >             }
> > > > > >     @@ -252,13 +252,14 @@ void page_cache_ra_unbounded(struct readahead_control *ractl,
> > > > > >                 folio_put(folio);
> > > > > >                 read_pages(ractl);
> > > > > >                 ractl->_index++;
> > > > > > -            i = ractl->_index + ractl->_nr_pages - index - 1;
> > > > > > +            i = ractl->_index + ractl->_nr_pages - index;
> > > > > >                 continue;
> > > > > >             }
> > > > >
> > > > > You changed index++ in the first hunk, but not the second hunk.  Is that
> > > > > intentional?
> > > >
> > > > Hmm. Looks you are right; it should be modified, too.
> > > > Will be fixing it up.
> > > You initially had also in the second hunk:
> > > ractl->index += folio_nr_pages(folio);
> > >
> > > and I changed it to what it is now.
> > >
> > > The reason is in my reply to willy:
> > > https://lore.kernel.org/linux-xfs/s4jn4t4betknd3y4ltfccqxyfktzdljiz7klgbqsrccmv3rwrd@orlwjz77oyxo/
> > >
> > > Let me know if you agree with it.
> > Bah. That really is overly complicated. When we attempt a conversion that conversion should be
> > stand-alone, not rely on some other patch modifications later on.
> > We definitely need to work on that to make it easier to review, even
> > without having to read the mail thread.
>
> I don't know understand what you mean by overly complicated. This conversion is standalone and it is
> wrong to use folio_nr_pages after we `put` the folio. This patch just reworks the loop and in the
> next patch I add min order support to readahead.
>
> This patch doesn't depend on the next patch.

Let me rephrase: what does 'ractl->_index' signify?
From my understanding it should be the index of the
first folio/page in ractl, right?

If so I find it hard to understand how we _could_ increase it by one; 
_index should _always_ in units of the minimal pagemap size.
And if we don't have it here (as you suggested in the mailthread)
I'd rather move this patch _after_ the minimal pagesize is introduced
to ensure that _index is always incremented by the right amount.

Cheers,

Hannes