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

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

[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

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

[Dan Williams]

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

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

[Li, Redhairer]

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

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

[Dan Williams]

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

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

[redhairer]

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

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

[Li, Redhairer]

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

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

[Dan Williams]

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

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

[redhairer]

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

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

[Li, Redhairer]

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

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

[Li, Redhairer]

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

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

[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