Re: [PATCH v1] fs/dax: fix folio splitting issue by resetting old folio order + _nr_pages

[David Hildenbrand <david@redhat.com>]

On 10.04.25 23:54, Matthew Wilcox wrote:
> On Thu, Apr 10, 2025 at 01:46:06PM -0700, Dan Williams wrote:
>> Matthew Wilcox wrote:
>>> On Thu, Apr 10, 2025 at 01:15:07PM -0700, Dan Williams wrote:
>>>> For consistency and clarity what about this incremental change, to make
>>>> the __split_folio_to_order() path reuse folio_reset_order(), and use
>>>> typical bitfield helpers for manipulating _flags_1?
>>>
>>> I dislike this intensely.  It obfuscates rather than providing clarity.
>>
>> I'm used to pushing folks to use bitfield.h in driver land, but will not
>> push it further here.
> 
> I think it can make sense in places.  Just not here.
> 
>> What about this hunk?
>>
>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>> index 2a47682d1ab7..301ca9459122 100644
>> --- a/mm/huge_memory.c
>> +++ b/mm/huge_memory.c
>> @@ -3404,7 +3404,7 @@ static void __split_folio_to_order(struct folio *folio, int old_order,
>>   	if (new_order)
>>   		folio_set_order(folio, new_order);
>>   	else
>> -		ClearPageCompound(&folio->page);
>> +		folio_reset_order(folio);
>>   }
> 
> I think that's wrong.  We're splitting this folio into order-0 folios,
> but folio_reset_order() is going to modify folio->_flags_1 which is in
> the next page.

Right, clearing in this context might only make sense at the very start.

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 2a47682d1ab77..4cd8b394b83a5 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -3312,6 +3312,8 @@ static void __split_folio_to_order(struct folio *folio, int old_order,
         long nr_pages = 1 << old_order;
         long i;
  
+       folio_reset_order(folio);
+
         /*
          * Skip the first new_nr_pages, since the new folio from them have all
          * the flags from the original folio.


While it looks cleaner, it's in practice not required here, because

1) We handle _nr_pages overlay by setting new_folio->memcg_data

	new_folio->memcg_data = folio->memcg_data;

2) We handle the order by setting new_folio->flags

	new_folio->flags &= ~PAGE_FLAGS_CHECK_AT_PREP;

That should clear all flags (excluding hwpoison), including the order.


It's worth noting that in free_pages_prepare(), we handle both using

if (compound) {
	page[1].flags &= ~PAGE_FLAGS_SECOND;
#ifdef NR_PAGES_IN_LARGE_FOLIO
	folio->_nr_pages = 0;
#endif


-- 
Cheers,

David / dhildenb