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

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

On 11/19/21 19:53, Jason Gunthorpe wrote:
> On Fri, Nov 19, 2021 at 07:26:44PM +0000, Joao Martins wrote:
>> On 11/19/21 16:55, Jason Gunthorpe wrote:
>>> On Fri, Nov 19, 2021 at 04:12:18PM +0000, Joao Martins wrote:
>>>
>>>>> 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").
>>>>>
>>>> Below is what I have staged so far as a percursor patch (see below scissors mark).
>>>>
>>>> It also lets me simplify compound page case for __dax_set_mapping() in this patch,
>>>> like below diff.
>>>>
>>>> But I still wonder whether this ordering adjustment of @mapping setting is best placed
>>>> as a percursor patch whenever pgmap/page refcount changes happen. Anyways it's just a
>>>> thought.
>>>
>>> naively I would have thought you'd set the mapping on all pages when
>>> you create the address_space and allocate pages into it. 
>>
>> Today in fsdax/device-dax (hugetlb too) this is set on fault and set once
>> only (as you say) on the mapped pages. fsdax WARN_ON() you when you clearing
>> a page mapping that was not set to the expected address_space (similar to
>> what I did here)
> 
> I would imagine that a normal FS case is to allocate some new memory
> and then join it to the address_space and set mapping, so that makes
> sense.
> 
> For fsdax, logically the DAX pages on the medium with struct pages
> could be in the address_space as soon as the inode is created. That
> would improve fault performance at the cost of making address_space
> creation a lot slower, so I can see why not to do that.
> 
>>> AFAIK devmap
>>> assigns all pages to a single address_space, so shouldn't the mapping
>>> just be done once?
>> Isn't it a bit more efficient that you set only when you try to map a page?
> 
> For devdax if you can set the address space as part of initializing
> each struct page and setting the compounds it would probably be a net
> win?
> 

Provided that we only set in the head yes, it would have a neligible cost
over region bringup as it only touches the head mapping.

Now with the base pages on device-dax the zone init would probably
jump considerably.

> Anyhow, I think what you did here is OK? 
Yeah, I wanted to hear Dan thoughts over -- or maybe I should just respin
the series with the added cleanup.

	Joao