Re: [PATCH v3 06/14] f2fs: stop using PG_private

[Tal Zussman]

On 2026-09-08 17:47 +0200, David Hildenbrand (Arm) wrote:
> On 9/8/26 04:56, Zi Yan wrote:
> > f2fs sets its PAGE_PRIVATE_* flags in page->private and checking
> > page->private != NULL is equivalent to checking PG_private. Change
> > PagePrivate() to page_private(). Meanwhile, in set_page_private_##name(),
> > page->private is first set to 0/NULL before an PAGE_PRIVATE_* flag is set,
> > but it can cause confusion when PG_private is removed and
> > page->private != NULL is used instead. Change it to initialize
> > page->private to PAGE_PRIVATE_NOT_POINTER instead and retain the original
> > semantics.
> > 
> > It prepares for a future commit that removes PG_private.
> > 
> > No functional change intended.
> > 
> > Assisted-by: Claude:claude-opus-4-8
> > Assisted-by: Codex:gpt-5
> > To: Jaegeuk Kim <jaegeuk@xxxxxxxxxx>
> > To: Chao Yu <chao@xxxxxxxxxx>
> > Cc: linux-f2fs-devel@xxxxxxxxxxxxxxxxxxxxx
> > Cc: linux-kernel@xxxxxxxxxxxxxxx
> > Acked-by: Usama Arif <usama.arif@xxxxxxxxx>
> > Acked-by: Chao Yu <chao@xxxxxxxxxx>
> > Signed-off-by: Zi Yan <ziy@xxxxxxxxxx>
> > ---
> >  fs/f2fs/f2fs.h | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > index 9940a6cecf1a2..2f7ab5888b078 100644
> > --- a/fs/f2fs/f2fs.h
> > +++ b/fs/f2fs/f2fs.h
> > @@ -2691,7 +2691,7 @@ static inline bool folio_test_f2fs_##name(const struct folio *folio)	\
> >  }									\
> >  static inline bool page_private_##name(struct page *page) \
> >  { \
> > -	return PagePrivate(page) && \
> > +	return page_private(page) && \
> >  		test_bit(PAGE_PRIVATE_NOT_POINTER, &page_private(page)) && \
> >  		test_bit(PAGE_PRIVATE_##flagname, &page_private(page)); \
> >  }
> > @@ -2710,9 +2710,9 @@ static inline void folio_set_f2fs_##name(struct folio *folio)		\
> >  }									\
> >  static inline void set_page_private_##name(struct page *page) \
> >  { \
> > -	if (!PagePrivate(page)) \
> > -		attach_page_private(page, (void *)0); \
> > -	set_bit(PAGE_PRIVATE_NOT_POINTER, &page_private(page)); \
> > +	if (!page_private(page)) \
> > +		attach_page_private(page, \
> > +				(void *)BIT(PAGE_PRIVATE_NOT_POINTER)); \
> >  	set_bit(PAGE_PRIVATE_##flagname, &page_private(page)); \
> >  }
> 
> Very weird interface. Why do we even need the page-based interface still?
> 
> $ git grep -E "(set|clear)_page_private"
> compress.c:             clear_page_private_gcing(cc->rpages[i]);
> compress.c:                     set_page_private_gcing(cc->rpages[i]);
> compress.c:             clear_page_private_gcing(cic->rpages[i]);
> f2fs.h:static inline void set_page_private_##name(struct page *page) \
> f2fs.h:static inline void clear_page_private_##name(struct page *page) \
> 
> Seeing code like:
> 
> clear_page_private_gcing(cc->rpages[i]);
> if (folio_test_writeback(page_folio(cc->rpages[i])))
> 	end_page_writeback(cc->rpages[i]);
> 
> Makes me wonder whether we can just use the folio helper instead?
> 
> In f2fs_iget(), we enable large folios only when !f2fs_compressed_file(inode).
> 
> So naive me would assume that we can just get rid of the
> set_page_private_/clear_page_private_ stuff entirely.
>

I have a WIP series of ~30 patches converting much of the remaining page
users in f2fs (including the below) to folios. Still have to do some
testing and clean it up, but hoping to send it out in the next couple of
weeks (in the hopes of eliminating some more folio_compat.c functions by
next cycle...)

> IOW something like:
> 
> 
> From efcde918178604de11d16c6dc5485542ab4dcdaf Mon Sep 17 00:00:00 2001
> From: "David Hildenbrand (Arm)" <david@xxxxxxxxxx>
> Date: Tue, 8 Sep 2026 17:46:06 +0200
> Subject: [PATCH] tmp
> 
> Signed-off-by: David Hildenbrand (Arm) <david@xxxxxxxxxx>
> ---
>  fs/f2fs/compress.c | 29 ++++++++++++++++++-----------
>  fs/f2fs/f2fs.h     | 13 -------------
>  2 files changed, 18 insertions(+), 24 deletions(-)
> 
> diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c
> index ce88092d9ce26..0e9cc0fa297a4 100644
> --- a/fs/f2fs/compress.c
> +++ b/fs/f2fs/compress.c
> @@ -1064,13 +1064,15 @@ static void cancel_cluster_writeback(struct compress_ctx *cc,
>  
>  	/* Cancel writeback and stay locked. */
>  	for (i = 0; i < cc->cluster_size; i++) {
> +		struct folio *folio = page_folio(cc->rpages[i]);
> +
>  		if (i < submitted) {
>  			inode_inc_dirty_pages(cc->inode);
> -			lock_page(cc->rpages[i]);
> +			folio_lock(folio);
>  		}
> -		clear_page_private_gcing(cc->rpages[i]);
> -		if (folio_test_writeback(page_folio(cc->rpages[i])))
> -			end_page_writeback(cc->rpages[i]);
> +		folio_clear_f2fs_gcing(folio);
> +		if (folio_test_writeback(folio))
> +			folio_end_writeback(folio);
>  	}
>  }
>  
> @@ -1078,11 +1080,15 @@ static void set_cluster_dirty(struct compress_ctx *cc)
>  {
>  	int i;
>  
> -	for (i = 0; i < cc->cluster_size; i++)
> -		if (cc->rpages[i]) {
> -			set_page_dirty(cc->rpages[i]);
> -			set_page_private_gcing(cc->rpages[i]);
> -		}
> +	for (i = 0; i < cc->cluster_size; i++) {
> +		struct folio *folio;
> +
> +		if (!cc->rpages[i])
> +			continue;
> +		folio = page_folio(cc->rpages[i]);
> +		folio_mark_dirty(folio);
> +		folio_set_f2fs_gcing(folio);
> +	}
>  }
>  
>  static int prepare_compress_overwrite(struct compress_ctx *cc,
> @@ -1477,8 +1483,9 @@ void f2fs_compress_write_end_io(struct bio *bio, struct folio *folio)
>  
>  	for (i = 0; i < cic->nr_rpages; i++) {
>  		WARN_ON(!cic->rpages[i]);
> -		clear_page_private_gcing(cic->rpages[i]);
> -		end_page_writeback(cic->rpages[i]);
> +		folio = page_folio(cic->rpages[i]);
> +		folio_clear_f2fs_gcing(folio);
> +		folio_end_writeback(folio);
>  	}
>  
>  	page_array_free(sbi, cic->rpages, cic->nr_rpages);
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 9940a6cecf1a2..0cfba8742d4b4 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -2707,13 +2707,6 @@ static inline void folio_set_f2fs_##name(struct folio *folio)		\
>  		v |= (unsigned long)folio->private;			\
>  		folio->private = (void *)v;				\
>  	}								\
> -}									\
> -static inline void set_page_private_##name(struct page *page) \
> -{ \
> -	if (!PagePrivate(page)) \
> -		attach_page_private(page, (void *)0); \
> -	set_bit(PAGE_PRIVATE_NOT_POINTER, &page_private(page)); \
> -	set_bit(PAGE_PRIVATE_##flagname, &page_private(page)); \
>  }
>  
>  #define PAGE_PRIVATE_CLEAR_FUNC(name, flagname) \
> @@ -2726,12 +2719,6 @@ static inline void folio_clear_f2fs_##name(struct folio *folio)		\
>  		folio_detach_private(folio);				\
>  	else								\
>  		folio->private = (void *)v;				\
> -}									\
> -static inline void clear_page_private_##name(struct page *page) \
> -{ \
> -	clear_bit(PAGE_PRIVATE_##flagname, &page_private(page)); \
> -	if (page_private(page) == BIT(PAGE_PRIVATE_NOT_POINTER)) \
> -		detach_page_private(page); \
>  }
>  
>  PAGE_PRIVATE_GET_FUNC(nonpointer, NOT_POINTER);
> -- 
> 2.43.0
> 
> 
> -- 
> Cheers,
> 
> David
>