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
