RE: [PATCH v5 2/9] dax: Introduce holder for dax_device

[ruansy.fnst@xxxxxxxxxxx]

> -----Original Message-----
> Subject: Re: [PATCH v5 2/9] dax: Introduce holder for dax_device
> 
> On Mon, Jun 28, 2021 at 08:02:11AM +0800, Shiyang Ruan wrote:
> > +int dax_holder_notify_failure(struct dax_device *dax_dev, loff_t offset,
> > +			      size_t size, void *data)
> > +{
> > +	int rc = -ENXIO;
> > +	if (!dax_dev)
> > +		return rc;
> > +
> > +	if (dax_dev->holder_data) {
> > +		rc = dax_dev->holder_ops->notify_failure(dax_dev, offset,
> > +							 size, data);
> > +		if (rc == -ENODEV)
> > +			rc = -ENXIO;
> > +	} else
> > +		rc = -EOPNOTSUPP;
> 
> The style looks a little odd.  Why not:
> 
> 	if (!dax_dev)
> 		return -ENXIO
> 	if (!dax_dev->holder_data)
> 		return -EOPNOTSUPP;
> 	return dax_dev->holder_ops->notify_failure(dax_dev, offset, size, data);
> 
> and let everyone deal with the same errno codes?
OK.

> 
> Also why do we even need the dax_dev NULL check?

Because this dax_dev is obtain by fs_dax_get_by_bdev() in XFS and dax_get_by_host() in MD.  According to their definition, NULL may be returned.  So I check the dax_dev here.

> 
> > +void dax_set_holder(struct dax_device *dax_dev, void *holder,
> > +		const struct dax_holder_operations *ops) {
> > +	if (!dax_dev)
> > +		return;
> 
> I don't think we really need that check here.
> 
> > +void *dax_get_holder(struct dax_device *dax_dev) {
> > +	void *holder_data;
> > +
> > +	if (!dax_dev)
> > +		return NULL;
> 
> Same here.
> 
> > +
> > +	down_read(&dax_dev->holder_rwsem);
> > +	holder_data = dax_dev->holder_data;
> > +	up_read(&dax_dev->holder_rwsem);
> > +
> > +	return holder_data;
> 
> That lock won't protect anything.  I think we simply must have synchronization
> to prevent unregistration while the ->notify_failure call is in progress.

Yes, I misunderstood the purpose of the lock. I'll fix this.


--
Thanks,
Ruan.