[PATCH RFC 00/14] Remove PG_private by using page/folio->private checks instead

[PATCH RFC 00/14] Remove PG_private by using page/folio->private checks instead

[Zi Yan]

Hi all,

This patchset removes PG_private to make space for upcoming PG_folio
(reserved as __PG_folio) for identifying pages from a folio (more details
in Note below). Instead of checking PG_private, all code is changed to
check page/folio->private != NULL instead.

MM people are cc'd on all patches and subsystem people are cc'd on the
cover letter and corresponding patches.

Overview
===
Most code uses folio_attach/detach/change_private() functions, so folio
refcount is increased and decreased when folio->private is set and reset,
respectively. There is no need to change them.

Changes are needed for exceptional users:
1. zsmalloc uses PG_private to indicate first component zpdesc page and
   page->private is used to store zspage in zpdesc. To remove PG_private,
   is_first_zpdesc() is changed to zpdesc->zspage->first_zpdesc == zpdesc
   instead of checking PG_private.

2. kernel/events/ring_buffer.c stores page order in page->private.
   Replacing PG_private with page->private != NULL works.

3. drivers/xen/grant-table.c stores xen_page_foreign in page->private,
   where on 32-bit, a pointer to xen_page_foreign is stored; on 64-bit,
   page->private is used as xen_page_foreign. PG_private check is replaced
   by page->private != NULL on 32-bit for xen_page_foreign deallocation.
   On 64-bit, page->private is cleared unconditionally since {domid=0,
   gref=0} (xen_page_foreign can be 0) is valid.

4. fs/crypto/crypto.c stores a folio pointer in page->private, PG_private
   checks are replaced by page->private != NULL.

5. fs/erofs has two different uses:

    5a. folio->private is used to form a reversed list of
    the outputs of readahead_folio(). readahead_folio_reverse() is added to
    output folios in reversed order, so that ->private is no longer needed.

    5b. folio->private is used as an in-flight I/O counter. Convert the
    code to use folio_attach/detach/get_private().

6. fs/nfs/write.c: folio refcount maintenance is in a bigger scope than
   folio->private. So folio_attach/detach/get_private() is not used.
   Nothing to change.

7. fs/f2fs uses attach_page_private() to first reset folio->private then
   immediately sets PAGE_PRIVATE_NOT_POINTER bit on it. Change it to use
   attach_page_private() to set PAGE_PRIVATE_NOT_POINTER bit directly to
   avoid folio->private == NULL gap inside set_page_private_##name().

8. hugetlb uses folio_change_private(folio, NULL) without folio refcount
   maintenance. Change it to folio->private = NULL.

After the above changes, PG_private ops are converted to
page/folio->private ops.

folio_test_fs_private() is added to check filesystem-only private data by
excluding swapcache and hugetlb folios, because swapcache folios overlap
swp_entry_t swap with ->private and hugetlb sets its own flags in
->private.

Note
===
1. KPF_PRIVATE has a minor semantic change. Since PG_private will be
   removed, KPF_PRIVATE represents pagecache folios whose ->mapping is not
   NULL and with ->private set. Currently KPF_PRIVATE can be set for
   orphaned pagecache folios with ->mapping == NULL.

2. Documentation/mm/hugetlbfs_reserv.rst is outdated, so I did not remove
   PG_private related text. It should be rewritten.

3. PG_folio is planned to be set on every page from a folio in
   page_rmappable_folio(), so folios with any order (currently
   PG_large_rmappable is used to identify >0 order folios) can be
   identified, vm_insert_*() can correctly reject all folios, and rmap code
   can accept only folios. Eventually, page_folio() will return NULL for
   non-folio pages.

Tests
===
1. allmodconfig build passed.

2. zsmalloc is tested using ext4 on a 1GB lz4 zram:
    2a. zram load + zsmalloc compaction;
    2b. concurrent zspage migration via memory compaction;
    2c. confirmed that multi-page zspages actually formed.

    Details: https://github.com/x-y-z/linux-dev/blob/b4/remove-pg_private/test_zsmalloc.md

3. erofs is tested on images created with -C4096 and lz4hc, lzma,
   deflate, and zstd algorithms:
   3a. cold read of all files, verify checksums match source;
   3b. readahead + reclaim/migration race.

   Details: https://github.com/x-y-z/linux-dev/blob/b4/remove-pg_private/test_erofs.md

4. fscrypt is tested on software-encrypted ext4 with writes to exercise
   bounce pages.

   Details: https://github.com/x-y-z/linux-dev/blob/b4/remove-pg_private/test_fscrypt.md

5. f2fs is tested on an image with inline_data,compress_algorithm=lz4:
    5a. INLINE_INODE — lots of tiny files;
    5b. REF_RESOURCE + general writeback — buffered write churn with fsync;
    5c. ONGOING_MIGRATION — force GC / page migration;
    5d. ATOMIC_WRITE — atomic-write ioctl path.

    Details: https://github.com/x-y-z/linux-dev/blob/b4/remove-pg_private/test_f2fs.md
    (I did not run xfstests)

6. MM selftests passed.

LLM use
===
Claude was used to form a concrete plan on what code needs to be changed
and how to change them. The plan was reviewed by Codex until no issue was
spotted.

Plan is at: https://github.com/x-y-z/linux-dev/blob/b4/remove-pg_private/plan.md

I then followed the plan to make code changes. I did bounce ideas with
Claude how to change fs/erofs, since I did not like the original idea.
After each change, I asked Claude to review my code and git commit message.
I also asked Claude to give me test plans (see above).

At last, Codex was used to review all patches.

Comments and suggestions are welcome. Thanks.

