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

From: Hannes Reinecke
Date: Tue Mar 26 2024 - 06:55:19 EST


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