| Message ID | 20210920072726.1159572-3-hch@lst.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/3] nvdimm/pmem: fix creating the dax group | expand |
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
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
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().
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.
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 --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);
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(-)