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

[Jan Kara]

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 <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

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.

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).

								Honza
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR