On 26/02/23 07:00PM, Ackerley Tng wrote:
> John Groves <[email protected]> writes:
>
> > From: John Groves <[email protected]>
> >
> > 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 <[email protected]>
> > Signed-off-by: John Groves <[email protected]>
> > ---
> > 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]
