| Message ID | 20190828200204.21750-1-vishal.l.verma@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [ndctl,RFC] ndctl/namespace: create namespaces greedily | expand |
Vishal Verma <vishal.l.verma@intel.com> writes: > When a --region=all option is supplied to ndctl-create-namespace, it > happily ignores it, since create-namespace has historically only created > one namespace per invocation. > > This can be cumbersome, so change create-namespace to create namespaces > greedily. When --region=all is specified, or if a --region option is > absent (same as 'all' from a filtering standpoint), create namespaces on > all regions. Cumbersome? Like, in the same way partitioning a disk is cumbersome? I don't understand what the problem is, I guess. If you want N namespaces of the same type/size/align, then script it. Why does there have to be a command for that? I definitely think that changing the behavior of create-namespace is a non-starter. Cheers, Jeff > > Note that this does has two important implications: > > 1. The user-facing behavior of create-namespace changes in a potentially > surprising way. It may be undesirable for an unadorned 'ndctl > create-namespace' command to suddenly start creating lots of namespaces. > > 2. Error handling becomes a bit inconsistent. As with all commands > accepting an 'all' option, error reporting becomes a bit tenuous. With > this change, we will continue to create namespaces so long as we don't > hit an error, but if we do, we bail and exit. Because of the special > ENOSPC detection and handling around this, it can be easy to construct a > scenario where en existing namespace on the last region in the scan list > happens to report an error exit, but if the existing namespace was in a > region at the start of the scan list, it gets passed over as a "just try > the next region" > > RFC comment: Maybe the solution is to keep the create-namespace > semantics unchanged, and introduce a new command - 'create-namespaces' > or 'create-names-ace-greedy' with the new behavior. I'm not sure if > users will care deeply about either of the two points above, hence > sending this as an RFC. > > Link: https://github.com/pmem/ndctl/issues/106 > Reported-by: Steve Scargal <steve.scargall@intel.com> > Cc: Jeff Moyer <jmoyer@redhat.com> > Cc: Dan Williams <dan.j.williams@intel.com> > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com> > --- > ndctl/namespace.c | 11 ++++++++--- > 1 file changed, 8 insertions(+), 3 deletions(-) > > diff --git a/ndctl/namespace.c b/ndctl/namespace.c > index af20a42..856ad82 100644 > --- a/ndctl/namespace.c > +++ b/ndctl/namespace.c > @@ -1365,9 +1365,12 @@ static int do_xaction_namespace(const char *namespace, > rc = namespace_create(region); > if (rc == -EAGAIN) > continue; > - if (rc == 0) > - *processed = 1; > - return rc; > + if (rc == 0) { > + (*processed)++; > + continue; > + } else { > + return rc; > + } > } > ndctl_namespace_foreach_safe(region, ndns, _n) { > ndns_name = ndctl_namespace_get_devname(ndns); > @@ -1487,6 +1490,8 @@ int cmd_create_namespace(int argc, const char **argv, struct ndctl_ctx *ctx) > rc = do_xaction_namespace(NULL, ACTION_CREATE, ctx, &created); > } > > + fprintf(stderr, "created %d namespace%s\n", created, > + created == 1 ? "" : "s"); > if (rc < 0 || (!namespace && created < 1)) { > fprintf(stderr, "failed to %s namespace: %s\n", namespace > ? "reconfigure" : "create", strerror(-rc));
Hi Jeff,
The issue is more of repetition. On an 8-socket system, should a user really be expected to type 'ndctl create-namespace' eight times vs. running 'ndctl create-namespace --region=all' once? SAP HANA is an example app the requires one namespace per region. Scripting is a viable solution, but that requires somebody to write the script and do all the error checking & handling. Each OEM/ISV/SysAdmin would have their own script. Pushing the logic to ndctl seems to be a reasonable approach and let the user handle any errors returned by ndctl.
The ndctl-man-page implies the 'ndctl create-namespace --region=all' feature exists today:
-r, --region=
A regionX device name, or a region id number. The keyword all can be specified to carry out the operation on every region in the system, optionally filtered by bus id (see --bus= option).
-Steve
-----Original Message-----
From: Jeff Moyer [mailto:jmoyer@redhat.com]
Sent: Wednesday, August 28, 2019 2:13 PM
To: Verma, Vishal L <vishal.l.verma@intel.com>
Cc: linux-nvdimm@lists.01.org; Scargall, Steve <steve.scargall@intel.com>; Williams, Dan J <dan.j.williams@intel.com>
Subject: Re: [ndctl RFC PATCH] ndctl/namespace: create namespaces greedily
Vishal Verma <vishal.l.verma@intel.com> writes:
> When a --region=all option is supplied to ndctl-create-namespace, it
> happily ignores it, since create-namespace has historically only
> created one namespace per invocation.
>
> This can be cumbersome, so change create-namespace to create
> namespaces greedily. When --region=all is specified, or if a --region
> option is absent (same as 'all' from a filtering standpoint), create
> namespaces on all regions.
Cumbersome? Like, in the same way partitioning a disk is cumbersome? I don't understand what the problem is, I guess. If you want N namespaces of the same type/size/align, then script it. Why does there have to be a command for that? I definitely think that changing the behavior of create-namespace is a non-starter.
Cheers,
Jeff
>
> Note that this does has two important implications:
>
> 1. The user-facing behavior of create-namespace changes in a
> potentially surprising way. It may be undesirable for an unadorned
> 'ndctl create-namespace' command to suddenly start creating lots of namespaces.
>
> 2. Error handling becomes a bit inconsistent. As with all commands
> accepting an 'all' option, error reporting becomes a bit tenuous. With
> this change, we will continue to create namespaces so long as we don't
> hit an error, but if we do, we bail and exit. Because of the special
> ENOSPC detection and handling around this, it can be easy to construct
> a scenario where en existing namespace on the last region in the scan
> list happens to report an error exit, but if the existing namespace
> was in a region at the start of the scan list, it gets passed over as
> a "just try the next region"
>
> RFC comment: Maybe the solution is to keep the create-namespace
> semantics unchanged, and introduce a new command - 'create-namespaces'
> or 'create-names-ace-greedy' with the new behavior. I'm not sure if
> users will care deeply about either of the two points above, hence
> sending this as an RFC.
>
> Link: https://github.com/pmem/ndctl/issues/106
> Reported-by: Steve Scargal <steve.scargall@intel.com>
> Cc: Jeff Moyer <jmoyer@redhat.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
> ndctl/namespace.c | 11 ++++++++---
> 1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/ndctl/namespace.c b/ndctl/namespace.c index
> af20a42..856ad82 100644
> --- a/ndctl/namespace.c
> +++ b/ndctl/namespace.c
> @@ -1365,9 +1365,12 @@ static int do_xaction_namespace(const char *namespace,
> rc = namespace_create(region);
> if (rc == -EAGAIN)
> continue;
> - if (rc == 0)
> - *processed = 1;
> - return rc;
> + if (rc == 0) {
> + (*processed)++;
> + continue;
> + } else {
> + return rc;
> + }
> }
> ndctl_namespace_foreach_safe(region, ndns, _n) {
> ndns_name = ndctl_namespace_get_devname(ndns);
> @@ -1487,6 +1490,8 @@ int cmd_create_namespace(int argc, const char **argv, struct ndctl_ctx *ctx)
> rc = do_xaction_namespace(NULL, ACTION_CREATE, ctx, &created);
> }
>
> + fprintf(stderr, "created %d namespace%s\n", created,
> + created == 1 ? "" : "s");
> if (rc < 0 || (!namespace && created < 1)) {
> fprintf(stderr, "failed to %s namespace: %s\n", namespace
> ? "reconfigure" : "create", strerror(-rc));
On Wed, 2019-08-28 at 13:47 -0700, Scargall, Steve wrote: > Hi Jeff, > > The issue is more of repetition. On an 8-socket system, should a > user really be expected to type 'ndctl create-namespace' eight times > vs. running 'ndctl create-namespace --region=all' once? SAP HANA is > an example app the requires one namespace per region. Scripting is a > viable solution, but that requires somebody to write the script and do > all the error checking & handling. Each OEM/ISV/SysAdmin would have > their own script. Pushing the logic to ndctl seems to be a reasonable > approach and let the user handle any errors returned by ndctl. A scripted solution can indeed be really simple - e.g.: # while read -r region; do ndctl create-namespace --region="$region"; done < <(ndctl list --bus=nfit_test.0 -R | jq -r '.[].dev') { "dev":"namespace5.0", "mode":"fsdax", "map":"dev", "size":"62.00 MiB (65.01 MB)", "uuid":"c8014457-c268-4f22-8eae-6386fbf08ceb", "sector_size":512, "align":2097152, "blockdev":"pmem5" } { "dev":"namespace4.0", "mode":"fsdax", "map":"dev", "size":"30.00 MiB (31.46 MB)", "uuid":"f9498ef6-cdd6-46c7-95f1-86469546ecb9", "sector_size":512, "align":2097152, "blockdev":"pmem4" } > > The ndctl-man-page implies the 'ndctl create-namespace --region=all' > feature exists today: > > -r, --region= > > A regionX device name, or a region id number. The keyword > all can be specified to carry out the operation on every region in the > system, optionally filtered by bus id (see --bus= option). > This is true, but unfortunately, the implementation has treated create- namespace as an exception to this since the start, and I agree with Jeff that changing its behavior now can cause other Hyrum's law-esqe [1] breakage. I think however it should be easy to make a compromise, and retain the legacy behavior of create-namespace, while creating a new create- namespace-greedy command with the new semantics. Thoughts on that? Thanks, -Vishal [1]: http://www.hyrumslaw.com/
On Wed, 2019-08-28 at 21:16 +0000, Verma, Vishal L wrote: > On Wed, 2019-08-28 at 13:47 -0700, Scargall, Steve wrote: > > Hi Jeff, > > > > The issue is more of repetition. On an 8-socket system, should a > > user really be expected to type 'ndctl create-namespace' eight times > > vs. running 'ndctl create-namespace --region=all' once? SAP HANA is > > an example app the requires one namespace per region. Scripting is a > > viable solution, but that requires somebody to write the script and do > > all the error checking & handling. Each OEM/ISV/SysAdmin would have > > their own script. Pushing the logic to ndctl seems to be a reasonable > > approach and let the user handle any errors returned by ndctl. > > A scripted solution can indeed be really simple - e.g.: > > # while read -r region; do ndctl create-namespace --region="$region"; > done < <(ndctl list --bus=nfit_test.0 -R | jq -r '.[].dev') > > { > "dev":"namespace5.0", > "mode":"fsdax", > "map":"dev", > "size":"62.00 MiB (65.01 MB)", > "uuid":"c8014457-c268-4f22-8eae-6386fbf08ceb", > "sector_size":512, > "align":2097152, > "blockdev":"pmem5" > } > { > "dev":"namespace4.0", > "mode":"fsdax", > "map":"dev", > "size":"30.00 MiB (31.46 MB)", > "uuid":"f9498ef6-cdd6-46c7-95f1-86469546ecb9", > "sector_size":512, > "align":2097152, > "blockdev":"pmem4" > } > > > The ndctl-man-page implies the 'ndctl create-namespace --region=all' > > feature exists today: > > > > -r, --region= > > > > A regionX device name, or a region id number. The keyword > > all can be specified to carry out the operation on every region in the > > system, optionally filtered by bus id (see --bus= option). > > > > This is true, but unfortunately, the implementation has treated create- > namespace as an exception to this since the start, and I agree with Jeff > that changing its behavior now can cause other Hyrum's law-esqe [1] > breakage. > > I think however it should be easy to make a compromise, and retain the > legacy behavior of create-namespace, while creating a new create- > namespace-greedy command with the new semantics. > .. And it doesn't even need to be a new command, a simple --greedy option to create-namespace should be sufficient.
On Wed, Aug 28, 2019 at 2:22 PM Verma, Vishal L <vishal.l.verma@intel.com> wrote: > > On Wed, 2019-08-28 at 21:16 +0000, Verma, Vishal L wrote: > > On Wed, 2019-08-28 at 13:47 -0700, Scargall, Steve wrote: > > > Hi Jeff, > > > > > > The issue is more of repetition. On an 8-socket system, should a > > > user really be expected to type 'ndctl create-namespace' eight times > > > vs. running 'ndctl create-namespace --region=all' once? SAP HANA is > > > an example app the requires one namespace per region. Scripting is a > > > viable solution, but that requires somebody to write the script and do > > > all the error checking & handling. Each OEM/ISV/SysAdmin would have > > > their own script. Pushing the logic to ndctl seems to be a reasonable > > > approach and let the user handle any errors returned by ndctl. > > > > A scripted solution can indeed be really simple - e.g.: > > > > # while read -r region; do ndctl create-namespace --region="$region"; > > done < <(ndctl list --bus=nfit_test.0 -R | jq -r '.[].dev') > > > > { > > "dev":"namespace5.0", > > "mode":"fsdax", > > "map":"dev", > > "size":"62.00 MiB (65.01 MB)", > > "uuid":"c8014457-c268-4f22-8eae-6386fbf08ceb", > > "sector_size":512, > > "align":2097152, > > "blockdev":"pmem5" > > } > > { > > "dev":"namespace4.0", > > "mode":"fsdax", > > "map":"dev", > > "size":"30.00 MiB (31.46 MB)", > > "uuid":"f9498ef6-cdd6-46c7-95f1-86469546ecb9", > > "sector_size":512, > > "align":2097152, > > "blockdev":"pmem4" > > } > > > > > The ndctl-man-page implies the 'ndctl create-namespace --region=all' > > > feature exists today: > > > > > > -r, --region= > > > > > > A regionX device name, or a region id number. The keyword > > > all can be specified to carry out the operation on every region in the > > > system, optionally filtered by bus id (see --bus= option). > > > > > > > This is true, but unfortunately, the implementation has treated create- > > namespace as an exception to this since the start, and I agree with Jeff > > that changing its behavior now can cause other Hyrum's law-esqe [1] > > breakage. > > > > I think however it should be easy to make a compromise, and retain the > > legacy behavior of create-namespace, while creating a new create- > > namespace-greedy command with the new semantics. > > > .. And it doesn't even need to be a new command, a simple --greedy > option to create-namespace should be sufficient. Perhaps "--continue" to proceed with creating namespace after the first successful invocation. I agree with Jeff that changing the default semantics would be surprising. The man page can also be fixed up to make it clear that it's the "singleton region capacity search", not "all possible namespaces" that "-r all" implies.
Thanks for the clarification. I have a much better understanding now. Updating the ndctl-create-namespace man page to clarify what '--region=all' does and does not do in combination with other arguments and options would be beneficial. This would actually provide a more immediate solution to the problem. It sounds like the general statement used in the other man pages can remain? Or are there other exceptions? With the proposed implementation that bails out on first error, `--continue` seems reasonable vs. `--greedy`. What do you think about `--all-regions` instead? It is more meaningful towards its intended action or use-case, but would it cause confusion with `--regions=all`? Documentation could provide the solution here. We would have to choose an arbitrary short option since `-a` and `-r` are already taken. Whatever the final decision, we should test for and return a usage error for a mutually exclusive set of inputs, i.e. `ndctl create-namespace --all-regions --region=region0` doesn't make sense - either you want to perform the create operation on all regions or only in region0. - Steve -----Original Message----- From: Dan Williams [mailto:dan.j.williams@intel.com] Sent: Wednesday, August 28, 2019 3:32 PM To: Verma, Vishal L <vishal.l.verma@intel.com> Cc: Scargall, Steve <steve.scargall@intel.com>; jmoyer@redhat.com; linux-nvdimm@lists.01.org Subject: Re: [ndctl RFC PATCH] ndctl/namespace: create namespaces greedily On Wed, Aug 28, 2019 at 2:22 PM Verma, Vishal L <vishal.l.verma@intel.com> wrote: > > On Wed, 2019-08-28 at 21:16 +0000, Verma, Vishal L wrote: > > On Wed, 2019-08-28 at 13:47 -0700, Scargall, Steve wrote: > > > Hi Jeff, > > > > > > The issue is more of repetition. On an 8-socket system, should a > > > user really be expected to type 'ndctl create-namespace' eight times > > > vs. running 'ndctl create-namespace --region=all' once? SAP HANA is > > > an example app the requires one namespace per region. Scripting > > > is a viable solution, but that requires somebody to write the > > > script and do all the error checking & handling. Each > > > OEM/ISV/SysAdmin would have their own script. Pushing the logic > > > to ndctl seems to be a reasonable approach and let the user handle any errors returned by ndctl. > > > > A scripted solution can indeed be really simple - e.g.: > > > > # while read -r region; do ndctl create-namespace > > --region="$region"; done < <(ndctl list --bus=nfit_test.0 -R | jq > > -r '.[].dev') > > > > { > > "dev":"namespace5.0", > > "mode":"fsdax", > > "map":"dev", > > "size":"62.00 MiB (65.01 MB)", > > "uuid":"c8014457-c268-4f22-8eae-6386fbf08ceb", > > "sector_size":512, > > "align":2097152, > > "blockdev":"pmem5" > > } > > { > > "dev":"namespace4.0", > > "mode":"fsdax", > > "map":"dev", > > "size":"30.00 MiB (31.46 MB)", > > "uuid":"f9498ef6-cdd6-46c7-95f1-86469546ecb9", > > "sector_size":512, > > "align":2097152, > > "blockdev":"pmem4" > > } > > > > > The ndctl-man-page implies the 'ndctl create-namespace --region=all' > > > feature exists today: > > > > > > -r, --region= > > > > > > A regionX device name, or a region id number. The > > > keyword all can be specified to carry out the operation on every > > > region in the system, optionally filtered by bus id (see --bus= option). > > > > > > > This is true, but unfortunately, the implementation has treated > > create- namespace as an exception to this since the start, and I > > agree with Jeff that changing its behavior now can cause other > > Hyrum's law-esqe [1] breakage. > > > > I think however it should be easy to make a compromise, and retain > > the legacy behavior of create-namespace, while creating a new > > create- namespace-greedy command with the new semantics. > > > .. And it doesn't even need to be a new command, a simple --greedy > option to create-namespace should be sufficient. Perhaps "--continue" to proceed with creating namespace after the first successful invocation. I agree with Jeff that changing the default semantics would be surprising. The man page can also be fixed up to make it clear that it's the "singleton region capacity search", not "all possible namespaces" that "-r all" implies.
On Wed, 2019-08-28 at 15:38 -0700, Scargall, Steve wrote: > Thanks for the clarification. I have a much better understanding > now. > > Updating the ndctl-create-namespace man page to clarify what ' > --region=all' does and does not do in combination with other arguments > and options would be beneficial. This would actually provide a more > immediate solution to the problem. It sounds like the general > statement used in the other man pages can remain? Or are there other > exceptions? Yep I have a patch clarifying the --region option for create-namespace. 'all' is used in two ways, --<object>=all or ndctl-<action> all. The former acts as a filter, where as the latter acts as a "act on all" directive. So 'ndctl disable-namespace --region=region0 all' disables all namespaces on region0, where as 'ndctl disable-namespace --region=all all' is the same as omitting the --region option (while still acceptable syntax), and acts on all namespaces regardless of the region. > > With the proposed implementation that bails out on first error, ` > --continue` seems reasonable vs. `--greedy`. What do you think about > `--all-regions` instead? It is more meaningful towards its intended > action or use-case, but would it cause confusion with ` > --regions=all`? Documentation could provide the solution here. We > would have to choose an arbitrary short option since `-a` and `-r` are > already taken. I prefer --continue/-c since it automatically extends to buses as well. As a result, with these changes, 'ndctl create-namespace -c' will create namespaces on all possible regions on all possible buses. > > Whatever the final decision, we should test for and return a usage > error for a mutually exclusive set of inputs, i.e. `ndctl create- > namespace --all-regions --region=region0` doesn't make sense - either > you want to perform the create operation on all regions or only in > region0. With --continue this isn't a big problem as the --region restriction will render the --continue meaningless.
diff --git a/ndctl/namespace.c b/ndctl/namespace.c index af20a42..856ad82 100644 --- a/ndctl/namespace.c +++ b/ndctl/namespace.c @@ -1365,9 +1365,12 @@ static int do_xaction_namespace(const char *namespace, rc = namespace_create(region); if (rc == -EAGAIN) continue; - if (rc == 0) - *processed = 1; - return rc; + if (rc == 0) { + (*processed)++; + continue; + } else { + return rc; + } } ndctl_namespace_foreach_safe(region, ndns, _n) { ndns_name = ndctl_namespace_get_devname(ndns); @@ -1487,6 +1490,8 @@ int cmd_create_namespace(int argc, const char **argv, struct ndctl_ctx *ctx) rc = do_xaction_namespace(NULL, ACTION_CREATE, ctx, &created); } + fprintf(stderr, "created %d namespace%s\n", created, + created == 1 ? "" : "s"); if (rc < 0 || (!namespace && created < 1)) { fprintf(stderr, "failed to %s namespace: %s\n", namespace ? "reconfigure" : "create", strerror(-rc));
When a --region=all option is supplied to ndctl-create-namespace, it happily ignores it, since create-namespace has historically only created one namespace per invocation. This can be cumbersome, so change create-namespace to create namespaces greedily. When --region=all is specified, or if a --region option is absent (same as 'all' from a filtering standpoint), create namespaces on all regions. Note that this does has two important implications: 1. The user-facing behavior of create-namespace changes in a potentially surprising way. It may be undesirable for an unadorned 'ndctl create-namespace' command to suddenly start creating lots of namespaces. 2. Error handling becomes a bit inconsistent. As with all commands accepting an 'all' option, error reporting becomes a bit tenuous. With this change, we will continue to create namespaces so long as we don't hit an error, but if we do, we bail and exit. Because of the special ENOSPC detection and handling around this, it can be easy to construct a scenario where en existing namespace on the last region in the scan list happens to report an error exit, but if the existing namespace was in a region at the start of the scan list, it gets passed over as a "just try the next region" RFC comment: Maybe the solution is to keep the create-namespace semantics unchanged, and introduce a new command - 'create-namespaces' or 'create-names-ace-greedy' with the new behavior. I'm not sure if users will care deeply about either of the two points above, hence sending this as an RFC. Link: https://github.com/pmem/ndctl/issues/106 Reported-by: Steve Scargal <steve.scargall@intel.com> Cc: Jeff Moyer <jmoyer@redhat.com> Cc: Dan Williams <dan.j.williams@intel.com> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com> --- ndctl/namespace.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)