Re: [PATCH 3/4] dax: Introduce alloc_dev_dax_id()

[Ira Weiny <ira.weiny@intel.com>]

Dan Williams wrote:
> Ira Weiny wrote:
> > Dan Williams wrote:
> > > The reference counting of dax_region objects is needlessly complicated,
> > > has lead to confusion [1], and has hidden a bug [2]. Towards cleaning up
> > > that mess introduce alloc_dev_dax_id() to minimize the holding of a
> > > dax_region reference to only what dev_dax_release() needs, the
> > > dax_region->ida.
> > > 
> > > Part of the reason for the mess was the design to dereference a
> > > dax_region in all cases in free_dev_dax_id() even if the id was
> > > statically assigned by the upper level dax_region driver. Remove the
> > > need to call "is_static(dax_region)" by tracking whether the id is
> > > dynamic directly in the dev_dax instance itself.
> > > 
> > > With that flag the dax_region pinning and release per dev_dax instance
> > > can move to alloc_dev_dax_id() and free_dev_dax_id() respectively.
> > > 
> > > A follow-on cleanup address the unnecessary references in the dax_region
> > > setup and drivers.
> > > 
> > > Fixes: 0f3da14a4f05 ("device-dax: introduce 'seed' devices")
> > > Link: http://lore.kernel.org/r/20221203095858.612027-1-liuyongqiang13@huawei.com [1]
> > > Link: http://lore.kernel.org/r/3cf0890b-4eb0-e70e-cd9c-2ecc3d496263@hpe.com [2]
> > > Reported-by: Yongqiang Liu <liuyongqiang13@huawei.com>
> > > Reported-by: Paul Cassella <cassella@hpe.com>
> > > Reported-by: Ira Weiny <ira.weiny@intel.com>
> > > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> [..]
> > > @@ -1326,6 +1340,7 @@ struct dev_dax *devm_create_dev_dax(struct dev_dax_data *data)
> > >  	if (!dev_dax)
> > >  		return ERR_PTR(-ENOMEM);
> > >  
> > > +	dev_dax->region = dax_region;
> > 
> > Overall I like that this reference is not needed to be carried and/or
> > managed by the callers.
> > 
> > However, here you are referencing the dax_region from the dev_dax in an
> > unrelated place to where the reference matters (in id management).
> > 
> > Could alloc_dev_dax_id() change to:
> > 
> > static int alloc_dev_dax_id(struct dev_dax *dev_dax, struct dax_region *dax_region)
> > {
> > ...
> > }
> > 
> > Then make this assignment next to where the kref is taken so it is clear
> > that this is the only user of the reference?
> > 
> > I did not pick up on the fact this reference was only needed to free the
> > id at all in reviewing the code and I think this would make it even more
> > clear.
> 
> I hesitate only for symmetry reasons. I.e. that there are many interfaces in
> this file, in addition to free_dev_dax_id(), where @dax_region is
> implicitly retrieved from the @dev_dax.


Ok but the reason we need this extra reference and for the dax_region to
live this long is because the ida within the dax_region.  Otherwise the
normal device references would be enough, right?

Regardless, I've convinced myself this is ok.

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

Ira