Re: [PATCH v5 8/8] device-dax: compound devmap support

[Jason Gunthorpe <jgg@ziepe.ca>]

On Mon, Nov 15, 2021 at 01:11:32PM +0100, Joao Martins wrote:
> On 11/12/21 16:34, Jason Gunthorpe wrote:
> > On Fri, Nov 12, 2021 at 04:08:24PM +0100, Joao Martins wrote:
> > 
> >> diff --git a/drivers/dax/device.c b/drivers/dax/device.c
> >> index a65c67ab5ee0..0c2ac97d397d 100644
> >> +++ b/drivers/dax/device.c
> >> @@ -192,6 +192,42 @@ static vm_fault_t __dev_dax_pud_fault(struct dev_dax *dev_dax,
> >>  }
> >>  #endif /* !CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD */
> >>  
> >> +static void set_page_mapping(struct vm_fault *vmf, pfn_t pfn,
> >> +			     unsigned long fault_size,
> >> +			     struct address_space *f_mapping)
> >> +{
> >> +	unsigned long i;
> >> +	pgoff_t pgoff;
> >> +
> >> +	pgoff = linear_page_index(vmf->vma, ALIGN(vmf->address, fault_size));
> >> +
> >> +	for (i = 0; i < fault_size / PAGE_SIZE; i++) {
> >> +		struct page *page;
> >> +
> >> +		page = pfn_to_page(pfn_t_to_pfn(pfn) + i);
> >> +		if (page->mapping)
> >> +			continue;
> >> +		page->mapping = f_mapping;
> >> +		page->index = pgoff + i;
> >> +	}
> >> +}
> >> +
> >> +static void set_compound_mapping(struct vm_fault *vmf, pfn_t pfn,
> >> +				 unsigned long fault_size,
> >> +				 struct address_space *f_mapping)
> >> +{
> >> +	struct page *head;
> >> +
> >> +	head = pfn_to_page(pfn_t_to_pfn(pfn));
> >> +	head = compound_head(head);
> >> +	if (head->mapping)
> >> +		return;
> >> +
> >> +	head->mapping = f_mapping;
> >> +	head->index = linear_page_index(vmf->vma,
> >> +			ALIGN(vmf->address, fault_size));
> >> +}
> > 
> > Should this stuff be setup before doing vmf_insert_pfn_XX?
> > 
> 
> Interestingly filesystem-dax does this, but not device-dax.

I think it may be a bug ?

> set_page_mapping/set_compound_mapping() could be moved to before and
> then torn down on @rc != VM_FAULT_NOPAGE (failure). I am not sure
> what's the benefit in this series..  besides the ordering (that you
> hinted below) ?

Well, it should probably be fixed in a precursor patch.

I think the general idea is that page->mapping/index are stable once
the page is published in a PTE?

> > In normal cases the page should be returned in the vmf and populated
> > to the page tables by the core code after all this is done.
>
> So I suppose by call sites examples as 'core code' is either hugetlbfs call to
> __filemap_add_folio() (on hugetlbfs fault handler), shmem_add_to_page_cache() or
> anon-equivalent.

I was talking more about the normal page insertion flow which is done
by setting vmf->page and then returning. finish_fault() will install
the PTE

If this is the best way then I would expect some future where maybe
there is a vmf->folio and finish_fault() will install a PUD/PMD/PTE
and we don't call vmf_insert_pfnxx in DAX.

Jason