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

[David Hildenbrand (Arm)]

On 9/8/26 19:05, Zi Yan wrote:
> On Tue Sep 8, 2026 at 12:04 PM EDT, David Hildenbrand (Arm) wrote:
>> On 9/8/26 04:56, 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_last() to achieve the same function without using
>>> folio->private. __readahead_advance() helper shares readahead_control
>>> adjustment code among __readahead_folio(), readahead_folio_last(), and
>>> __readahead_batch() by checking new private member, _forward, of
>>> readahead_control.
>>>
>>> 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.
>>
>> Ah, I was just about to ask. So it's really about folios never using
>> folio->private manually (without the attach/detach).
>>
>>>
>>> No functional change intended.
>>>
>>> Assisted-by: Claude:claude-opus-4-8
>>> Assisted-by: Codex:gpt-5
>>> Signed-off-by: Zi Yan <ziy@xxxxxxxxxx>
>>> To: Gao Xiang <xiang@xxxxxxxxxx>
>>> To: Chao Yu <chao@xxxxxxxxxx>
>>> To: "Matthew Wilcox (Oracle)" <willy@xxxxxxxxxxxxx>
>>> To: Jan Kara <jack@xxxxxxx>
>>> Cc: Yue Hu <zbestahu@xxxxxxxxx>
>>> Cc: Jeffle Xu <jefflexu@xxxxxxxxxxxxxxxxx>
>>> Cc: Sandeep Dhavale <dhavale@xxxxxxxxxx>
>>> Cc: Hongbo Li <hongbohbli@xxxxxxxxxxx>
>>> Cc: Chunhai Guo <guochunhai@xxxxxxxx>
>>> Cc: linux-erofs@xxxxxxxxxxxxxxxx
>>> Cc: linux-kernel@xxxxxxxxxxxxxxx
>>> Cc: linux-fsdevel@xxxxxxxxxxxxxxx
>>> Cc: linux-mm@xxxxxxxxx
>>> ---
>>>  fs/erofs/zdata.c        | 13 +++---------
>>>  include/linux/pagemap.h | 56 ++++++++++++++++++++++++++++++++++++++++++-------
>>>  2 files changed, 51 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/fs/erofs/zdata.c b/fs/erofs/zdata.c
>>> index e1e25ca0d1904..78fd7d980e957 100644
>>> --- a/fs/erofs/zdata.c
>>> +++ b/fs/erofs/zdata.c
>>> @@ -1898,21 +1898,14 @@ static void z_erofs_readahead(struct readahead_control *rac)
>>>  	struct inode *realinode = erofs_real_inode(sharedinode, &need_iput);
>>>  	Z_EROFS_DEFINE_FRONTEND(f, realinode, sharedinode, readahead_pos(rac));
>>>  	unsigned int nrpages = readahead_count(rac);
>>> -	struct folio *head = NULL, *folio;
>>> +	struct folio *folio;
>>>  	int err;
>>>  
>>>  	trace_erofs_readahead(realinode, readahead_index(rac), nrpages, false);
>>>  	z_erofs_pcluster_readmore(&f, rac, true);
>>> -	while ((folio = readahead_folio(rac))) {
>>> -		folio->private = head;
>>> -		head = folio;
>>> -	}
>>> -
>>> -	/* traverse in reverse order for best metadata I/O performance */
>>> -	while (head) {
>>> -		folio = head;
>>> -		head = folio_get_private(folio);
>>>  
>>> +	/* traverse from last to first for best metadata I/O performance */
>>> +	while ((folio = readahead_folio_last(rac))) {
>>
>> Intuitively, this should be called readahead_folio_reverse /
>> readahead_folio_reversed, thinking of list_for_each_entry_reverse()?
>>
>> 	list_for_each_entry_reverse - iterate backwards over list of given type.
>>
>> or maybe readahead_folio_backwards (which matches the forward below)
>>
>> But I'm not a readahead expert :)
> 
> Jan suggested the name[1]. It can be readahead_folio_reverse() if you
> prefer it, like Jan said.
> 
> [1] https://lore.kernel.org/all/332rknj4vo3cfhvfhhlf6pvg37s3lbrnzbbnv4swa6gctsiu6a@ndotgokvnglc/

Heh, to me _reverse() is clearer; whatever people prefer.

Acked-by: David Hildenbrand (Arm) <david@xxxxxxxxxx>

-- 
Cheers,

David