| Message ID | 20200426095232.27524-1-redhairer.li@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/1] ndctl/namespace: Fix disable-namespace accounting relative to seed devices | expand |
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.
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.
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.
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.
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?
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?
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?
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);
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(-)