Re: [PATCH V7 02/19] dax: Factor out dax_folio_reset_order() helper

[John Groves <John@groves.net>]

On 26/02/23 07:00PM, Ackerley Tng wrote:
> John Groves <john@jagalactic.com> writes:
> 
> > 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>
> > 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;
> > +}
> > +
> 
> I'm implementing something similar for guest_memfd and was going to
> reuse __split_folio_to_order(). Would you consider using the
> __split_folio_to_order() function?
> 
> I see that dax_folio_reset_order() needs to set f->share to 0 though,
> which is a union with index, and __split_folio_to_order() sets non-0
> indices.
> 
> Also, __split_folio_to_order() doesn't handle f->pgmap (or f->lru).
> 
> Could these two steps be added to a separate loop after
> __split_folio_to_order()?
> 
> Does dax_folio_reset_order() need to handle any of the folio flags that
> __split_folio_to_order() handles?

Sorry to reply slowly; this took some thought.

I'm nervous about sharing folio initialization code between the page cache
and dax. Might this be something we could unify after the fact - if it
passes muster? 

Unifying paths like this could be regression-prone (page cache changes
breaking dax or vice versa) unless it's really well conceived...

> 
> >  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().
> > -		 */
> 
> Actually, where's the call to prep_compound_page()? Was that in
> dax_folio_init()? Is this comment still valid and does pgmap have to be
> reset?

Yep, in dax_folio_init()...


Thanks,
John

[snip]