Re: [PATCH v1] fs/dax: fix folio splitting issue by resetting old folio order + _nr_pages

[Alistair Popple <apopple@nvidia.com>]

On Fri, Apr 11, 2025 at 10:37:17AM +0200, David Hildenbrand wrote:
> (adding CC list again, because I assume it was dropped by accident)

Whoops. Thanks.

> > > diff --git a/fs/dax.c b/fs/dax.c
> > > index af5045b0f476e..676303419e9e8 100644
> > > --- a/fs/dax.c
> > > +++ b/fs/dax.c
> > > @@ -396,6 +396,7 @@ static inline unsigned long dax_folio_put(struct folio *folio)
> > >   	order = folio_order(folio);
> > >   	if (!order)
> > >   		return 0;
> > > +	folio_reset_order(folio);
> > 
> > Wouldn't it be better to also move the loop below into this function? The intent
> > of this loop was to reinitialise the small folios after splitting which is what
> > I think this helper should be doing.
> 
> As the function does nothing on small folios (as documented), I think this
> is good enough for now.
> 
> Once we decouple folio from page, this code will likely have to change
> either way ...
> 
> The first large folio will become a small folio (so resetting kind-of makes
> sense), but the other small folios would have to allocate a new "struct
> folio" for small folios.
> 
> > 
> > >   	for (i = 0; i < (1UL << order); i++) {
> > >   		struct dev_pagemap *pgmap = page_pgmap(&folio->page);
> > > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > > index b7f13f087954b..bf55206935c46 100644
> > > --- a/include/linux/mm.h
> > > +++ b/include/linux/mm.h
> > > @@ -1218,6 +1218,23 @@ static inline unsigned int folio_order(const struct folio *folio)
> > >   	return folio_large_order(folio);
> > >   }
> > > +/**
> > > + * folio_reset_order - Reset the folio order and derived _nr_pages
> > > + * @folio: The folio.
> > > + *
> > > + * Reset the order and derived _nr_pages to 0. Must only be used in the
> > > + * process of splitting large folios.
> > > + */
> > > +static inline void folio_reset_order(struct folio *folio)
> > > +{
> > > +	if (WARN_ON_ONCE(!folio_test_large(folio)))
> > > +		return;
> > > +	folio->_flags_1 &= ~0xffUL;
> > > +#ifdef NR_PAGES_IN_LARGE_FOLIO
> > > +	folio->_nr_pages = 0;
> > > +#endif
> > > +}
> > > +
> 
> 
> I'm still not sure if this splitting code in fs/dax.c is more similar to THP
> splitting or to "splitting when freeing in the buddy". I think it's
> something in between: we want small folios, but the new folios are
> essentially free.

I'm not too familiar with the code for "splitting when freeing in the buddy"
but conceptually that sounds pretty similar to what we're doing here. The large
folio (and all pages within it) are free and we need to split it back to small
free folios ready to be allocated again in dax_folio_init().

> Likely, to be future-proof, we should also look into doing
> 
> folio->_flags_1 &= ~PAGE_FLAGS_SECOND;
> 
> Or alternatively (better?)
> 
> new_folio->flags &= ~PAGE_FLAGS_CHECK_AT_PREP;

That seems reasonable.

> ... but that problem will go away once we decouple page from folio (see
> above), so I'm not sure if we should really do that at this point unless
> there is an issue.
> 
> -- 
> Cheers,
> 
> David / dhildenb
>