diff mbox series

[2/3] nvdimm/pmem: move dax_attribute_group from dax to pmem

Message ID 20210920072726.1159572-3-hch@lst.de (mailing list archive)
State New, archived
Headers show
Series [1/3] nvdimm/pmem: fix creating the dax group | expand

Commit Message

Christoph Hellwig Sept. 20, 2021, 7:27 a.m. UTC 
dax_attribute_group is only used by the pmem driver, and can avoid the
completely pointless lookup by the disk name if moved there.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/dax/super.c   | 64 -------------------------------------------
 drivers/nvdimm/pmem.c | 43 +++++++++++++++++++++++++++++
 include/linux/dax.h   |  2 --
 3 files changed, 43 insertions(+), 66 deletions(-)

Comments

kernel test robot Sept. 20, 2021, 6 p.m. UTC | #1 
Hi Christoph,

I love your patch! Yet something to improve:

[auto build test ERROR on axboe-block/for-next]
[also build test ERROR on linus/master v5.15-rc2 next-20210920]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Christoph-Hellwig/nvdimm-pmem-fix-creating-the-dax-group/20210920-162804
base:   https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-next
config: riscv-buildonly-randconfig-r006-20210920 (attached as .config)
compiler: riscv64-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/1a16d0f32f70bcd159a9f8cf32794f4024e8711e
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Christoph-Hellwig/nvdimm-pmem-fix-creating-the-dax-group/20210920-162804
        git checkout 1a16d0f32f70bcd159a9f8cf32794f4024e8711e
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross ARCH=riscv 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/dax/super.c:375:6: error: no previous prototype for 'run_dax' [-Werror=missing-prototypes]
     375 | void run_dax(struct dax_device *dax_dev)
         |      ^~~~~~~
>> drivers/dax/super.c:70:27: error: 'dax_get_by_host' defined but not used [-Werror=unused-function]
      70 | static struct dax_device *dax_get_by_host(const char *host)
         |                           ^~~~~~~~~~~~~~~
   cc1: all warnings being treated as errors


vim +/dax_get_by_host +70 drivers/dax/super.c

1b7646014e0d838b Christoph Hellwig 2021-08-26  65  
1b7646014e0d838b Christoph Hellwig 2021-08-26  66  /**
1b7646014e0d838b Christoph Hellwig 2021-08-26  67   * dax_get_by_host() - temporary lookup mechanism for filesystem-dax
1b7646014e0d838b Christoph Hellwig 2021-08-26  68   * @host: alternate name for the device registered by a dax driver
1b7646014e0d838b Christoph Hellwig 2021-08-26  69   */
1b7646014e0d838b Christoph Hellwig 2021-08-26 @70  static struct dax_device *dax_get_by_host(const char *host)
1b7646014e0d838b Christoph Hellwig 2021-08-26  71  {
1b7646014e0d838b Christoph Hellwig 2021-08-26  72  	struct dax_device *dax_dev, *found = NULL;
1b7646014e0d838b Christoph Hellwig 2021-08-26  73  	int hash, id;
1b7646014e0d838b Christoph Hellwig 2021-08-26  74  
1b7646014e0d838b Christoph Hellwig 2021-08-26  75  	if (!host)
1b7646014e0d838b Christoph Hellwig 2021-08-26  76  		return NULL;
1b7646014e0d838b Christoph Hellwig 2021-08-26  77  
1b7646014e0d838b Christoph Hellwig 2021-08-26  78  	hash = dax_host_hash(host);
1b7646014e0d838b Christoph Hellwig 2021-08-26  79  
1b7646014e0d838b Christoph Hellwig 2021-08-26  80  	id = dax_read_lock();
1b7646014e0d838b Christoph Hellwig 2021-08-26  81  	spin_lock(&dax_host_lock);
1b7646014e0d838b Christoph Hellwig 2021-08-26  82  	hlist_for_each_entry(dax_dev, &dax_host_list[hash], list) {
1b7646014e0d838b Christoph Hellwig 2021-08-26  83  		if (!dax_alive(dax_dev)
1b7646014e0d838b Christoph Hellwig 2021-08-26  84  				|| strcmp(host, dax_dev->host) != 0)
1b7646014e0d838b Christoph Hellwig 2021-08-26  85  			continue;
1b7646014e0d838b Christoph Hellwig 2021-08-26  86  
1b7646014e0d838b Christoph Hellwig 2021-08-26  87  		if (igrab(&dax_dev->inode))
1b7646014e0d838b Christoph Hellwig 2021-08-26  88  			found = dax_dev;
1b7646014e0d838b Christoph Hellwig 2021-08-26  89  		break;
1b7646014e0d838b Christoph Hellwig 2021-08-26  90  	}
1b7646014e0d838b Christoph Hellwig 2021-08-26  91  	spin_unlock(&dax_host_lock);
1b7646014e0d838b Christoph Hellwig 2021-08-26  92  	dax_read_unlock(id);
1b7646014e0d838b Christoph Hellwig 2021-08-26  93  
1b7646014e0d838b Christoph Hellwig 2021-08-26  94  	return found;
1b7646014e0d838b Christoph Hellwig 2021-08-26  95  }
1b7646014e0d838b Christoph Hellwig 2021-08-26  96  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Ira Weiny Sept. 20, 2021, 10:51 p.m. UTC | #2 
On Mon, Sep 20, 2021 at 09:27:25AM +0200, Christoph Hellwig wrote:

