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
