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

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.
> >> 
> >> No funtional change intended.
> >> 
> >> Assisted-by: Claude:claude-opus-4-8
> >> Assisted-by: Codex:gpt-5
> >> Signed-off-by: Zi Yan <[email protected]>
> >> To: Gao Xiang <[email protected]>
> >> To: Chao Yu <[email protected]>
> >> To: "Matthew Wilcox (Oracle)" <[email protected]>
> >> To: Jan Kara <[email protected]>
> >> Cc: Yue Hu <[email protected]>
> >> Cc: Jeffle Xu <[email protected]>
> >> Cc: Sandeep Dhavale <[email protected]>
> >> Cc: Hongbo Li <[email protected]>
> >> Cc: Chunhai Guo <[email protected]>
> >> Cc: [email protected]
> >> Cc: [email protected]
> >> Cc: [email protected]
> >> Cc: [email protected]
> >
> > One comment regarding the generic infrastructure below.
> >
> >> diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
> >> index 4e8b2b29f6d3e..90904a4d173b7 100644
> >> --- a/include/linux/pagemap.h
> >> +++ b/include/linux/pagemap.h
> >> @@ -1549,6 +1549,37 @@ static inline struct folio *readahead_folio(struct readahead_control *ractl)
> >>  	return folio;
> >>  }
> >>  
> >> +/**
> >> + * readahead_folio_reverse - Get the next folio to read, from the tail.
> >> + * @ractl: The current readahead request.
> >> + *
> >> + * Like readahead_folio(), but walks the range back-to-front. The folio is
> >> + * returned locked with its refcount dropped; the caller unlocks it once I/O
> >> + * completes. Compound folios are returned once, at their head index.
> >> + *
> >> + * Context: The folio is locked.
> >> + * Return: A pointer to the next folio, or %NULL when done.
> >> + */
> >> +static inline struct folio *readahead_folio_reverse(struct readahead_control *ractl)
> >> +{
> >> +	struct folio *folio;
> >> +
> >> +	if (!ractl->_nr_pages)
> >> +		return NULL;
> >> +
> >> +	/* xa_load() follows sibling entries, so a tail index returns the head */
> >> +	folio = xa_load(&ractl->mapping->i_pages,
> >> +			ractl->_index + ractl->_nr_pages - 1);
> >> +	VM_WARN_ON_ONCE_FOLIO(!folio_test_locked(folio), folio);
> >> +
> >> +	/* Shrink the window from the tail down to this folio's head index */
> >> +	ractl->_nr_pages = folio->index - ractl->_index;
> >> +	ractl->_batch_count = 0;
> >
> > Thanks for the patch! Currently there's the invariant that the returned
> > folio is still inside the _index .. _index+_nr_pages range. I think when we
> > are providing a generic helper, we should keep that to make code more
> > robust for the future when more people start using it.
> 
> Definitely.
> 
> >
> > What I'd suggest doing is add bool in struct readahead_control telling
> > whether the last folio (batch) was taken from the head or tail of the
> > range, advance _nr_pages and _index accordingly in the functions returning
> > folios (probably hide this in a helper function __readahead_advance()
> > because it will be used in 3 places) and maybe call this new function
> > readahead_folio_last() instead of _reverse() (but I have only a slight
> > preference here so .._reverse() is ok with me if other people prefer it).
> 
> 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.

Also I have some minor comments below.

> diff --git a/fs/erofs/zdata.c b/fs/erofs/zdata.c
> index b59f2745a8e72..23f423c22ac8c 100644
> --- a/fs/erofs/zdata.c
> +++ b/fs/erofs/zdata.c
> @@ -1908,8 +1908,8 @@ static void z_erofs_readahead(struct readahead_control *rac)
>  	trace_erofs_readahead(realinode, readahead_index(rac), nrpages, false);
>  	z_erofs_pcluster_readmore(&f, rac, true);
>  
> -	/* traverse in reverse order for best metadata I/O performance */
> -	while ((folio = readahead_folio_reverse(rac))) {
> +	/* traverse from last to first for best metadata I/O performance */
> +	while ((folio = readahead_folio_last(rac))) {
>  		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 5ca5aa365f319..2cc3de5594518 100644
> --- a/include/linux/pagemap.h
> +++ b/include/linux/pagemap.h
> @@ -1510,13 +1510,21 @@ 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,
> +		bool read_from_head)
> +{
> +	if (read_from_head)
> +		rac->_index += rac->_batch_count;
> +
> +	rac->_nr_pages -= rac->_batch_count;
> +}

Maybe we can add:

	rac->_batch_count = 0;

as well since after the advance the _batch_count isn't valid anymore? We
can then also remove it from the callers.

> +
>  static inline struct folio *__readahead_folio(struct readahead_control *ractl)
>  {
> -	struct folio *folio;
> +	struct folio *folio = NULL;

Not sure why this initialization got here...

>  
>  	BUG_ON(ractl->_batch_count > ractl->_nr_pages);
> -	ractl->_nr_pages -= ractl->_batch_count;
> -	ractl->_index += ractl->_batch_count;
> +	__readahead_advance(ractl, /* read_from_head= */ true);
				   ^^^^
This is not really a kernel style :), please delete this comment. I'm ok with
pure false/true here - it is an internal helper used in few places. If
things get wider use, we tend to switch to 'unsigned flags' with explicit
flag names to ease code reading. But that's not the case here.

> @@ -1548,7 +1556,7 @@ static inline struct folio *readahead_folio(struct readahead_control *ractl)
>  }
>  
>  /**
> - * readahead_folio_reverse - Get the next folio to read, from the tail.
> + * readahead_folio_last - Get the next folio to read, from the tail.
>   * @ractl: The current readahead request.
>   *
>   * Like readahead_folio(), but walks the range back-to-front. The folio is
> @@ -1558,21 +1566,24 @@ static inline struct folio *readahead_folio(struct readahead_control *ractl)
>   * Context: The folio is locked.
>   * Return: A pointer to the next folio, or %NULL when done.
>   */
> -static inline struct folio *readahead_folio_reverse(struct readahead_control *ractl)
> +static inline struct folio *readahead_folio_last(struct readahead_control *ractl)
>  {
>  	struct folio *folio;
>  
> -	if (!ractl->_nr_pages)
> +	/* Shrink the window from the tail down to this folio's head index */
> +	__readahead_advance(ractl, /* read_from_head= */ false);
> +
> +	if (!ractl->_nr_pages) {
> +		ractl->_batch_count = 0;
>  		return NULL;
> +	}
>  
>  	/* xa_load() follows sibling entries, so a tail index returns the head */
>  	folio = xa_load(&ractl->mapping->i_pages,
>  			ractl->_index + ractl->_nr_pages - 1);
>  	VM_WARN_ON_ONCE_FOLIO(!folio_test_locked(folio), folio);
>  
> -	/* Shrink the window from the tail down to this folio's head index */
> -	ractl->_nr_pages = folio->index - ractl->_index;
> -	ractl->_batch_count = 0;
> +	ractl->_batch_count = folio_nr_pages(folio);
>  
>  	folio_put(folio);
>  	return folio;
> @@ -1583,11 +1594,10 @@ static inline unsigned int __readahead_batch(struct readahead_control *rac,
>  {
>  	unsigned int i = 0;
>  	XA_STATE(xas, &rac->mapping->i_pages, 0);
> -	struct folio *folio;
> +	struct folio *folio = NULL;

Again not sure why this initialization got here...

>  	BUG_ON(rac->_batch_count > rac->_nr_pages);
> -	rac->_nr_pages -= rac->_batch_count;
> -	rac->_index += rac->_batch_count;
> +	__readahead_advance(rac, /* read_from_head= */ true);
>  	rac->_batch_count = 0;
>  
>  	xas_set(&xas, rac->_index);
> 
> 
> 
> 
> -- 
> Best Regards,
> Yan, Zi
> 
-- 
Jan Kara <[email protected]>
SUSE Labs, CR