Assisted-by: Claude:claude-opus-4-8
Assisted-by: Codex:gpt-5
Signed-off-by: Zi Yan <ziy@nvidia.com>
---
Zi Yan (14):
      mm/zsmalloc: replace PG_private with pointer comparison
      perf/ring_buffer: stop using PG_private as AUX page high-order marker
      xen/grant-table: stop setting PG_private on pages for grant mapping
      fs/crypto: stop setting PG_private on bounce page
      mm/hugetlb: use direct assignment instead of folio_change_private()
      fs/f2fs: stop using PG_private
      fs/erofs: mm/pagemap: add readahead_folio_reverse() to avoid folio->private
      fs/erofs: use folio_attach/detach_private() instead of direct assignment
      mm/page-flags: check page/folio->private instead of PG_private
      mm/page-flags: introduce folio_test_fs_private()
      treewide: remove folio_set/clear_private()
      treewide: replace PagePrivate() with page_private()
      treewide: adjust comments on PagePrivate and PG_private
      mm/page-flags: remove PG_private

 Documentation/admin-guide/kdump/vmcoreinfo.rst |  2 +-
 Documentation/filesystems/vfs.rst              |  6 ++--
 arch/x86/events/intel/bts.c                    |  3 --
 arch/x86/events/intel/pt.c                     |  6 ++--
 drivers/md/md-bitmap.c                         |  6 ++--
 drivers/xen/balloon.c                          |  5 ++++
 drivers/xen/grant-table.c                      |  7 ++---
 fs/ceph/addr.c                                 |  8 ++----
 fs/crypto/crypto.c                             |  2 --
 fs/erofs/data.c                                |  5 ++--
 fs/erofs/zdata.c                               | 11 ++------
 fs/f2fs/f2fs.h                                 |  8 +++---
 fs/nfs/file.c                                  |  4 +--
 fs/nfs/write.c                                 |  2 --
 fs/proc/page.c                                 |  5 +++-
 fs/ubifs/file.c                                |  6 ++--
 include/linux/buffer_head.h                    |  6 ----
 include/linux/mm.h                             | 16 +++++------
 include/linux/mm_types.h                       |  4 +--
 include/linux/page-flags.h                     | 38 ++++++++++++++++++--------
 include/linux/pagemap.h                        | 35 ++++++++++++++++++++++--
 include/trace/events/mmflags.h                 |  2 +-
 include/trace/events/pagemap.h                 |  2 +-
 kernel/events/ring_buffer.c                    |  7 ++---
 kernel/vmcore_info.c                           |  1 -
 mm/huge_memory.c                               |  2 +-
 mm/hugetlb.c                                   |  6 ++--
 mm/migrate.c                                   |  3 +-
 mm/page-writeback.c                            |  5 +++-
 mm/vmscan.c                                    |  3 +-
 mm/zpdesc.h                                    |  2 +-
 mm/zsmalloc.c                                  | 15 ++--------
 32 files changed, 127 insertions(+), 106 deletions(-)
---
base-commit: bcd5eb68a6a189497eb26c1b9f622538aa48895d
change-id: 20260728-remove-pg_private-cfe926c7f83c

Best regards,
-- 
Yan, Zi

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

[Zi Yan]

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@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        | 11 ++---------
 include/linux/pagemap.h | 31 +++++++++++++++++++++++++++++++
 2 files changed, 33 insertions(+), 9 deletions(-)

diff --git a/fs/erofs/zdata.c b/fs/erofs/zdata.c
index 74520e9102596..b59f2745a8e72 100644
--- a/fs/erofs/zdata.c
+++ b/fs/erofs/zdata.c
@@ -1902,21 +1902,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);
-
+	while ((folio = readahead_folio_reverse(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 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;
+
+	folio_put(folio);
+	return folio;
+}
+
 static inline unsigned int __readahead_batch(struct readahead_control *rac,
 		struct page **array, unsigned int array_sz)
 {

-- 
2.53.0

[PATCH RFC 08/14] fs/erofs: use folio_attach/detach_private() instead of direct assignment

[Zi Yan]

erofs_onelinefolio_init/split/end() use folio->private without setting
PG_private or increase folio refcount and it works. But after PG_private is
replaced by checking folio->private in a future commit, it can break
folio_expected_ref_count(), since the folio has private data without
elevated refcount. Change it now.

It prepares for a future commit that removes PG_private.

No funtional 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>
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
---
 fs/erofs/data.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/erofs/data.c b/fs/erofs/data.c
index 9aa48c8d67d12..356665f025fb9 100644
--- a/fs/erofs/data.c
+++ b/fs/erofs/data.c
@@ -263,7 +263,8 @@ void erofs_onlinefolio_init(struct folio *folio)
 		void *v;
 	} u = { .o = ATOMIC_INIT(1) };
 
-	folio->private = u.v;	/* valid only if file-backed folio is locked */
+	/* valid only if file-backed folio is locked */
+	folio_attach_private(folio, u.v);
 }
 
 void erofs_onlinefolio_split(struct folio *folio)
@@ -284,7 +285,7 @@ void erofs_onlinefolio_end(struct folio *folio, int err, bool dirty)
 
 	if (v & (BIT(EROFS_ONLINEFOLIO_DIRTY) - 1))
 		return;
-	folio->private = 0;
+	folio_detach_private(folio);
 	if (v & BIT(EROFS_ONLINEFOLIO_DIRTY))
 		flush_dcache_folio(folio);
 	folio_end_read(folio, !(v & BIT(EROFS_ONLINEFOLIO_EIO)));

-- 
2.53.0

Re: [PATCH RFC 00/14] Remove PG_private by using page/folio->private checks instead

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 637 bytes --]

On 01.08.26 04:13, Zi Yan wrote:
> Hi all,
> 
> This patchset removes PG_private to make space for upcoming PG_folio
> (reserved as __PG_folio) for identifying pages from a folio (more details
> in Note below). Instead of checking PG_private, all code is changed to
> check page/folio->private != NULL instead.

I'm a little bit worried that page/folio->private is in a union, so today
it could (in theory) be != NULL while PG_private isn't set.

Is it really not possible to enter a path where PG_private is tested while
page/folio->private != NULL due to the union being used otherwise (PG_private
not set)?


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

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

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@suse.com>
SUSE Labs, CR

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

[Zi Yan]

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

Re: [PATCH RFC 00/14] Remove PG_private by using page/folio->private checks instead

[Zi Yan]

On Mon Aug 3, 2026 at 5:07 AM EDT, Jürgen Groß wrote:
> On 01.08.26 04:13, Zi Yan wrote:
>> Hi all,
>> 
>> This patchset removes PG_private to make space for upcoming PG_folio
>> (reserved as __PG_folio) for identifying pages from a folio (more details
>> in Note below). Instead of checking PG_private, all code is changed to
>> check page/folio->private != NULL instead.
>
> I'm a little bit worried that page/folio->private is in a union, so today
> it could (in theory) be != NULL while PG_private isn't set.
>
> Is it really not possible to enter a path where PG_private is tested while
> page/folio->private != NULL due to the union being used otherwise (PG_private
> not set)?

Yes, it is possible. See: #5 in the exceptional users: erofs uses
->private for reverse linked list and in-flight counters without setting
PG_private. I get rid of the first one and converted the second one to
use folio_attach/detach/get_private() to follow the general ->private
use pattern..

