On 2023-09-20 16:13, Hannes Reinecke wrote:
> On 9/20/23 13:56, Pankaj Raghav wrote:
>> On Mon, Sep 18, 2023 at 01:04:53PM +0200, Hannes Reinecke wrote:
>>> if (folio && !xa_is_value(folio)) {
>>> @@ -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;
>> I am not entirely sure if this is correct.
>>
>> The above if condition only verifies if a folio is in the page cache but
>> doesn't tell if it is uptodate. But we are advancing the ractl->index
>> past this folio irrespective of that.
>>
>> Am I missing something?
>
> Confused. Which condition?
> I'm not changing the condition at all, just changing the way how the 'i' index is calculated during
> the loop (ie getting rid of the weird decrement and increment on 'i' during the loop).
> Please clarify.
>
I made a mistake of pointing out the wrong line in my reply. I was concerned about the increment to
the ractl->_index:
if (folio && !xa_is_value(folio)) {
....
read_pages(ractl);
ractl->_index += folio_nr_pages(folio); // This increment to the ractl->_index
...
}
But I think I was missing this point, as willy explained in his reply:
If there's a !uptodate folio in the page cache, <snip>. If that happened, we
should not try reading it **again in readahead**; we should let the thread
retry the read when it actually tries to access the folio.
Plus your changes optimizes the code path for a large folio where we increment the index by 1 each
time instead of moving the index directly to the next folio in the page cache.
Please ignore the noise!
