* [ndctl PATCH 1/2] ndctl/namespace: remove open coded is_namespace_active() @ 2019-11-01 20:27 Vishal Verma 2019-11-01 20:27 ` [ndctl PATCH 2/2] ndctl/namespace: introduce ndctl_namespace_is_configurable() Vishal Verma 0 siblings, 1 reply; 6+ messages in thread From: Vishal Verma @ 2019-11-01 20:27 UTC (permalink / raw) To: linux-nvdimm; +Cc: Aneesh Kumar K.V Remove the open coded namespace active check with the one provided by libndctl - ndctl_namespace_is_active(). is_namespace_active() additionally checked for a non-NULL 'ndns', which the libndctl API doesn't do. However, all the callers either performed that check themselves, or made prior assumptions about ndns being valid by dereferencing it earlier. Cc: Dan Williams <dan.j.williams@intel.com> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com> --- ndctl/namespace.c | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/ndctl/namespace.c b/ndctl/namespace.c index 7fb0007..ccd46d0 100644 --- a/ndctl/namespace.c +++ b/ndctl/namespace.c @@ -453,14 +453,6 @@ static int setup_namespace(struct ndctl_region *region, return rc; } -static int is_namespace_active(struct ndctl_namespace *ndns) -{ - return ndns && (ndctl_namespace_is_enabled(ndns) - || ndctl_namespace_get_pfn(ndns) - || ndctl_namespace_get_dax(ndns) - || ndctl_namespace_get_btt(ndns)); -} - /* * validate_namespace_options - init parameters for setup_namespace * @region: parent of the namespace to create / reconfigure @@ -787,7 +779,7 @@ static struct ndctl_namespace *region_get_namespace(struct ndctl_region *region) /* prefer the 0th namespace if it is idle */ ndctl_namespace_foreach(region, ndns) if (ndctl_namespace_get_id(ndns) == 0 - && !is_namespace_active(ndns)) + && !ndctl_namespace_is_active(ndns)) return ndns; return ndctl_region_get_namespace_seed(region); } @@ -827,7 +819,7 @@ static int namespace_create(struct ndctl_region *region) p.size = available; ndns = region_get_namespace(region); - if (!ndns || is_namespace_active(ndns)) { + if (!ndns || ndctl_namespace_is_active(ndns)) { debug("%s: no %s namespace seed\n", devname, ndns ? "idle" : "available"); return -EAGAIN; @@ -1074,7 +1066,7 @@ static int namespace_reconfig(struct ndctl_region *region, } ndns = region_get_namespace(region); - if (!ndns || is_namespace_active(ndns)) { + if (!ndns || !ndctl_namespace_is_active(ndns)) { debug("%s: no %s namespace seed\n", ndctl_region_get_devname(region), ndns ? "idle" : "available"); -- 2.20.1 _______________________________________________ Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org To unsubscribe send an email to linux-nvdimm-leave@lists.01.org ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [ndctl PATCH 2/2] ndctl/namespace: introduce ndctl_namespace_is_configurable() 2019-11-01 20:27 [ndctl PATCH 1/2] ndctl/namespace: remove open coded is_namespace_active() Vishal Verma @ 2019-11-01 20:27 ` Vishal Verma 2019-11-04 16:35 ` Dan Williams 0 siblings, 1 reply; 6+ messages in thread From: Vishal Verma @ 2019-11-01 20:27 UTC (permalink / raw) To: linux-nvdimm; +Cc: Aneesh Kumar K.V The motivation for this change is that we want to refrain from (re)configuring what appear to be partially configured namespaces. Namespaces may end up in a state that looks partially configured when the kernel isn't able to enable a namespace due to mismatched PAGE_SIZE expectations. In such cases, ndctl should not treat those as unconfigured 'seed' namespaces, and reuse them. Add a new ndctl_namespace_is_configurable API, whish tests whether a namespaces is active, and whether it is partially configured. If neither are true, then it can be used for (re)configuration. Additionally, deal with the corner case of ND_DEVICE_NAMESPACE_IO (legacy PMEM) namespaces by testing whether the size attribute is read-only (which indicates such a namespace). Legacy namespaces always appear as configured, since their size cannot be changed, but they are also always re-configurable. Use this API in namespace_reconfig() and namespace_create() to enable this partial configuration detection. Cc: Dan Williams <dan.j.williams@intel.com> Reported-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com> --- Hi Aneesh - could you please test these patches and see if they work for the pfn_sb PAGE_SIZE mismatch scenario? ndctl/lib/libndctl.c | 37 +++++++++++++++++++++++++++++++++++++ ndctl/lib/libndctl.sym | 4 ++++ ndctl/libndctl.h | 1 + ndctl/namespace.c | 6 +++--- util/sysfs.c | 18 ++++++++++++++++++ util/sysfs.h | 3 +++ 6 files changed, 66 insertions(+), 3 deletions(-) diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c index 6596f94..1674cf9 100644 --- a/ndctl/lib/libndctl.c +++ b/ndctl/lib/libndctl.c @@ -3985,6 +3985,21 @@ static void region_refresh_children(struct ndctl_region *region) daxs_init(region); } +static bool namespace_size_is_writeable(struct ndctl_namespace *ndns) +{ + struct ndctl_ctx *ctx = ndctl_namespace_get_ctx(ndns); + char *path = ndns->ndns_buf; + int len = ndns->buf_len; + + if (snprintf(path, len, "%s/size", ndns->ndns_path) >= len) { + err(ctx, "%s: buffer too small!\n", + ndctl_namespace_get_devname(ndns)); + return false; + } + + return sysfs_attr_writable(ctx, path); +} + NDCTL_EXPORT bool ndctl_namespace_is_active(struct ndctl_namespace *ndns) { struct ndctl_btt *btt = ndctl_namespace_get_btt(ndns); @@ -4182,6 +4197,28 @@ NDCTL_EXPORT int ndctl_namespace_is_configured(struct ndctl_namespace *ndns) } } +/* + * Check if a given 'seed' namespace is ok to configure. + * If a size or uuid is present, it is considered not configurable, except + * in the case of legacy (ND_DEVICE_NAMESPACE_IO) namespaces. In that case, + * the size is never zero, but the namespace can still be reconfigured. + * Detect this by testing whether the size is writable. If it is not writable, + * then it is ok to (re)configure. In other words, legacy namespaces are always + * 'configured', but they are also always 'configurable'. + */ +NDCTL_EXPORT int ndctl_namespace_is_configurable(struct ndctl_namespace *ndns) +{ + if (ndctl_namespace_is_active(ndns)) + return 0; + if (ndctl_namespace_is_configured(ndns)) { + if (!namespace_size_is_writeable(ndns)) + return 1; + return 0; + } + /* !active and !configured is configurable */ + return 1; +} + NDCTL_EXPORT void ndctl_namespace_get_uuid(struct ndctl_namespace *ndns, uuid_t uu) { memcpy(uu, ndns->uuid, sizeof(uuid_t)); diff --git a/ndctl/lib/libndctl.sym b/ndctl/lib/libndctl.sym index c93c1ee..835d36b 100644 --- a/ndctl/lib/libndctl.sym +++ b/ndctl/lib/libndctl.sym @@ -423,3 +423,7 @@ LIBNDCTL_21 { LIBNDCTL_22 { ndctl_dimm_security_is_frozen; } LIBNDCTL_21; + +LIBNDCTL_23 { + ndctl_namespace_is_configurable; +} LIBNDCTL_22; diff --git a/ndctl/libndctl.h b/ndctl/libndctl.h index db398b3..e4dd22f 100644 --- a/ndctl/libndctl.h +++ b/ndctl/libndctl.h @@ -491,6 +491,7 @@ int ndctl_namespace_disable_safe(struct ndctl_namespace *ndns); bool ndctl_namespace_is_active(struct ndctl_namespace *ndns); int ndctl_namespace_is_valid(struct ndctl_namespace *ndns); int ndctl_namespace_is_configured(struct ndctl_namespace *ndns); +int ndctl_namespace_is_configurable(struct ndctl_namespace *ndns); int ndctl_namespace_delete(struct ndctl_namespace *ndns); int ndctl_namespace_set_uuid(struct ndctl_namespace *ndns, uuid_t uu); void ndctl_namespace_get_uuid(struct ndctl_namespace *ndns, uuid_t uu); diff --git a/ndctl/namespace.c b/ndctl/namespace.c index ccd46d0..2979e78 100644 --- a/ndctl/namespace.c +++ b/ndctl/namespace.c @@ -779,7 +779,7 @@ static struct ndctl_namespace *region_get_namespace(struct ndctl_region *region) /* prefer the 0th namespace if it is idle */ ndctl_namespace_foreach(region, ndns) if (ndctl_namespace_get_id(ndns) == 0 - && !ndctl_namespace_is_active(ndns)) + && ndctl_namespace_is_configurable(ndns)) return ndns; return ndctl_region_get_namespace_seed(region); } @@ -819,7 +819,7 @@ static int namespace_create(struct ndctl_region *region) p.size = available; ndns = region_get_namespace(region); - if (!ndns || ndctl_namespace_is_active(ndns)) { + if (!ndns || !ndctl_namespace_is_configurable(ndns)) { debug("%s: no %s namespace seed\n", devname, ndns ? "idle" : "available"); return -EAGAIN; @@ -1066,7 +1066,7 @@ static int namespace_reconfig(struct ndctl_region *region, } ndns = region_get_namespace(region); - if (!ndns || !ndctl_namespace_is_active(ndns)) { + if (!ndns || !ndctl_namespace_is_configurable(ndns)) { debug("%s: no %s namespace seed\n", ndctl_region_get_devname(region), ndns ? "idle" : "available"); diff --git a/util/sysfs.c b/util/sysfs.c index 9f7bc1f..773eeff 100644 --- a/util/sysfs.c +++ b/util/sysfs.c @@ -20,6 +20,7 @@ #include <ctype.h> #include <fcntl.h> #include <dirent.h> +#include <stdbool.h> #include <sys/stat.h> #include <sys/types.h> #include <sys/ioctl.h> @@ -84,6 +85,23 @@ int __sysfs_write_attr_quiet(struct log_ctx *ctx, const char *path, return write_attr(ctx, path, buf, 1); } +bool __sysfs_attr_writable(struct log_ctx *ctx, const char *path) +{ + int fd = open(path, O_WRONLY|O_CLOEXEC); + bool rc = false; + + if (fd < 0) { + if (errno != EACCES) + log_dbg(ctx, "failed to open %s: %s\n", + path, strerror(errno)); + } else { + rc = true; + } + + close(fd); + return rc; +} + int __sysfs_device_parse(struct log_ctx *ctx, const char *base_path, const char *dev_name, void *parent, add_dev_fn add_dev) { diff --git a/util/sysfs.h b/util/sysfs.h index fb169c6..e487ed4 100644 --- a/util/sysfs.h +++ b/util/sysfs.h @@ -14,6 +14,7 @@ #define __UTIL_SYSFS_H__ #include <string.h> +#include <stdbool.h> typedef void *(*add_dev_fn)(void *parent, int id, const char *dev_path); @@ -24,12 +25,14 @@ int __sysfs_read_attr(struct log_ctx *ctx, const char *path, char *buf); int __sysfs_write_attr(struct log_ctx *ctx, const char *path, const char *buf); int __sysfs_write_attr_quiet(struct log_ctx *ctx, const char *path, const char *buf); +bool __sysfs_attr_writable(struct log_ctx *ctx, const char *path); int __sysfs_device_parse(struct log_ctx *ctx, const char *base_path, const char *dev_name, void *parent, add_dev_fn add_dev); #define sysfs_read_attr(c, p, b) __sysfs_read_attr(&(c)->ctx, (p), (b)) #define sysfs_write_attr(c, p, b) __sysfs_write_attr(&(c)->ctx, (p), (b)) #define sysfs_write_attr_quiet(c, p, b) __sysfs_write_attr_quiet(&(c)->ctx, (p), (b)) +#define sysfs_attr_writable(c, p) __sysfs_attr_writable(&(c)->ctx, (p)) #define sysfs_device_parse(c, b, d, p, fn) __sysfs_device_parse(&(c)->ctx, \ (b), (d), (p), (fn)) -- 2.20.1 _______________________________________________ Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org To unsubscribe send an email to linux-nvdimm-leave@lists.01.org ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [ndctl PATCH 2/2] ndctl/namespace: introduce ndctl_namespace_is_configurable() 2019-11-01 20:27 ` [ndctl PATCH 2/2] ndctl/namespace: introduce ndctl_namespace_is_configurable() Vishal Verma @ 2019-11-04 16:35 ` Dan Williams 2019-11-04 18:48 ` Verma, Vishal L 0 siblings, 1 reply; 6+ messages in thread From: Dan Williams @ 2019-11-04 16:35 UTC (permalink / raw) To: Vishal Verma; +Cc: linux-nvdimm, Aneesh Kumar K.V On Fri, Nov 1, 2019 at 1:27 PM Vishal Verma <vishal.l.verma@intel.com> wrote: > > The motivation for this change is that we want to refrain from > (re)configuring what appear to be partially configured namespaces. > Namespaces may end up in a state that looks partially configured when > the kernel isn't able to enable a namespace due to mismatched > PAGE_SIZE expectations. In such cases, ndctl should not treat those > as unconfigured 'seed' namespaces, and reuse them. > > Add a new ndctl_namespace_is_configurable API, whish tests whether a > namespaces is active, and whether it is partially configured. If neither > are true, then it can be used for (re)configuration. Additionally, deal > with the corner case of ND_DEVICE_NAMESPACE_IO (legacy PMEM) namespaces > by testing whether the size attribute is read-only (which indicates such > a namespace). Legacy namespaces always appear as configured, since their > size cannot be changed, but they are also always re-configurable. > > Use this API in namespace_reconfig() and namespace_create() to enable > this partial configuration detection. A couple comments.... I think it's probably sufficient to just check for ND_DEVICE_NAMESPACE_IO as I don't anticipate we'll ever have more than one namespace type with a read-only size attribute. Also, how about s/ndctl_namespace_is_configurable/ndctl_namespace_is_configuration_idle/? Because to me all namespaces are always "configurable", but some may have active non-default properties set. _______________________________________________ Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org To unsubscribe send an email to linux-nvdimm-leave@lists.01.org ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [ndctl PATCH 2/2] ndctl/namespace: introduce ndctl_namespace_is_configurable() 2019-11-04 16:35 ` Dan Williams @ 2019-11-04 18:48 ` Verma, Vishal L 2019-11-04 18:58 ` Dan Williams 0 siblings, 1 reply; 6+ messages in thread From: Verma, Vishal L @ 2019-11-04 18:48 UTC (permalink / raw) To: Williams, Dan J; +Cc: aneesh.kumar, linux-nvdimm On Mon, 2019-11-04 at 08:35 -0800, Dan Williams wrote: > On Fri, Nov 1, 2019 at 1:27 PM Vishal Verma <vishal.l.verma@intel.com> wrote: > > The motivation for this change is that we want to refrain from > > (re)configuring what appear to be partially configured namespaces. > > Namespaces may end up in a state that looks partially configured when > > the kernel isn't able to enable a namespace due to mismatched > > PAGE_SIZE expectations. In such cases, ndctl should not treat those > > as unconfigured 'seed' namespaces, and reuse them. > > > > Add a new ndctl_namespace_is_configurable API, whish tests whether a > > namespaces is active, and whether it is partially configured. If neither > > are true, then it can be used for (re)configuration. Additionally, deal > > with the corner case of ND_DEVICE_NAMESPACE_IO (legacy PMEM) namespaces > > by testing whether the size attribute is read-only (which indicates such > > a namespace). Legacy namespaces always appear as configured, since their > > size cannot be changed, but they are also always re-configurable. > > > > Use this API in namespace_reconfig() and namespace_create() to enable > > this partial configuration detection. > > A couple comments.... I think it's probably sufficient to just check > for ND_DEVICE_NAMESPACE_IO as I don't anticipate we'll ever have more > than one namespace type with a read-only size attribute. Yep I did notice I could do that, but I decided to go to the source of it instead of adding another ND_DEVICE_NAMESPACE_IO assumption. Also I had already written sysfs_attr_writable() before I noticed that :) But there are already assumptions around ND_DEVICE_NAMESPACE_IO, so adding another one here is fine. > Also, how about s/ndctl_namespace_is_configurable/ndctl_namespace_is_configuration_idle/? > Because to me all namespaces are always "configurable", but some may > have active non-default properties set. Sounds reasonable, but how about ndctl_namespace_has_partial_config(), which I think describes the situation better, and makes it straightforward for a potential future --really-force (or -ff) option where we might still want to blow away anything on this namespace and reclaim it. _______________________________________________ Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org To unsubscribe send an email to linux-nvdimm-leave@lists.01.org ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [ndctl PATCH 2/2] ndctl/namespace: introduce ndctl_namespace_is_configurable() 2019-11-04 18:48 ` Verma, Vishal L @ 2019-11-04 18:58 ` Dan Williams 2019-11-04 19:04 ` Verma, Vishal L 0 siblings, 1 reply; 6+ messages in thread From: Dan Williams @ 2019-11-04 18:58 UTC (permalink / raw) To: Verma, Vishal L; +Cc: aneesh.kumar, linux-nvdimm On Mon, Nov 4, 2019 at 10:48 AM Verma, Vishal L <vishal.l.verma@intel.com> wrote: > > On Mon, 2019-11-04 at 08:35 -0800, Dan Williams wrote: > > On Fri, Nov 1, 2019 at 1:27 PM Vishal Verma <vishal.l.verma@intel.com> wrote: > > > The motivation for this change is that we want to refrain from > > > (re)configuring what appear to be partially configured namespaces. > > > Namespaces may end up in a state that looks partially configured when > > > the kernel isn't able to enable a namespace due to mismatched > > > PAGE_SIZE expectations. In such cases, ndctl should not treat those > > > as unconfigured 'seed' namespaces, and reuse them. > > > > > > Add a new ndctl_namespace_is_configurable API, whish tests whether a > > > namespaces is active, and whether it is partially configured. If neither > > > are true, then it can be used for (re)configuration. Additionally, deal > > > with the corner case of ND_DEVICE_NAMESPACE_IO (legacy PMEM) namespaces > > > by testing whether the size attribute is read-only (which indicates such > > > a namespace). Legacy namespaces always appear as configured, since their > > > size cannot be changed, but they are also always re-configurable. > > > > > > Use this API in namespace_reconfig() and namespace_create() to enable > > > this partial configuration detection. > > > > A couple comments.... I think it's probably sufficient to just check > > for ND_DEVICE_NAMESPACE_IO as I don't anticipate we'll ever have more > > than one namespace type with a read-only size attribute. > > Yep I did notice I could do that, but I decided to go to the source of > it instead of adding another ND_DEVICE_NAMESPACE_IO assumption. Also I > had already written sysfs_attr_writable() before I noticed that :) > But there are already assumptions around ND_DEVICE_NAMESPACE_IO, so > adding another one here is fine. > > > Also, how about s/ndctl_namespace_is_configurable/ndctl_namespace_is_configuration_idle/? > > Because to me all namespaces are always "configurable", but some may > > have active non-default properties set. > > Sounds reasonable, but how about ndctl_namespace_has_partial_config(), > which I think describes the situation better, and makes it > straightforward for a potential future --really-force (or -ff) option > where we might still want to blow away anything on this namespace and > reclaim it. Does "_has_partial_config()" imply "_has_full_config()"? In other words, what ndctl cares about are namespaces that can effectively be used as seeds with no existing configuration parameters having been set. So "_is_configuration_idle()" seems unambiguous where "_has_partial_config()" does not. _______________________________________________ Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org To unsubscribe send an email to linux-nvdimm-leave@lists.01.org ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [ndctl PATCH 2/2] ndctl/namespace: introduce ndctl_namespace_is_configurable() 2019-11-04 18:58 ` Dan Williams @ 2019-11-04 19:04 ` Verma, Vishal L 0 siblings, 0 replies; 6+ messages in thread From: Verma, Vishal L @ 2019-11-04 19:04 UTC (permalink / raw) To: Williams, Dan J; +Cc: aneesh.kumar, linux-nvdimm On Mon, 2019-11-04 at 10:58 -0800, Dan Williams wrote: > > > > > Also, how about s/ndctl_namespace_is_configurable/ndctl_namespace_is_configuration_idle/? > > > Because to me all namespaces are always "configurable", but some may > > > have active non-default properties set. > > > > Sounds reasonable, but how about ndctl_namespace_has_partial_config(), > > which I think describes the situation better, and makes it > > straightforward for a potential future --really-force (or -ff) option > > where we might still want to blow away anything on this namespace and > > reclaim it. > > Does "_has_partial_config()" imply "_has_full_config()"? In other > words, what ndctl cares about are namespaces that can effectively be > used as seeds with no existing configuration parameters having been > set. So "_is_configuration_idle()" seems unambiguous where > "_has_partial_config()" does not. Good point, I'll go with _is_configuration_idle() _______________________________________________ Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org To unsubscribe send an email to linux-nvdimm-leave@lists.01.org ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-11-04 19:05 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-11-01 20:27 [ndctl PATCH 1/2] ndctl/namespace: remove open coded is_namespace_active() Vishal Verma 2019-11-01 20:27 ` [ndctl PATCH 2/2] ndctl/namespace: introduce ndctl_namespace_is_configurable() Vishal Verma 2019-11-04 16:35 ` Dan Williams 2019-11-04 18:48 ` Verma, Vishal L 2019-11-04 18:58 ` Dan Williams 2019-11-04 19:04 ` Verma, Vishal L
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).