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

[David Hildenbrand]

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