From: John Groves <John@Groves.net>
Both fs/dax.c:dax_folio_put() and drivers/dax/fsdev.c:
fsdev_clear_folio_state() (the latter coming in the next commit after this
one) contain nearly identical code to reset a compound DAX folio back to
order-0 pages. Factor this out into a shared helper function.
The new dax_folio_reset_order() function:
- Clears the folio's mapping and share count
- Resets compound folio state via folio_reset_order()
- Clears PageHead and compound_head for each sub-page
- Restores the pgmap pointer for each resulting order-0 folio
- Returns the original folio order (for callers that need to advance by
that many pages)
This simplifies fsdev_clear_folio_state() from ~50 lines to ~15 lines while
maintaining the same functionality in both call sites.
Suggested-by: Jonathan Cameron <jonathan.cameron@huawei.com>
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
Signed-off-by: John Groves <john@groves.net>
---
fs/dax.c | 60 +++++++++++++++++++++++++++++++++++++++-----------------
1 file changed, 42 insertions(+), 18 deletions(-)
diff --git a/fs/dax.c b/fs/dax.c
index 289e6254aa30..7d7bbfb32c41 100644
--- a/fs/dax.c
+++ b/fs/dax.c
@@ -378,6 +378,45 @@ static void dax_folio_make_shared(struct folio *folio)
folio->share = 1;
}
+/**
+ * dax_folio_reset_order - Reset a compound DAX folio to order-0 pages
+ * @folio: The folio to reset
+ *
+ * Splits a compound folio back into individual order-0 pages,
+ * clearing compound state and restoring pgmap pointers.
+ *
+ * Returns: the original folio order (0 if already order-0)
+ */
+int dax_folio_reset_order(struct folio *folio)
+{
+ struct dev_pagemap *pgmap = page_pgmap(&folio->page);
+ int order = folio_order(folio);
+ int i;
+
+ folio->mapping = NULL;
+ folio->share = 0;
+
+ if (!order) {
+ folio->pgmap = pgmap;
+ return 0;
+ }
+
+ folio_reset_order(folio);
+
+ for (i = 0; i < (1UL << order); i++) {
+ struct page *page = folio_page(folio, i);
+ struct folio *f = (struct folio *)page;
+
+ ClearPageHead(page);
+ clear_compound_head(page);
+ f->mapping = NULL;
+ f->share = 0;
+ f->pgmap = pgmap;
+ }
+
+ return order;
+}
+
static inline unsigned long dax_folio_put(struct folio *folio)
{
unsigned long ref;
@@ -391,28 +430,13 @@ static inline unsigned long dax_folio_put(struct folio *folio)
if (ref)
return ref;
- folio->mapping = NULL;
- order = folio_order(folio);
- if (!order)
- return 0;
- folio_reset_order(folio);
+ order = dax_folio_reset_order(folio);
+ /* Debug check: verify refcounts are zero for all sub-folios */
for (i = 0; i < (1UL << order); i++) {
- struct dev_pagemap *pgmap = page_pgmap(&folio->page);
struct page *page = folio_page(folio, i);
- struct folio *new_folio = (struct folio *)page;
- ClearPageHead(page);
- clear_compound_head(page);
-
- new_folio->mapping = NULL;
- /*
- * Reset pgmap which was over-written by
- * prep_compound_page().
- */
- new_folio->pgmap = pgmap;
- new_folio->share = 0;
- WARN_ON_ONCE(folio_ref_count(new_folio));
+ WARN_ON_ONCE(folio_ref_count((struct folio *)page));
}
return ref;
--
2.53.0On Wed, 18 Mar 2026 20:28:20 -0500
John Groves <john@groves.net> wrote:
> From: John Groves <John@Groves.net>
>
> Both fs/dax.c:dax_folio_put() and drivers/dax/fsdev.c:
> fsdev_clear_folio_state() (the latter coming in the next commit after this
> one) contain nearly identical code to reset a compound DAX folio back to
> order-0 pages. Factor this out into a shared helper function.
>
> The new dax_folio_reset_order() function:
> - Clears the folio's mapping and share count
> - Resets compound folio state via folio_reset_order()
> - Clears PageHead and compound_head for each sub-page
> - Restores the pgmap pointer for each resulting order-0 folio
> - Returns the original folio order (for callers that need to advance by
> that many pages)
>
> This simplifies fsdev_clear_folio_state() from ~50 lines to ~15 lines while
> maintaining the same functionality in both call sites.
>
> Suggested-by: Jonathan Cameron <jonathan.cameron@huawei.com>
> Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> Signed-off-by: John Groves <john@groves.net>
Comment below. I may well be needing more coffee, or failing wrt
to background knowledge as I only occasionally dip into dax.
> ---
> fs/dax.c | 60 +++++++++++++++++++++++++++++++++++++++-----------------
> 1 file changed, 42 insertions(+), 18 deletions(-)
>
> diff --git a/fs/dax.c b/fs/dax.c
> index 289e6254aa30..7d7bbfb32c41 100644
> --- a/fs/dax.c
> +++ b/fs/dax.c
> @@ -378,6 +378,45 @@ static void dax_folio_make_shared(struct folio *folio)
> folio->share = 1;
> }
>
> +/**
> + * dax_folio_reset_order - Reset a compound DAX folio to order-0 pages
> + * @folio: The folio to reset
> + *
> + * Splits a compound folio back into individual order-0 pages,
> + * clearing compound state and restoring pgmap pointers.
> + *
> + * Returns: the original folio order (0 if already order-0)
> + */
> +int dax_folio_reset_order(struct folio *folio)
> +{
> + struct dev_pagemap *pgmap = page_pgmap(&folio->page);
> + int order = folio_order(folio);
> + int i;
> +
> + folio->mapping = NULL;
> + folio->share = 0;
This is different from the code you are replacing..
Just above the call to this in dax_folio_put()
if (!dax_folio_is_shared(folio))
// in here is the interesting bit...
ref = 0;
else
//this is fine because either it's still > 0 and we return
//or it is zero and you are writing that again.
ref = --folio->share;
if (ref)
return ref;
So the path that bothers me is if
!dax_folio_is_shared() can return false with shared != 0
/*
* A DAX folio is considered shared if it has no mapping set and ->share (which
* shares the ->index field) is non-zero. Note this may return false even if the
* page is shared between multiple files but has not yet actually been mapped
* into multiple address spaces.
*/
static inline bool dax_folio_is_shared(struct folio *folio)
{
return !folio->mapping && folio->share;
}
So it can if !folio->mapping is false (i.e. folio->mapping is set)
Now I have zero idea of whether this is a real path and have
a long review queue so not looking into it for now.
However if it's not then I'd expect some commentary in the patch description
to say why it's not a problem. Maybe even a precursor patch adding
the folio->share so there is a place to state clearly that it doesn't
matter and why.
> +
> + if (!order) {
> + folio->pgmap = pgmap;
This is also different...
> + return 0;
> + }
> +
> + folio_reset_order(folio);
> +
> + for (i = 0; i < (1UL << order); i++) {
I'd take advantage of evolving conventions and do
for (int i = 0; i < ...)
> + struct page *page = folio_page(folio, i);
> + struct folio *f = (struct folio *)page;
> +
> + ClearPageHead(page);
> + clear_compound_head(page);
> + f->mapping = NULL;
> + f->share = 0;
> + f->pgmap = pgmap;
> + }
> +
> + return order;
> +}
> +
> static inline unsigned long dax_folio_put(struct folio *folio)
> {
> unsigned long ref;
> @@ -391,28 +430,13 @@ static inline unsigned long dax_folio_put(struct folio *folio)
> if (ref)
> return ref;
>
> - folio->mapping = NULL;
> - order = folio_order(folio);
> - if (!order)
> - return 0;
> - folio_reset_order(folio);
> + order = dax_folio_reset_order(folio);
>
> + /* Debug check: verify refcounts are zero for all sub-folios */
> for (i = 0; i < (1UL << order); i++) {
> - struct dev_pagemap *pgmap = page_pgmap(&folio->page);
> struct page *page = folio_page(folio, i);
> - struct folio *new_folio = (struct folio *)page;
>
> - ClearPageHead(page);
> - clear_compound_head(page);
> -
> - new_folio->mapping = NULL;
> - /*
> - * Reset pgmap which was over-written by
> - * prep_compound_page().
> - */
> - new_folio->pgmap = pgmap;
> - new_folio->share = 0;
> - WARN_ON_ONCE(folio_ref_count(new_folio));
> + WARN_ON_ONCE(folio_ref_count((struct folio *)page));
> }
>
> return ref;
On 26/03/19 11:30AM, Jonathan Cameron wrote:
> On Wed, 18 Mar 2026 20:28:20 -0500
> John Groves <john@groves.net> wrote:
>
> > From: John Groves <John@Groves.net>
> >
> > Both fs/dax.c:dax_folio_put() and drivers/dax/fsdev.c:
> > fsdev_clear_folio_state() (the latter coming in the next commit after this
> > one) contain nearly identical code to reset a compound DAX folio back to
> > order-0 pages. Factor this out into a shared helper function.
> >
> > The new dax_folio_reset_order() function:
> > - Clears the folio's mapping and share count
> > - Resets compound folio state via folio_reset_order()
> > - Clears PageHead and compound_head for each sub-page
> > - Restores the pgmap pointer for each resulting order-0 folio
> > - Returns the original folio order (for callers that need to advance by
> > that many pages)
> >
> > This simplifies fsdev_clear_folio_state() from ~50 lines to ~15 lines while
> > maintaining the same functionality in both call sites.
> >
> > Suggested-by: Jonathan Cameron <jonathan.cameron@huawei.com>
> > Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> > Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> > Signed-off-by: John Groves <john@groves.net>
>
> Comment below. I may well be needing more coffee, or failing wrt
> to background knowledge as I only occasionally dip into dax.
thanks!
>
>
> > ---
> > fs/dax.c | 60 +++++++++++++++++++++++++++++++++++++++-----------------
> > 1 file changed, 42 insertions(+), 18 deletions(-)
> >
> > diff --git a/fs/dax.c b/fs/dax.c
> > index 289e6254aa30..7d7bbfb32c41 100644
> > --- a/fs/dax.c
> > +++ b/fs/dax.c
> > @@ -378,6 +378,45 @@ static void dax_folio_make_shared(struct folio *folio)
> > folio->share = 1;
> > }
> >
> > +/**
> > + * dax_folio_reset_order - Reset a compound DAX folio to order-0 pages
> > + * @folio: The folio to reset
> > + *
> > + * Splits a compound folio back into individual order-0 pages,
> > + * clearing compound state and restoring pgmap pointers.
> > + *
> > + * Returns: the original folio order (0 if already order-0)
> > + */
> > +int dax_folio_reset_order(struct folio *folio)
> > +{
> > + struct dev_pagemap *pgmap = page_pgmap(&folio->page);
> > + int order = folio_order(folio);
> > + int i;
> > +
> > + folio->mapping = NULL;
> > + folio->share = 0;
>
> This is different from the code you are replacing..
>
> Just above the call to this in dax_folio_put()
>
> if (!dax_folio_is_shared(folio))
> // in here is the interesting bit...
> ref = 0;
> else
> //this is fine because either it's still > 0 and we return
> //or it is zero and you are writing that again.
> ref = --folio->share;
> if (ref)
> return ref;
>
> So the path that bothers me is if
> !dax_folio_is_shared() can return false with shared != 0
>
> /*
> * A DAX folio is considered shared if it has no mapping set and ->share (which
> * shares the ->index field) is non-zero. Note this may return false even if the
> * page is shared between multiple files but has not yet actually been mapped
> * into multiple address spaces.
> */
> static inline bool dax_folio_is_shared(struct folio *folio)
> {
> return !folio->mapping && folio->share;
> }
>
> So it can if !folio->mapping is false (i.e. folio->mapping is set)
>
> Now I have zero idea of whether this is a real path and have
> a long review queue so not looking into it for now.
> However if it's not then I'd expect some commentary in the patch description
> to say why it's not a problem. Maybe even a precursor patch adding
> the folio->share so there is a place to state clearly that it doesn't
> matter and why.
I believe it is correct, and I'm adding a clarifying comment above as follows:
/*
* DAX maintains the invariant that folio->share != 0 only when
* folio->mapping == NULL (enforced by dax_folio_make_shared()).
* Equivalently: folio->mapping != NULL implies folio->share == 0.
* Callers ensure share has been decremented to zero before calling
* here, so unconditionally clearing both fields is correct.
*/
folio->mapping = NULL;
folio->share = 0;
...
>
> > +
> > + if (!order) {
> > + folio->pgmap = pgmap;
> This is also different...
Here too, I think it is correct, and I'm adding a comment as follows:
if (!order) {
/*
* Restore pgmap explicitly even for order-0 folios. For the
* dax_folio_put() caller this is a no-op (same value), but
* fsdev_clear_folio_state() may call this on folios that were
* previously compound and need pgmap re-established.
*/
folio->pgmap = pgmap;
return 0;
}
...but if I'm missing anything I hope somebody will point it out!
>
> > + return 0;
> > + }
> > +
> > + folio_reset_order(folio);
> > +
> > + for (i = 0; i < (1UL << order); i++) {
>
> I'd take advantage of evolving conventions and do
>
> for (int i = 0; i < ...)
Done, thanks!
John
<snip>
© 2016 - 2026 Red Hat, Inc.