Re: [PATCH RFC 07/14] fs/erofs: mm/pagemap: add readahead_folio_reverse() to avoid folio->private

[Zi Yan <ziy@nvidia.com>]

On 5 Aug 2026, at 7:42, Zi Yan wrote:

> On Wed Aug 5, 2026 at 5:25 AM EDT, Jan Kara wrote:
>> On Tue 04-08-26 13:09:46, Zi Yan wrote:
>>> On Tue Aug 4, 2026 at 1:04 PM EDT, Jan Kara wrote:
>>>> On Tue 04-08-26 11:54:41, Zi Yan wrote:
>>>>> On Tue Aug 4, 2026 at 5:32 AM EDT, Jan Kara wrote:
>>>>>> On Mon 03-08-26 12:56:36, Zi Yan wrote:
>>>>>>> On Mon Aug 3, 2026 at 5:54 AM EDT, Jan Kara wrote:
>>>>>>>> On Fri 31-07-26 22:13:30, Zi Yan wrote:
>>>>>>>>> erofs needs to traverse readahead folios in reverse order to achieve
>>>>>>>>> maximum performance by
>>>>>>>>> 1. reading all folios from readahead_folio();
>>>>>>>>> 2. storing the prior folio pointer in folio->private;
>>>>>>>>> 3. traverse from the last folio to the first one.
>>>>>>>>>
>>>>>>>>> Add readahead_folio_reverse() to achieve the same function without using
>>>>>>>>> folio->private.
>>>>>>>>>
>>>>>>>>> It prepares for a future commit that replaces PG_private checks with
>>>>>>>>> !folio->private checks. After switching the checks, erofs's use of
>>>>>>>>> folio->private without bumping folio refcount can cause unexpected
>>>>>>>>> outcomes, e.g., in filemap_release_folio(), try_to_free_buffers() becomes
>>>>>>>>> reachable.
>>>>>
>>>>> <snip>
>>>>>
>>>>>>>
>>>>>>> The below is what I come up with. I did not add a bool to
>>>>>>> readahead_control, since I think that is the decision of caller of
>>>>>>> __readahead_advance(). But let me know if you disagree.
>>>>>>
>>>>>> The reason why I wanted bool in readahead_control is that if some code
>>>>>> ends up mixing readahead_folio() with readahead_folio_last() things will
>>>>>> get confused (because __readahead_advance() really wants to skip the batch
>>>>>> returned from the *previous* call to readahead_folio[_last]()). With the
>>>>>> bool in rac, even mixed use will properly advance the state of the
>>>>>> readahead_control. I don't think mixed use is very realistic (at this
>>>>>> point at least) so I'm ok with leaving that for later if you don't like it.
>>>>>
>>>>> Got it. I am trying to figure out your mental model of how the mix of
>>>>> readahead_folio() and readahead_folio_last() works with the bool inside
>>>>> ractl. By looking at readahead_folio_last() code, it is almost the same
>>>>> as readahead_folio() with __readahead_folio() inlined
>>>>> (__readahead_folio() is only used by readahead_folio(), so the inline
>>>>> can happen without any issue). As a result, we can get rid of
>>>>> readahead_folio_last(), add set_readahead_direction() to set the
>>>>> embedded bool read_from_head, and use readahead_folio() only. This
>>>>> removes redundant code in readahead_folio_last(). One thing I am not
>>>>> certain is whether we want to
>>>>>
>>>>> 1. use set_readahead_direction() explicit and warn readahead_folio() if
>>>>> read_from_head is not initialized, or
>>>>>
>>>>> 2. set read_from_head to true by default, so that only erofs needs to
>>>>> call set_readahead_direction() to change read_from_head.
>>>>>
>>>>> The former is less confusing but changes how readahead_folio() works;
>>>>> the latter is simpler but implicit read_from_head state might confuse
>>>>> people at some point.
>>>>
>>>> My idea was: readahead_folio() will call __readahead_advance() and then set
>>>> rac->forward = true. readahead_folio_last() will call __readahead_advance()
>>>> and set rac->forward = false. __readahead_advance() advances from beginning
>>>> / end based on rac->_forward value.
>>>
>>> Got it. I can do that. Just to be clear, it should be that
>>> readahead_folio() first sets rac->forward = true, then calls
>>> __readahead_advance(), since __readahead_advance() advances based on
>>> rac->forward, right? readahead_folio_last() as well.
>>
>> No. I wrote "and then set" which means after and that is what I really
>> wanted to say. You still don't seem to be understanding the logic of handling
>> the _batch_count. _batch_count is the length of the returned batch.
>> __readahead_advance() updates _index and _nr_pages to remove the folios
>> returned in the last batch from the range. So _forward needs to contain
>> whether the last returned batch was taken from the beginning or the end of
>> the range and __readahead_advance() uses it to update current range
>> accordingly (before we go and return the next batch). We cannot clobber
>> _forward before calling __readahead_advance(). I hope things are clearer
>> now.
>
> Got it. Sorry I made some assumption instead of asking my question, so I
> misinterpret your words. My question is who sets the initial value of
> _forward? So that __readahead_advance() can update _index and _nr_pages
> correctly at the first time __readahead_folio() is called?
>
> __readahead_folio() does:
>
> 1. update _nr_pages and _index,
> 2. return NULL if _nr_pages is 0 and set _batch_count to 0,
> 3. return folio using xa_load and set _batch_count to folio_nr_pages().
>
> after the change:
>
> 1. call __readahead_advance() to update _index, _nr_pages, and
> _batch_count based on _forward,
> 2. update _forward to true, since it is __readahead_folio()
> 3. return NULL or folio based on _nr_pages.
>
> Then the first time __readahead_folio() is called, who sets _forward to
> make 1 work correctly?

Never mind. Codex answered this:

The first-call initialization is not a problem: DEFINE_READAHEAD()
zero-initializes omitted fields, and _batch_count starts as zero,
so the first advance is a no-op.

I will fix my patch. Thanks.

Best Regards,
Yan, Zi