For non file system folios, anon swapcache puts swap_entry_t in
->private and hugetlb puts its flags in ->private. I added
folio_test_fs_private() to exclude them, but this helper is planned to
be used by core MM, since filesystem code should not encounter these
two.


-- 
Best Regards,
Yan, Zi

Re: [PATCH RFC 08/14] fs/erofs: use folio_attach/detach_private() instead of direct assignment

[Gao Xiang]

Hi Zi,

On Fri, Jul 31, 2026 at 10:13:31PM -0400, Zi Yan wrote:
> erofs_onelinefolio_init/split/end() use folio->private without setting
> PG_private or increase folio refcount and it works. But after PG_private is
> replaced by checking folio->private in a future commit, it can break
> folio_expected_ref_count(), since the folio has private data without
> elevated refcount. Change it now.
> 
> It prepares for a future commit that removes PG_private.
> 
> No funtional 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>
> 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

It looks fine as long as PG_private flag will be removed in the
follow-up patches:

Reviewed-by: Gao Xiang <xiang@kernel.org>

Thanks,
Gao Xiang

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

[Gao Xiang]

On Fri, Jul 31, 2026 at 10:13:30PM -0400, 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@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        | 11 ++---------
>  include/linux/pagemap.h | 31 +++++++++++++++++++++++++++++++
>  2 files changed, 33 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/erofs/zdata.c b/fs/erofs/zdata.c
> index 74520e9102596..b59f2745a8e72 100644
> --- a/fs/erofs/zdata.c
> +++ b/fs/erofs/zdata.c
> @@ -1902,21 +1902,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);
> -
> +	while ((folio = readahead_folio_reverse(rac))) {

Yes, it's needed due to EROFS compression metadata design and on-demand
partial decompression, the last extent in the readahead request can be
parsed as a partial extent (means from the starting logical offset of
extents to the necessary offset.).   Since there may be many extents
in a single readahead request, so it needs to iterate backwards here;
but the actual compressed data I/Os will be issued forwards.

Previously I tend to avoid touching core-mm so it uses folio->private
but if MM folks can provide a new helper, that would be very helpful
(one more words: all folios are locked in the forward order previously,
so it won't have any deadlock risk).

Thanks,
Gao Xiang

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

[Jan Kara]

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 <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
> >
> > 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 <jack@suse.com>
SUSE Labs, CR

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

[Zi Yan]

On Tue Aug 4, 2026 at 5:32 AM EDT, Jan Kara wrote:
> 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.

<snip>

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

Got it. I am trying to figure out your mental model of how the mix of
readahead_folio() and readahead_folio_last() works with the bool inside
ractl. By looking at readahead_folio_last() code, it is almost the same
as readahead_folio() with __readahead_folio() inlined
(__readahead_folio() is only used by readahead_folio(), so the inline
can happen without any issue). As a result, we can get rid of
readahead_folio_last(), add set_readahead_direction() to set the
embedded bool read_from_head, and use readahead_folio() only. This
removes redundant code in readahead_folio_last(). One thing I am not
certain is whether we want to

1. use set_readahead_direction() explicit and warn readahead_folio() if
read_from_head is not initialized, or

2. set read_from_head to true by default, so that only erofs needs to
call set_readahead_direction() to change read_from_head.

The former is less confusing but changes how readahead_folio() works;
the latter is simpler but implicit read_from_head state might confuse
people at some point.

Let me know your thoughts. Thanks.

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

Yes, for __readahead_batch(), _batch_count is zeroed right after. For
__readahead_folio() and readahead_folio_last(), _batch_count is either
zeroed or overwritten by folio_nr_pages() before any use.

>
>> +
>>  static inline struct folio *__readahead_folio(struct readahead_control *ractl)
>>  {
>> -	struct folio *folio;
>> +	struct folio *folio = NULL;
>
> Not sure why this initialization got here...

Will remove it. It is some leftover during my development. Thank you for
pointing it out.

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

We did this in some MM code. I will delete the comment like you
suggested.

<snip>

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

Will remove.

-- 
Best Regards,
Yan, Zi

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

[Jan Kara]

On Tue 04-08-26 11:54:41, Zi Yan wrote:
> On Tue Aug 4, 2026 at 5:32 AM EDT, Jan Kara wrote:
> > 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.
> 
> <snip>
> 
> >> 
> >> 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.
> 
> Got it. I am trying to figure out your mental model of how the mix of
> readahead_folio() and readahead_folio_last() works with the bool inside
> ractl. By looking at readahead_folio_last() code, it is almost the same
> as readahead_folio() with __readahead_folio() inlined
> (__readahead_folio() is only used by readahead_folio(), so the inline
> can happen without any issue). As a result, we can get rid of
> readahead_folio_last(), add set_readahead_direction() to set the
> embedded bool read_from_head, and use readahead_folio() only. This
> removes redundant code in readahead_folio_last(). One thing I am not
> certain is whether we want to
> 
> 1. use set_readahead_direction() explicit and warn readahead_folio() if
> read_from_head is not initialized, or
> 
> 2. set read_from_head to true by default, so that only erofs needs to
> call set_readahead_direction() to change read_from_head.
> 
> The former is less confusing but changes how readahead_folio() works;
> the latter is simpler but implicit read_from_head state might confuse
> people at some point.

My idea was: readahead_folio() will call __readahead_advance() and then set
rac->forward = true. readahead_folio_last() will call __readahead_advance()
and set rac->forward = false. __readahead_advance() advances from beginning
/ end based on rac->_forward value.

								Honza


> > 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.
> 
> Yes, for __readahead_batch(), _batch_count is zeroed right after. For
> __readahead_folio() and readahead_folio_last(), _batch_count is either
> zeroed or overwritten by folio_nr_pages() before any use.
> 
> >
> >> +
> >>  static inline struct folio *__readahead_folio(struct readahead_control *ractl)
> >>  {
> >> -	struct folio *folio;
> >> +	struct folio *folio = NULL;
> >
> > Not sure why this initialization got here...
> 
> Will remove it. It is some leftover during my development. Thank you for
> pointing it out.
> 
> >
> >>  
> >>  	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.
> 
> We did this in some MM code. I will delete the comment like you
> suggested.
> 
> <snip>
> 
> >> @@ -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...
> 
> Will remove.
> 
> -- 
> Best Regards,
> Yan, Zi
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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

[Zi Yan]

On Tue Aug 4, 2026 at 1:04 PM EDT, Jan Kara wrote:
> On Tue 04-08-26 11:54:41, Zi Yan wrote:
>> On Tue Aug 4, 2026 at 5:32 AM EDT, Jan Kara wrote:
>> > 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.
>> 
>> <snip>
>> 
>> >> 
>> >> 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.
>> 
>> Got it. I am trying to figure out your mental model of how the mix of
>> readahead_folio() and readahead_folio_last() works with the bool inside
>> ractl. By looking at readahead_folio_last() code, it is almost the same
>> as readahead_folio() with __readahead_folio() inlined
>> (__readahead_folio() is only used by readahead_folio(), so the inline
>> can happen without any issue). As a result, we can get rid of
>> readahead_folio_last(), add set_readahead_direction() to set the
>> embedded bool read_from_head, and use readahead_folio() only. This
>> removes redundant code in readahead_folio_last(). One thing I am not
>> certain is whether we want to
>> 
>> 1. use set_readahead_direction() explicit and warn readahead_folio() if
>> read_from_head is not initialized, or
>> 
>> 2. set read_from_head to true by default, so that only erofs needs to
>> call set_readahead_direction() to change read_from_head.
>> 
>> The former is less confusing but changes how readahead_folio() works;
>> the latter is simpler but implicit read_from_head state might confuse
>> people at some point.
>
> My idea was: readahead_folio() will call __readahead_advance() and then set
> rac->forward = true. readahead_folio_last() will call __readahead_advance()
> and set rac->forward = false. __readahead_advance() advances from beginning
> / end based on rac->_forward value.

Got it. I can do that. Just to be clear, it should be that
readahead_folio() first sets rac->forward = true, then calls
__readahead_advance(), since __readahead_advance() advances based on
rac->forward, right? readahead_folio_last() as well.

-- 
Best Regards,
Yan, Zi

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

[Zi Yan]

On Tue Aug 4, 2026 at 1:09 PM EDT, Zi Yan wrote:
> On Tue Aug 4, 2026 at 1:04 PM EDT, Jan Kara wrote:
>> On Tue 04-08-26 11:54:41, Zi Yan wrote:
>>> On Tue Aug 4, 2026 at 5:32 AM EDT, Jan Kara wrote:
>>> > 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.
>>> 
>>> <snip>
>>> 
>>> >> 
>>> >> 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.
>>> 
>>> Got it. I am trying to figure out your mental model of how the mix of
>>> readahead_folio() and readahead_folio_last() works with the bool inside
>>> ractl. By looking at readahead_folio_last() code, it is almost the same
>>> as readahead_folio() with __readahead_folio() inlined
>>> (__readahead_folio() is only used by readahead_folio(), so the inline
>>> can happen without any issue). As a result, we can get rid of
>>> readahead_folio_last(), add set_readahead_direction() to set the
>>> embedded bool read_from_head, and use readahead_folio() only. This
>>> removes redundant code in readahead_folio_last(). One thing I am not
>>> certain is whether we want to
>>> 
>>> 1. use set_readahead_direction() explicit and warn readahead_folio() if
>>> read_from_head is not initialized, or
>>> 
>>> 2. set read_from_head to true by default, so that only erofs needs to
>>> call set_readahead_direction() to change read_from_head.
>>> 
>>> The former is less confusing but changes how readahead_folio() works;
>>> the latter is simpler but implicit read_from_head state might confuse
>>> people at some point.
>>
>> My idea was: readahead_folio() will call __readahead_advance() and then set
>> rac->forward = true. readahead_folio_last() will call __readahead_advance()
>> and set rac->forward = false. __readahead_advance() advances from beginning
>> / end based on rac->_forward value.
>
> Got it. I can do that. Just to be clear, it should be that
> readahead_folio() first sets rac->forward = true, then calls
> __readahead_advance(), since __readahead_advance() advances based on
> rac->forward, right? readahead_folio_last() as well.

This is revised patch:


diff --git a/fs/erofs/zdata.c b/fs/erofs/zdata.c
index 74520e9102596..23f423c22ac8c 100644
--- a/fs/erofs/zdata.c
+++ b/fs/erofs/zdata.c
@@ -1902,21 +1902,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))) {
 		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 4e8b2b29f6d3e..cc69d60b2a9d2 100644
--- a/include/linux/pagemap.h
+++ b/include/linux/pagemap.h
@@ -1448,6 +1448,7 @@ struct readahead_control {
 	bool dropbehind;
 	bool _workingset;
 	unsigned long _pflags;
+	bool forward;
 };
 
 #define DEFINE_READAHEAD(ractl, f, r, m, i)				\
@@ -1512,18 +1513,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;
+
+	rac->_nr_pages -= rac->_batch_count;
+	rac->_batch_count = 0;
+}
+
 static inline struct folio *__readahead_folio(struct readahead_control *ractl)
 {
 	struct folio *folio;
 
 	BUG_ON(ractl->_batch_count > ractl->_nr_pages);
-	ractl->_nr_pages -= ractl->_batch_count;
-	ractl->_index += ractl->_batch_count;
+	ractl->forward = true;
+	__readahead_advance(ractl);
 
-	if (!ractl->_nr_pages) {
-		ractl->_batch_count = 0;
+	if (!ractl->_nr_pages)
 		return NULL;
-	}
 
 	folio = xa_load(&ractl->mapping->i_pages, ractl->_index);
 	VM_BUG_ON_FOLIO(!folio_test_locked(folio), folio);
@@ -1549,6 +1557,39 @@ static inline struct folio *readahead_folio(struct readahead_control *ractl)
 	return folio;
 }
 
+/**
+ * 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
+ * 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_last(struct readahead_control *ractl)
+{
+	struct folio *folio;
+
+	/* Shrink the window from the tail down to this folio's head index */
+	ractl->forward = false;
+	__readahead_advance(ractl);
+
+	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);
+
+	ractl->_batch_count = folio_nr_pages(folio);
+
+	folio_put(folio);
+	return folio;
+}
+
 static inline unsigned int __readahead_batch(struct readahead_control *rac,
 		struct page **array, unsigned int array_sz)
 {
@@ -1557,9 +1598,8 @@ static inline unsigned int __readahead_batch(struct readahead_control *rac,
 	struct folio *folio;
 
 	BUG_ON(rac->_batch_count > rac->_nr_pages);
-	rac->_nr_pages -= rac->_batch_count;
-	rac->_index += rac->_batch_count;
-	rac->_batch_count = 0;
+	rac->forward = true;
+	__readahead_advance(rac);
 
 	xas_set(&xas, rac->_index);
 	rcu_read_lock();


-- 
Best Regards,
Yan, Zi

Re: [PATCH RFC 08/14] fs/erofs: use folio_attach/detach_private() instead of direct assignment

[Zi Yan]

On Mon Aug 3, 2026 at 7:40 PM EDT, Gao Xiang wrote:
> Hi Zi,
>
> On Fri, Jul 31, 2026 at 10:13:31PM -0400, Zi Yan wrote:
>> erofs_onelinefolio_init/split/end() use folio->private without setting
>> PG_private or increase folio refcount and it works. But after PG_private is
>> replaced by checking folio->private in a future commit, it can break
>> folio_expected_ref_count(), since the folio has private data without
>> elevated refcount. Change it now.
>> 
>> It prepares for a future commit that removes PG_private.
>> 
>> No funtional 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>
>> 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
>
> It looks fine as long as PG_private flag will be removed in the
> follow-up patches:
>

Hi Gao,

Sashiko spot an issue in this patch[1]. Basically, ->private can be 0 if
I/O completes without any issue or being dirty and it causes
folio_detach_private() not to folio_put(). My fix is to add a bias, 1,
to the counter, so that ->private stays non NULL throughout online folio
process. The revised patch is below. Let me know your thoughts. Thanks.

[1] https://sashiko.dev/#/patchset/20260731-remove-pg_private-v1-0-142c97ba3562%40nvidia.com?part=8

From d8fadd13fee03e72c03a718af65ed41927ccbcca Mon Sep 17 00:00:00 2001
From: Zi Yan <ziy@nvidia.com>
Date: Thu, 30 Jul 2026 11:04:00 -0400
Subject: [PATCH] erofs: use folio_attach/detach_private() instead of direct
 assignment

erofs_onlinefolio_init/split/end() use folio->private without setting
PG_private or increasing folio refcount and it works. But after PG_private
is replaced by checking folio->private in a future commit, it can break
folio_expected_ref_count(), since the folio has private data without
elevated refcount. Change them to use folio_attach/detach_private().

Furthermore, because folio->private is used to store in-flight I/O counter
and the counter reaches 0 when all I/O completes successfully without error
or being dirty, ->private=0 causes folio_detach_private() to not drop the
elevated folio refcount. Solve this issue by using bias=1 for the counter,
so that ->private stays non NULL throughout every attach-to-detach process.
Add a macro EROFS_ONLINEFOLIO_BIAS=1. While at it, fix the comment about
->private bit layout and add EROFS_ONLINEFOLIO_COUNT_MASK.

It prepares for a future commit that removes PG_private.

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>
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
---
 fs/erofs/data.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/fs/erofs/data.c b/fs/erofs/data.c
index 9aa48c8d67d12..81e9dab247e0f 100644
--- a/fs/erofs/data.c
+++ b/fs/erofs/data.c
@@ -251,19 +251,23 @@ int erofs_map_dev(struct super_block *sb, struct erofs_map_dev *map)
 /*
  * bit 30: I/O error occurred on this folio
  * bit 29: CPU has dirty data in D-cache (needs aliasing handling);
- * bit 0 - 29: remaining parts to complete this folio
+ * bit 0 - 28: remaining parts to complete this folio, biased by 1 so that
+ *	       ->private stays non-NULL while the folio is attached
  */
 #define EROFS_ONLINEFOLIO_EIO		30
 #define EROFS_ONLINEFOLIO_DIRTY		29
+#define EROFS_ONLINEFOLIO_COUNT_MASK	(BIT(EROFS_ONLINEFOLIO_DIRTY) - 1)
+#define EROFS_ONLINEFOLIO_BIAS		1
 
 void erofs_onlinefolio_init(struct folio *folio)
 {
 	union {
 		atomic_t o;
 		void *v;
-	} u = { .o = ATOMIC_INIT(1) };
+	} u = { .o = ATOMIC_INIT(1 + EROFS_ONLINEFOLIO_BIAS) };
 
-	folio->private = u.v;	/* valid only if file-backed folio is locked */
+	/* valid only if file-backed folio is locked */
+	folio_attach_private(folio, u.v);
 }
 
 void erofs_onlinefolio_split(struct folio *folio)
@@ -277,14 +281,14 @@ void erofs_onlinefolio_end(struct folio *folio, int err, bool dirty)
 
 	do {
 		orig = atomic_read((atomic_t *)&folio->private);
-		DBG_BUGON(orig <= 0);
+		DBG_BUGON((orig & EROFS_ONLINEFOLIO_COUNT_MASK) <= EROFS_ONLINEFOLIO_BIAS);
 		v = dirty << EROFS_ONLINEFOLIO_DIRTY;
 		v |= (orig - 1) | (!!err << EROFS_ONLINEFOLIO_EIO);
 	} while (atomic_cmpxchg((atomic_t *)&folio->private, orig, v) != orig);
 
-	if (v & (BIT(EROFS_ONLINEFOLIO_DIRTY) - 1))
+	if ((v & EROFS_ONLINEFOLIO_COUNT_MASK) != EROFS_ONLINEFOLIO_BIAS)
 		return;
-	folio->private = 0;
+	folio_detach_private(folio);
 	if (v & BIT(EROFS_ONLINEFOLIO_DIRTY))
 		flush_dcache_folio(folio);
 	folio_end_read(folio, !(v & BIT(EROFS_ONLINEFOLIO_EIO)));
-- 
2.53.0




-- 
Best Regards,
Yan, Zi

Re: [PATCH RFC 08/14] fs/erofs: use folio_attach/detach_private() instead of direct assignment

[Gao Xiang]

Hi Zi,

On Tue, Aug 04, 2026 at 10:41:23PM -0400, Zi Yan wrote:
> On Mon Aug 3, 2026 at 7:40 PM EDT, Gao Xiang wrote:
> > Hi Zi,
> >
> > On Fri, Jul 31, 2026 at 10:13:31PM -0400, Zi Yan wrote:
> >> erofs_onelinefolio_init/split/end() use folio->private without setting
> >> PG_private or increase folio refcount and it works. But after PG_private is
> >> replaced by checking folio->private in a future commit, it can break
> >> folio_expected_ref_count(), since the folio has private data without
> >> elevated refcount. Change it now.
> >> 
> >> It prepares for a future commit that removes PG_private.
> >> 
> >> No funtional 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>
> >> 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
> >
> > It looks fine as long as PG_private flag will be removed in the
> > follow-up patches:
> >
> 
> Hi Gao,
> 
> Sashiko spot an issue in this patch[1]. Basically, ->private can be 0 if
> I/O completes without any issue or being dirty and it causes
> folio_detach_private() not to folio_put(). My fix is to add a bias, 1,
> to the counter, so that ->private stays non NULL throughout online folio
> process. The revised patch is below. Let me know your thoughts. Thanks.
> 

Yes, that is a valid issue: ->private can be decreased to 0 without
folio_detach_private() for a short period so a +1 bias is indeed
a solution.

The following diff looks good to me, you could use it in your next
version.

Thanks,
Gao Xiang

> [1] https://sashiko.dev/#/patchset/20260731-remove-pg_private-v1-0-142c97ba3562%40nvidia.com?part=8
> 
> >From d8fadd13fee03e72c03a718af65ed41927ccbcca Mon Sep 17 00:00:00 2001
> From: Zi Yan <ziy@nvidia.com>
> Date: Thu, 30 Jul 2026 11:04:00 -0400
> Subject: [PATCH] erofs: use folio_attach/detach_private() instead of direct
>  assignment
> 
> erofs_onlinefolio_init/split/end() use folio->private without setting
> PG_private or increasing folio refcount and it works. But after PG_private
> is replaced by checking folio->private in a future commit, it can break
> folio_expected_ref_count(), since the folio has private data without
> elevated refcount. Change them to use folio_attach/detach_private().
> 
> Furthermore, because folio->private is used to store in-flight I/O counter
> and the counter reaches 0 when all I/O completes successfully without error
> or being dirty, ->private=0 causes folio_detach_private() to not drop the
> elevated folio refcount. Solve this issue by using bias=1 for the counter,
> so that ->private stays non NULL throughout every attach-to-detach process.
> Add a macro EROFS_ONLINEFOLIO_BIAS=1. While at it, fix the comment about
> ->private bit layout and add EROFS_ONLINEFOLIO_COUNT_MASK.
> 
> It prepares for a future commit that removes PG_private.
> 
> 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>
> 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
> ---
>  fs/erofs/data.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/erofs/data.c b/fs/erofs/data.c
> index 9aa48c8d67d12..81e9dab247e0f 100644
> --- a/fs/erofs/data.c
> +++ b/fs/erofs/data.c
> @@ -251,19 +251,23 @@ int erofs_map_dev(struct super_block *sb, struct erofs_map_dev *map)
>  /*
>   * bit 30: I/O error occurred on this folio
>   * bit 29: CPU has dirty data in D-cache (needs aliasing handling);
> - * bit 0 - 29: remaining parts to complete this folio
> + * bit 0 - 28: remaining parts to complete this folio, biased by 1 so that
> + *	       ->private stays non-NULL while the folio is attached
>   */
>  #define EROFS_ONLINEFOLIO_EIO		30
>  #define EROFS_ONLINEFOLIO_DIRTY		29
> +#define EROFS_ONLINEFOLIO_COUNT_MASK	(BIT(EROFS_ONLINEFOLIO_DIRTY) - 1)
> +#define EROFS_ONLINEFOLIO_BIAS		1
>  
>  void erofs_onlinefolio_init(struct folio *folio)
>  {
>  	union {
>  		atomic_t o;
>  		void *v;
> -	} u = { .o = ATOMIC_INIT(1) };
> +	} u = { .o = ATOMIC_INIT(1 + EROFS_ONLINEFOLIO_BIAS) };
>  
> -	folio->private = u.v;	/* valid only if file-backed folio is locked */
> +	/* valid only if file-backed folio is locked */
> +	folio_attach_private(folio, u.v);
>  }
>  
>  void erofs_onlinefolio_split(struct folio *folio)
> @@ -277,14 +281,14 @@ void erofs_onlinefolio_end(struct folio *folio, int err, bool dirty)
>  
>  	do {
>  		orig = atomic_read((atomic_t *)&folio->private);
> -		DBG_BUGON(orig <= 0);
> +		DBG_BUGON((orig & EROFS_ONLINEFOLIO_COUNT_MASK) <= EROFS_ONLINEFOLIO_BIAS);
>  		v = dirty << EROFS_ONLINEFOLIO_DIRTY;
>  		v |= (orig - 1) | (!!err << EROFS_ONLINEFOLIO_EIO);
>  	} while (atomic_cmpxchg((atomic_t *)&folio->private, orig, v) != orig);
>  
> -	if (v & (BIT(EROFS_ONLINEFOLIO_DIRTY) - 1))
> +	if ((v & EROFS_ONLINEFOLIO_COUNT_MASK) != EROFS_ONLINEFOLIO_BIAS)
>  		return;
> -	folio->private = 0;
> +	folio_detach_private(folio);
>  	if (v & BIT(EROFS_ONLINEFOLIO_DIRTY))
>  		flush_dcache_folio(folio);
>  	folio_end_read(folio, !(v & BIT(EROFS_ONLINEFOLIO_EIO)));
> -- 
> 2.53.0
> 
> 
> 
> 
> -- 
> Best Regards,
> Yan, Zi
> 

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

