lkml.org 
[lkml]   [2021]   [Jul]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH v5 2/9] dax: Introduce holder for dax_device
Date
> -----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.
\
 
 \ /
  Last update: 2021-07-20 12:41    [W:0.109 / U:32.208 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site