Re: [PATCH v6 08/26] fs/dax: Remove PAGE_MAPPING_DAX_SHARED mapping flag

[Alistair Popple <apopple@nvidia.com>]

On Mon, Jan 13, 2025 at 04:52:34PM -0800, Dan Williams wrote:
> Alistair Popple wrote:
> > PAGE_MAPPING_DAX_SHARED is the same as PAGE_MAPPING_ANON. 
> 
> I think a bit a bit more detail is warranted, how about?
> 
> The page ->mapping pointer can have magic values like
> PAGE_MAPPING_DAX_SHARED and PAGE_MAPPING_ANON for page owner specific
> usage. In fact, PAGE_MAPPING_DAX_SHARED and PAGE_MAPPING_ANON alias the
> same value.

Massaged it slightly but sounds good.

> > This isn't currently a problem because FS DAX pages are treated
> > specially.
> 
> s/are treated specially/are never seen by the anonymous mapping code and
> vice versa/
> 
> > However a future change will make FS DAX pages more like
> > normal pages, so folio_test_anon() must not return true for a FS DAX
> > page.
> > 
> > We could explicitly test for a FS DAX page in folio_test_anon(),
> > etc. however the PAGE_MAPPING_DAX_SHARED flag isn't actually
> > needed. Instead we can use the page->mapping field to implicitly track
> > the first mapping of a page. If page->mapping is non-NULL it implies
> > the page is associated with a single mapping at page->index. If the
> > page is associated with a second mapping clear page->mapping and set
> > page->share to 1.
> > 
> > This is possible because a shared mapping implies the file-system
> > implements dax_holder_operations which makes the ->mapping and
> > ->index, which is a union with ->share, unused.
> > 
> > The page is considered shared when page->mapping == NULL and
> > page->share > 0 or page->mapping != NULL, implying it is present in at
> > least one address space. This also makes it easier for a future change
> > to detect when a page is first mapped into an address space which
> > requires special handling.
> > 
> > Signed-off-by: Alistair Popple <apopple@nvidia.com>
> > ---
> >  fs/dax.c                   | 45 +++++++++++++++++++++++++--------------
> >  include/linux/page-flags.h |  6 +-----
> >  2 files changed, 29 insertions(+), 22 deletions(-)
> > 
> > diff --git a/fs/dax.c b/fs/dax.c
> > index 4e49cc4..d35dbe1 100644
> > --- a/fs/dax.c
> > +++ b/fs/dax.c
> > @@ -351,38 +351,41 @@ static unsigned long dax_end_pfn(void *entry)
> >  	for (pfn = dax_to_pfn(entry); \
> >  			pfn < dax_end_pfn(entry); pfn++)
> >  
> > +/*
> > + * A DAX page 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_page_is_shared(struct page *page)
> >  {
> > -	return page->mapping == PAGE_MAPPING_DAX_SHARED;
> > +	return !page->mapping && page->share;
> >  }
> >  
> >  /*
> > - * Set the page->mapping with PAGE_MAPPING_DAX_SHARED flag, increase the
> > - * refcount.
> > + * Increase the page share refcount, warning if the page is not marked as shared.
> >   */
> >  static inline void dax_page_share_get(struct page *page)
> >  {
> > -	if (page->mapping != PAGE_MAPPING_DAX_SHARED) {
> > -		/*
> > -		 * Reset the index if the page was already mapped
> > -		 * regularly before.
> > -		 */
> > -		if (page->mapping)
> > -			page->share = 1;
> > -		page->mapping = PAGE_MAPPING_DAX_SHARED;
> > -	}
> > +	WARN_ON_ONCE(!page->share);
> > +	WARN_ON_ONCE(page->mapping);
> 
> Given the only caller of this function is dax_associate_entry() it seems
> like overkill to check that a function only a few lines away manipulated
> ->mapping correctly.

Good call.

> I don't see much reason for dax_page_share_get() to exist after your
> changes.
> 
> Perhaps all that is needed is a dax_make_shared() helper that does the
> initial fiddling of '->mapping = NULL' and '->share = 1'?

Ok. I was going to make the argument that dax_make_shared() was overkill as
well, but as noted below it's a good place to put the comment describing how
this all works so have done that.

> >  	page->share++;
> >  }
> >  
> >  static inline unsigned long dax_page_share_put(struct page *page)
> >  {
> > +	WARN_ON_ONCE(!page->share);
> >  	return --page->share;
> >  }
> >  
> >  /*
> > - * When it is called in dax_insert_entry(), the shared flag will indicate that
> > - * whether this entry is shared by multiple files.  If so, set the page->mapping
> > - * PAGE_MAPPING_DAX_SHARED, and use page->share as refcount.
> > + * When it is called in dax_insert_entry(), the shared flag will indicate
> > + * whether this entry is shared by multiple files. If the page has not
> > + * previously been associated with any mappings the ->mapping and ->index
> > + * fields will be set. If it has already been associated with a mapping
> > + * the mapping will be cleared and the share count set. It's then up to the
> > + * file-system to track which mappings contain which pages, ie. by implementing
> > + * dax_holder_operations.
> 
> This feels like a good comment for a new dax_make_shared() not
> dax_associate_entry().
> 
> I would also:
> 
> s/up to the file-system to track which mappings contain which pages, ie. by implementing
>  dax_holder_operations/up to reverse map users like memory_failure() to
> call back into the filesystem to recover ->mapping and ->index
> information/

Sounds good, although I left a reference to dax_holder_operations in the comment
because it's not immediately obvious how file-systems do this currently and I
had to relearn that more times than I'd care to admit :-)

> >   */
> >  static void dax_associate_entry(void *entry, struct address_space *mapping,
> >  		struct vm_area_struct *vma, unsigned long address, bool shared)
> > @@ -397,7 +400,17 @@ static void dax_associate_entry(void *entry, struct address_space *mapping,
> >  	for_each_mapped_pfn(entry, pfn) {
> >  		struct page *page = pfn_to_page(pfn);
> >  
> > -		if (shared) {
> > +		if (shared && page->mapping && page->share) {
> 
> How does this case happen? I don't think any page would ever enter with
> both ->mapping and ->share set, right?

Sigh. You're right - it can't. This patch series is getting a litte bit large
and unweildy with all the prerequisite bugfixes and cleanups. Obviously I fixed
this when developing the main fs dax count fixup but forgot to rebase the fix
further back in the series.

Anyway I have fixed that now, thanks.

> If the file was mapped then reflinked then ->share should be zero at the
> first mapping attempt. It might not be zero because it is aliased with
> index until it is converted to a shared page.