[Jan Kara]

On Tue 04-08-26 13:09:46, Zi Yan wrote:
> On Tue Aug 4, 2026 at 1:04 PM EDT, Jan Kara wrote:
> > On Tue 04-08-26 11:54:41, Zi Yan wrote:
> >> On Tue Aug 4, 2026 at 5:32 AM EDT, Jan Kara wrote:
> >> > 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.
> >> 
> >> <snip>
> >> 
> >> >> 
> >> >> 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.
> >> 
> >> Got it. I am trying to figure out your mental model of how the mix of
> >> readahead_folio() and readahead_folio_last() works with the bool inside
> >> ractl. By looking at readahead_folio_last() code, it is almost the same
> >> as readahead_folio() with __readahead_folio() inlined
> >> (__readahead_folio() is only used by readahead_folio(), so the inline
> >> can happen without any issue). As a result, we can get rid of
> >> readahead_folio_last(), add set_readahead_direction() to set the
> >> embedded bool read_from_head, and use readahead_folio() only. This
> >> removes redundant code in readahead_folio_last(). One thing I am not
> >> certain is whether we want to
> >> 
> >> 1. use set_readahead_direction() explicit and warn readahead_folio() if
> >> read_from_head is not initialized, or
> >> 
> >> 2. set read_from_head to true by default, so that only erofs needs to
> >> call set_readahead_direction() to change read_from_head.
> >> 
> >> The former is less confusing but changes how readahead_folio() works;
> >> the latter is simpler but implicit read_from_head state might confuse
> >> people at some point.
> >
> > My idea was: readahead_folio() will call __readahead_advance() and then set
> > rac->forward = true. readahead_folio_last() will call __readahead_advance()
> > and set rac->forward = false. __readahead_advance() advances from beginning
> > / end based on rac->_forward value.
> 
> Got it. I can do that. Just to be clear, it should be that
> readahead_folio() first sets rac->forward = true, then calls
> __readahead_advance(), since __readahead_advance() advances based on
> rac->forward, right? readahead_folio_last() as well.

