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

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.

-- 
Best Regards,
Yan, Zi