* [PATCH 1/1] ndctl/namespace: Fix disable-namespace accounting relative to seed devices
@ 2020-04-26 9:52 Redhairer Li
2020-04-27 20:31 ` Dan Williams
0 siblings, 1 reply; 11+ messages in thread
From: Redhairer Li @ 2020-04-26 9:52 UTC (permalink / raw)
To: linux-nvdimm, dan.j.williams; +Cc: Redhairer Li
Seed namespaces are included in "ndctl disable-namespace all". However
since the user never "creates" them it is surprising to see
"disable-namespace" report 1 more namespace relative to the number that
have been created. Catch attempts to disable a zero-sized namespace:
Before:
{
"dev":"namespace1.0",
"size":"492.00 MiB (515.90 MB)",
"blockdev":"pmem1"
}
{
"dev":"namespace1.1",
"size":"492.00 MiB (515.90 MB)",
"blockdev":"pmem1.1"
}
{
"dev":"namespace1.2",
"size":"492.00 MiB (515.90 MB)",
"blockdev":"pmem1.2"
}
disabled 4 namespaces
After:
{
"dev":"namespace1.0",
"size":"492.00 MiB (515.90 MB)",
"blockdev":"pmem1"
}
{
"dev":"namespace1.3",
"size":"492.00 MiB (515.90 MB)",
"blockdev":"pmem1.3"
}
{
"dev":"namespace1.1",
"size":"492.00 MiB (515.90 MB)",
"blockdev":"pmem1.1"
}
disabled 3 namespaces
Signed-off-by: Redhairer Li <redhairer.li@intel.com>
---
ndctl/lib/libndctl.c | 11 ++++++++---
ndctl/region.c | 4 +++-
2 files changed, 11 insertions(+), 4 deletions(-)
diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
index ee737cb..49f362b 100644
--- a/ndctl/lib/libndctl.c
+++ b/ndctl/lib/libndctl.c
@@ -4231,6 +4231,7 @@ NDCTL_EXPORT int ndctl_namespace_disable_safe(struct ndctl_namespace *ndns)
const char *bdev = NULL;
char path[50];
int fd;
+ unsigned long long size = ndctl_namespace_get_size(ndns);
if (pfn && ndctl_pfn_is_enabled(pfn))
bdev = ndctl_pfn_get_block_device(pfn);
@@ -4260,9 +4261,13 @@ NDCTL_EXPORT int ndctl_namespace_disable_safe(struct ndctl_namespace *ndns)
devname, bdev, strerror(errno));
return -errno;
}
- } else
- ndctl_namespace_disable_invalidate(ndns);
-
+ } else {
+ if (size == 0)
+ /* Don't try to disable idle namespace (no capacity allocated) */
+ return -ENXIO;
+ else
+ ndctl_namespace_disable_invalidate(ndns);
+ }
return 0;
}
diff --git a/ndctl/region.c b/ndctl/region.c
index 7945007..0014bb9 100644
--- a/ndctl/region.c
+++ b/ndctl/region.c
@@ -72,6 +72,7 @@ static int region_action(struct ndctl_region *region, enum device_action mode)
{
struct ndctl_namespace *ndns;
int rc = 0;
+ unsigned long long size;
switch (mode) {
case ACTION_ENABLE:
@@ -80,7 +81,8 @@ static int region_action(struct ndctl_region *region, enum device_action mode)
case ACTION_DISABLE:
ndctl_namespace_foreach(region, ndns) {
rc = ndctl_namespace_disable_safe(ndns);
- if (rc)
+ size = ndctl_namespace_get_size(ndns);
+ if (rc && size != 0)
return rc;
}
rc = ndctl_region_disable_invalidate(region);
--
2.20.1.windows.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] 11+ messages in thread
* Re: [PATCH 1/1] ndctl/namespace: Fix disable-namespace accounting relative to seed devices
2020-04-26 9:52 [PATCH 1/1] ndctl/namespace: Fix disable-namespace accounting relative to seed devices Redhairer Li
@ 2020-04-27 20:31 ` Dan Williams
2020-04-28 1:09 ` Li, Redhairer
0 siblings, 1 reply; 11+ messages in thread
From: Dan Williams @ 2020-04-27 20:31 UTC (permalink / raw)
To: Redhairer Li; +Cc: linux-nvdimm
On Sun, Apr 26, 2020 at 2:53 AM Redhairer Li <redhairer.li@intel.com> wrote:
>
> Seed namespaces are included in "ndctl disable-namespace all". However
> since the user never "creates" them it is surprising to see
> "disable-namespace" report 1 more namespace relative to the number that
> have been created. Catch attempts to disable a zero-sized namespace:
>
> Before:
> {
> "dev":"namespace1.0",
> "size":"492.00 MiB (515.90 MB)",
> "blockdev":"pmem1"
> }
> {
> "dev":"namespace1.1",
> "size":"492.00 MiB (515.90 MB)",
> "blockdev":"pmem1.1"
> }
> {
> "dev":"namespace1.2",
> "size":"492.00 MiB (515.90 MB)",
> "blockdev":"pmem1.2"
> }
> disabled 4 namespaces
>
> After:
> {
> "dev":"namespace1.0",
> "size":"492.00 MiB (515.90 MB)",
> "blockdev":"pmem1"
> }
> {
> "dev":"namespace1.3",
> "size":"492.00 MiB (515.90 MB)",
> "blockdev":"pmem1.3"
> }
> {
> "dev":"namespace1.1",
> "size":"492.00 MiB (515.90 MB)",
> "blockdev":"pmem1.1"
> }
> disabled 3 namespaces
>
> Signed-off-by: Redhairer Li <redhairer.li@intel.com>
> ---
> ndctl/lib/libndctl.c | 11 ++++++++---
> ndctl/region.c | 4 +++-
> 2 files changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
> index ee737cb..49f362b 100644
> --- a/ndctl/lib/libndctl.c
> +++ b/ndctl/lib/libndctl.c
> @@ -4231,6 +4231,7 @@ NDCTL_EXPORT int ndctl_namespace_disable_safe(struct ndctl_namespace *ndns)
> const char *bdev = NULL;
> char path[50];
> int fd;
> + unsigned long long size = ndctl_namespace_get_size(ndns);
>
> if (pfn && ndctl_pfn_is_enabled(pfn))
> bdev = ndctl_pfn_get_block_device(pfn);
> @@ -4260,9 +4261,13 @@ NDCTL_EXPORT int ndctl_namespace_disable_safe(struct ndctl_namespace *ndns)
> devname, bdev, strerror(errno));
> return -errno;
> }
> - } else
> - ndctl_namespace_disable_invalidate(ndns);
> -
> + } else {
> + if (size == 0)
> + /* Don't try to disable idle namespace (no capacity allocated) */
> + return -ENXIO;
> + else
> + ndctl_namespace_disable_invalidate(ndns);
> + }
Maybe make this return 0 in the case when size is zero since by
definition the namespace must already be disabled if it has zero size.
} else if (size)
ndctl_namespace_disable_invalidate(ndns);
return 0;
>
> diff --git a/ndctl/region.c b/ndctl/region.c
> index 7945007..0014bb9 100644
> --- a/ndctl/region.c
> +++ b/ndctl/region.c
> @@ -72,6 +72,7 @@ static int region_action(struct ndctl_region *region, enum device_action mode)
> {
> struct ndctl_namespace *ndns;
> int rc = 0;
> + unsigned long long size;
>
> switch (mode) {
> case ACTION_ENABLE:
> @@ -80,7 +81,8 @@ static int region_action(struct ndctl_region *region, enum device_action mode)
> case ACTION_DISABLE:
> ndctl_namespace_foreach(region, ndns) {
> rc = ndctl_namespace_disable_safe(ndns);
> - if (rc)
> + size = ndctl_namespace_get_size(ndns);
> + if (rc && size != 0)
> return rc;
...then you wouldn't need to have this special case here since
ndctl_namespace_disable_safe() will not fail.
_______________________________________________
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] 11+ messages in thread
* RE: [PATCH 1/1] ndctl/namespace: Fix disable-namespace accounting relative to seed devices
2020-04-27 20:31 ` Dan Williams
@ 2020-04-28 1:09 ` Li, Redhairer
2021-01-13 7:07 ` Dan Williams
0 siblings, 1 reply; 11+ messages in thread
From: Li, Redhairer @ 2020-04-28 1:09 UTC (permalink / raw)
To: Williams, Dan J; +Cc: linux-nvdimm
If make this return 0 in the case when size is zero, then seed device will be counted in due to
case ACTION_DISABLE:
rc = ndctl_namespace_disable_safe(ndns);
if (rc == 0)
(*processed)++;
break;
I ever think make it return 1.
It also can return correct number which not count in seed device.
But there will be no error msg when I disable seed device.
It will make user confuse.
Eg.
root@ubuntu-red:~/git/ndctl# ndctl/ndctl disable-namespace namespce0.0
disabled 0 namespace
So I follow enable-namespace to return -ENXIO and it will look like
root@ubuntu-red:~/git/ndctl# ndctl/ndctl disable-namespace namespce0.0
error disabling namespaces: No such device or address
disabled 0 namespaces
But no matter return -ENXIO or 1, it will make test/create.sh fail. This is why I modify region.c
+ modprobe nfit_test
+ ../ndctl/ndctl disable-region -b nfit_test.0 all
disabled 0 regions
+ ../ndctl/ndctl zero-labels -b nfit_test.0 all
nmem1: regions active, abort label write
nmem3: regions active, abort label write
nmem0: regions active, abort label write
nmem2: regions active, abort label write
zeroed 0 nmem
++ err 28
+++ basename ./create.sh
++ echo test/create.sh: failed at line 28
-----Original Message-----
From: Dan Williams <dan.j.williams@intel.com>
Sent: Tuesday, April 28, 2020 4:31 AM
To: Li, Redhairer <redhairer.li@intel.com>
Cc: linux-nvdimm <linux-nvdimm@lists.01.org>
Subject: Re: [PATCH 1/1] ndctl/namespace: Fix disable-namespace accounting relative to seed devices
On Sun, Apr 26, 2020 at 2:53 AM Redhairer Li <redhairer.li@intel.com> wrote:
>
> Seed namespaces are included in "ndctl disable-namespace all". However
> since the user never "creates" them it is surprising to see
> "disable-namespace" report 1 more namespace relative to the number
> that have been created. Catch attempts to disable a zero-sized namespace:
>
> Before:
> {
> "dev":"namespace1.0",
> "size":"492.00 MiB (515.90 MB)",
> "blockdev":"pmem1"
> }
> {
> "dev":"namespace1.1",
> "size":"492.00 MiB (515.90 MB)",
> "blockdev":"pmem1.1"
> }
> {
> "dev":"namespace1.2",
> "size":"492.00 MiB (515.90 MB)",
> "blockdev":"pmem1.2"
> }
> disabled 4 namespaces
>
> After:
> {
> "dev":"namespace1.0",
> "size":"492.00 MiB (515.90 MB)",
> "blockdev":"pmem1"
> }
> {
> "dev":"namespace1.3",
> "size":"492.00 MiB (515.90 MB)",
> "blockdev":"pmem1.3"
> }
> {
> "dev":"namespace1.1",
> "size":"492.00 MiB (515.90 MB)",
> "blockdev":"pmem1.1"
> }
> disabled 3 namespaces
>
> Signed-off-by: Redhairer Li <redhairer.li@intel.com>
> ---
> ndctl/lib/libndctl.c | 11 ++++++++---
> ndctl/region.c | 4 +++-
> 2 files changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c index
> ee737cb..49f362b 100644
> --- a/ndctl/lib/libndctl.c
> +++ b/ndctl/lib/libndctl.c
> @@ -4231,6 +4231,7 @@ NDCTL_EXPORT int ndctl_namespace_disable_safe(struct ndctl_namespace *ndns)
> const char *bdev = NULL;
> char path[50];
> int fd;
> + unsigned long long size = ndctl_namespace_get_size(ndns);
>
> if (pfn && ndctl_pfn_is_enabled(pfn))
> bdev = ndctl_pfn_get_block_device(pfn); @@ -4260,9
> +4261,13 @@ NDCTL_EXPORT int ndctl_namespace_disable_safe(struct ndctl_namespace *ndns)
> devname, bdev, strerror(errno));
> return -errno;
> }
> - } else
> - ndctl_namespace_disable_invalidate(ndns);
> -
> + } else {
> + if (size == 0)
> + /* Don't try to disable idle namespace (no capacity allocated) */
> + return -ENXIO;
> + else
> + ndctl_namespace_disable_invalidate(ndns);
> + }
Maybe make this return 0 in the case when size is zero since by definition the namespace must already be disabled if it has zero size.
} else if (size)
ndctl_namespace_disable_invalidate(ndns);
return 0;
>
> diff --git a/ndctl/region.c b/ndctl/region.c index 7945007..0014bb9
> 100644
> --- a/ndctl/region.c
> +++ b/ndctl/region.c
> @@ -72,6 +72,7 @@ static int region_action(struct ndctl_region
> *region, enum device_action mode) {
> struct ndctl_namespace *ndns;
> int rc = 0;
> + unsigned long long size;
>
> switch (mode) {
> case ACTION_ENABLE:
> @@ -80,7 +81,8 @@ static int region_action(struct ndctl_region *region, enum device_action mode)
> case ACTION_DISABLE:
> ndctl_namespace_foreach(region, ndns) {
> rc = ndctl_namespace_disable_safe(ndns);
> - if (rc)
> + size = ndctl_namespace_get_size(ndns);
> + if (rc && size != 0)
> return rc;
...then you wouldn't need to have this special case here since
ndctl_namespace_disable_safe() will not fail.
_______________________________________________
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] 11+ messages in thread
* Re: [PATCH 1/1] ndctl/namespace: Fix disable-namespace accounting relative to seed devices
2020-04-28 1:09 ` Li, Redhairer
@ 2021-01-13 7:07 ` Dan Williams
2021-01-15 14:50 ` redhairer
2021-01-15 17:22 ` Li, Redhairer
0 siblings, 2 replies; 11+ messages in thread
From: Dan Williams @ 2021-01-13 7:07 UTC (permalink / raw)
To: Li, Redhairer; +Cc: linux-nvdimm
Hi Redhairer, sorry for the long delay circling back to this...
On Mon, Apr 27, 2020 at 6:10 PM Li, Redhairer <redhairer.li@intel.com> wrote:
>
> If make this return 0 in the case when size is zero, then seed device will be counted in due to
> case ACTION_DISABLE:
> rc = ndctl_namespace_disable_safe(ndns);
> if (rc == 0)
> (*processed)++;
> break;
> I ever think make it return 1.
I think returning 1 is the right answer, because this isn't a failure
and there are several other places that call
ndctl_namespace_disable_safe() that need to be updated to treat rc < 0
as failure and rc >= 0 as success / no disable necessary.
ndctl/namespace.c:1127: rc = ndctl_namespace_disable_safe(ndns);
ndctl/namespace.c-1128- if (rc)
--
ndctl/namespace.c:1433: rc = ndctl_namespace_disable_safe(ndns);
ndctl/namespace.c-1434- if (rc) {
--
ndctl/namespace.c:1457: rc = ndctl_namespace_disable_safe(ndns);
ndctl/namespace.c-1458- if (rc) {
--
ndctl/namespace.c:1480: rc = ndctl_namespace_disable_safe(ndns);
ndctl/namespace.c-1481- if (rc) {
--
ndctl/namespace.c:2215: rc =
ndctl_namespace_disable_safe(ndns);
ndctl/namespace.c-2216- if (rc == 0)
--
ndctl/region.c:72: rc = ndctl_namespace_disable_safe(ndns);
ndctl/region.c-73- if (rc)
> It also can return correct number which not count in seed device.
> But there will be no error msg when I disable seed device.
> It will make user confuse.
> Eg.
> root@ubuntu-red:~/git/ndctl# ndctl/ndctl disable-namespace namespce0.0
> disabled 0 namespace
Why is that confusing isn't the namespace already disabled?
>
> So I follow enable-namespace to return -ENXIO and it will look like
> root@ubuntu-red:~/git/ndctl# ndctl/ndctl disable-namespace namespce0.0
> error disabling namespaces: No such device or address
> disabled 0 namespaces
>
> But no matter return -ENXIO or 1, it will make test/create.sh fail. This is why I modify region.c
region.c and several other places need to be updated. I would split
this into 2 patches.
The first patch updates all callers of ndctl_disable_namespace_safe()
to treat rc > 0 as success.
Then the second patch can treat cmd_disable_namespace() to not count
rc > 0 as error, but also don't count it as disabled.
_______________________________________________
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] 11+ messages in thread
* [PATCH 1/1] ndctl/namespace: Fix disable-namespace accounting relative to seed devices
2021-01-13 7:07 ` Dan Williams
@ 2021-01-15 14:50 ` redhairer
2021-01-15 17:22 ` Li, Redhairer
1 sibling, 0 replies; 11+ messages in thread
From: redhairer @ 2021-01-15 14:50 UTC (permalink / raw)
To: linux-nvdimm, dan.j.williams; +Cc: Redhairer Li
From: Redhairer Li <redhairer.li@intel.com>
Seed namespaces are included in "ndctl disable-namespace all". However
since the user never "creates" them it is surprising to see
"disable-namespace" report 1 more namespace relative to the number that
have been created. Catch attempts to disable a zero-sized namespace:
Before:
{
"dev":"namespace1.0",
"size":"492.00 MiB (515.90 MB)",
"blockdev":"pmem1"
}
{
"dev":"namespace1.1",
"size":"492.00 MiB (515.90 MB)",
"blockdev":"pmem1.1"
}
{
"dev":"namespace1.2",
"size":"492.00 MiB (515.90 MB)",
"blockdev":"pmem1.2"
}
disabled 4 namespaces
After:
{
"dev":"namespace1.0",
"size":"492.00 MiB (515.90 MB)",
"blockdev":"pmem1"
}
{
"dev":"namespace1.3",
"size":"492.00 MiB (515.90 MB)",
"blockdev":"pmem1.3"
}
{
"dev":"namespace1.1",
"size":"492.00 MiB (515.90 MB)",
"blockdev":"pmem1.1"
}
disabled 3 namespaces
Signed-off-by: Redhairer Li <redhairer.li@intel.com>
---
ndctl/lib/libndctl.c | 11 ++++++++---
ndctl/namespace.c | 8 ++++----
ndctl/region.c | 2 +-
3 files changed, 13 insertions(+), 8 deletions(-)
diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
index 5546963..b477098 100644
--- a/ndctl/lib/libndctl.c
+++ b/ndctl/lib/libndctl.c
@@ -4614,6 +4614,7 @@ NDCTL_EXPORT int ndctl_namespace_disable_safe(struct ndctl_namespace *ndns)
const char *bdev = NULL;
char path[50];
int fd;
+ unsigned long long size = ndctl_namespace_get_size(ndns);
if (pfn && ndctl_pfn_is_enabled(pfn))
bdev = ndctl_pfn_get_block_device(pfn);
@@ -4643,9 +4644,13 @@ NDCTL_EXPORT int ndctl_namespace_disable_safe(struct ndctl_namespace *ndns)
devname, bdev, strerror(errno));
return -errno;
}
- } else
- ndctl_namespace_disable_invalidate(ndns);
-
+ } else {
+ if (size == 0)
+ /* No disable necessary due to no capacity allocated */
+ return 1;
+ else
+ ndctl_namespace_disable_invalidate(ndns);
+ }
return 0;
}
diff --git a/ndctl/namespace.c b/ndctl/namespace.c
index e734248..77a75b4 100644
--- a/ndctl/namespace.c
+++ b/ndctl/namespace.c
@@ -1110,7 +1110,7 @@ static int namespace_destroy(struct ndctl_region *region,
return -EBUSY;
} else {
rc = ndctl_namespace_disable_safe(ndns);
- if (rc)
+ if (rc < 0)
return rc;
}
@@ -1395,7 +1395,7 @@ static int dax_clear_badblocks(struct ndctl_dax *dax)
return -ENXIO;
rc = ndctl_namespace_disable_safe(ndns);
- if (rc) {
+ if (rc < 0) {
error("%s: unable to disable namespace: %s\n", devname,
strerror(-rc));
return rc;
@@ -1419,7 +1419,7 @@ static int pfn_clear_badblocks(struct ndctl_pfn *pfn)
return -ENXIO;
rc = ndctl_namespace_disable_safe(ndns);
- if (rc) {
+ if (rc < 0) {
error("%s: unable to disable namespace: %s\n", devname,
strerror(-rc));
return rc;
@@ -1442,7 +1442,7 @@ static int raw_clear_badblocks(struct ndctl_namespace *ndns)
return -ENXIO;
rc = ndctl_namespace_disable_safe(ndns);
- if (rc) {
+ if (rc < 0) {
error("%s: unable to disable namespace: %s\n", devname,
strerror(-rc));
return rc;
diff --git a/ndctl/region.c b/ndctl/region.c
index 7945007..df64fc9 100644
--- a/ndctl/region.c
+++ b/ndctl/region.c
@@ -80,7 +80,7 @@ static int region_action(struct ndctl_region *region, enum device_action mode)
case ACTION_DISABLE:
ndctl_namespace_foreach(region, ndns) {
rc = ndctl_namespace_disable_safe(ndns);
- if (rc)
+ if (rc < 0)
return rc;
}
rc = ndctl_region_disable_invalidate(region);
--
2.27.0.windows.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] 11+ messages in thread
* RE: [PATCH 1/1] ndctl/namespace: Fix disable-namespace accounting relative to seed devices
2021-01-13 7:07 ` Dan Williams
2021-01-15 14:50 ` redhairer
@ 2021-01-15 17:22 ` Li, Redhairer
2021-01-28 8:47 ` Dan Williams
1 sibling, 1 reply; 11+ messages in thread
From: Li, Redhairer @ 2021-01-15 17:22 UTC (permalink / raw)
To: Williams, Dan J; +Cc: linux-nvdimm
Hi Dan,
Your comment is make sense.
ndctl_namespace_disable_safe will return 1 if namespace size is 0.
I send a new patch out for review.
But I am not sure what do you mean for 2nd patch.
In cmd_disable_namespace, it already print error if rc<0.
rc = do_xaction_namespace(namespace, ACTION_DISABLE, ctx, &disabled);
if (rc < 0 && !err_count)
fprintf(stderr, "error disabling namespaces: %s\n",
strerror(-rc));
my patch is based on v70 due to latest one will see "FAIL: create.sh" when make check even not include my change.
-----Original Message-----
From: Dan Williams <dan.j.williams@intel.com>
Sent: Wednesday, January 13, 2021 3:08 PM
To: Li, Redhairer <redhairer.li@intel.com>
Cc: linux-nvdimm <linux-nvdimm@lists.01.org>
Subject: Re: [PATCH 1/1] ndctl/namespace: Fix disable-namespace accounting relative to seed devices
Hi Redhairer, sorry for the long delay circling back to this...
On Mon, Apr 27, 2020 at 6:10 PM Li, Redhairer <redhairer.li@intel.com> wrote:
>
> If make this return 0 in the case when size is zero, then seed device
> will be counted in due to case ACTION_DISABLE:
> rc = ndctl_namespace_disable_safe(ndns);
> if (rc == 0)
> (*processed)++;
> break;
> I ever think make it return 1.
I think returning 1 is the right answer, because this isn't a failure and there are several other places that call
ndctl_namespace_disable_safe() that need to be updated to treat rc < 0 as failure and rc >= 0 as success / no disable necessary.
ndctl/namespace.c:1127: rc = ndctl_namespace_disable_safe(ndns);
ndctl/namespace.c-1128- if (rc)
--
ndctl/namespace.c:1433: rc = ndctl_namespace_disable_safe(ndns);
ndctl/namespace.c-1434- if (rc) {
--
ndctl/namespace.c:1457: rc = ndctl_namespace_disable_safe(ndns);
ndctl/namespace.c-1458- if (rc) {
--
ndctl/namespace.c:1480: rc = ndctl_namespace_disable_safe(ndns);
ndctl/namespace.c-1481- if (rc) {
--
ndctl/namespace.c:2215: rc =
ndctl_namespace_disable_safe(ndns);
ndctl/namespace.c-2216- if (rc == 0)
--
ndctl/region.c:72: rc = ndctl_namespace_disable_safe(ndns);
ndctl/region.c-73- if (rc)
> It also can return correct number which not count in seed device.
> But there will be no error msg when I disable seed device.
> It will make user confuse.
> Eg.
> root@ubuntu-red:~/git/ndctl# ndctl/ndctl disable-namespace namespce0.0
> disabled 0 namespace
Why is that confusing isn't the namespace already disabled?
>
> So I follow enable-namespace to return -ENXIO and it will look like
> root@ubuntu-red:~/git/ndctl# ndctl/ndctl disable-namespace namespce0.0
> error disabling namespaces: No such device or address disabled 0
> namespaces
>
> But no matter return -ENXIO or 1, it will make test/create.sh fail.
> This is why I modify region.c
region.c and several other places need to be updated. I would split this into 2 patches.
The first patch updates all callers of ndctl_disable_namespace_safe() to treat rc > 0 as success.
Then the second patch can treat cmd_disable_namespace() to not count rc > 0 as error, but also don't count it as disabled.
_______________________________________________
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] 11+ messages in thread
* Re: [PATCH 1/1] ndctl/namespace: Fix disable-namespace accounting relative to seed devices
2021-01-15 17:22 ` Li, Redhairer
@ 2021-01-28 8:47 ` Dan Williams
2021-01-28 14:03 ` redhairer
2021-01-28 16:16 ` Li, Redhairer
0 siblings, 2 replies; 11+ messages in thread
From: Dan Williams @ 2021-01-28 8:47 UTC (permalink / raw)
To: Li, Redhairer; +Cc: linux-nvdimm
On Fri, Jan 15, 2021 at 9:22 AM Li, Redhairer <redhairer.li@intel.com> wrote:
>
> Hi Dan,
>
> Your comment is make sense.
> ndctl_namespace_disable_safe will return 1 if namespace size is 0.
> I send a new patch out for review.
It looks ok but it does not apply to the current tip of tree now v71.1
can you resend?
>
> But I am not sure what do you mean for 2nd patch.
> In cmd_disable_namespace, it already print error if rc<0.
> rc = do_xaction_namespace(namespace, ACTION_DISABLE, ctx, &disabled);
> if (rc < 0 && !err_count)
> fprintf(stderr, "error disabling namespaces: %s\n",
> strerror(-rc));
Hmm, you're right, once you change to the positive error code the
report will just work. Did you give it a try does it fix the
accounting problem with just your first patch?
>
> my patch is based on v70 due to latest one will see "FAIL: create.sh" when make check even not include my change.
I know of at least one create.sh failure that was fixed by:
Kernel commit:
2dd2a1740ee1 libnvdimm/namespace: Fix reaping of invalidated
block-window-namespace labels
...which is now in v5.11-rc1 and backported to v5.10-rc4. However,
that bug only started triggering after ndctl changed to reconfigure
namespaces in place with commit:
d4bc247faeda ndctl/namespace: Reconfigure in-place
..which was only merged into ndctl in v71.
Another kernel change that may be causing your failures is:
d1c5246e08eb x86/mm: Fix leak of pmd ptlock
...which was merged for v5.11-rc3 and backported to v5.10.7.
Can you run latest kernel and ndctl and see if you still see the
create.sh failure?
_______________________________________________
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] 11+ messages in thread
* [PATCH 1/1] ndctl/namespace: Fix disable-namespace accounting relative to seed devices
2021-01-28 8:47 ` Dan Williams
@ 2021-01-28 14:03 ` redhairer
2021-01-28 16:16 ` Li, Redhairer
1 sibling, 0 replies; 11+ messages in thread
From: redhairer @ 2021-01-28 14:03 UTC (permalink / raw)
To: linux-nvdimm, dan.j.williams; +Cc: Redhairer Li
From: Redhairer Li <redhairer.li@intel.com>
Seed namespaces are included in "ndctl disable-namespace all". However
since the user never "creates" them it is surprising to see
"disable-namespace" report 1 more namespace relative to the number that
have been created. Catch attempts to disable a zero-sized namespace:
Before:
{
"dev":"namespace1.0",
"size":"492.00 MiB (515.90 MB)",
"blockdev":"pmem1"
}
{
"dev":"namespace1.1",
"size":"492.00 MiB (515.90 MB)",
"blockdev":"pmem1.1"
}
{
"dev":"namespace1.2",
"size":"492.00 MiB (515.90 MB)",
"blockdev":"pmem1.2"
}
disabled 4 namespaces
After:
{
"dev":"namespace1.0",
"size":"492.00 MiB (515.90 MB)",
"blockdev":"pmem1"
}
{
"dev":"namespace1.3",
"size":"492.00 MiB (515.90 MB)",
"blockdev":"pmem1.3"
}
{
"dev":"namespace1.1",
"size":"492.00 MiB (515.90 MB)",
"blockdev":"pmem1.1"
}
disabled 3 namespaces
Signed-off-by: Redhairer Li <redhairer.li@intel.com>
---
ndctl/lib/libndctl.c | 10 ++++++++--
ndctl/namespace.c | 8 ++++----
ndctl/region.c | 2 +-
3 files changed, 13 insertions(+), 7 deletions(-)
diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
index 36fb6fe..2f6d806 100644
--- a/ndctl/lib/libndctl.c
+++ b/ndctl/lib/libndctl.c
@@ -4602,6 +4602,7 @@ NDCTL_EXPORT int ndctl_namespace_disable_safe(struct ndctl_namespace *ndns)
const char *bdev = NULL;
char path[50];
int fd;
+ unsigned long long size = ndctl_namespace_get_size(ndns);
if (pfn && ndctl_pfn_is_enabled(pfn))
bdev = ndctl_pfn_get_block_device(pfn);
@@ -4631,8 +4632,13 @@ NDCTL_EXPORT int ndctl_namespace_disable_safe(struct ndctl_namespace *ndns)
devname, bdev, strerror(errno));
return -errno;
}
- } else
- ndctl_namespace_disable_invalidate(ndns);
+ } else {
+ if (size == 0)
+ /* No disable necessary due to no capacity allocated */
+ return 1;
+ else
+ ndctl_namespace_disable_invalidate(ndns);
+ }
return 0;
}
diff --git a/ndctl/namespace.c b/ndctl/namespace.c
index 0c8df9f..1feb74d 100644
--- a/ndctl/namespace.c
+++ b/ndctl/namespace.c
@@ -1125,7 +1125,7 @@ static int namespace_prep_reconfig(struct ndctl_region *region,
}
rc = ndctl_namespace_disable_safe(ndns);
- if (rc)
+ if (rc < 0)
return rc;
ndctl_namespace_set_enforce_mode(ndns, NDCTL_NS_MODE_RAW);
@@ -1431,7 +1431,7 @@ static int dax_clear_badblocks(struct ndctl_dax *dax)
return -ENXIO;
rc = ndctl_namespace_disable_safe(ndns);
- if (rc) {
+ if (rc < 0) {
error("%s: unable to disable namespace: %s\n", devname,
strerror(-rc));
return rc;
@@ -1455,7 +1455,7 @@ static int pfn_clear_badblocks(struct ndctl_pfn *pfn)
return -ENXIO;
rc = ndctl_namespace_disable_safe(ndns);
- if (rc) {
+ if (rc < 0) {
error("%s: unable to disable namespace: %s\n", devname,
strerror(-rc));
return rc;
@@ -1478,7 +1478,7 @@ static int raw_clear_badblocks(struct ndctl_namespace *ndns)
return -ENXIO;
rc = ndctl_namespace_disable_safe(ndns);
- if (rc) {
+ if (rc < 0) {
error("%s: unable to disable namespace: %s\n", devname,
strerror(-rc));
return rc;
diff --git a/ndctl/region.c b/ndctl/region.c
index 3edb9b3..4552c4a 100644
--- a/ndctl/region.c
+++ b/ndctl/region.c
@@ -70,7 +70,7 @@ static int region_action(struct ndctl_region *region, enum device_action mode)
case ACTION_DISABLE:
ndctl_namespace_foreach(region, ndns) {
rc = ndctl_namespace_disable_safe(ndns);
- if (rc)
+ if (rc < 0)
return rc;
}
rc = ndctl_region_disable_invalidate(region);
--
2.27.0.windows.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] 11+ messages in thread
* RE: [PATCH 1/1] ndctl/namespace: Fix disable-namespace accounting relative to seed devices
2021-01-28 8:47 ` Dan Williams
2021-01-28 14:03 ` redhairer
@ 2021-01-28 16:16 ` Li, Redhairer
2021-01-29 8:44 ` Li, Redhairer
1 sibling, 1 reply; 11+ messages in thread
From: Li, Redhairer @ 2021-01-28 16:16 UTC (permalink / raw)
To: Williams, Dan J; +Cc: linux-nvdimm
Resend it base on v71.1.
-----Original Message-----
From: Dan Williams <dan.j.williams@intel.com>
Sent: Thursday, January 28, 2021 4:47 PM
To: Li, Redhairer <redhairer.li@intel.com>
Cc: linux-nvdimm <linux-nvdimm@lists.01.org>
Subject: Re: [PATCH 1/1] ndctl/namespace: Fix disable-namespace accounting relative to seed devices
On Fri, Jan 15, 2021 at 9:22 AM Li, Redhairer <redhairer.li@intel.com> wrote:
>
> Hi Dan,
>
> Your comment is make sense.
> ndctl_namespace_disable_safe will return 1 if namespace size is 0.
> I send a new patch out for review.
It looks ok but it does not apply to the current tip of tree now v71.1 can you resend?
>
> But I am not sure what do you mean for 2nd patch.
> In cmd_disable_namespace, it already print error if rc<0.
> rc = do_xaction_namespace(namespace, ACTION_DISABLE, ctx, &disabled);
> if (rc < 0 && !err_count)
> fprintf(stderr, "error disabling namespaces: %s\n",
> strerror(-rc));
Hmm, you're right, once you change to the positive error code the report will just work. Did you give it a try does it fix the accounting problem with just your first patch?
>
> my patch is based on v70 due to latest one will see "FAIL: create.sh" when make check even not include my change.
I know of at least one create.sh failure that was fixed by:
Kernel commit:
2dd2a1740ee1 libnvdimm/namespace: Fix reaping of invalidated block-window-namespace labels
...which is now in v5.11-rc1 and backported to v5.10-rc4. However, that bug only started triggering after ndctl changed to reconfigure namespaces in place with commit:
d4bc247faeda ndctl/namespace: Reconfigure in-place
..which was only merged into ndctl in v71.
Another kernel change that may be causing your failures is:
d1c5246e08eb x86/mm: Fix leak of pmd ptlock
...which was merged for v5.11-rc3 and backported to v5.10.7.
Can you run latest kernel and ndctl and see if you still see the create.sh failure?
_______________________________________________
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] 11+ messages in thread
* RE: [PATCH 1/1] ndctl/namespace: Fix disable-namespace accounting relative to seed devices
2021-01-28 16:16 ` Li, Redhairer
@ 2021-01-29 8:44 ` Li, Redhairer
0 siblings, 0 replies; 11+ messages in thread
From: Li, Redhairer @ 2021-01-29 8:44 UTC (permalink / raw)
To: Williams, Dan J; +Cc: linux-nvdimm
After update kernel to 5.10-rc4, all tests will PASS with "make check"
-----Original Message-----
From: Li, Redhairer
Sent: Friday, January 29, 2021 12:16 AM
To: Dan Williams <dan.j.williams@intel.com>
Cc: linux-nvdimm <linux-nvdimm@lists.01.org>
Subject: RE: [PATCH 1/1] ndctl/namespace: Fix disable-namespace accounting relative to seed devices
Resend it base on v71.1.
-----Original Message-----
From: Dan Williams <dan.j.williams@intel.com>
Sent: Thursday, January 28, 2021 4:47 PM
To: Li, Redhairer <redhairer.li@intel.com>
Cc: linux-nvdimm <linux-nvdimm@lists.01.org>
Subject: Re: [PATCH 1/1] ndctl/namespace: Fix disable-namespace accounting relative to seed devices
On Fri, Jan 15, 2021 at 9:22 AM Li, Redhairer <redhairer.li@intel.com> wrote:
>
> Hi Dan,
>
> Your comment is make sense.
> ndctl_namespace_disable_safe will return 1 if namespace size is 0.
> I send a new patch out for review.
It looks ok but it does not apply to the current tip of tree now v71.1 can you resend?
>
> But I am not sure what do you mean for 2nd patch.
> In cmd_disable_namespace, it already print error if rc<0.
> rc = do_xaction_namespace(namespace, ACTION_DISABLE, ctx, &disabled);
> if (rc < 0 && !err_count)
> fprintf(stderr, "error disabling namespaces: %s\n",
> strerror(-rc));
Hmm, you're right, once you change to the positive error code the report will just work. Did you give it a try does it fix the accounting problem with just your first patch?
>
> my patch is based on v70 due to latest one will see "FAIL: create.sh" when make check even not include my change.
I know of at least one create.sh failure that was fixed by:
Kernel commit:
2dd2a1740ee1 libnvdimm/namespace: Fix reaping of invalidated block-window-namespace labels
...which is now in v5.11-rc1 and backported to v5.10-rc4. However, that bug only started triggering after ndctl changed to reconfigure namespaces in place with commit:
d4bc247faeda ndctl/namespace: Reconfigure in-place
..which was only merged into ndctl in v71.
Another kernel change that may be causing your failures is:
d1c5246e08eb x86/mm: Fix leak of pmd ptlock
...which was merged for v5.11-rc3 and backported to v5.10.7.
Can you run latest kernel and ndctl and see if you still see the create.sh failure?
_______________________________________________
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] 11+ messages in thread
* [PATCH 1/1] ndctl/namespace: Fix disable-namespace accounting relative to seed devices
@ 2020-05-10 15:32 Redhairer Li
0 siblings, 0 replies; 11+ messages in thread
From: Redhairer Li @ 2020-05-10 15:32 UTC (permalink / raw)
To: linux-nvdimm, dan.j.williams; +Cc: Redhairer Li
Seed namespaces are included in "ndctl disable-namespace all". However
since the user never "creates" them it is surprising to see
"disable-namespace" report 1 more namespace relative to the number that
have been created. Catch attempts to disable a zero-sized namespace:
Before:
{
"dev":"namespace1.0",
"size":"492.00 MiB (515.90 MB)",
"blockdev":"pmem1"
}
{
"dev":"namespace1.1",
"size":"492.00 MiB (515.90 MB)",
"blockdev":"pmem1.1"
}
{
"dev":"namespace1.2",
"size":"492.00 MiB (515.90 MB)",
"blockdev":"pmem1.2"
}
disabled 4 namespaces
After:
{
"dev":"namespace1.0",
"size":"492.00 MiB (515.90 MB)",
"blockdev":"pmem1"
}
{
"dev":"namespace1.3",
"size":"492.00 MiB (515.90 MB)",
"blockdev":"pmem1.3"
}
{
"dev":"namespace1.1",
"size":"492.00 MiB (515.90 MB)",
"blockdev":"pmem1.1"
}
disabled 3 namespaces
Signed-off-by: Redhairer Li <redhairer.li@intel.com>
---
ndctl/namespace.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/ndctl/namespace.c b/ndctl/namespace.c
index 0550580..1bfe736 100644
--- a/ndctl/namespace.c
+++ b/ndctl/namespace.c
@@ -2034,6 +2034,7 @@ static int do_xaction_namespace(const char *namespace,
struct ndctl_region *region;
const char *ndns_name;
struct ndctl_bus *bus;
+ unsigned long long size;
*processed = 0;
@@ -2134,7 +2135,8 @@ static int do_xaction_namespace(const char *namespace,
switch (action) {
case ACTION_DISABLE:
rc = ndctl_namespace_disable_safe(ndns);
- if (rc == 0)
+ size = ndctl_namespace_get_size(ndns);
+ if (rc == 0 && size != 0)
(*processed)++;
break;
case ACTION_ENABLE:
--
2.20.1.windows.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] 11+ messages in thread
end of thread, other threads:[~2021-01-29 8:44 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-26 9:52 [PATCH 1/1] ndctl/namespace: Fix disable-namespace accounting relative to seed devices Redhairer Li
2020-04-27 20:31 ` Dan Williams
2020-04-28 1:09 ` Li, Redhairer
2021-01-13 7:07 ` Dan Williams
2021-01-15 14:50 ` redhairer
2021-01-15 17:22 ` Li, Redhairer
2021-01-28 8:47 ` Dan Williams
2021-01-28 14:03 ` redhairer
2021-01-28 16:16 ` Li, Redhairer
2021-01-29 8:44 ` Li, Redhairer
2020-05-10 15:32 Redhairer Li
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).