Re: [PATCH V8 2/8] dax: Factor out dax_folio_reset_order() helper

[Jonathan Cameron <jonathan.cameron@huawei.com>]

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.


> ---
>  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;