RE: [PATCH 1/1] ndctl/namespace: Fix disable-namespace accounting relative to seed devices

["Li, Redhairer" <redhairer.li@intel.com>]

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