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

[Joao Martins <joao.m.martins@oracle.com>]

On 11/15/21 17:49, Jason Gunthorpe wrote:
> 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 ?
> 
Possibly.

Dan, any thoughts (see also below) ? You probably hold all that
history since its inception on commit 2232c6382a4 ("device-dax: Enable page_mapping()")
and commit 35de299547d1 ("device-dax: Set page->index").

>> 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.
> 
Yeap -- I would move page_mapping prior to introduce set_compound_mapping().
Now I am thinking again and with that logic it makes more sense to ammend inside
set_page_mapping() -- should have less nested around in the fault handler.

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

>>> 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
> 
I misunderstood you earlier -- I thought you were suggesting me to look at
how mapping/index is set (in the context of the flow you just described)

	Joao