No. I wrote "and then set" which means after and that is what I really
wanted to say. You still don't seem to be understanding the logic of handling
the _batch_count. _batch_count is the length of the returned batch.
__readahead_advance() updates _index and _nr_pages to remove the folios
returned in the last batch from the range. So _forward needs to contain
whether the last returned batch was taken from the beginning or the end of
the range and __readahead_advance() uses it to update current range
accordingly (before we go and return the next batch). We cannot clobber
_forward before calling __readahead_advance(). I hope things are clearer
now.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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

[Zi Yan]

On Wed Aug 5, 2026 at 5:25 AM EDT, Jan Kara wrote:
> On Tue 04-08-26 13:09:46, Zi Yan wrote:
>> On Tue Aug 4, 2026 at 1:04 PM EDT, Jan Kara wrote:
>> > On Tue 04-08-26 11:54:41, Zi Yan wrote:
>> >> On Tue Aug 4, 2026 at 5:32 AM EDT, Jan Kara wrote:
>> >> > 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.
>> >> 
>> >> <snip>
>> >> 
>> >> >> 
>> >> >> 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.
>> >> 
>> >> Got it. I am trying to figure out your mental model of how the mix of
>> >> readahead_folio() and readahead_folio_last() works with the bool inside
>> >> ractl. By looking at readahead_folio_last() code, it is almost the same
>> >> as readahead_folio() with __readahead_folio() inlined
>> >> (__readahead_folio() is only used by readahead_folio(), so the inline
>> >> can happen without any issue). As a result, we can get rid of
>> >> readahead_folio_last(), add set_readahead_direction() to set the
>> >> embedded bool read_from_head, and use readahead_folio() only. This
>> >> removes redundant code in readahead_folio_last(). One thing I am not
>> >> certain is whether we want to
>> >> 
>> >> 1. use set_readahead_direction() explicit and warn readahead_folio() if
>> >> read_from_head is not initialized, or
>> >> 
>> >> 2. set read_from_head to true by default, so that only erofs needs to
>> >> call set_readahead_direction() to change read_from_head.
>> >> 
>> >> The former is less confusing but changes how readahead_folio() works;
>> >> the latter is simpler but implicit read_from_head state might confuse
>> >> people at some point.
>> >
>> > My idea was: readahead_folio() will call __readahead_advance() and then set
>> > rac->forward = true. readahead_folio_last() will call __readahead_advance()
>> > and set rac->forward = false. __readahead_advance() advances from beginning
>> > / end based on rac->_forward value.
>> 
>> Got it. I can do that. Just to be clear, it should be that
>> readahead_folio() first sets rac->forward = true, then calls
>> __readahead_advance(), since __readahead_advance() advances based on
>> rac->forward, right? readahead_folio_last() as well.
>
> No. I wrote "and then set" which means after and that is what I really
> wanted to say. You still don't seem to be understanding the logic of handling
> the _batch_count. _batch_count is the length of the returned batch.
> __readahead_advance() updates _index and _nr_pages to remove the folios
> returned in the last batch from the range. So _forward needs to contain
> whether the last returned batch was taken from the beginning or the end of
> the range and __readahead_advance() uses it to update current range
> accordingly (before we go and return the next batch). We cannot clobber
> _forward before calling __readahead_advance(). I hope things are clearer
> now.

