> -----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.
