Returned mail: Data format error
The original message was received at Wed, 11 Apr 2018 11:29:58 +0800 from lists.01.org [3.200.103.101] - The following addresses had permanent fatal errors -___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
[PATCH v4] ndctl: add support in libndctl to provide deep flush
Providing an API call in libndctl to support accessing the region deep_flush in sysfs. Signed-off-by: Dave Jiang--- v4: Make setup of deep_flush open not interfere with add_region. (Dan) v3: - Add "\n" to sysfs write. (Dan) - add O_CLOEXEC to open() call for sysfs. (Dan) v2: Cover case where deep_flush doesn't exist, i.e. memmap=nn!ss. (Dan) ndctl/lib/libndctl.c | 31 +++ ndctl/lib/libndctl.sym |1 + ndctl/libndctl.h |1 + 3 files changed, 33 insertions(+) diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c index 580a450e..5d91d34e 100644 --- a/ndctl/lib/libndctl.c +++ b/ndctl/lib/libndctl.c @@ -181,6 +181,8 @@ struct ndctl_region { FILE *badblocks; struct badblock bb; enum ndctl_persistence_domain persistence_domain; + /* file descriptor for deep flush sysfs entry */ + int flush_fd; }; /** @@ -511,6 +513,8 @@ static void free_region(struct ndctl_region *region) free(region->region_path); if (region->badblocks) fclose(region->badblocks); + if (region->flush_fd > 0) + close(region->flush_fd); free(region); } @@ -1049,6 +1053,14 @@ NDCTL_EXPORT unsigned long long ndctl_region_get_resource(struct ndctl_region *r return strtoull(buf, NULL, 0); } +NDCTL_EXPORT int ndctl_region_deep_flush(struct ndctl_region *region) +{ + int rc = pwrite(region->flush_fd, "1\n", 1, 0); + + return (rc == -1) ? -errno : 0; +} + + NDCTL_EXPORT const char *ndctl_bus_get_cmd_name(struct ndctl_bus *bus, int cmd) { return nvdimm_bus_cmd_name(cmd); @@ -1791,6 +1803,7 @@ static void *add_region(void *parent, int id, const char *region_base) struct ndctl_bus *bus = parent; struct ndctl_ctx *ctx = bus->ctx; char *path = calloc(1, strlen(region_base) + 100); + int perm; if (!path) return NULL; @@ -1866,6 +1879,24 @@ static void *add_region(void *parent, int id, const char *region_base) else region->persistence_domain = region_get_pd_type(buf); + sprintf(path, "%s/deep_flush", region_base); + region->flush_fd = open(path, O_RDWR | O_CLOEXEC); + if (region->flush_fd == -1) + goto out; + + if (pread(region->flush_fd, buf, 1, 0) == -1) { + close(region->flush_fd); + region->flush_fd = -1; + goto out; + } + + perm = strtol(buf, NULL, 0); + if (perm == 0) { + region->flush_fd = -1; + close(region->flush_fd); + } + + out: free(path); return region; diff --git a/ndctl/lib/libndctl.sym b/ndctl/lib/libndctl.sym index 3209aefe..38cc3b9a 100644 --- a/ndctl/lib/libndctl.sym +++ b/ndctl/lib/libndctl.sym @@ -352,4 +352,5 @@ global: ndctl_dimm_fw_update_supported; ndctl_region_get_persistence_domain; ndctl_bus_get_persistence_domain; + ndctl_region_deep_flush; } LIBNDCTL_14; diff --git a/ndctl/libndctl.h b/ndctl/libndctl.h index cf6a77fd..1a622ae5 100644 --- a/ndctl/libndctl.h +++ b/ndctl/libndctl.h @@ -365,6 +365,7 @@ int ndctl_region_enable(struct ndctl_region *region); int ndctl_region_disable_invalidate(struct ndctl_region *region); int ndctl_region_disable_preserve(struct ndctl_region *region); void ndctl_region_cleanup(struct ndctl_region *region); +int ndctl_region_deep_flush(struct ndctl_region *region); struct ndctl_interleave_set; struct ndctl_interleave_set *ndctl_region_get_interleave_set( ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH v3] ndctl: add support in libndctl to provide deep flush
On Tue, Apr 10, 2018 at 4:35 PM, Dave Jiangwrote: > Providing an API call in libndctl to support accessing the region deep_flush > in sysfs. > > Signed-off-by: Dave Jiang > --- > > v3: > - Add "\n" to sysfs write. (Dan) > - add O_CLOEXEC to open() call for sysfs. (Dan) > > v2: Cover case where deep_flush doesn't exist, i.e. memmap=nn!ss. (Dan) > > ndctl/lib/libndctl.c | 35 +++ > ndctl/lib/libndctl.sym |1 + > ndctl/libndctl.h |1 + > 3 files changed, 37 insertions(+) > > diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c > index 580a450e..03a18985 100644 > --- a/ndctl/lib/libndctl.c > +++ b/ndctl/lib/libndctl.c > @@ -181,6 +181,8 @@ struct ndctl_region { > FILE *badblocks; > struct badblock bb; > enum ndctl_persistence_domain persistence_domain; > + /* file descriptor for deep flush sysfs entry */ > + int flush_fd; > }; > > /** > @@ -511,6 +513,8 @@ static void free_region(struct ndctl_region *region) > free(region->region_path); > if (region->badblocks) > fclose(region->badblocks); > + if (region->flush_fd > 0) > + close(region->flush_fd); > free(region); > } > > @@ -1049,6 +1053,14 @@ NDCTL_EXPORT unsigned long long > ndctl_region_get_resource(struct ndctl_region *r > return strtoull(buf, NULL, 0); > } > > +NDCTL_EXPORT int ndctl_region_deep_flush(struct ndctl_region *region) > +{ > + int rc = pwrite(region->flush_fd, "1\n", 1, 0); > + > + return (rc == -1) ? -errno : 0; > +} > + > + > NDCTL_EXPORT const char *ndctl_bus_get_cmd_name(struct ndctl_bus *bus, int > cmd) > { > return nvdimm_bus_cmd_name(cmd); > @@ -1791,6 +1803,7 @@ static void *add_region(void *parent, int id, const > char *region_base) > struct ndctl_bus *bus = parent; > struct ndctl_ctx *ctx = bus->ctx; > char *path = calloc(1, strlen(region_base) + 100); > + int perm; > > if (!path) > return NULL; > @@ -1866,6 +1879,28 @@ static void *add_region(void *parent, int id, const > char *region_base) > else > region->persistence_domain = region_get_pd_type(buf); > > + sprintf(path, "%s/deep_flush", region_base); > + region->flush_fd = open(path, O_RDWR | O_CLOEXEC); > + if (region->flush_fd == -1) { > + /* for those that do not export deep_flush sysfs file */ > + if (errno == ENOENT) Hmm, what about EPERM? For example, it should be fine for non-root users (who can't open this file as writable) can still list regions. So, I think lets kill all "goto err_read" for this thing because ->flush_fd will be -1 and the user will discover that fact if they actually try to trigger the flush. Otherwise pure "ndctl list" users should not see any failures to open this file as writable. ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [ndctl PATCH v2] ndctl: complete move to "fsdax" and "devdax"
On Tue, 2018-04-10 at 11:09 -0600, Ross Zwisler wrote: > Add on to the work started by: > > commit ebb4fb605e68 ("ndctl, create-namespace: introduce "fsdax" and > "devdax" modes") > > and change some more user visible places to use "fsdax" and "devdax" > modes > instead of "memory" and "dax", respectively. Having multiple terms for > the > same mode is confusing for users. > > We will continue to accept "memory" and "dax" as parameters, but all > output > and man pages will now use the updated terms. > > Note that after the above referenced commit we still printed the old > names > in the default 'ndctl list' output for backward compatibility with > scripts. > This patch intentionally breaks that backward compatibility in favor of > avoiding confusion and using the new mode names everywhere. > > Signed-off-by: Ross Zwisler> --- > Documentation/ndctl/ndctl-inject-error.txt | 2 +- > Documentation/ndctl/ndctl-list.txt | 6 +++--- > ndctl/namespace.c | 16 > util/json.c| 10 ++ > 4 files changed, 14 insertions(+), 20 deletions(-) This also needs unit test updates to create.sh and clear.sh as they look for 'memory' > > diff --git a/Documentation/ndctl/ndctl-inject-error.txt > b/Documentation/ndctl/ndctl-inject-error.txt > index 01f6c22..94c4e69 100644 > --- a/Documentation/ndctl/ndctl-inject-error.txt > +++ b/Documentation/ndctl/ndctl-inject-error.txt > @@ -45,7 +45,7 @@ OPTIONS > > NOTE: The offset is interpreted in different ways based on the > "mode" > of the namespace. For "raw" mode, the offset is the base > namespace > - offset. For "memory" mode (i.e. a "pfn" namespace), the offset > is > + offset. For "fsdax" mode (i.e. a "pfn" namespace), the offset is > relative to the user-visible part of the namespace, and the > offset > introduced by the kernel's metadata will be accounted for. For a > "sector" mode namespace (i.e. a "BTT" namespace), the offset is > diff --git a/Documentation/ndctl/ndctl-list.txt > b/Documentation/ndctl/ndctl-list.txt > index 04affc4..2abc572 100644 > --- a/Documentation/ndctl/ndctl-list.txt > +++ b/Documentation/ndctl/ndctl-list.txt > @@ -49,7 +49,7 @@ EXAMPLE >"namespaces":[ > { >"dev":"namespace0.0", > - "mode":"memory", > + "mode":"fsdax", >"size":8589934592, >"blockdev":"pmem0" > } > @@ -132,11 +132,11 @@ include::xable-region-options.txt[] > -X:: > --device-dax:: > Include device-dax ("daxregion") details when a namespace is in > - "dax" mode. > + "devdax" mode. > [verse] > { >"dev":"namespace0.0", > - "mode":"dax", > + "mode":"devdax", >"size":4225761280, >"uuid":"18ae1bbb-bb62-4efc-86df-4a5caacb5dcc", >"daxregion":{ > diff --git a/ndctl/namespace.c b/ndctl/namespace.c > index f2c5644..fe86d82 100644 > --- a/ndctl/namespace.c > +++ b/ndctl/namespace.c > @@ -103,7 +103,7 @@ OPT_STRING('n', "name", , "name", \ > OPT_STRING('s', "size", , "size", \ > "specify the namespace size in bytes (default: available > capacity)"), \ > OPT_STRING('m', "mode", , "operation-mode", \ > - "specify a mode for the namespace, 'sector', 'memory', or > 'raw'"), \ > + "specify a mode for the namespace, 'sector', 'fsdax', 'devdax' > or 'raw'"), \ > OPT_STRING('M', "map", , "memmap-location", \ > "specify 'mem' or 'dev' for the location of the memmap"), \ > OPT_STRING('l', "sector-size", _size, "lba-size", \ > @@ -533,7 +533,7 @@ static int validate_namespace_options(struct > ndctl_region *region, >* supported a 2M default alignment when >* ndctl_pfn_has_align() returns false. >*/ > - debug("%s not support 'align' for memory > mode\n", > + debug("%s not support 'align' for fsdax mode\n", > region_name); > return -EAGAIN; > } else if (p->mode == NDCTL_NS_MODE_DAX > @@ -542,7 +542,7 @@ static int validate_namespace_options(struct > ndctl_region *region, >* Unlike the pfn case, we require the kernel to >* have 'align' support for device-dax. >*/ > - debug("%s not support 'align' for dax mode\n", > + debug("%s not support 'align' for devdax > mode\n", > region_name); > return -EAGAIN; > } else if (!param.align_default > @@ -696,7 +696,7 @@ static int validate_namespace_options(struct > ndctl_region *region, > > if (ndns && p->mode != NDCTL_NS_MODE_MEMORY > && p->mode != NDCTL_NS_MODE_DAX) { > - debug("%s: --map= only valid for memory mode > namespace\n", > + debug("%s: --map= only
[PATCH v3] ndctl: add support in libndctl to provide deep flush
Providing an API call in libndctl to support accessing the region deep_flush in sysfs. Signed-off-by: Dave Jiang--- v3: - Add "\n" to sysfs write. (Dan) - add O_CLOEXEC to open() call for sysfs. (Dan) v2: Cover case where deep_flush doesn't exist, i.e. memmap=nn!ss. (Dan) ndctl/lib/libndctl.c | 35 +++ ndctl/lib/libndctl.sym |1 + ndctl/libndctl.h |1 + 3 files changed, 37 insertions(+) diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c index 580a450e..03a18985 100644 --- a/ndctl/lib/libndctl.c +++ b/ndctl/lib/libndctl.c @@ -181,6 +181,8 @@ struct ndctl_region { FILE *badblocks; struct badblock bb; enum ndctl_persistence_domain persistence_domain; + /* file descriptor for deep flush sysfs entry */ + int flush_fd; }; /** @@ -511,6 +513,8 @@ static void free_region(struct ndctl_region *region) free(region->region_path); if (region->badblocks) fclose(region->badblocks); + if (region->flush_fd > 0) + close(region->flush_fd); free(region); } @@ -1049,6 +1053,14 @@ NDCTL_EXPORT unsigned long long ndctl_region_get_resource(struct ndctl_region *r return strtoull(buf, NULL, 0); } +NDCTL_EXPORT int ndctl_region_deep_flush(struct ndctl_region *region) +{ + int rc = pwrite(region->flush_fd, "1\n", 1, 0); + + return (rc == -1) ? -errno : 0; +} + + NDCTL_EXPORT const char *ndctl_bus_get_cmd_name(struct ndctl_bus *bus, int cmd) { return nvdimm_bus_cmd_name(cmd); @@ -1791,6 +1803,7 @@ static void *add_region(void *parent, int id, const char *region_base) struct ndctl_bus *bus = parent; struct ndctl_ctx *ctx = bus->ctx; char *path = calloc(1, strlen(region_base) + 100); + int perm; if (!path) return NULL; @@ -1866,6 +1879,28 @@ static void *add_region(void *parent, int id, const char *region_base) else region->persistence_domain = region_get_pd_type(buf); + sprintf(path, "%s/deep_flush", region_base); + region->flush_fd = open(path, O_RDWR | O_CLOEXEC); + if (region->flush_fd == -1) { + /* for those that do not export deep_flush sysfs file */ + if (errno == ENOENT) + goto out; + else + goto err_read; + } + + if (pread(region->flush_fd, buf, 1, 0) == -1) { + close(region->flush_fd); + goto err_read; + } + + perm = strtol(buf, NULL, 0); + if (perm == 0) { + region->flush_fd = -1; + close(region->flush_fd); + } + + out: free(path); return region; diff --git a/ndctl/lib/libndctl.sym b/ndctl/lib/libndctl.sym index 3209aefe..38cc3b9a 100644 --- a/ndctl/lib/libndctl.sym +++ b/ndctl/lib/libndctl.sym @@ -352,4 +352,5 @@ global: ndctl_dimm_fw_update_supported; ndctl_region_get_persistence_domain; ndctl_bus_get_persistence_domain; + ndctl_region_deep_flush; } LIBNDCTL_14; diff --git a/ndctl/libndctl.h b/ndctl/libndctl.h index cf6a77fd..1a622ae5 100644 --- a/ndctl/libndctl.h +++ b/ndctl/libndctl.h @@ -365,6 +365,7 @@ int ndctl_region_enable(struct ndctl_region *region); int ndctl_region_disable_invalidate(struct ndctl_region *region); int ndctl_region_disable_preserve(struct ndctl_region *region); void ndctl_region_cleanup(struct ndctl_region *region); +int ndctl_region_deep_flush(struct ndctl_region *region); struct ndctl_interleave_set; struct ndctl_interleave_set *ndctl_region_get_interleave_set( ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH v2] ndctl: add support in libndctl to provide deep flush
On Tue, Apr 10, 2018 at 3:06 PM, Dave Jiangwrote: > Providing an API call in libndctl to support accessing the region deep_flush > in sysfs. > > Signed-off-by: Dave Jiang > --- > > v2: Cover case where deep_flush doesn't exist, i.e. memmap=nn!ss. (Dan) > > ndctl/lib/libndctl.c | 35 +++ > ndctl/lib/libndctl.sym |1 + > ndctl/libndctl.h |1 + > 3 files changed, 37 insertions(+) > > diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c > index 580a450e..c34f1e09 100644 > --- a/ndctl/lib/libndctl.c > +++ b/ndctl/lib/libndctl.c > @@ -181,6 +181,8 @@ struct ndctl_region { > FILE *badblocks; > struct badblock bb; > enum ndctl_persistence_domain persistence_domain; > + /* file descriptor for deep flush sysfs entry */ > + int flush_fd; > }; > > /** > @@ -511,6 +513,8 @@ static void free_region(struct ndctl_region *region) > free(region->region_path); > if (region->badblocks) > fclose(region->badblocks); > + if (region->flush_fd > 0) > + close(region->flush_fd); > free(region); > } > > @@ -1049,6 +1053,14 @@ NDCTL_EXPORT unsigned long long > ndctl_region_get_resource(struct ndctl_region *r > return strtoull(buf, NULL, 0); > } > > +NDCTL_EXPORT int ndctl_region_deep_flush(struct ndctl_region *region) > +{ > + int rc = pwrite(region->flush_fd, "1", 1, 0); Make this write "1\n" to match standard sysfs manipulation done by "echo" that includes a newline. > + > + return (rc == -1) ? -errno : 0; > +} > + > + > NDCTL_EXPORT const char *ndctl_bus_get_cmd_name(struct ndctl_bus *bus, int > cmd) > { > return nvdimm_bus_cmd_name(cmd); > @@ -1791,6 +1803,7 @@ static void *add_region(void *parent, int id, const > char *region_base) > struct ndctl_bus *bus = parent; > struct ndctl_ctx *ctx = bus->ctx; > char *path = calloc(1, strlen(region_base) + 100); > + int perm; > > if (!path) > return NULL; > @@ -1866,6 +1879,28 @@ static void *add_region(void *parent, int id, const > char *region_base) > else > region->persistence_domain = region_get_pd_type(buf); > > + sprintf(path, "%s/deep_flush", region_base); > + region->flush_fd = open(path, O_RDWR); Make this O_CLOEXEC, so we don't leak fds on exec. ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
[PATCH 5/5] tools/testing/nvdimm: enable labels for nfit_test.1 dimms
Enable test cases for the kernel's fallback to label-less mode. Signed-off-by: Dan Williams--- tools/testing/nvdimm/test/nfit.c |3 +++ 1 file changed, 3 insertions(+) diff --git a/tools/testing/nvdimm/test/nfit.c b/tools/testing/nvdimm/test/nfit.c index 51fc41c24a77..4ea385be528f 100644 --- a/tools/testing/nvdimm/test/nfit.c +++ b/tools/testing/nvdimm/test/nfit.c @@ -2265,6 +2265,9 @@ static void nfit_test1_setup(struct nfit_test *t) set_bit(ND_CMD_ARS_STATUS, _desc->bus_cmd_force_en); set_bit(ND_CMD_CLEAR_ERROR, _desc->bus_cmd_force_en); set_bit(ND_INTEL_ENABLE_LSS_STATUS, _desc->dimm_cmd_force_en); + set_bit(ND_CMD_GET_CONFIG_SIZE, _desc->dimm_cmd_force_en); + set_bit(ND_CMD_GET_CONFIG_DATA, _desc->dimm_cmd_force_en); + set_bit(ND_CMD_SET_CONFIG_DATA, _desc->dimm_cmd_force_en); } static int nfit_test_blk_do_io(struct nd_blk_region *ndbr, resource_size_t dpa, ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
[PATCH 4/5] tools/testing/nvdimm: fix missing newline in nfit_test_dimm 'handle' attribute
Sysfs userspace tooling generally expects the kernel to emit a newlines when reading sysfs attributes. Signed-off-by: Dan Williams--- tools/testing/nvdimm/test/nfit.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/testing/nvdimm/test/nfit.c b/tools/testing/nvdimm/test/nfit.c index c8c88363311b..51fc41c24a77 100644 --- a/tools/testing/nvdimm/test/nfit.c +++ b/tools/testing/nvdimm/test/nfit.c @@ -1193,7 +1193,7 @@ static ssize_t handle_show(struct device *dev, struct device_attribute *attr, if (dimm < 0) return dimm; - return sprintf(buf, "%#x", handle[dimm]); + return sprintf(buf, "%#x\n", handle[dimm]); } DEVICE_ATTR_RO(handle); ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
[PATCH 3/5] tools/testing/nvdimm: support nfit_test_dimm attributes under nfit_test.1
The nfit_test.1 bus provides a pmem topology without blk-aperture enabling, so it presents different failure modes for label space handling. Allow custom DSM command error injection. Signed-off-by: Dan Williams--- tools/testing/nvdimm/test/nfit.c | 43 ++ 1 file changed, 25 insertions(+), 18 deletions(-) diff --git a/tools/testing/nvdimm/test/nfit.c b/tools/testing/nvdimm/test/nfit.c index dc6cf5630280..c8c88363311b 100644 --- a/tools/testing/nvdimm/test/nfit.c +++ b/tools/testing/nvdimm/test/nfit.c @@ -1166,12 +1166,12 @@ static int ars_state_init(struct device *dev, struct ars_state *ars_state) static void put_dimms(void *data) { - struct device **dimm_dev = data; + struct nfit_test *t = data; int i; - for (i = 0; i < NUM_DCR; i++) - if (dimm_dev[i]) - device_unregister(dimm_dev[i]); + for (i = 0; i < t->num_dcr; i++) + if (t->dimm_dev[i]) + device_unregister(t->dimm_dev[i]); } static struct class *nfit_test_dimm; @@ -1180,13 +1180,11 @@ static int dimm_name_to_id(struct device *dev) { int dimm; - if (sscanf(dev_name(dev), "test_dimm%d", ) != 1 - || dimm >= NUM_DCR || dimm < 0) + if (sscanf(dev_name(dev), "test_dimm%d", ) != 1) return -ENXIO; return dimm; } - static ssize_t handle_show(struct device *dev, struct device_attribute *attr, char *buf) { @@ -1259,7 +1257,6 @@ static ssize_t fail_cmd_code_store(struct device *dev, struct device_attribute * } static DEVICE_ATTR_RW(fail_cmd_code); - static struct attribute *nfit_test_dimm_attributes[] = { _attr_fail_cmd.attr, _attr_fail_cmd_code.attr, @@ -1276,6 +1273,23 @@ static const struct attribute_group *nfit_test_dimm_attribute_groups[] = { NULL, }; +static int nfit_test_dimm_init(struct nfit_test *t) +{ + int i; + + if (devm_add_action_or_reset(>pdev.dev, put_dimms, t)) + return -ENOMEM; + for (i = 0; i < t->num_dcr; i++) { + t->dimm_dev[i] = device_create_with_groups(nfit_test_dimm, + >pdev.dev, 0, NULL, + nfit_test_dimm_attribute_groups, + "test_dimm%d", i + t->dcr_idx); + if (!t->dimm_dev[i]) + return -ENOMEM; + } + return 0; +} + static void smart_init(struct nfit_test *t) { int i; @@ -1371,17 +1385,8 @@ static int nfit_test0_alloc(struct nfit_test *t) if (!t->_fit) return -ENOMEM; - if (devm_add_action_or_reset(>pdev.dev, put_dimms, t->dimm_dev)) + if (nfit_test_dimm_init(t)) return -ENOMEM; - for (i = 0; i < NUM_DCR; i++) { - t->dimm_dev[i] = device_create_with_groups(nfit_test_dimm, - >pdev.dev, 0, NULL, - nfit_test_dimm_attribute_groups, - "test_dimm%d", i); - if (!t->dimm_dev[i]) - return -ENOMEM; - } - smart_init(t); return ars_state_init(>pdev.dev, >ars_state); } @@ -1413,6 +1418,8 @@ static int nfit_test1_alloc(struct nfit_test *t) if (!t->spa_set[1]) return -ENOMEM; + if (nfit_test_dimm_init(t)) + return -ENOMEM; smart_init(t); return ars_state_init(>pdev.dev, >ars_state); } ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
[PATCH 2/5] tools/testing/nvdimm: allow custom error code injection
Given that libnvdimm driver stack takes specific actions on DIMM command error codes like -EACCES, provide a facility to inject custom failures. Signed-off-by: Dan Williams--- tools/testing/nvdimm/test/nfit.c | 38 +- 1 file changed, 37 insertions(+), 1 deletion(-) diff --git a/tools/testing/nvdimm/test/nfit.c b/tools/testing/nvdimm/test/nfit.c index cb166be4918d..dc6cf5630280 100644 --- a/tools/testing/nvdimm/test/nfit.c +++ b/tools/testing/nvdimm/test/nfit.c @@ -138,6 +138,7 @@ static u32 handle[] = { }; static unsigned long dimm_fail_cmd_flags[NUM_DCR]; +static int dimm_fail_cmd_code[NUM_DCR]; struct nfit_test_fw { enum intel_fw_update_state state; @@ -892,8 +893,11 @@ static int get_dimm(struct nfit_mem *nfit_mem, unsigned int func) if (i >= ARRAY_SIZE(handle)) return -ENXIO; - if ((1 << func) & dimm_fail_cmd_flags[i]) + if ((1 << func) & dimm_fail_cmd_flags[i]) { + if (dimm_fail_cmd_code[i]) + return dimm_fail_cmd_code[i]; return -EIO; + } return i; } @@ -1225,8 +1229,40 @@ static ssize_t fail_cmd_store(struct device *dev, struct device_attribute *attr, } static DEVICE_ATTR_RW(fail_cmd); +static ssize_t fail_cmd_code_show(struct device *dev, struct device_attribute *attr, + char *buf) +{ + int dimm = dimm_name_to_id(dev); + + if (dimm < 0) + return dimm; + + return sprintf(buf, "%d\n", dimm_fail_cmd_code[dimm]); +} + +static ssize_t fail_cmd_code_store(struct device *dev, struct device_attribute *attr, + const char *buf, size_t size) +{ + int dimm = dimm_name_to_id(dev); + unsigned long val; + ssize_t rc; + + if (dimm < 0) + return dimm; + + rc = kstrtol(buf, 0, ); + if (rc) + return rc; + + dimm_fail_cmd_code[dimm] = val; + return size; +} +static DEVICE_ATTR_RW(fail_cmd_code); + + static struct attribute *nfit_test_dimm_attributes[] = { _attr_fail_cmd.attr, + _attr_fail_cmd_code.attr, _attr_handle.attr, NULL, }; ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
[PATCH 1/5] libnvdimm, dimm: handle EACCES failures from label reads
The new support for the standard _LSR and _LSW methods neglected to also update the nvdimm_init_config_data() and nvdimm_set_config_data() to return the translated error code from failed commands. This precision is necessary because the locked status that was previously returned on ND_CMD_GET_CONFIG_SIZE commands is now returned on ND_CMD_{GET,SET}_CONFIG_DATA commands. If the kernel misses this indication it can inadvertently fall back to label-less mode when it should otherwise avoid all access to locked regions. Cc:Fixes: 4b27db7e26cd ("acpi, nfit: add support for the _LSI, _LSR, and...") Signed-off-by: Dan Williams --- drivers/nvdimm/dimm_devs.c | 22 -- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/drivers/nvdimm/dimm_devs.c b/drivers/nvdimm/dimm_devs.c index e00d45522b80..8d348b22ba45 100644 --- a/drivers/nvdimm/dimm_devs.c +++ b/drivers/nvdimm/dimm_devs.c @@ -88,9 +88,9 @@ int nvdimm_init_nsarea(struct nvdimm_drvdata *ndd) int nvdimm_init_config_data(struct nvdimm_drvdata *ndd) { struct nvdimm_bus *nvdimm_bus = walk_to_nvdimm_bus(ndd->dev); + int rc = validate_dimm(ndd), cmd_rc = 0; struct nd_cmd_get_config_data_hdr *cmd; struct nvdimm_bus_descriptor *nd_desc; - int rc = validate_dimm(ndd); u32 max_cmd_size, config_size; size_t offset; @@ -124,9 +124,11 @@ int nvdimm_init_config_data(struct nvdimm_drvdata *ndd) cmd->in_offset = offset; rc = nd_desc->ndctl(nd_desc, to_nvdimm(ndd->dev), ND_CMD_GET_CONFIG_DATA, cmd, - cmd->in_length + sizeof(*cmd), NULL); - if (rc || cmd->status) { - rc = -ENXIO; + cmd->in_length + sizeof(*cmd), _rc); + if (rc < 0) + break; + if (cmd_rc < 0) { + rc = cmd_rc; break; } memcpy(ndd->data + offset, cmd->out_buf, cmd->in_length); @@ -140,9 +142,9 @@ int nvdimm_init_config_data(struct nvdimm_drvdata *ndd) int nvdimm_set_config_data(struct nvdimm_drvdata *ndd, size_t offset, void *buf, size_t len) { - int rc = validate_dimm(ndd); size_t max_cmd_size, buf_offset; struct nd_cmd_set_config_hdr *cmd; + int rc = validate_dimm(ndd), cmd_rc = 0; struct nvdimm_bus *nvdimm_bus = walk_to_nvdimm_bus(ndd->dev); struct nvdimm_bus_descriptor *nd_desc = nvdimm_bus->nd_desc; @@ -164,7 +166,6 @@ int nvdimm_set_config_data(struct nvdimm_drvdata *ndd, size_t offset, for (buf_offset = 0; len; len -= cmd->in_length, buf_offset += cmd->in_length) { size_t cmd_size; - u32 *status; cmd->in_offset = offset + buf_offset; cmd->in_length = min(max_cmd_size, len); @@ -172,12 +173,13 @@ int nvdimm_set_config_data(struct nvdimm_drvdata *ndd, size_t offset, /* status is output in the last 4-bytes of the command buffer */ cmd_size = sizeof(*cmd) + cmd->in_length + sizeof(u32); - status = ((void *) cmd) + cmd_size - sizeof(u32); rc = nd_desc->ndctl(nd_desc, to_nvdimm(ndd->dev), - ND_CMD_SET_CONFIG_DATA, cmd, cmd_size, NULL); - if (rc || *status) { - rc = rc ? rc : -ENXIO; + ND_CMD_SET_CONFIG_DATA, cmd, cmd_size, _rc); + if (rc < 0) + break; + if (cmd_rc < 0) { + rc = cmd_rc; break; } } ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
[PATCH 0/5] libnvdimm: fix locked status detection
The new ACPI _LSx methods moved the 'dimm locked' error status from the result of ND_CMD_GET_CONFIG_SIZE to an error status result of ND_CMD_GET_CONFIG_DATA. Error code translation prevents the proper error code from making it back to the 'nd_dimm' driver. Fix the error code propagation and add some unit test infrastructure for regression testing this case. --- Dan Williams (5): libnvdimm, dimm: handle EACCES failures from label reads tools/testing/nvdimm: allow custom error code injection tools/testing/nvdimm: support nfit_test_dimm attributes under nfit_test.1 tools/testing/nvdimm: fix missing newline in nfit_test_dimm 'handle' attribute tools/testing/nvdimm: enable labels for nfit_test.1 dimms drivers/nvdimm/dimm_devs.c | 22 +- tools/testing/nvdimm/test/nfit.c | 84 +- 2 files changed, 77 insertions(+), 29 deletions(-) ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
[PATCH v2] ndctl: add support in libndctl to provide deep flush
Providing an API call in libndctl to support accessing the region deep_flush in sysfs. Signed-off-by: Dave Jiang--- v2: Cover case where deep_flush doesn't exist, i.e. memmap=nn!ss. (Dan) ndctl/lib/libndctl.c | 35 +++ ndctl/lib/libndctl.sym |1 + ndctl/libndctl.h |1 + 3 files changed, 37 insertions(+) diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c index 580a450e..c34f1e09 100644 --- a/ndctl/lib/libndctl.c +++ b/ndctl/lib/libndctl.c @@ -181,6 +181,8 @@ struct ndctl_region { FILE *badblocks; struct badblock bb; enum ndctl_persistence_domain persistence_domain; + /* file descriptor for deep flush sysfs entry */ + int flush_fd; }; /** @@ -511,6 +513,8 @@ static void free_region(struct ndctl_region *region) free(region->region_path); if (region->badblocks) fclose(region->badblocks); + if (region->flush_fd > 0) + close(region->flush_fd); free(region); } @@ -1049,6 +1053,14 @@ NDCTL_EXPORT unsigned long long ndctl_region_get_resource(struct ndctl_region *r return strtoull(buf, NULL, 0); } +NDCTL_EXPORT int ndctl_region_deep_flush(struct ndctl_region *region) +{ + int rc = pwrite(region->flush_fd, "1", 1, 0); + + return (rc == -1) ? -errno : 0; +} + + NDCTL_EXPORT const char *ndctl_bus_get_cmd_name(struct ndctl_bus *bus, int cmd) { return nvdimm_bus_cmd_name(cmd); @@ -1791,6 +1803,7 @@ static void *add_region(void *parent, int id, const char *region_base) struct ndctl_bus *bus = parent; struct ndctl_ctx *ctx = bus->ctx; char *path = calloc(1, strlen(region_base) + 100); + int perm; if (!path) return NULL; @@ -1866,6 +1879,28 @@ static void *add_region(void *parent, int id, const char *region_base) else region->persistence_domain = region_get_pd_type(buf); + sprintf(path, "%s/deep_flush", region_base); + region->flush_fd = open(path, O_RDWR); + if (region->flush_fd == -1) { + /* for those that do not export deep_flush sysfs file */ + if (errno == ENOENT) + goto out; + else + goto err_read; + } + + if (pread(region->flush_fd, buf, 1, 0) == -1) { + close(region->flush_fd); + goto err_read; + } + + perm = strtol(buf, NULL, 0); + if (perm == 0) { + region->flush_fd = -1; + close(region->flush_fd); + } + + out: free(path); return region; diff --git a/ndctl/lib/libndctl.sym b/ndctl/lib/libndctl.sym index 3209aefe..38cc3b9a 100644 --- a/ndctl/lib/libndctl.sym +++ b/ndctl/lib/libndctl.sym @@ -352,4 +352,5 @@ global: ndctl_dimm_fw_update_supported; ndctl_region_get_persistence_domain; ndctl_bus_get_persistence_domain; + ndctl_region_deep_flush; } LIBNDCTL_14; diff --git a/ndctl/libndctl.h b/ndctl/libndctl.h index cf6a77fd..1a622ae5 100644 --- a/ndctl/libndctl.h +++ b/ndctl/libndctl.h @@ -365,6 +365,7 @@ int ndctl_region_enable(struct ndctl_region *region); int ndctl_region_disable_invalidate(struct ndctl_region *region); int ndctl_region_disable_preserve(struct ndctl_region *region); void ndctl_region_cleanup(struct ndctl_region *region); +int ndctl_region_deep_flush(struct ndctl_region *region); struct ndctl_interleave_set; struct ndctl_interleave_set *ndctl_region_get_interleave_set( ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
[PATCH] ndctl: add support in libndctl to provide deep flush
Providing an API call in libndctl to support accessing the region deep_flush in sysfs. Signed-off-by: Dave Jiang--- 0 files changed diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c index 580a450e..fb4dca73 100644 --- a/ndctl/lib/libndctl.c +++ b/ndctl/lib/libndctl.c @@ -181,6 +181,8 @@ struct ndctl_region { FILE *badblocks; struct badblock bb; enum ndctl_persistence_domain persistence_domain; + /* file descriptor for deep flush sysfs entry */ + int flush_fd; }; /** @@ -511,6 +513,8 @@ static void free_region(struct ndctl_region *region) free(region->region_path); if (region->badblocks) fclose(region->badblocks); + if (region->flush_fd > 0) + close(region->flush_fd); free(region); } @@ -1049,6 +1053,12 @@ NDCTL_EXPORT unsigned long long ndctl_region_get_resource(struct ndctl_region *r return strtoull(buf, NULL, 0); } +NDCTL_EXPORT int ndctl_region_deep_flush(struct ndctl_region *region) +{ + return pwrite(region->flush_fd, "1", 1, 0); +} + + NDCTL_EXPORT const char *ndctl_bus_get_cmd_name(struct ndctl_bus *bus, int cmd) { return nvdimm_bus_cmd_name(cmd); @@ -1791,6 +1801,7 @@ static void *add_region(void *parent, int id, const char *region_base) struct ndctl_bus *bus = parent; struct ndctl_ctx *ctx = bus->ctx; char *path = calloc(1, strlen(region_base) + 100); + int perm; if (!path) return NULL; @@ -1866,6 +1877,22 @@ static void *add_region(void *parent, int id, const char *region_base) else region->persistence_domain = region_get_pd_type(buf); + sprintf(path, "%s/deep_flush", region_base); + region->flush_fd = open(path, O_RDWR); + if (region->flush_fd == -1) + goto err_read; + + if (pread(region->flush_fd, buf, 1, 0) == -1) { + close(region->flush_fd); + goto err_read; + } + + perm = strtol(buf, NULL, 0); + if (perm == 0) { + region->flush_fd = -1; + close(region->flush_fd); + } + free(path); return region; diff --git a/ndctl/lib/libndctl.sym b/ndctl/lib/libndctl.sym index 3209aefe..38cc3b9a 100644 --- a/ndctl/lib/libndctl.sym +++ b/ndctl/lib/libndctl.sym @@ -352,4 +352,5 @@ global: ndctl_dimm_fw_update_supported; ndctl_region_get_persistence_domain; ndctl_bus_get_persistence_domain; + ndctl_region_deep_flush; } LIBNDCTL_14; diff --git a/ndctl/libndctl.h b/ndctl/libndctl.h index cf6a77fd..1a622ae5 100644 --- a/ndctl/libndctl.h +++ b/ndctl/libndctl.h @@ -365,6 +365,7 @@ int ndctl_region_enable(struct ndctl_region *region); int ndctl_region_disable_invalidate(struct ndctl_region *region); int ndctl_region_disable_preserve(struct ndctl_region *region); void ndctl_region_cleanup(struct ndctl_region *region); +int ndctl_region_deep_flush(struct ndctl_region *region); struct ndctl_interleave_set; struct ndctl_interleave_set *ndctl_region_get_interleave_set( ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
[PATCH v2] libnvdimm: fix NULL ptr access in nvdimm_flush when region is disabled
When a region is disabled, there is no driver attached. Therefore dev->driverdata is NULL. An attempt to write to regionN/deep_flush via sysfs would cause a NULL pointer dereference. Bail when dev->driver is NULL to protect this scenario. Fix: ab630891ce0eb(libnvdimm, region: sysfs trigger for nvdimm_flush()) Signed-off-by: Dave Jiang--- v2: Move to deep_flush_store. (Dan) drivers/nvdimm/region_devs.c |5 + 1 file changed, 5 insertions(+) diff --git a/drivers/nvdimm/region_devs.c b/drivers/nvdimm/region_devs.c index a612be6f019d..65cc2a5b48b8 100644 --- a/drivers/nvdimm/region_devs.c +++ b/drivers/nvdimm/region_devs.c @@ -290,6 +290,11 @@ static ssize_t deep_flush_store(struct device *dev, struct device_attribute *att return rc; if (!flush) return -EINVAL; + + /* protect against disabled region */ + if (!nd_region->dev.driver) + return -ENXIO; + nvdimm_flush(nd_region); return len; ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH] libnvdimm: fix NULL ptr access in nvdimm_flush when region is disabled
On Tue, Apr 10, 2018 at 1:42 PM, Dave Jiangwrote: > When a region is disabled, there is no driver attached. Therefore > dev->driverdata is NULL. An attempt to write to regionN/deep_flush via sysfs > would cause a NULL pointer dereference. Bail when dev->driver is NULL to > protect this scenario. > > Fix: ab630891ce0eb(libnvdimm, region: sysfs trigger for nvdimm_flush()) > > Signed-off-by: Dave Jiang > --- > drivers/nvdimm/region_devs.c |4 > 1 file changed, 4 insertions(+) > > diff --git a/drivers/nvdimm/region_devs.c b/drivers/nvdimm/region_devs.c > index a612be6f019d..d5619b7feb6a 100644 > --- a/drivers/nvdimm/region_devs.c > +++ b/drivers/nvdimm/region_devs.c > @@ -1074,6 +1074,10 @@ void nvdimm_flush(struct nd_region *nd_region) > struct nd_region_data *ndrd = dev_get_drvdata(_region->dev); > int i, idx; > > + /* protect against disabled region */ > + if (!nd_region->dev.driver) > + return; > + Move this to deep_flush_store(). That's the only caller that can trigger nvdimm_flush() while the region might be disabled, and that needs to return an error code. ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
[PATCH] libnvdimm: fix NULL ptr access in nvdimm_flush when region is disabled
When a region is disabled, there is no driver attached. Therefore dev->driverdata is NULL. An attempt to write to regionN/deep_flush via sysfs would cause a NULL pointer dereference. Bail when dev->driver is NULL to protect this scenario. Fix: ab630891ce0eb(libnvdimm, region: sysfs trigger for nvdimm_flush()) Signed-off-by: Dave Jiang--- drivers/nvdimm/region_devs.c |4 1 file changed, 4 insertions(+) diff --git a/drivers/nvdimm/region_devs.c b/drivers/nvdimm/region_devs.c index a612be6f019d..d5619b7feb6a 100644 --- a/drivers/nvdimm/region_devs.c +++ b/drivers/nvdimm/region_devs.c @@ -1074,6 +1074,10 @@ void nvdimm_flush(struct nd_region *nd_region) struct nd_region_data *ndrd = dev_get_drvdata(_region->dev); int i, idx; + /* protect against disabled region */ + if (!nd_region->dev.driver) + return; + /* * Try to encourage some diversity in flush hint addresses * across cpus assuming a limited number of flush hints. ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH v2] ndctl: fix libdaxctl memory leak
On 04/10/2018 10:38 AM, Plewa, Lukasz wrote: > On Tue, Apr 10, 2018 at 7:17 PM, Dave Jiangwrote: >> When daxctl_unref is releasing the context, we should make sure that the >> regions and devices are also being released. free_region() will free all the >> devices under the region. >> >> Signed-off-by: Dave Jiang >> --- >> >> v2: Use list_for_each_safe() for region removal. (Dan) >> >> daxctl/lib/libdaxctl.c |8 >> 1 file changed, 8 insertions(+) >> >> diff --git a/daxctl/lib/libdaxctl.c b/daxctl/lib/libdaxctl.c index >> 9e503201..22f4210a 100644 >> --- a/daxctl/lib/libdaxctl.c >> +++ b/daxctl/lib/libdaxctl.c >> @@ -29,6 +29,8 @@ >> >> static const char *attrs = "dax_region"; >> >> +static void free_region(struct daxctl_region *region, struct list_head >> +*head); >> + >> /** >> * struct daxctl_ctx - library user context to find "nd" instances >> * >> @@ -119,11 +121,17 @@ DAXCTL_EXPORT struct daxctl_ctx >> *daxctl_ref(struct daxctl_ctx *ctx) >> */ >> DAXCTL_EXPORT void daxctl_unref(struct daxctl_ctx *ctx) { >> +struct daxctl_region *region, *_r; >> + >> if (ctx == NULL) >> return; >> ctx->refcount--; >> if (ctx->refcount > 0) >> return; >> + >> +list_for_each_safe(>regions, region, _r, list) >> +free_region(region, >regions); >> + >> info(ctx, "context %p released\n", ctx); >> free(ctx); >> } > > As daxctl_region has its own refcount, you need daxctl_ref() in > add_dax_region() and daxctl_unref() in free_region().( or daxctl_ref() in > daxctl_region_ref() and daxctl_unref() in daxctl_region_unref()) > > Without it, this code will cause segfault: > > daxctl_new(); > region = daxctl_new_region(...); > daxctl_region_ref(region); > daxctl_unref(ctx); > daxctl_region_unref(region); Shouldn't it go in this order: daxctl_region_unref(region); daxctl_unref(ctx); In this case, you won't segfault right? I think ctx should be the very last thing you free. > > Ćukasz > ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
[PATCH v2] ndctl: fix libdaxctl memory leak
When daxctl_unref is releasing the context, we should make sure that the regions and devices are also being released. free_region() will free all the devices under the region. Signed-off-by: Dave Jiang--- v2: Use list_for_each_safe() for region removal. (Dan) daxctl/lib/libdaxctl.c |8 1 file changed, 8 insertions(+) diff --git a/daxctl/lib/libdaxctl.c b/daxctl/lib/libdaxctl.c index 9e503201..22f4210a 100644 --- a/daxctl/lib/libdaxctl.c +++ b/daxctl/lib/libdaxctl.c @@ -29,6 +29,8 @@ static const char *attrs = "dax_region"; +static void free_region(struct daxctl_region *region, struct list_head *head); + /** * struct daxctl_ctx - library user context to find "nd" instances * @@ -119,11 +121,17 @@ DAXCTL_EXPORT struct daxctl_ctx *daxctl_ref(struct daxctl_ctx *ctx) */ DAXCTL_EXPORT void daxctl_unref(struct daxctl_ctx *ctx) { + struct daxctl_region *region, *_r; + if (ctx == NULL) return; ctx->refcount--; if (ctx->refcount > 0) return; + + list_for_each_safe(>regions, region, _r, list) + free_region(region, >regions); + info(ctx, "context %p released\n", ctx); free(ctx); } ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
[ndctl PATCH v2] ndctl: complete move to "fsdax" and "devdax"
Add on to the work started by: commit ebb4fb605e68 ("ndctl, create-namespace: introduce "fsdax" and "devdax" modes") and change some more user visible places to use "fsdax" and "devdax" modes instead of "memory" and "dax", respectively. Having multiple terms for the same mode is confusing for users. We will continue to accept "memory" and "dax" as parameters, but all output and man pages will now use the updated terms. Note that after the above referenced commit we still printed the old names in the default 'ndctl list' output for backward compatibility with scripts. This patch intentionally breaks that backward compatibility in favor of avoiding confusion and using the new mode names everywhere. Signed-off-by: Ross Zwisler--- Documentation/ndctl/ndctl-inject-error.txt | 2 +- Documentation/ndctl/ndctl-list.txt | 6 +++--- ndctl/namespace.c | 16 util/json.c| 10 ++ 4 files changed, 14 insertions(+), 20 deletions(-) diff --git a/Documentation/ndctl/ndctl-inject-error.txt b/Documentation/ndctl/ndctl-inject-error.txt index 01f6c22..94c4e69 100644 --- a/Documentation/ndctl/ndctl-inject-error.txt +++ b/Documentation/ndctl/ndctl-inject-error.txt @@ -45,7 +45,7 @@ OPTIONS NOTE: The offset is interpreted in different ways based on the "mode" of the namespace. For "raw" mode, the offset is the base namespace - offset. For "memory" mode (i.e. a "pfn" namespace), the offset is + offset. For "fsdax" mode (i.e. a "pfn" namespace), the offset is relative to the user-visible part of the namespace, and the offset introduced by the kernel's metadata will be accounted for. For a "sector" mode namespace (i.e. a "BTT" namespace), the offset is diff --git a/Documentation/ndctl/ndctl-list.txt b/Documentation/ndctl/ndctl-list.txt index 04affc4..2abc572 100644 --- a/Documentation/ndctl/ndctl-list.txt +++ b/Documentation/ndctl/ndctl-list.txt @@ -49,7 +49,7 @@ EXAMPLE "namespaces":[ { "dev":"namespace0.0", - "mode":"memory", + "mode":"fsdax", "size":8589934592, "blockdev":"pmem0" } @@ -132,11 +132,11 @@ include::xable-region-options.txt[] -X:: --device-dax:: Include device-dax ("daxregion") details when a namespace is in - "dax" mode. + "devdax" mode. [verse] { "dev":"namespace0.0", - "mode":"dax", + "mode":"devdax", "size":4225761280, "uuid":"18ae1bbb-bb62-4efc-86df-4a5caacb5dcc", "daxregion":{ diff --git a/ndctl/namespace.c b/ndctl/namespace.c index f2c5644..fe86d82 100644 --- a/ndctl/namespace.c +++ b/ndctl/namespace.c @@ -103,7 +103,7 @@ OPT_STRING('n', "name", , "name", \ OPT_STRING('s', "size", , "size", \ "specify the namespace size in bytes (default: available capacity)"), \ OPT_STRING('m', "mode", , "operation-mode", \ - "specify a mode for the namespace, 'sector', 'memory', or 'raw'"), \ + "specify a mode for the namespace, 'sector', 'fsdax', 'devdax' or 'raw'"), \ OPT_STRING('M', "map", , "memmap-location", \ "specify 'mem' or 'dev' for the location of the memmap"), \ OPT_STRING('l', "sector-size", _size, "lba-size", \ @@ -533,7 +533,7 @@ static int validate_namespace_options(struct ndctl_region *region, * supported a 2M default alignment when * ndctl_pfn_has_align() returns false. */ - debug("%s not support 'align' for memory mode\n", + debug("%s not support 'align' for fsdax mode\n", region_name); return -EAGAIN; } else if (p->mode == NDCTL_NS_MODE_DAX @@ -542,7 +542,7 @@ static int validate_namespace_options(struct ndctl_region *region, * Unlike the pfn case, we require the kernel to * have 'align' support for device-dax. */ - debug("%s not support 'align' for dax mode\n", + debug("%s not support 'align' for devdax mode\n", region_name); return -EAGAIN; } else if (!param.align_default @@ -696,7 +696,7 @@ static int validate_namespace_options(struct ndctl_region *region, if (ndns && p->mode != NDCTL_NS_MODE_MEMORY && p->mode != NDCTL_NS_MODE_DAX) { - debug("%s: --map= only valid for memory mode namespace\n", + debug("%s: --map= only valid for fsdax mode namespace\n", ndctl_namespace_get_devname(ndns)); return -EINVAL; } @@ -709,10 +709,10 @@ static int validate_namespace_options(struct ndctl_region *region, struct ndctl_pfn *pfn =
Re: [PATCH] ndctl: fix libdaxctl memory leak
On 04/10/2018 10:01 AM, Dan Williams wrote: > On Tue, Apr 10, 2018 at 9:56 AM, Dave Jiangwrote: >> When daxctl_unref is releasing the context, we should make sure that the >> regions and devices are also being released. >> >> Signed-off-by: Dave Jiang >> --- >> daxctl/lib/libdaxctl.c |7 +++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/daxctl/lib/libdaxctl.c b/daxctl/lib/libdaxctl.c >> index 9e503201..0552f6d7 100644 >> --- a/daxctl/lib/libdaxctl.c >> +++ b/daxctl/lib/libdaxctl.c >> @@ -119,11 +119,18 @@ DAXCTL_EXPORT struct daxctl_ctx *daxctl_ref(struct >> daxctl_ctx *ctx) >> */ >> DAXCTL_EXPORT void daxctl_unref(struct daxctl_ctx *ctx) >> { >> + struct daxctl_region *region; >> + >> if (ctx == NULL) >> return; >> ctx->refcount--; >> if (ctx->refcount > 0) >> return; >> + >> + while ((region = list_top(>regions, struct daxctl_region, >> list)) != >> + NULL) >> + daxctl_region_unref(region); >> + > > Sorry, should have mentioned this before. Why not use > list_for_each_entry_safe() to iterate and delete regions like all the > other free routines? > Somehow I missed that even though I was looking for it. Will fix. ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH] ndctl: fix libdaxctl memory leak
On Tue, Apr 10, 2018 at 9:56 AM, Dave Jiangwrote: > When daxctl_unref is releasing the context, we should make sure that the > regions and devices are also being released. > > Signed-off-by: Dave Jiang > --- > daxctl/lib/libdaxctl.c |7 +++ > 1 file changed, 7 insertions(+) > > diff --git a/daxctl/lib/libdaxctl.c b/daxctl/lib/libdaxctl.c > index 9e503201..0552f6d7 100644 > --- a/daxctl/lib/libdaxctl.c > +++ b/daxctl/lib/libdaxctl.c > @@ -119,11 +119,18 @@ DAXCTL_EXPORT struct daxctl_ctx *daxctl_ref(struct > daxctl_ctx *ctx) > */ > DAXCTL_EXPORT void daxctl_unref(struct daxctl_ctx *ctx) > { > + struct daxctl_region *region; > + > if (ctx == NULL) > return; > ctx->refcount--; > if (ctx->refcount > 0) > return; > + > + while ((region = list_top(>regions, struct daxctl_region, list)) > != > + NULL) > + daxctl_region_unref(region); > + Sorry, should have mentioned this before. Why not use list_for_each_entry_safe() to iterate and delete regions like all the other free routines? ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH] ndctl: fix daxctl list memory leak
On Tue, Apr 10, 2018 at 8:59 AM, Dave Jiangwrote: > > > On 04/09/2018 07:39 PM, Dan Williams wrote: >> On Mon, Apr 9, 2018 at 5:13 PM, Dave Jiang wrote: >>> daxctl list is not calling daxctl_unref() when executed succesfully. At the >>> same >>> time, daxctl_region_unref() is not being called when daxctl_unref() >>> executes. >>> Valgrind is reporting unfreed memory. Adding the appropriate calls to make >>> sure >>> all memory allocated are freed for daxctl list. >>> >>> Signed-off-by: Dave Jiang >>> --- >>> daxctl/lib/libdaxctl.c |7 +++ >>> daxctl/list.c |1 + >>> 2 files changed, 8 insertions(+) >>> >>> diff --git a/daxctl/lib/libdaxctl.c b/daxctl/lib/libdaxctl.c >>> index 9e503201..0552f6d7 100644 >>> --- a/daxctl/lib/libdaxctl.c >>> +++ b/daxctl/lib/libdaxctl.c >>> @@ -119,11 +119,18 @@ DAXCTL_EXPORT struct daxctl_ctx *daxctl_ref(struct >>> daxctl_ctx *ctx) >>> */ >>> DAXCTL_EXPORT void daxctl_unref(struct daxctl_ctx *ctx) >>> { >>> + struct daxctl_region *region; >>> + >>> if (ctx == NULL) >>> return; >>> ctx->refcount--; >>> if (ctx->refcount > 0) >>> return; >>> + >>> + while ((region = list_top(>regions, struct daxctl_region, >>> list)) != >>> + NULL) >>> + daxctl_region_unref(region); >>> + >>> info(ctx, "context %p released\n", ctx); >>> free(ctx); >>> } >>> diff --git a/daxctl/list.c b/daxctl/list.c >>> index 254f0ac9..9b18ae8d 100644 >>> --- a/daxctl/list.c >>> +++ b/daxctl/list.c >>> @@ -134,6 +134,7 @@ int cmd_list(int argc, const char **argv, void *ctx) >>> else if (jdevs) >>> util_display_json_array(stdout, jdevs, jflag); >>> >>> + daxctl_unref(ctx); >> >> This should move to daxctl/daxctl.c similar to the ndctl_unref() in >> ndctl/ndctl.c >> > With the way things are coded right now that's the exit function and we > can't have it in daxctl.c. main_handle_internal_commands gets called and > that invokes exit() on the matched commands. We never return from that. Then we need to fix that. cmd_list() should not be releasing references that it did not take. How is daxctl::cmd_list() leaky but ndctl::cmd_list() is not? ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH] ndctl: fix daxctl list memory leak
On 04/09/2018 07:39 PM, Dan Williams wrote: > On Mon, Apr 9, 2018 at 5:13 PM, Dave Jiangwrote: >> daxctl list is not calling daxctl_unref() when executed succesfully. At the >> same >> time, daxctl_region_unref() is not being called when daxctl_unref() executes. >> Valgrind is reporting unfreed memory. Adding the appropriate calls to make >> sure >> all memory allocated are freed for daxctl list. >> >> Signed-off-by: Dave Jiang >> --- >> daxctl/lib/libdaxctl.c |7 +++ >> daxctl/list.c |1 + >> 2 files changed, 8 insertions(+) >> >> diff --git a/daxctl/lib/libdaxctl.c b/daxctl/lib/libdaxctl.c >> index 9e503201..0552f6d7 100644 >> --- a/daxctl/lib/libdaxctl.c >> +++ b/daxctl/lib/libdaxctl.c >> @@ -119,11 +119,18 @@ DAXCTL_EXPORT struct daxctl_ctx *daxctl_ref(struct >> daxctl_ctx *ctx) >> */ >> DAXCTL_EXPORT void daxctl_unref(struct daxctl_ctx *ctx) >> { >> + struct daxctl_region *region; >> + >> if (ctx == NULL) >> return; >> ctx->refcount--; >> if (ctx->refcount > 0) >> return; >> + >> + while ((region = list_top(>regions, struct daxctl_region, >> list)) != >> + NULL) >> + daxctl_region_unref(region); >> + >> info(ctx, "context %p released\n", ctx); >> free(ctx); >> } >> diff --git a/daxctl/list.c b/daxctl/list.c >> index 254f0ac9..9b18ae8d 100644 >> --- a/daxctl/list.c >> +++ b/daxctl/list.c >> @@ -134,6 +134,7 @@ int cmd_list(int argc, const char **argv, void *ctx) >> else if (jdevs) >> util_display_json_array(stdout, jdevs, jflag); >> >> + daxctl_unref(ctx); > > This should move to daxctl/daxctl.c similar to the ndctl_unref() in > ndctl/ndctl.c > With the way things are coded right now that's the exit function and we can't have it in daxctl.c. main_handle_internal_commands gets called and that invokes exit() on the matched commands. We never return from that. ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH v3 1/3] resource: Use list_head to link resource sibling
Hi Rob, Thanks a lot for looking into this and involve Nico to this thread! On 04/09/18 at 09:49am, Rob Herring wrote: > +Nico who has been working on tinification of the kernel. > > On Mon, Apr 9, 2018 at 4:08 AM, Baoquan Hewrote: > > The struct resource uses singly linked list to link siblings. It's not > > easy to do reverse iteration on sibling list. So replace it with list_head. > > Why is reverse iteration needed? This is the explanation I made when Andrew helped to review the v1 post: https://lkml.org/lkml/2018/3/23/78 Because we have been using kexec-tools utility to search available System RAM space for loading kernel/initrd/purgatory from top to down. That is done in user space by searching /proc/iomem. While later added kexec_file interface, the searching code happened in kernel, and it only search System RAM region bottom up, then take an area in that found RAM region from top to down. We need unify these two interfaces on behaviour since they are the same on essense from the users' point of view, though implementation is different. As you know, the singly linked list implementation of the current resource's sibling linking, makes the searching from top to down very hard to satisfy people. Below is the v1 post, we make an temporary array to copy iomem_resource's first level of children, then iterate the array reversedly. Andrew suggested me to try list_head after reviewing. In fact we can optimize that patch to only copy resource pointer into array, still the way is ugly. https://lkml.org/lkml/2018/3/21/952 Then Wei pasted a patch he had made as below. He didn't mention if he also has requirement on reversed iteration of resource. That is an O(n*n) way, from personal feelings, hard to say if it's bettern than v1 post. https://lkml.org/lkml/2018/3/24/157 That's why I would like to have a try of the list_head linking. > > > And code refactoring makes codes in kernel/resource.c more readable than > > pointer operation. > > resource_for_each_* helpers could solve that without the size increase. Nico mentioned llist, that is a linux kernel standard singly linked list, very elegant code existed. Still can not satisfy the reversed iteration requirement. > > > Besides, type of member variables of struct resource, sibling and child, are > > changed from 'struct resource *' to 'struct list_head'. Kernel size will > > increase because of those statically defined struct resource instances. > > The DT struct device_node also has the same tree structure with > parent, child, sibling pointers and converting to list_head had been > on the todo list for a while. ACPI also has some tree walking > functions (drivers/acpi/acpica/pstree.c). Perhaps there should be a > common tree struct and helpers defined either on top of list_head or a > new struct if that saves some size. We have had many this kind of trees in kernel, the obvious examples is task_struct. And struct task_group {} too. They have used list_head already. struct task_struct { ... /* Real parent process: */ struct task_struct __rcu*real_parent; /* Recipient of SIGCHLD, wait4() reports: */ struct task_struct __rcu*parent; struct list_headchildren; struct list_headsibling; ... } root /// | \\\ ///|\\\ /// | \\\ /// | \\\ (child)<->(child)<->(child) /// | \\\ /// | \\\ /// | \\\ ///| \\\ (grandchild)<->(grandchild)<->(grandchild) If define an new struct, , like tree_list, or tlist? struct tlist { void*parent; struct list_headsibling; struct list_headchild; } Just a rough idea, feel it might not bring too much benefit compared with direct list operation. > > > > > Signed-off-by: Baoquan He > > --- > > v2->v3: > > Make sibling() and first_child() global so that they can be called > > out of kernel/resource.c to simplify code. > > These should probably be inline functions. Or exported if not. > > > > > Fix several code bugs found by kbuild test robot. > > > > Got report from lkp that kernel size increased. It's on purpose since > > the type change of sibling and