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. > Cheers, > > Hannes >
