Re: [PATCH v8 19/20] fs/dax: Properly refcount fs dax pages

[Alistair Popple <apopple@nvidia.com>]

On Tue, Feb 18, 2025 at 12:37:28PM +0100, David Hildenbrand wrote:
> On 18.02.25 04:55, Alistair Popple wrote:
> > Currently fs dax pages are considered free when the refcount drops to
> > one and their refcounts are not increased when mapped via PTEs or
> > decreased when unmapped. This requires special logic in mm paths to
> > detect that these pages should not be properly refcounted, and to
> > detect when the refcount drops to one instead of zero.
> > 
> > On the other hand get_user_pages(), etc. will properly refcount fs dax
> > pages by taking a reference and dropping it when the page is
> > unpinned.
> > 
> > Tracking this special behaviour requires extra PTE bits
> > (eg. pte_devmap) and introduces rules that are potentially confusing
> > and specific to FS DAX pages. To fix this, and to possibly allow
> > removal of the special PTE bits in future, convert the fs dax page
> > refcounts to be zero based and instead take a reference on the page
> > each time it is mapped as is currently the case for normal pages.
> > 
> > This may also allow a future clean-up to remove the pgmap refcounting
> > that is currently done in mm/gup.c.
> > 
> > Signed-off-by: Alistair Popple <apopple@nvidia.com>
> > Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> 
> A couple of nits (sorry that I didn't manage to review the whole thing the
> last time, I am a slow reviewer ...).

No problem. I appreciate your indepth review comments.

