| Message ID | 20180626153736.6770-3-keith.busch@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 2018-06-26 at 09:37 -0600, Keith Busch wrote: > The available_size attribute returns all the unused regions, but a > namespace has to use contiguous free regions. This patch uses the > attribute returning the largest capacity that can be created for > determining if the namespace can be created. > > Signed-off-by: Keith Busch <keith.busch@intel.com> > --- > ndctl/lib/libndctl.c | 30 ++++++++++++++++++++++++++++++ > ndctl/lib/libndctl.sym | 1 + > ndctl/libndctl.h | 2 ++ > ndctl/namespace.c | 2 +- > 4 files changed, 34 insertions(+), 1 deletion(-) Hi Keith, The patch looks good, but just a couple of 'meta' comments. 1. We typically send ndctl patches separately from kernel patches (i.e. not thraded together). 2. for ndctl patches, an 'ndctl PATCH' prefix is recommended. You can set a repo local config parameter for doing this automatically on git format- patch. git config format.subjectprefix "ndctl PATCH" I'm thinking the kernel changes will be queued for 4.19, which means the ndctl changes will go into v62. Thanks, -Vishal > > diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c > index 47e005e..a820fb3 100644 > --- a/ndctl/lib/libndctl.c > +++ b/ndctl/lib/libndctl.c > @@ -2025,6 +2025,36 @@ NDCTL_EXPORT unsigned long long > ndctl_region_get_available_size( > return strtoull(buf, NULL, 0); > } > > +NDCTL_EXPORT unsigned long long ndctl_region_get_max_available_extent( > + struct ndctl_region *region) > +{ > + unsigned int nstype = ndctl_region_get_nstype(region); > + struct ndctl_ctx *ctx = ndctl_region_get_ctx(region); > + char *path = region->region_buf; > + int len = region->buf_len; > + char buf[SYSFS_ATTR_SIZE]; > + > + switch (nstype) { > + case ND_DEVICE_NAMESPACE_PMEM: > + case ND_DEVICE_NAMESPACE_BLK: > + break; > + default: > + return 0; > + } > + > + if (snprintf(path, len, > + "%s/max_available_extent", region->region_path) >= > len) { > + err(ctx, "%s: buffer too small!\n", > + ndctl_region_get_devname(region)); > + return ULLONG_MAX; > + } > + > + if (sysfs_read_attr(ctx, path, buf) < 0) > + return ULLONG_MAX; > + > + return strtoull(buf, NULL, 0); > +} > + > NDCTL_EXPORT unsigned int ndctl_region_get_range_index(struct > ndctl_region *region) > { > return region->range_index; > diff --git a/ndctl/lib/libndctl.sym b/ndctl/lib/libndctl.sym > index c1228e5..22fd026 100644 > --- a/ndctl/lib/libndctl.sym > +++ b/ndctl/lib/libndctl.sym > @@ -123,6 +123,7 @@ global: > ndctl_region_get_mappings; > ndctl_region_get_size; > ndctl_region_get_available_size; > + ndctl_region_get_max_available_extent; > ndctl_region_get_type; > ndctl_region_get_namespace_seed; > ndctl_region_get_btt_seed; > diff --git a/ndctl/libndctl.h b/ndctl/libndctl.h > index be997ac..624115d 100644 > --- a/ndctl/libndctl.h > +++ b/ndctl/libndctl.h > @@ -338,6 +338,8 @@ unsigned int ndctl_region_get_interleave_ways(struct > ndctl_region *region); > unsigned int ndctl_region_get_mappings(struct ndctl_region *region); > unsigned long long ndctl_region_get_size(struct ndctl_region *region); > unsigned long long ndctl_region_get_available_size(struct ndctl_region > *region); > +unsigned long long ndctl_region_get_max_available_extent( > + struct ndctl_region *region); > unsigned int ndctl_region_get_range_index(struct ndctl_region *region); > unsigned int ndctl_region_get_type(struct ndctl_region *region); > struct ndctl_namespace *ndctl_region_get_namespace_seed( > diff --git a/ndctl/namespace.c b/ndctl/namespace.c > index fe86d82..4a562a2 100644 > --- a/ndctl/namespace.c > +++ b/ndctl/namespace.c > @@ -764,7 +764,7 @@ static int namespace_create(struct ndctl_region > *region) > return -EAGAIN; > } > > - available = ndctl_region_get_available_size(region); > + available = ndctl_region_get_max_available_extent(region); > if (!available || p.size > available) { > debug("%s: insufficient capacity size: %llx avail: > %llx\n", > devname, p.size, available);
On Tue, 2018-06-26 at 10:29 -0600, Keith Busch wrote: > On Tue, Jun 26, 2018 at 09:19:28AM -0700, Verma, Vishal L wrote: > > On Tue, 2018-06-26 at 09:37 -0600, Keith Busch wrote: > > > The available_size attribute returns all the unused regions, but a > > > namespace has to use contiguous free regions. This patch uses the > > > attribute returning the largest capacity that can be created for > > > determining if the namespace can be created. > > > > > > Signed-off-by: Keith Busch <keith.busch@intel.com> > > > --- > > > ndctl/lib/libndctl.c | 30 ++++++++++++++++++++++++++++++ > > > ndctl/lib/libndctl.sym | 1 + > > > ndctl/libndctl.h | 2 ++ > > > ndctl/namespace.c | 2 +- > > > 4 files changed, 34 insertions(+), 1 deletion(-) > > > > Hi Keith, > > > > The patch looks good, but just a couple of 'meta' comments. > > 1. We typically send ndctl patches separately from kernel patches (i.e. > > not > > thraded together). > > 2. for ndctl patches, an 'ndctl PATCH' prefix is recommended. You can > > set a > > repo local config parameter for doing this automatically on git format- > > patch. > > git config format.subjectprefix "ndctl PATCH" > > > > I'm thinking the kernel changes will be queued for 4.19, which means > > the > > ndctl changes will go into v62. > > Thanks for the info. I'll make those changes for next time. > > I think I may need to send a v2 for this. Should we have this fall back > to > the available_size for the older kernels where the max_available_extents > attribute is not provided? I actually had that in my repo and used a > slightly older patch here, but I'm not sure if its okay to strongly > couple an ndctl release to a kernel version. I was thinking that too. Typically we don't guarantee ndctl to work with old kernels, but this does seem like a bit of an invasive change. Dan, thoughts?
On Tue, Jun 26, 2018 at 09:19:28AM -0700, Verma, Vishal L wrote: > On Tue, 2018-06-26 at 09:37 -0600, Keith Busch wrote: > > The available_size attribute returns all the unused regions, but a > > namespace has to use contiguous free regions. This patch uses the > > attribute returning the largest capacity that can be created for > > determining if the namespace can be created. > > > > Signed-off-by: Keith Busch <keith.busch@intel.com> > > --- > > ndctl/lib/libndctl.c | 30 ++++++++++++++++++++++++++++++ > > ndctl/lib/libndctl.sym | 1 + > > ndctl/libndctl.h | 2 ++ > > ndctl/namespace.c | 2 +- > > 4 files changed, 34 insertions(+), 1 deletion(-) > > Hi Keith, > > The patch looks good, but just a couple of 'meta' comments. > 1. We typically send ndctl patches separately from kernel patches (i.e. not > thraded together). > 2. for ndctl patches, an 'ndctl PATCH' prefix is recommended. You can set a > repo local config parameter for doing this automatically on git format- > patch. > git config format.subjectprefix "ndctl PATCH" > > I'm thinking the kernel changes will be queued for 4.19, which means the > ndctl changes will go into v62. Thanks for the info. I'll make those changes for next time. I think I may need to send a v2 for this. Should we have this fall back to the available_size for the older kernels where the max_available_extents attribute is not provided? I actually had that in my repo and used a slightly older patch here, but I'm not sure if its okay to strongly couple an ndctl release to a kernel version.
On Tue, Jun 26, 2018 at 9:27 AM, Verma, Vishal L <vishal.l.verma@intel.com> wrote: > On Tue, 2018-06-26 at 10:29 -0600, Keith Busch wrote: >> On Tue, Jun 26, 2018 at 09:19:28AM -0700, Verma, Vishal L wrote: >> > On Tue, 2018-06-26 at 09:37 -0600, Keith Busch wrote: >> > > The available_size attribute returns all the unused regions, but a >> > > namespace has to use contiguous free regions. This patch uses the >> > > attribute returning the largest capacity that can be created for >> > > determining if the namespace can be created. >> > > >> > > Signed-off-by: Keith Busch <keith.busch@intel.com> >> > > --- >> > > ndctl/lib/libndctl.c | 30 ++++++++++++++++++++++++++++++ >> > > ndctl/lib/libndctl.sym | 1 + >> > > ndctl/libndctl.h | 2 ++ >> > > ndctl/namespace.c | 2 +- >> > > 4 files changed, 34 insertions(+), 1 deletion(-) >> > >> > Hi Keith, >> > >> > The patch looks good, but just a couple of 'meta' comments. >> > 1. We typically send ndctl patches separately from kernel patches (i.e. >> > not >> > thraded together). >> > 2. for ndctl patches, an 'ndctl PATCH' prefix is recommended. You can >> > set a >> > repo local config parameter for doing this automatically on git format- >> > patch. >> > git config format.subjectprefix "ndctl PATCH" >> > >> > I'm thinking the kernel changes will be queued for 4.19, which means >> > the >> > ndctl changes will go into v62. >> >> Thanks for the info. I'll make those changes for next time. >> >> I think I may need to send a v2 for this. Should we have this fall back >> to >> the available_size for the older kernels where the max_available_extents >> attribute is not provided? I actually had that in my repo and used a >> slightly older patch here, but I'm not sure if its okay to strongly >> couple an ndctl release to a kernel version. > > I was thinking that too. Typically we don't guarantee ndctl to work with > old kernels, but this does seem like a bit of an invasive change. > > Dan, thoughts? It should fall back if the attribute is not available. Our *tests* aren't guaranteed to pass on older kernels, but ndctl proper should try it's best to accommodate old kernels.
On Tue, 2018-06-26 at 09:32 -0700, Dan Williams wrote: > On Tue, Jun 26, 2018 at 9:27 AM, Verma, Vishal L > <vishal.l.verma@intel.com> wrote: > > On Tue, 2018-06-26 at 10:29 -0600, Keith Busch wrote: > > > On Tue, Jun 26, 2018 at 09:19:28AM -0700, Verma, Vishal L wrote: > > > > On Tue, 2018-06-26 at 09:37 -0600, Keith Busch wrote: > > > > > The available_size attribute returns all the unused regions, but > > > > > a > > > > > namespace has to use contiguous free regions. This patch uses the > > > > > attribute returning the largest capacity that can be created for > > > > > determining if the namespace can be created. > > > > > > > > > > Signed-off-by: Keith Busch <keith.busch@intel.com> > > > > > --- > > > > > ndctl/lib/libndctl.c | 30 ++++++++++++++++++++++++++++++ > > > > > ndctl/lib/libndctl.sym | 1 + > > > > > ndctl/libndctl.h | 2 ++ > > > > > ndctl/namespace.c | 2 +- > > > > > 4 files changed, 34 insertions(+), 1 deletion(-) > > > > > > > > Hi Keith, > > > > > > > > The patch looks good, but just a couple of 'meta' comments. > > > > 1. We typically send ndctl patches separately from kernel patches > > > > (i.e. > > > > not > > > > thraded together). > > > > 2. for ndctl patches, an 'ndctl PATCH' prefix is recommended. You > > > > can > > > > set a > > > > repo local config parameter for doing this automatically on git > > > > format- > > > > patch. > > > > git config format.subjectprefix "ndctl PATCH" > > > > > > > > I'm thinking the kernel changes will be queued for 4.19, which > > > > means > > > > the > > > > ndctl changes will go into v62. > > > > > > Thanks for the info. I'll make those changes for next time. > > > > > > I think I may need to send a v2 for this. Should we have this fall > > > back > > > to > > > the available_size for the older kernels where the > > > max_available_extents > > > attribute is not provided? I actually had that in my repo and used a > > > slightly older patch here, but I'm not sure if its okay to strongly > > > couple an ndctl release to a kernel version. > > > > I was thinking that too. Typically we don't guarantee ndctl to work > > with > > old kernels, but this does seem like a bit of an invasive change. > > > > Dan, thoughts? > > It should fall back if the attribute is not available. Our *tests* > aren't guaranteed to pass on older kernels, but ndctl proper should > try it's best to accommodate old kernels. Ah yes makes sense.
diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c index 47e005e..a820fb3 100644 --- a/ndctl/lib/libndctl.c +++ b/ndctl/lib/libndctl.c @@ -2025,6 +2025,36 @@ NDCTL_EXPORT unsigned long long ndctl_region_get_available_size( return strtoull(buf, NULL, 0); } +NDCTL_EXPORT unsigned long long ndctl_region_get_max_available_extent( + struct ndctl_region *region) +{ + unsigned int nstype = ndctl_region_get_nstype(region); + struct ndctl_ctx *ctx = ndctl_region_get_ctx(region); + char *path = region->region_buf; + int len = region->buf_len; + char buf[SYSFS_ATTR_SIZE]; + + switch (nstype) { + case ND_DEVICE_NAMESPACE_PMEM: + case ND_DEVICE_NAMESPACE_BLK: + break; + default: + return 0; + } + + if (snprintf(path, len, + "%s/max_available_extent", region->region_path) >= len) { + err(ctx, "%s: buffer too small!\n", + ndctl_region_get_devname(region)); + return ULLONG_MAX; + } + + if (sysfs_read_attr(ctx, path, buf) < 0) + return ULLONG_MAX; + + return strtoull(buf, NULL, 0); +} + NDCTL_EXPORT unsigned int ndctl_region_get_range_index(struct ndctl_region *region) { return region->range_index; diff --git a/ndctl/lib/libndctl.sym b/ndctl/lib/libndctl.sym index c1228e5..22fd026 100644 --- a/ndctl/lib/libndctl.sym +++ b/ndctl/lib/libndctl.sym @@ -123,6 +123,7 @@ global: ndctl_region_get_mappings; ndctl_region_get_size; ndctl_region_get_available_size; + ndctl_region_get_max_available_extent; ndctl_region_get_type; ndctl_region_get_namespace_seed; ndctl_region_get_btt_seed; diff --git a/ndctl/libndctl.h b/ndctl/libndctl.h index be997ac..624115d 100644 --- a/ndctl/libndctl.h +++ b/ndctl/libndctl.h @@ -338,6 +338,8 @@ unsigned int ndctl_region_get_interleave_ways(struct ndctl_region *region); unsigned int ndctl_region_get_mappings(struct ndctl_region *region); unsigned long long ndctl_region_get_size(struct ndctl_region *region); unsigned long long ndctl_region_get_available_size(struct ndctl_region *region); +unsigned long long ndctl_region_get_max_available_extent( + struct ndctl_region *region); unsigned int ndctl_region_get_range_index(struct ndctl_region *region); unsigned int ndctl_region_get_type(struct ndctl_region *region); struct ndctl_namespace *ndctl_region_get_namespace_seed( diff --git a/ndctl/namespace.c b/ndctl/namespace.c index fe86d82..4a562a2 100644 --- a/ndctl/namespace.c +++ b/ndctl/namespace.c @@ -764,7 +764,7 @@ static int namespace_create(struct ndctl_region *region) return -EAGAIN; } - available = ndctl_region_get_available_size(region); + available = ndctl_region_get_max_available_extent(region); if (!available || p.size > available) { debug("%s: insufficient capacity size: %llx avail: %llx\n", devname, p.size, available);
The available_size attribute returns all the unused regions, but a namespace has to use contiguous free regions. This patch uses the attribute returning the largest capacity that can be created for determining if the namespace can be created. Signed-off-by: Keith Busch <keith.busch@intel.com> --- ndctl/lib/libndctl.c | 30 ++++++++++++++++++++++++++++++ ndctl/lib/libndctl.sym | 1 + ndctl/libndctl.h | 2 ++ ndctl/namespace.c | 2 +- 4 files changed, 34 insertions(+), 1 deletion(-)