Messages in this thread Patch in this message |  | | Date | Fri, 11 Apr 2025 10:28:43 +0200 | | Subject | Re: [PATCH v1] fs/dax: fix folio splitting issue by resetting old folio order + _nr_pages | | From | 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
|  |