> Likely that can all be adjsuted on
> top, no need for a full resend IMHO.
> 
> > index 6674540..cf96f3d 100644
> > --- a/fs/dax.c
> > +++ b/fs/dax.c
> > @@ -71,6 +71,11 @@ static unsigned long dax_to_pfn(void *entry)
> >   	return xa_to_value(entry) >> DAX_SHIFT;
> >   }
> > +static struct folio *dax_to_folio(void *entry)
> > +{
> > +	return page_folio(pfn_to_page(dax_to_pfn(entry)));
> 
> Nit: return pfn_folio(dax_to_pfn(entry));
> 
> > +}
> > +
> 
> [...]
> 
> > -static inline unsigned long dax_folio_share_put(struct folio *folio)
> > +static inline unsigned long dax_folio_put(struct folio *folio)
> >   {
> > -	return --folio->page.share;
> > +	unsigned long ref;
> > +	int order, i;
> > +
> > +	if (!dax_folio_is_shared(folio))
> > +		ref = 0;
> > +	else
> > +		ref = --folio->share;
> > +
> 
> out of interest, what synchronizes access to folio->share?

Actually that's an excellent question as I hadn't looked too closely at this
given I wasn't changing the overall flow with regards to synchronization, merely
representation of the "shared" state. So I don't have a good answer for you off
the top of my head - Dan maybe you can shed some light here?

> > +	if (ref)
> > +		return ref;
> > +
> > +	folio->mapping = NULL;
> > +	order = folio_order(folio);
> > +	if (!order)
> > +		return 0;
> > +
> > +	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().
> > +		 */
> > +		new_folio->pgmap = pgmap;
> > +		new_folio->share = 0;
> > +		WARN_ON_ONCE(folio_ref_count(new_folio));
> > +	}
> > +
> > +	return ref;
> > +}
> > +
> > +static void dax_folio_init(void *entry)
> > +{
> > +	struct folio *folio = dax_to_folio(entry);
> > +	int order = dax_entry_order(entry);
> > +
> > +	/*
> > +	 * Folio should have been split back to order-0 pages in
> > +	 * dax_folio_put() when they were removed from their
> > +	 * final mapping.
> > +	 */
> > +	WARN_ON_ONCE(folio_order(folio));
> > +
> > +	if (order > 0) {
> > +		prep_compound_page(&folio->page, order);
> > +		if (order > 1)
> > +			INIT_LIST_HEAD(&folio->_deferred_list);
> 
> Nit: prep_compound_page() -> prep_compound_head() should be taking care of
> initializing all folio fields already, so this very likely can be dropped.

Thanks. That's only the case for >= v6.10, so I guess I started this patch
series before then.

> > +		WARN_ON_ONCE(folio_ref_count(folio));
> > +	}
> >   }
> 
> 
> [...]
> 
> 
> >   }
> > @@ -1808,7 +1843,8 @@ static vm_fault_t dax_fault_iter(struct vm_fault *vmf,
> >   	loff_t pos = (loff_t)xas->xa_index << PAGE_SHIFT;
> >   	bool write = iter->flags & IOMAP_WRITE;
> >   	unsigned long entry_flags = pmd ? DAX_PMD : 0;
> > -	int err = 0;
> > +	struct folio *folio;
> > +	int ret, err = 0;
> >   	pfn_t pfn;
> >   	void *kaddr;
> > @@ -1840,17 +1876,19 @@ static vm_fault_t dax_fault_iter(struct vm_fault *vmf,
> >   			return dax_fault_return(err);
> >   	}
> > +	folio = dax_to_folio(*entry);
> >   	if (dax_fault_is_synchronous(iter, vmf->vma))
> >   		return dax_fault_synchronous_pfnp(pfnp, pfn);
> > -	/* insert PMD pfn */
> > +	folio_ref_inc(folio);
> 
> Why is that not a folio_get() ? Could the refcount be 0? Might deserve a
> comment then.

Right, refcount is most likely 0 as this is when the folio is allocated for uses
other than by the owning driver of the page.

> >   	if (pmd)
> > -		return vmf_insert_pfn_pmd(vmf, pfn, write);
> > +		ret = vmf_insert_folio_pmd(vmf, pfn_folio(pfn_t_to_pfn(pfn)),
> > +					write);
> > +	else
> > +		ret = vmf_insert_page_mkwrite(vmf, pfn_t_to_page(pfn), write);
> > +	folio_put(folio);
> > -	/* insert PTE pfn */
> > -	if (write)
> > -		return vmf_insert_mixed_mkwrite(vmf->vma, vmf->address, pfn);
> > -	return vmf_insert_mixed(vmf->vma, vmf->address, pfn);
> > +	return ret;
> >   }
> >   static vm_fault_t dax_iomap_pte_fault(struct vm_fault *vmf, pfn_t *pfnp,
> > @@ -2089,6 +2127,7 @@ dax_insert_pfn_mkwrite(struct vm_fault *vmf, pfn_t pfn, unsigned int order)
> >   {
> >   	struct address_space *mapping = vmf->vma->vm_file->f_mapping;
> >   	XA_STATE_ORDER(xas, &mapping->i_pages, vmf->pgoff, order);
> > +	struct folio *folio;
> >   	void *entry;
> >   	vm_fault_t ret;
> > @@ -2106,14 +2145,17 @@ dax_insert_pfn_mkwrite(struct vm_fault *vmf, pfn_t pfn, unsigned int order)
> >   	xas_set_mark(&xas, PAGECACHE_TAG_DIRTY);
> >   	dax_lock_entry(&xas, entry);
> >   	xas_unlock_irq(&xas);
> > +	folio = pfn_folio(pfn_t_to_pfn(pfn));
> > +	folio_ref_inc(folio);
> 
> Same thought.

Yes, will add a comment, either as a respin or a fixup depending what Andrew prefers.

> > diff --git a/include/linux/dax.h b/include/linux/dax.h
> > index 2333c30..dcc9fcd 100644
> > --- a/include/linux/dax.h
> > +++ b/include/linux/dax.h
> > @@ -209,7 +209,7 @@ int dax_truncate_page(struct inode *inode, loff_t pos, bool *did_zero,
> 
> [...]
> 
> > diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> > index d189826..1a0d6a8 100644
> > --- a/mm/huge_memory.c
> > +++ b/mm/huge_memory.c
> > @@ -2225,7 +2225,7 @@ int zap_huge_pmd(struct mmu_gather *tlb, struct vm_area_struct *vma,
> >   						tlb->fullmm);
> >   	arch_check_zapped_pmd(vma, orig_pmd);
> >   	tlb_remove_pmd_tlb_entry(tlb, pmd, addr);
> > -	if (vma_is_special_huge(vma)) {
> > +	if (!vma_is_dax(vma) && vma_is_special_huge(vma)) {
> 
> I wonder if we actually want to remove the vma_is_dax() check from
> vma_is_special_huge(), and instead add it to the remaining callers of
> vma_is_special_huge() that still need it -- if any need it.
> 
> Did we sanity-check which callers of vma_is_special_huge() still need it? Is
> there still reason to have that DAX check in vma_is_special_huge()?

If by "we" you mean "me" then yes :) There are still a few callers of it, mainly
for page splitting.

> But vma_is_special_huge() is rather confusing from me ... the whole
> vma_is_special_huge() thing should probably be removed. That's a cleanup for
> another day, though.

But after double checking I have come to the same conclusion as you - it should
be removed. I will add that to my ever growing clean-up series that can go on
top of this one.

> 
> -- 
> Cheers,
> 
> David / dhildenb
>