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?
Also why do we even need the dax_dev NULL check?
> +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.
