lkml.org 
[lkml]   [2021]   [Jul]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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?

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.

\
 
 \ /
  Last update: 2021-07-19 18:09    [W:0.140 / U:3.376 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site