Got it. Sorry I made some assumption instead of asking my question, so I
misinterpret your words. My question is who sets the initial value of
_forward? So that __readahead_advance() can update _index and _nr_pages
correctly at the first time __readahead_folio() is called?

__readahead_folio() does:

1. update _nr_pages and _index,
2. return NULL if _nr_pages is 0 and set _batch_count to 0,
3. return folio using xa_load and set _batch_count to folio_nr_pages().

after the change:

1. call __readahead_advance() to update _index, _nr_pages, and
_batch_count based on _forward,
2. update _forward to true, since it is __readahead_folio()
3. return NULL or folio based on _nr_pages.

Then the first time __readahead_folio() is called, who sets _forward to
make 1 work correctly?

Thanks.


-- 
Best Regards,
Yan, Zi

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

[Zi Yan]

On 5 Aug 2026, at 7:42, Zi Yan wrote:

> On Wed Aug 5, 2026 at 5:25 AM EDT, Jan Kara wrote:
>> On Tue 04-08-26 13:09:46, Zi Yan wrote:
>>> On Tue Aug 4, 2026 at 1:04 PM EDT, Jan Kara wrote:
>>>> On Tue 04-08-26 11:54:41, Zi Yan wrote:
>>>>> On Tue Aug 4, 2026 at 5:32 AM EDT, Jan Kara wrote:
>>>>>> 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.
>>>>>
>>>>> <snip>
>>>>>
>>>>>>>
>>>>>>> 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.
>>>>>
>>>>> Got it. I am trying to figure out your mental model of how the mix of
>>>>> readahead_folio() and readahead_folio_last() works with the bool inside
>>>>> ractl. By looking at readahead_folio_last() code, it is almost the same
>>>>> as readahead_folio() with __readahead_folio() inlined
>>>>> (__readahead_folio() is only used by readahead_folio(), so the inline
>>>>> can happen without any issue). As a result, we can get rid of
>>>>> readahead_folio_last(), add set_readahead_direction() to set the
>>>>> embedded bool read_from_head, and use readahead_folio() only. This
>>>>> removes redundant code in readahead_folio_last(). One thing I am not
>>>>> certain is whether we want to
>>>>>
>>>>> 1. use set_readahead_direction() explicit and warn readahead_folio() if
>>>>> read_from_head is not initialized, or
>>>>>
>>>>> 2. set read_from_head to true by default, so that only erofs needs to
>>>>> call set_readahead_direction() to change read_from_head.
>>>>>
>>>>> The former is less confusing but changes how readahead_folio() works;
>>>>> the latter is simpler but implicit read_from_head state might confuse
>>>>> people at some point.
>>>>
>>>> My idea was: readahead_folio() will call __readahead_advance() and then set
>>>> rac->forward = true. readahead_folio_last() will call __readahead_advance()
>>>> and set rac->forward = false. __readahead_advance() advances from beginning
>>>> / end based on rac->_forward value.
>>>
>>> Got it. I can do that. Just to be clear, it should be that
>>> readahead_folio() first sets rac->forward = true, then calls
>>> __readahead_advance(), since __readahead_advance() advances based on
>>> rac->forward, right? readahead_folio_last() as well.
>>
>> No. I wrote "and then set" which means after and that is what I really
>> wanted to say. You still don't seem to be understanding the logic of handling
>> the _batch_count. _batch_count is the length of the returned batch.
>> __readahead_advance() updates _index and _nr_pages to remove the folios
>> returned in the last batch from the range. So _forward needs to contain
>> whether the last returned batch was taken from the beginning or the end of
>> the range and __readahead_advance() uses it to update current range
>> accordingly (before we go and return the next batch). We cannot clobber
>> _forward before calling __readahead_advance(). I hope things are clearer
>> now.
>
> Got it. Sorry I made some assumption instead of asking my question, so I
> misinterpret your words. My question is who sets the initial value of
> _forward? So that __readahead_advance() can update _index and _nr_pages
> correctly at the first time __readahead_folio() is called?
>
> __readahead_folio() does:
>
> 1. update _nr_pages and _index,
> 2. return NULL if _nr_pages is 0 and set _batch_count to 0,
> 3. return folio using xa_load and set _batch_count to folio_nr_pages().
>
> after the change:
>
> 1. call __readahead_advance() to update _index, _nr_pages, and
> _batch_count based on _forward,
> 2. update _forward to true, since it is __readahead_folio()
> 3. return NULL or folio based on _nr_pages.
>
> Then the first time __readahead_folio() is called, who sets _forward to
> make 1 work correctly?

