On Fri, 2023-12-15 at 05:56 +0000, Matthew Wilcox wrote:
> On Thu, Dec 14, 2023 at 10:25:27PM -0700, Vishal Verma wrote:
> > @@ -294,13 +294,10 @@ static ssize_t available_size_show(struct device *dev,
> > struct device_attribute *attr, char *buf)
> > {
> > struct dax_region *dax_region = dev_get_drvdata(dev);
> > - unsigned long long size;
> >
> > - device_lock(dev);
> > - size = dax_region_avail_size(dax_region);
> > - device_unlock(dev);
> > + guard(device)(dev);
> >
> > - return sprintf(buf, "%llu\n", size);
> > + return sprintf(buf, "%llu\n", dax_region_avail_size(dax_region));
> > }
>
> Is this an appropriate use of guard()? sprintf is not the fastest of
> functions, so we will end up holding the device_lock for longer than
> we used to.
Hi Matthew,
Agreed that we end up holding the lock for a bit longer in many of
these. I'm inclined to say this is okay, since these are all user
configuration paths through sysfs, not affecting any sort of runtime
performance.
>
> > @@ -908,9 +890,8 @@ static ssize_t size_show(struct device *dev,
> > struct dev_dax *dev_dax = to_dev_dax(dev);
> > unsigned long long size;
> >
> > - device_lock(dev);
> > + guard(device)(dev);
> > size = dev_dax_size(dev_dax);
> > - device_unlock(dev);
> >
> > return sprintf(buf, "%llu\n", size);
> > }
>
> If it is appropriate, then you can do without the 'size' variable here.
Yep will remove. I suppose a lot of these can also switch to sysfs_emit
as Greg pointed out in a previous posting. I can add that as a separate
cleanup patch.
>
> > @@ -1137,21 +1117,20 @@ static ssize_t mapping_store(struct device *dev, struct device_attribute *attr,
> > if (rc)
> > return rc;
> >
> > - rc = -ENXIO;
> > - device_lock(dax_region->dev);
> > - if (!dax_region->dev->driver) {
> > - device_unlock(dax_region->dev);
> > - return rc;
> > - }
> > - device_lock(dev);
> > + guard(device)(dax_region->dev);
> > + if (!dax_region->dev->driver)
> > + return -ENXIO;
> >
> > + guard(device)(dev);
> > to_alloc = range_len(&r);
> > - if (alloc_is_aligned(dev_dax, to_alloc))
> > - rc = alloc_dev_dax_range(dev_dax, r.start, to_alloc);
> > - device_unlock(dev);
> > - device_unlock(dax_region->dev);
> > + if (!alloc_is_aligned(dev_dax, to_alloc))
> > + return -ENXIO;
> >
> > - return rc == 0 ? len : rc;
> > + rc = alloc_dev_dax_range(dev_dax, r.start, to_alloc);
> > + if (rc)
> > + return rc;
> > +
> > + return len;
> > }
>
> Have I mentioned how much I hate the "rc" naming convention? It tells
> you nothing useful about the contents of the variable. If you called it
> 'err', I'd know it was an error, and then the end of this function would
> make sense.
>
> if (err)
> return err;
> return len;
>
I'm a little hesitant to change this because the 'rc' convention is
used all over this file, and while I don't mind making this change for
the bits I touch in this patch, it would just result in a mix of 'rc'
and 'err' in this file.
