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? Thanks. -- Best Regards, Yan, Zi