...

> diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
> index ef4950f808326..bbeb3f46db157 100644
> --- a/drivers/nvdimm/pmem.c
> +++ b/drivers/nvdimm/pmem.c
> @@ -328,6 +328,49 @@ static const struct dax_operations pmem_dax_ops = {
>  	.zero_page_range = pmem_dax_zero_page_range,
>  };
>  
> +static ssize_t write_cache_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	struct pmem_device *pmem = dev_to_disk(dev)->private_data;

I want to say this should be dax_get_private()...  However, looking at the use
of dax_get_private() not a single caller checks for NULL!  :-(

So now I wonder why dax_get_private() exists...  :-/

A quick history search does not make anything apparent.  When the DAXDEV_ALIVE
check was added to dax_get_private() no callers were changed to account for a
potential NULL return.

Dan?

> +
> +	return sprintf(buf, "%d\n", !!dax_write_cache_enabled(pmem->dax_dev));
> +}
> +
> +static ssize_t write_cache_store(struct device *dev,
> +		struct device_attribute *attr, const char *buf, size_t len)
> +{
> +	struct pmem_device *pmem = dev_to_disk(dev)->private_data;

Same here...

Ira
Dan Williams Sept. 20, 2021, 11:29 p.m. UTC | #3 
On Mon, Sep 20, 2021 at 11:01 AM kernel test robot <lkp@intel.com> wrote:
>
> Hi Christoph,
>
> I love your patch! Yet something to improve:
>
> [auto build test ERROR on axboe-block/for-next]
> [also build test ERROR on linus/master v5.15-rc2 next-20210920]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
>
> url:    https://github.com/0day-ci/linux/commits/Christoph-Hellwig/nvdimm-pmem-fix-creating-the-dax-group/20210920-162804
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-next
> config: riscv-buildonly-randconfig-r006-20210920 (attached as .config)
> compiler: riscv64-linux-gcc (GCC) 11.2.0
> reproduce (this is a W=1 build):
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # https://github.com/0day-ci/linux/commit/1a16d0f32f70bcd159a9f8cf32794f4024e8711e
>         git remote add linux-review https://github.com/0day-ci/linux
>         git fetch --no-tags linux-review Christoph-Hellwig/nvdimm-pmem-fix-creating-the-dax-group/20210920-162804
>         git checkout 1a16d0f32f70bcd159a9f8cf32794f4024e8711e
>         # save the attached .config to linux build tree
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross ARCH=riscv
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
>
> All errors (new ones prefixed by >>):
>
>    drivers/dax/super.c:375:6: error: no previous prototype for 'run_dax' [-Werror=missing-prototypes]
>      375 | void run_dax(struct dax_device *dax_dev)
>          |      ^~~~~~~

In this new -Werror world drivers/dax/bus.c needs:

#include "bus.h"

> >> drivers/dax/super.c:70:27: error: 'dax_get_by_host' defined but not used [-Werror=unused-function]
>       70 | static struct dax_device *dax_get_by_host(const char *host)
>          |                           ^~~~~~~~~~~~~~~
>    cc1: all warnings being treated as errors

Nice additional cleanup that now fs_dax_get_by_bdev() is the only
caller of dax_get_by_host().
Dan Williams Sept. 20, 2021, 11:36 p.m. UTC | #4 
On Mon, Sep 20, 2021 at 3:51 PM Ira Weiny <ira.weiny@intel.com> wrote:
>
> On Mon, Sep 20, 2021 at 09:27:25AM +0200, Christoph Hellwig wrote:
>
> ...
>
> > diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
> > index ef4950f808326..bbeb3f46db157 100644
> > --- a/drivers/nvdimm/pmem.c
> > +++ b/drivers/nvdimm/pmem.c
> > @@ -328,6 +328,49 @@ static const struct dax_operations pmem_dax_ops = {
> >       .zero_page_range = pmem_dax_zero_page_range,
> >  };
> >
> > +static ssize_t write_cache_show(struct device *dev,
> > +             struct device_attribute *attr, char *buf)
> > +{
> > +     struct pmem_device *pmem = dev_to_disk(dev)->private_data;
>
> I want to say this should be dax_get_private()...  However, looking at the use

No, this wants to do from @dev to @dax_dev. dax_get_private() assumes
that @dax_dev is already known. Also, in this case @dev is the gendisk
device, so this is a gendisk-to-dax-device with special knowledge that
the gendisk is for a pmem device.

> of dax_get_private() not a single caller checks for NULL!  :-(

All the callers are correctly assuming that their usage is before kill_dax().

>
> So now I wonder why dax_get_private() exists...  :-/

It exists so that the definition of 'struct dax_device' can remain
private, as no one should be directly mucking with dax_device
internals outside of the provided APIs.

> A quick history search does not make anything apparent.  When the DAXDEV_ALIVE
> check was added to dax_get_private() no callers were changed to account for a
> potential NULL return.
>
> Dan?

I double checked, but this all looks ok to me.
Dan Williams Sept. 20, 2021, 11:37 p.m. UTC | #5 
On Mon, Sep 20, 2021 at 12:29 AM Christoph Hellwig <hch@lst.de> wrote:
>
> dax_attribute_group is only used by the pmem driver, and can avoid the
> completely pointless lookup by the disk name if moved there.

After the additional fixups that 0day-robot found, you can add:

Reviewed-by: Dan Williams <dan.j.williams@intel.com>
diff mbox series

Patch

diff --git a/drivers/dax/super.c b/drivers/dax/super.c
index fc89e91beea7c..e03d94bdc0449 100644
--- a/drivers/dax/super.c
+++ b/drivers/dax/super.c
@@ -231,70 +231,6 @@  enum dax_device_flags {
 	DAXDEV_SYNC,
 };
 
-static ssize_t write_cache_show(struct device *dev,
-		struct device_attribute *attr, char *buf)
-{
-	struct dax_device *dax_dev = dax_get_by_host(dev_name(dev));
-	ssize_t rc;
-
-	WARN_ON_ONCE(!dax_dev);
-	if (!dax_dev)
-		return -ENXIO;
-
-	rc = sprintf(buf, "%d\n", !!dax_write_cache_enabled(dax_dev));
-	put_dax(dax_dev);
-	return rc;
-}
-
-static ssize_t write_cache_store(struct device *dev,
-		struct device_attribute *attr, const char *buf, size_t len)
-{
-	bool write_cache;
-	int rc = strtobool(buf, &write_cache);
-	struct dax_device *dax_dev = dax_get_by_host(dev_name(dev));
-
-	WARN_ON_ONCE(!dax_dev);
-	if (!dax_dev)
-		return -ENXIO;
-
-	if (rc)
-		len = rc;
-	else
-		dax_write_cache(dax_dev, write_cache);
-
-	put_dax(dax_dev);
-	return len;
-}
-static DEVICE_ATTR_RW(write_cache);
-
-static umode_t dax_visible(struct kobject *kobj, struct attribute *a, int n)
-{
-	struct device *dev = container_of(kobj, typeof(*dev), kobj);
-	struct dax_device *dax_dev = dax_get_by_host(dev_name(dev));
-
-	WARN_ON_ONCE(!dax_dev);
-	if (!dax_dev)
-		return 0;
-
-#ifndef CONFIG_ARCH_HAS_PMEM_API
-	if (a == &dev_attr_write_cache.attr)
-		return 0;
-#endif
-	return a->mode;
-}
-
-static struct attribute *dax_attributes[] = {
-	&dev_attr_write_cache.attr,
-	NULL,
-};
-
-struct attribute_group dax_attribute_group = {
-	.name = "dax",
-	.attrs = dax_attributes,
-	.is_visible = dax_visible,
-};
-EXPORT_SYMBOL_GPL(dax_attribute_group);
-
 /**
  * dax_direct_access() - translate a device pgoff to an absolute pfn
  * @dax_dev: a dax_device instance representing the logical memory range
diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
index ef4950f808326..bbeb3f46db157 100644
--- a/drivers/nvdimm/pmem.c
+++ b/drivers/nvdimm/pmem.c
@@ -328,6 +328,49 @@  static const struct dax_operations pmem_dax_ops = {
 	.zero_page_range = pmem_dax_zero_page_range,
 };
 
+static ssize_t write_cache_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct pmem_device *pmem = dev_to_disk(dev)->private_data;
+
+	return sprintf(buf, "%d\n", !!dax_write_cache_enabled(pmem->dax_dev));
+}
+
+static ssize_t write_cache_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t len)
+{
+	struct pmem_device *pmem = dev_to_disk(dev)->private_data;
+	bool write_cache;
+	int rc;
+
+	rc = strtobool(buf, &write_cache);
+	if (rc)
+		return rc;
+	dax_write_cache(pmem->dax_dev, write_cache);
+	return len;
+}
+static DEVICE_ATTR_RW(write_cache);
+
+static umode_t dax_visible(struct kobject *kobj, struct attribute *a, int n)
+{
+#ifndef CONFIG_ARCH_HAS_PMEM_API
+	if (a == &dev_attr_write_cache.attr)
+		return 0;
+#endif
+	return a->mode;
+}
+
+static struct attribute *dax_attributes[] = {
+	&dev_attr_write_cache.attr,
+	NULL,
+};
+
+static const struct attribute_group dax_attribute_group = {
+	.name		= "dax",
+	.attrs		= dax_attributes,
+	.is_visible	= dax_visible,
+};
+
 static const struct attribute_group *pmem_attribute_groups[] = {
 	&dax_attribute_group,
 	NULL,
diff --git a/include/linux/dax.h b/include/linux/dax.h
index 2619d94c308d4..8623caa673889 100644
--- a/include/linux/dax.h
+++ b/include/linux/dax.h
@@ -38,8 +38,6 @@  struct dax_operations {
 	int (*zero_page_range)(struct dax_device *, pgoff_t, size_t);
 };
 
-extern struct attribute_group dax_attribute_group;
-
 #if IS_ENABLED(CONFIG_DAX)
 struct dax_device *alloc_dax(void *private, const char *host,
 		const struct dax_operations *ops, unsigned long flags);