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

[John Groves <john@groves.net>]

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>