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

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.


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;
+}
+
 static inline struct folio *__readahead_folio(struct readahead_control *ractl)
 {
-	struct folio *folio;
+	struct folio *folio = NULL;
 
 	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);
 
 	if (!ractl->_nr_pages) {
 		ractl->_batch_count = 0;
@@ -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;
 
 	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