Never mind. Codex answered this:

The first-call initialization is not a problem: DEFINE_READAHEAD()
zero-initializes omitted fields, and _batch_count starts as zero,
so the first advance is a no-op.

I will fix my patch. Thanks.

Best Regards,
Yan, Zi

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

[Jan Kara]

On Wed 05-08-26 07:42:37, Zi Yan wrote:
> On Wed Aug 5, 2026 at 5:25 AM EDT, Jan Kara wrote:
> > On Tue 04-08-26 13:09:46, Zi Yan wrote:
> >> On Tue Aug 4, 2026 at 1:04 PM EDT, Jan Kara wrote:
> >> > On Tue 04-08-26 11:54:41, Zi Yan wrote:
> >> >> On Tue Aug 4, 2026 at 5:32 AM EDT, Jan Kara wrote:
> >> >> > 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.
> >> >> 
> >> >> <snip>
> >> >> 
> >> >> >> 
> >> >> >> 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.
> >> >> 
> >> >> Got it. I am trying to figure out your mental model of how the mix of
> >> >> readahead_folio() and readahead_folio_last() works with the bool inside
> >> >> ractl. By looking at readahead_folio_last() code, it is almost the same
> >> >> as readahead_folio() with __readahead_folio() inlined
> >> >> (__readahead_folio() is only used by readahead_folio(), so the inline
> >> >> can happen without any issue). As a result, we can get rid of
> >> >> readahead_folio_last(), add set_readahead_direction() to set the
> >> >> embedded bool read_from_head, and use readahead_folio() only. This
> >> >> removes redundant code in readahead_folio_last(). One thing I am not
> >> >> certain is whether we want to
> >> >> 
> >> >> 1. use set_readahead_direction() explicit and warn readahead_folio() if
> >> >> read_from_head is not initialized, or
> >> >> 
> >> >> 2. set read_from_head to true by default, so that only erofs needs to
> >> >> call set_readahead_direction() to change read_from_head.
> >> >> 
> >> >> The former is less confusing but changes how readahead_folio() works;
> >> >> the latter is simpler but implicit read_from_head state might confuse
> >> >> people at some point.
> >> >
> >> > My idea was: readahead_folio() will call __readahead_advance() and then set
> >> > rac->forward = true. readahead_folio_last() will call __readahead_advance()
> >> > and set rac->forward = false. __readahead_advance() advances from beginning
> >> > / end based on rac->_forward value.
> >> 
> >> Got it. I can do that. Just to be clear, it should be that
> >> readahead_folio() first sets rac->forward = true, then calls
> >> __readahead_advance(), since __readahead_advance() advances based on
> >> rac->forward, right? readahead_folio_last() as well.
> >
> > No. I wrote "and then set" which means after and that is what I really
> > wanted to say. You still don't seem to be understanding the logic of handling
> > the _batch_count. _batch_count is the length of the returned batch.
> > __readahead_advance() updates _index and _nr_pages to remove the folios
> > returned in the last batch from the range. So _forward needs to contain
> > whether the last returned batch was taken from the beginning or the end of
> > the range and __readahead_advance() uses it to update current range
> > accordingly (before we go and return the next batch). We cannot clobber
> > _forward before calling __readahead_advance(). I hope things are clearer
> > now.
> 
> Got it. Sorry I made some assumption instead of asking my question, so I
> misinterpret your words. My question is who sets the initial value of
> _forward? So that __readahead_advance() can update _index and _nr_pages
> correctly at the first time __readahead_folio() is called?

This first time __readahead_folio() is called _batch_count is 0 so the
value of _forward doesn't really matter... But DEFINE_READAHEAD() will
implicitly initialize it to 'false' which is fine.

> __readahead_folio() does:
> 
> 1. update _nr_pages and _index,
> 2. return NULL if _nr_pages is 0 and set _batch_count to 0,
> 3. return folio using xa_load and set _batch_count to folio_nr_pages().
> 
> after the change:
> 
> 1. call __readahead_advance() to update _index, _nr_pages, and
> _batch_count based on _forward,
> 2. update _forward to true, since it is __readahead_folio()
> 3. return NULL or folio based on _nr_pages.
> 
> Then the first time __readahead_folio() is called, who sets _forward to
> make 1 work correctly?

See above...

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR