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

["Zi Yan" <ziy@nvidia.com>]

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@nvidia.com>
>> To: Gao Xiang <xiang@kernel.org>
>> To: Chao Yu <chao@kernel.org>
>> To: "Matthew Wilcox (Oracle)" <willy@infradead.org>
>> To: Jan Kara <jack@suse.cz>
>> Cc: Yue Hu <zbestahu@gmail.com>
>> Cc: Jeffle Xu <jefflexu@linux.alibaba.com>
>> Cc: Sandeep Dhavale <dhavale@google.com>
>> Cc: Hongbo Li <hongbohbli@tencent.com>
>> Cc: Chunhai Guo <guochunhai@vivo.com>
>> Cc: linux-erofs@lists.ozlabs.org
>> Cc: linux-kernel@vger.kernel.org
>> Cc: linux-fsdevel@vger.kernel.org
>> Cc: linux-mm@kvack.org
>> ---
>>  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/

>
>>  		err = z_erofs_scan_folio(&f, folio, true);
>>  		if (err && err != -EINTR)
>>  			erofs_err(realinode->i_sb, "readahead error at folio %lu @ nid %llu",
>> diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
>> index 939f3a5e973f6..2257df004305e 100644
>> --- a/include/linux/pagemap.h
>> +++ b/include/linux/pagemap.h
>> @@ -1415,6 +1415,7 @@ struct readahead_control {
>>  	bool dropbehind;
>>  	bool _workingset;
>>  	unsigned long _pflags;
>> +	bool _forward;
>>  };
>>  
>>  #define DEFINE_READAHEAD(ractl, f, r, m, i)				\
>> @@ -1479,18 +1480,25 @@ void page_cache_async_readahead(struct address_space *mapping,
>>  	page_cache_async_ra(&ractl, folio, req_count);
>>  }
>>  
>> +static inline void __readahead_advance(struct readahead_control *rac)
>> +{
>> +	if (rac->_forward)
>> +		rac->_index += rac->_batch_count;
>
> No expert, but shouldn't we decrement the _index somewhere in the other case? Or
> where is that done? A comment might help :)

A readahead folio comes from [_index, _index + _nr_pages), so for
last/reverse/backwards case, the code only needs to decrease _nr_pages.

Will add a comment about this.

-- 
Best Regards,
Yan, Zi