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

[David Hildenbrand (Arm)]

On 9/10/26 04:41, Chao Yu wrote:
> On 9/8/26 23:47, 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.
> 
> David,
> 
> Thanks for the patch, at a glance, it seems fine, can you please send
> a formal patch? then we can apply to dev-test for test.

I'm hoping Zi will drag that along as part of this patch set. Is this fine or do
you want it sent as a complete standalone patch (will make merging Zi's series
slightly more complicated :) ).

-- 
Cheers,

David