Re: [PATCH 1/8] dax: add region-available-size attribute

From: Dan Williams
Date: Wed Dec 14 2016 - 10:53:53 EST


On Wed, Dec 14, 2016 at 6:38 AM, Johannes Thumshirn <jthumshirn@xxxxxxx> wrote:
> Hi Dan,
>
> On Sat, Dec 10, 2016 at 10:28:30PM -0800, Dan Williams wrote:
>> In preparation for a facility that enables dax regions to be
>> sub-divided, introduce a 'dax/available_size' attribute. This attribute
>> appears under the parent device that registered the device-dax region,
>> and it assumes that the device-dax-core owns the driver-data for that
>> device.
>>
>> 'dax/available_size' adjusts dynamically as dax-device instances are
>> registered and unregistered.
>>
>> As a side effect of using __request_region() to reserve capacity from
>> the dax_region we now track pointers to those returned resources rather
>> than duplicating the passed in resource array.
>>
>> Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
>> ---
>
> [...]
>
>> +static const struct attribute_group *dax_region_attribute_groups[] = {
>> + &dax_region_attribute_group,
>> + NULL,
>> };
>>
>> static struct inode *dax_alloc_inode(struct super_block *sb)
>> @@ -200,12 +251,27 @@ void dax_region_put(struct dax_region *dax_region)
>> }
>> EXPORT_SYMBOL_GPL(dax_region_put);
>>
>> +
>
> Stray extra newline?
>
> [...]
>
>> struct dax_region *alloc_dax_region(struct device *parent, int region_id,
>> struct resource *res, unsigned int align, void *addr,
>> unsigned long pfn_flags)
>> {
>> struct dax_region *dax_region;
>>
>> + if (dev_get_drvdata(parent)) {
>> + dev_WARN(parent, "dax core found drvdata already in use\n");
>> + return NULL;
>> + }
>> +
>
> My first thought was, it might be interesting to see who already claimed
> the drvdata. Then I figured, how are multiple sub-regions of a dax-device
> supposed to work? What am I missing here?

This is a check similar to the -EBUSY return you would get from
request_mem_region(). In fact if all dax drivers are correctly calling
request_mem_region() before alloc_dax_region() then it would be
impossible for this check to ever fire. It's already impossible
because there's only one dax driver upstream (dax_pmem). It's not
really benefiting the kernel at all until we have multiple dax
drivers, I'll remove it.