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
