[PATCH] nvdimm: Remove minimum size requirement

[PATCH] nvdimm: Remove minimum size requirement

[Matthew Wilcox]

From: Matthew Wilcox <mawilcox@microsoft.com>

There was no need to have a minimum size of 4MB for NV-DIMMs; it was
just a sanity check.  Keep a check that it's at least one page in size
because we really can't add less than a page to the memory map.  Promote
the print statement from 'debug' level to 'warning', since there was no
information for my colleague who stumbled over this problem while
attempting to add a 2MB chunk of memory.

Reported-by: Cheng-mean Liu <soccerl@microsoft.com>
Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
---
 drivers/nvdimm/namespace_devs.c | 6 +++---
 include/uapi/linux/ndctl.h      | 4 ----
 2 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/nvdimm/namespace_devs.c b/drivers/nvdimm/namespace_devs.c
index 5f1c6756e57c..95169308078a 100644
--- a/drivers/nvdimm/namespace_devs.c
+++ b/drivers/nvdimm/namespace_devs.c
@@ -1689,9 +1689,9 @@ struct nd_namespace_common *nvdimm_namespace_common_probe(struct device *dev)
 	}
 
 	size = nvdimm_namespace_capacity(ndns);
-	if (size < ND_MIN_NAMESPACE_SIZE) {
-		dev_dbg(&ndns->dev, "%pa, too small must be at least %#x\n",
-				&size, ND_MIN_NAMESPACE_SIZE);
+	if (size < PAGE_SIZE) {
+		dev_warn(&ndns->dev, "%pa, too small must be at least %ld\n",
+				&size, PAGE_SIZE);
 		return ERR_PTR(-ENODEV);
 	}
 
diff --git a/include/uapi/linux/ndctl.h b/include/uapi/linux/ndctl.h
index 6d3c54264d8e..3ad1623bb585 100644
--- a/include/uapi/linux/ndctl.h
+++ b/include/uapi/linux/ndctl.h
@@ -299,10 +299,6 @@ enum nd_driver_flags {
 	ND_DRIVER_DAX_PMEM	  = 1 << ND_DEVICE_DAX_PMEM,
 };
 
-enum {
-	ND_MIN_NAMESPACE_SIZE = 0x00400000,
-};
-
 enum ars_masks {
 	ARS_STATUS_MASK = 0x0000FFFF,
 	ARS_EXT_STATUS_SHIFT = 16,
-- 
2.11.0

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH] nvdimm: Remove minimum size requirement

[Dan Williams]

On Mon, Jul 10, 2017 at 11:32 AM, Matthew Wilcox <willy@infradead.org> wrote:
> From: Matthew Wilcox <mawilcox@microsoft.com>
>
> There was no need to have a minimum size of 4MB for NV-DIMMs; it was
> just a sanity check.  Keep a check that it's at least one page in size
> because we really can't add less than a page to the memory map.  Promote
> the print statement from 'debug' level to 'warning', since there was no
> information for my colleague who stumbled over this problem while
> attempting to add a 2MB chunk of memory.
>
> Reported-by: Cheng-mean Liu <soccerl@microsoft.com>
> Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
> ---
>  drivers/nvdimm/namespace_devs.c | 6 +++---
>  include/uapi/linux/ndctl.h      | 4 ----
>  2 files changed, 3 insertions(+), 7 deletions(-)

Looks good to me, just one fix up:

>
> diff --git a/drivers/nvdimm/namespace_devs.c b/drivers/nvdimm/namespace_devs.c
> index 5f1c6756e57c..95169308078a 100644
> --- a/drivers/nvdimm/namespace_devs.c
> +++ b/drivers/nvdimm/namespace_devs.c
> @@ -1689,9 +1689,9 @@ struct nd_namespace_common *nvdimm_namespace_common_probe(struct device *dev)
>         }
>
>         size = nvdimm_namespace_capacity(ndns);
> -       if (size < ND_MIN_NAMESPACE_SIZE) {
> -               dev_dbg(&ndns->dev, "%pa, too small must be at least %#x\n",
> -                               &size, ND_MIN_NAMESPACE_SIZE);
> +       if (size < PAGE_SIZE) {

If we're going to change the print level away from 'debug' then I
think this should be "if (size && size < PAGE_SIZE)". The sub-system
pre-creates 0-sized devices that are later configured into full
namespaces, in those cases we shouldn't fire the warning.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH] nvdimm: Remove minimum size requirement

[Matthew Wilcox]

On Mon, Jul 10, 2017 at 11:41:08AM -0700, Dan Williams wrote:
> >         size = nvdimm_namespace_capacity(ndns);
> > -       if (size < ND_MIN_NAMESPACE_SIZE) {
> > -               dev_dbg(&ndns->dev, "%pa, too small must be at least %#x\n",
> > -                               &size, ND_MIN_NAMESPACE_SIZE);
> > +       if (size < PAGE_SIZE) {
> 
> If we're going to change the print level away from 'debug' then I
> think this should be "if (size && size < PAGE_SIZE)". The sub-system
> pre-creates 0-sized devices that are later configured into full
> namespaces, in those cases we shouldn't fire the warning.

Should we allow drivers to claim zero-sized devices?  If not, then the
dev_warn() should be predicated on size being non-zero, but the return
-ENODEV should still trigger.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

RE: [PATCH] nvdimm: Remove minimum size requirement

[Cheng-mean Liu (SOCCER)]

I have tough time to image any scenarios that could make sense a zero-sized device.

-----Original Message-----
From: Matthew Wilcox [mailto:willy-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org] 
Sent: Monday, July 10, 2017 1:30 PM
To: Dan Williams <dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Cc: linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw@public.gmane.org; Cheng-mean Liu (SOCCER) <soccerl-0li6OtcxBFHby3iVrkZq2A@public.gmane.org>; Matthew Wilcox <mawilcox-0li6OtcxBFHby3iVrkZq2A@public.gmane.org>
Subject: Re: [PATCH] nvdimm: Remove minimum size requirement

On Mon, Jul 10, 2017 at 11:41:08AM -0700, Dan Williams wrote:
> >         size = nvdimm_namespace_capacity(ndns);
> > -       if (size < ND_MIN_NAMESPACE_SIZE) {
> > -               dev_dbg(&ndns->dev, "%pa, too small must be at least %#x\n",
> > -                               &size, ND_MIN_NAMESPACE_SIZE);
> > +       if (size < PAGE_SIZE) {
> 
> If we're going to change the print level away from 'debug' then I 
> think this should be "if (size && size < PAGE_SIZE)". The sub-system 
> pre-creates 0-sized devices that are later configured into full 
> namespaces, in those cases we shouldn't fire the warning.

Should we allow drivers to claim zero-sized devices?  If not, then the
dev_warn() should be predicated on size being non-zero, but the return -ENODEV should still trigger.

Re: [PATCH] nvdimm: Remove minimum size requirement

[Dan Williams]

On Mon, Jul 10, 2017 at 1:30 PM, Matthew Wilcox <willy@infradead.org> wrote:
> On Mon, Jul 10, 2017 at 11:41:08AM -0700, Dan Williams wrote:
>> >         size = nvdimm_namespace_capacity(ndns);
>> > -       if (size < ND_MIN_NAMESPACE_SIZE) {
>> > -               dev_dbg(&ndns->dev, "%pa, too small must be at least %#x\n",
>> > -                               &size, ND_MIN_NAMESPACE_SIZE);
>> > +       if (size < PAGE_SIZE) {
>>
>> If we're going to change the print level away from 'debug' then I
>> think this should be "if (size && size < PAGE_SIZE)". The sub-system
>> pre-creates 0-sized devices that are later configured into full
>> namespaces, in those cases we shouldn't fire the warning.
>
> Should we allow drivers to claim zero-sized devices?  If not, then the
> dev_warn() should be predicated on size being non-zero, but the return
> -ENODEV should still trigger.

You're right. It should return -ENODEV regardless, but the warning
should be for non-zero too small namespaces.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH] nvdimm: Remove minimum size requirement

[Matthew Wilcox]

> You're right. It should return -ENODEV regardless, but the warning
> should be for non-zero too small namespaces.

OK, try this:

---- 8< ----

From: Matthew Wilcox <mawilcox@microsoft.com>
Date: Mon, 10 Jul 2017 14:26:59 -0400
Subject: [PATCH] nvdimm: Remove minimum size requirement

There was no need to have a minimum size of 4MB for NV-DIMMs; it was
just a sanity check.  Keep a check that it's at least one page in size
because we really can't add less than a page to the memory map.

Promote the print statement from 'debug' level to 'warning', since there
was no information for my colleague who stumbled over this problem while
attempting to add a 2MB chunk of memory.  There may be some zero-sized
devices on the list, and those are not warned about.

Truncate the resource_size_t to an int and print it using %d rather
than printing it in hex as a pointer to an address.  We know it's less
than PAGE_SIZE, so it'll fit in an int.

Reported-by: Cheng-mean Liu <soccerl@microsoft.com>
Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
---
 drivers/nvdimm/namespace_devs.c | 7 ++++---
 include/uapi/linux/ndctl.h      | 4 ----
 2 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/nvdimm/namespace_devs.c b/drivers/nvdimm/namespace_devs.c
index 5f1c6756e57c..518c3507f67f 100644
--- a/drivers/nvdimm/namespace_devs.c
+++ b/drivers/nvdimm/namespace_devs.c
@@ -1689,9 +1689,10 @@ struct nd_namespace_common *nvdimm_namespace_common_probe(struct device *dev)
 	}
 
 	size = nvdimm_namespace_capacity(ndns);
-	if (size < ND_MIN_NAMESPACE_SIZE) {
-		dev_dbg(&ndns->dev, "%pa, too small must be at least %#x\n",
-				&size, ND_MIN_NAMESPACE_SIZE);
+	if (size < PAGE_SIZE) {
+		if (size)
+			dev_warn(&ndns->dev, "%d too small must be at least %ld\n",
+					(int)size, PAGE_SIZE);
 		return ERR_PTR(-ENODEV);
 	}
 
diff --git a/include/uapi/linux/ndctl.h b/include/uapi/linux/ndctl.h
index 6d3c54264d8e..3ad1623bb585 100644
--- a/include/uapi/linux/ndctl.h
+++ b/include/uapi/linux/ndctl.h
@@ -299,10 +299,6 @@ enum nd_driver_flags {
 	ND_DRIVER_DAX_PMEM	  = 1 << ND_DEVICE_DAX_PMEM,
 };
 
-enum {
-	ND_MIN_NAMESPACE_SIZE = 0x00400000,
-};
-
 enum ars_masks {
 	ARS_STATUS_MASK = 0x0000FFFF,
 	ARS_EXT_STATUS_SHIFT = 16,
-- 
2.11.0

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH] nvdimm: Remove minimum size requirement

[Dan Williams]

On Wed, Jul 12, 2017 at 8:04 AM, Matthew Wilcox <willy@infradead.org> wrote:
>> You're right. It should return -ENODEV regardless, but the warning
>> should be for non-zero too small namespaces.
>
> OK, try this:
>
> ---- 8< ----
>
> From: Matthew Wilcox <mawilcox@microsoft.com>
> Date: Mon, 10 Jul 2017 14:26:59 -0400
> Subject: [PATCH] nvdimm: Remove minimum size requirement
>
> There was no need to have a minimum size of 4MB for NV-DIMMs; it was
> just a sanity check.  Keep a check that it's at least one page in size
> because we really can't add less than a page to the memory map.
>
> Promote the print statement from 'debug' level to 'warning', since there
> was no information for my colleague who stumbled over this problem while
> attempting to add a 2MB chunk of memory.  There may be some zero-sized
> devices on the list, and those are not warned about.
>
> Truncate the resource_size_t to an int and print it using %d rather
> than printing it in hex as a pointer to an address.  We know it's less
> than PAGE_SIZE, so it'll fit in an int.
>
> Reported-by: Cheng-mean Liu <soccerl@microsoft.com>
> Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
> ---
>  drivers/nvdimm/namespace_devs.c | 7 ++++---
>  include/uapi/linux/ndctl.h      | 4 ----
>  2 files changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/nvdimm/namespace_devs.c b/drivers/nvdimm/namespace_devs.c
> index 5f1c6756e57c..518c3507f67f 100644
> --- a/drivers/nvdimm/namespace_devs.c
> +++ b/drivers/nvdimm/namespace_devs.c
> @@ -1689,9 +1689,10 @@ struct nd_namespace_common *nvdimm_namespace_common_probe(struct device *dev)
>         }
>
>         size = nvdimm_namespace_capacity(ndns);
> -       if (size < ND_MIN_NAMESPACE_SIZE) {
> -               dev_dbg(&ndns->dev, "%pa, too small must be at least %#x\n",
> -                               &size, ND_MIN_NAMESPACE_SIZE);
> +       if (size < PAGE_SIZE) {
> +               if (size)
> +                       dev_warn(&ndns->dev, "%d too small must be at least %ld\n",
> +                                       (int)size, PAGE_SIZE);
>                 return ERR_PTR(-ENODEV);
>         }
>
> diff --git a/include/uapi/linux/ndctl.h b/include/uapi/linux/ndctl.h
> index 6d3c54264d8e..3ad1623bb585 100644
> --- a/include/uapi/linux/ndctl.h
> +++ b/include/uapi/linux/ndctl.h
> @@ -299,10 +299,6 @@ enum nd_driver_flags {
>         ND_DRIVER_DAX_PMEM        = 1 << ND_DEVICE_DAX_PMEM,
>  };
>
> -enum {
> -       ND_MIN_NAMESPACE_SIZE = 0x00400000,
> -};
> -

I ran this patch through the unit tests. We'll need to keep this
definition around until we can kill it's usage in ndctl since it's
part of the uapi. I also need to go fix up the dpa-alloc test to
understand the new assumptions.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH] nvdimm: Remove minimum size requirement

[Socer Liu]

Thanks great to hear.

Dan,
   Could you share the patch you ran though the unittests?

I would like to give the new patch a try in my scenario.

Thanks
Cheng-mean






> On Jul 12, 2017, at 10:13 AM, Dan Williams <dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
> 
> On Wed, Jul 12, 2017 at 8:04 AM, Matthew Wilcox <willy-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org> wrote:
>>> You're right. It should return -ENODEV regardless, but the warning
>>> should be for non-zero too small namespaces.
>> 
>> OK, try this:
>> 
>> ---- 8< ----
>> 
>> From: Matthew Wilcox <mawilcox-0li6OtcxBFHby3iVrkZq2A@public.gmane.org>
>> Date: Mon, 10 Jul 2017 14:26:59 -0400
>> Subject: [PATCH] nvdimm: Remove minimum size requirement
>> 
>> There was no need to have a minimum size of 4MB for NV-DIMMs; it was
>> just a sanity check.  Keep a check that it's at least one page in size
>> because we really can't add less than a page to the memory map.
>> 
>> Promote the print statement from 'debug' level to 'warning', since there
>> was no information for my colleague who stumbled over this problem while
>> attempting to add a 2MB chunk of memory.  There may be some zero-sized
>> devices on the list, and those are not warned about.
>> 
>> Truncate the resource_size_t to an int and print it using %d rather
>> than printing it in hex as a pointer to an address.  We know it's less
>> than PAGE_SIZE, so it'll fit in an int.
>> 
>> Reported-by: Cheng-mean Liu <soccerl-0li6OtcxBFHby3iVrkZq2A@public.gmane.org>
>> Signed-off-by: Matthew Wilcox <mawilcox-0li6OtcxBFHby3iVrkZq2A@public.gmane.org>
>> ---
>> drivers/nvdimm/namespace_devs.c | 7 ++++---
>> include/uapi/linux/ndctl.h      | 4 ----
>> 2 files changed, 4 insertions(+), 7 deletions(-)
>> 
>> diff --git a/drivers/nvdimm/namespace_devs.c b/drivers/nvdimm/namespace_devs.c
>> index 5f1c6756e57c..518c3507f67f 100644
>> --- a/drivers/nvdimm/namespace_devs.c
>> +++ b/drivers/nvdimm/namespace_devs.c
>> @@ -1689,9 +1689,10 @@ struct nd_namespace_common *nvdimm_namespace_common_probe(struct device *dev)
>>        }
>> 
>>        size = nvdimm_namespace_capacity(ndns);
>> -       if (size < ND_MIN_NAMESPACE_SIZE) {
>> -               dev_dbg(&ndns->dev, "%pa, too small must be at least %#x\n",
>> -                               &size, ND_MIN_NAMESPACE_SIZE);
>> +       if (size < PAGE_SIZE) {
>> +               if (size)
>> +                       dev_warn(&ndns->dev, "%d too small must be at least %ld\n",
>> +                                       (int)size, PAGE_SIZE);
>>                return ERR_PTR(-ENODEV);
>>        }
>> 
>> diff --git a/include/uapi/linux/ndctl.h b/include/uapi/linux/ndctl.h
>> index 6d3c54264d8e..3ad1623bb585 100644
>> --- a/include/uapi/linux/ndctl.h
>> +++ b/include/uapi/linux/ndctl.h
>> @@ -299,10 +299,6 @@ enum nd_driver_flags {
>>        ND_DRIVER_DAX_PMEM        = 1 << ND_DEVICE_DAX_PMEM,
>> };
>> 
>> -enum {
>> -       ND_MIN_NAMESPACE_SIZE = 0x00400000,
>> -};
>> -
> 
> I ran this patch through the unit tests. We'll need to keep this
> definition around until we can kill it's usage in ndctl since it's
> part of the uapi. I also need to go fix up the dpa-alloc test to
> understand the new assumptions.
> _______________________________________________
> Linux-nvdimm mailing list
> Linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw@public.gmane.org
> https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH] nvdimm: Remove minimum size requirement

[Dan Williams]

On Wed, Jul 12, 2017 at 10:49 PM, Socer Liu <soccer_liu@yahoo.com> wrote:
> Thanks great to hear.
>
> Dan,
>    Could you share the patch you ran though the unittests?

That patch is still failing the unit tests, so I have some more work
to do to fix the test to understand the new minimum.

> I would like to give the new patch a try in my scenario.

I'll share it when I have the tests running again.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

RE: [PATCH] nvdimm: Remove minimum size requirement

[Cheng-mean Liu (SOCCER)]

Hi Dan:

   I am wondering if failing on those unittests is still an issue for this minimum size requirement change.

Thanks
Cheng-mean

-----Original Message-----
From: Dan Williams [mailto:dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org] 
Sent: Thursday, July 13, 2017 5:14 PM
To: Socer Liu <soccer_liu-/E1597aS9LQAvxtiuMwx3w@public.gmane.org>
Cc: Matthew Wilcox <willy-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>; Cheng-mean Liu (SOCCER) <soccerl-0li6OtcxBFHby3iVrkZq2A@public.gmane.org>; Matthew Wilcox <mawilcox-0li6OtcxBFHby3iVrkZq2A@public.gmane.org>; linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw@public.gmane.org
Subject: Re: [PATCH] nvdimm: Remove minimum size requirement

On Wed, Jul 12, 2017 at 10:49 PM, Socer Liu <soccer_liu-/E1597aS9LQAvxtiuMwx3w@public.gmane.org> wrote:
> Thanks great to hear.
>
> Dan,
>    Could you share the patch you ran though the unittests?

That patch is still failing the unit tests, so I have some more work to do to fix the test to understand the new minimum.

> I would like to give the new patch a try in my scenario.

I'll share it when I have the tests running again.

Re: [PATCH] nvdimm: Remove minimum size requirement

[Dan Williams]

On Mon, Aug 7, 2017 at 11:09 AM, Cheng-mean Liu (SOCCER)
<soccerl@microsoft.com> wrote:
> Hi Dan:
>
>    I am wondering if failing on those unittests is still an issue for this minimum size requirement change.

Yes, I just haven't had a chance to circle back and get this fixed up.

You can reproduce by running:

    make TESTS=dpa-alloc check

...in a checkout of the ndctl project: https://github.com/pmem/ndctl

If you attempt that, note the required setup of the nfit_test modules
documented in README.md in that same repository.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH] nvdimm: Remove minimum size requirement

[Dan Williams]

On Mon, Aug 7, 2017 at 11:13 AM, Dan Williams <dan.j.williams@intel.com> wrote:
> On Mon, Aug 7, 2017 at 11:09 AM, Cheng-mean Liu (SOCCER)
> <soccerl@microsoft.com> wrote:
>> Hi Dan:
>>
>>    I am wondering if failing on those unittests is still an issue for this minimum size requirement change.
>
> Yes, I just haven't had a chance to circle back and get this fixed up.
>
> You can reproduce by running:
>
>     make TESTS=dpa-alloc check
>
> ...in a checkout of the ndctl project: https://github.com/pmem/ndctl
>
> If you attempt that, note the required setup of the nfit_test modules
> documented in README.md in that same repository.

I have not had any time to fix up the unit test for this. Soccer, can
you take a look?
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH] nvdimm: Remove minimum size requirement

[Socer Liu]

Sure, I will look into this.
Thanks for the instructions





> On Aug 31, 2017, at 3:30 PM, Dan Williams <dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
> 
>> On Mon, Aug 7, 2017 at 11:13 AM, Dan Williams <dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
>> On Mon, Aug 7, 2017 at 11:09 AM, Cheng-mean Liu (SOCCER)
>> <soccerl-0li6OtcxBFHby3iVrkZq2A@public.gmane.org> wrote:
>>> Hi Dan:
>>> 
>>>   I am wondering if failing on those unittests is still an issue for this minimum size requirement change.
>> 
>> Yes, I just haven't had a chance to circle back and get this fixed up.
>> 
>> You can reproduce by running:
>> 
>>    make TESTS=dpa-alloc check
>> 
>> ...in a checkout of the ndctl project: https://github.com/pmem/ndctl
>> 
>> If you attempt that, note the required setup of the nfit_test modules
>> documented in README.md in that same repository.
> 
> I have not had any time to fix up the unit test for this. Soccer, can
> you take a look?

RE: [PATCH] nvdimm: Remove minimum size requirement

[Cheng-mean Liu (SOCCER)]

Hi Dan:


This is regarding the new unittest failure from the reducing ND_MIN_NAMESPACE_SIZE from 0x00400000 to 0x00001000.

Code change:


My current changes:



In https://github.com/torvalds/linux

include/uapi/linux/ndctl.h


--- a/include/uapi/linux/ndctl.h
+++ b/include/uapi/linux/ndctl.h
@@ -263,7 +263,9 @@ enum nd_driver_flags {
};

enum {
-       ND_MIN_NAMESPACE_SIZE = 0x00400000,
+       ND_MIN_NAMESPACE_SIZE = 0x00001000,
+

};



In https://github.com/pmem/ndctl:


--- a/ndctl/ndctl.h
+++ b/ndctl/ndctl.h
@@ -263,7 +263,9 @@ enum nd_driver_flags {
};

enum {
-       ND_MIN_NAMESPACE_SIZE = 0x00400000,
+       ND_MIN_NAMESPACE_SIZE = 0x00001000,
+
};


Ndctl unittest failure:

# make TESTS=dpa-alloc check

FAIL: dpa-alloc
============================================================================
Testsuite summary for ndctl 58.2.dirty
============================================================================
# TOTAL: 1
# PASS:  0
# SKIP:  0
# XFAIL: 0
# FAIL:  1
# XPASS: 0
# ERROR: 0
============================================================================
See test/test-suite.log
Please report to linux-nvdimm@lists.01.org
============================================================================

# cat test/test-suite.log
…
failed to delete 23774df6-797e-46ca-b5c9-5e6226c86ae7


error code is -16 (EBUSY),





Cause:



EBUSY  this was returned by ndctl_namespace_delete  https://github.com/pmem/ndctl/blob/0a628fdf4fe58a283b16c1bbaa49bb28b1842bf9/ndctl/lib/libndctl.c#L3735

when it found the namespace being deleted was in “enabled” state.





I found before https://github.com/pmem/ndctl/blob/0a628fdf4fe58a283b16c1bbaa49bb28b1842bf9/test/dpa-alloc.c#L194, all four namespaces in namespace array were in disabled state.




163 ndctl_region_disable_invalidate(region);



164 rc = ndctl_region_enable(region);

Right after the region was re-enabled, namespace0.0 was stayed disabled (as expected) but somehow it turned namespace0.1, namespace0.2, and namespace0.3 into enable state, which caused the failure when testing deletion and merging
https://github.com/pmem/ndctl/blob/0a628fdf4fe58a283b16c1bbaa49bb28b1842bf9/test/dpa-alloc.c#L240


// Here are the logging from my testing bits:

namespace[0].size =524288
libndctl: is_enabled: checking drvpaht=/sys/devices/platform/nfit_test.0/ndbus0/region0/namespace0.0/driver:  disabled
namespace[0] 2b13ffb4-ceae-49ad-8757-98cd483fce1a is Disabled
namespace type = 6
namespace[1].size =520192
libndctl: is_enabled: checking drvpaht=/sys/devices/platform/nfit_test.0/ndbus0/region0/namespace0.1/driver:  disabled
namespace[1] 23774df6-797e-46ca-b5c9-5e6226c86ae7 is Disabled
namespace type = 6
namespace[2].size =520192
libndctl: is_enabled: checking drvpaht=/sys/devices/platform/nfit_test.0/ndbus0/region0/namespace0.2/driver:  disabled
namespace[2] 6c0a0fc2-4378-420a-a6e3-de22d05f07da is Disabled
namespace type = 6
namespace[3].size =520192
libndctl: is_enabled: checking drvpaht=/sys/devices/platform/nfit_test.0/ndbus0/region0/namespace0.3/driver:  disabled
namespace[3] 26214d49-eb37-4ad9-b6dc-5fa9f86a6323 is Disabled
namespace type = 6


**** ndctl_region_disable_invalidate ***

libndctl: is_enabled: checking drvpaht=/sys/devices/platform/nfit_test.0/ndbus0/region0/driver :enabled
libndctl: ndctl_unbind: ndctl_unbind: devpath=/sys/devices/platform/nfit_test.0/ndbus0/region0
libndctl: ndctl_unbind: path =/sys/devices/platform/nfit_test.0/ndbus0/region0/driver/unbind
libndctl: is_enabled: checking drvpaht=/sys/devices/platform/nfit_test.0/ndbus0/region0/driver :  disabled
ndctl_region_disable_invalidate() returned 0


**** ndctl_region_enable ***

libndctl: ndctl_region_enable: Soccerl 1000: Calling ndctl_region!
libndctl: is_enabled: Soccerl is_enabled: checking drvpaht=/sys/devices/platform/nfit_test.0/ndbus0/region0/driver:  disabled
libndctl: ndctl_bind: ndctl_bind: devname=region0
libndctl: ndctl_bind: drv_path = /sys/bus/nd/drivers/nd_pmem/bind
libndctl: ndctl_bind: drv_path = /sys/bus/nd/drivers/nd_blk/bind
libndctl: ndctl_bind: drv_path = /sys/bus/nd/drivers/nd_bus/bind
libndctl: ndctl_bind: drv_path = /sys/bus/nd/drivers/dax_pmem/bind
libndctl: ndctl_bind: drv_path = /sys/bus/nd/drivers/nd_region/bind
libndctl: ndctl_region_enable: Soccerl 1001: returned from ndctl_bind region0 rc=0
libndctl: is_enabled: Soccerl is_enabled: checking drvpaht=/sys/devices/platform/nfit_test.0/ndbus0/region0/driver:  enabled
libndctl: ndctl_region_enable: Soccerl 1002: ntctl_region_is_enabled returned successfully (region0)


libndctl: is_enabled: Soccerl is_enabled: checking drvpaht=/sys/devices/platform/nfit_test.0/ndbus0/region0/namespace0.3/driver:  enabled
ndns 23774df6-797e-46ca-b5c9-5e6226c86ae7 is Enabled
libndctl: is_enabled: Soccerl is_enabled: checking drvpaht=/sys/devices/platform/nfit_test.0/ndbus0/region0/namespace0.0/driver:  disabled
ndns 2b13ffb4-ceae-49ad-8757-98cd483fce1a is Disabled
libndctl: is_enabled: Soccerl is_enabled: checking drvpaht=/sys/devices/platform/nfit_test.0/ndbus0/region0/namespace0.2/driver:  enabled
ndns 6c0a0fc2-4378-420a-a6e3-de22d05f07da is Enabled
libndctl: is_enabled: Soccerl is_enabled: checking drvpaht=/sys/devices/platform/nfit_test.0/ndbus0/region0/namespace0.1/driver:  enabled
ndns 26214d49-eb37-4ad9-b6dc-5fa9f86a6323 is Enabled


My question  is:

I was suspecting it could be related to how a region was disabled thus tried both ndctl_region_disable_preserve  and ndctl_region_disable_invalidate, it didn’t make any difference.
Any idea on why ndctl_region_enable(region) could change the state if its namespaces?

Any suggestions on where I could dig into next?

Thanks
Cheng-mean





On Thursday, August 31, 2017 3:31 PM, Dan Williams <dan.j.williams@intel.com<mailto:dan.j.williams@intel.com>> wrote:

On Mon, Aug 7, 2017 at 11:13 AM, Dan Williams <dan.j.williams@intel.com<mailto:dan.j.williams@intel.com>> wrote:

> On Mon, Aug 7, 2017 at 11:09 AM, Cheng-mean Liu (SOCCER)
> <soccerl@microsoft.com<mailto:soccerl@microsoft.com>> wrote:
>> Hi Dan:
>>
>>    I am wondering if failing on those unittests is still an issue for this minimum size requirement change.
>
> Yes, I just haven't had a chance to circle back and get this fixed up.
>
> You can reproduce by running:
>
>    make TESTS=dpa-alloc check
>
> ...in a checkout of the ndctl project: https://github.com/pmem/ndctl<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fpmem%2Fndctl&data=02%7C01%7Csoccerl%40microsoft.com%7C884956f6878643449b2008d50081a3da%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636415479163023272&sdata=YS4OWo6PNhdnompDbZUIXcMtnbeeDrFn7cd6WlXvvo4%3D&reserved=0>
>
> If you attempt that, note the required setup of the nfit_test modules
> documented in README.md in that same repository.


I have not had any time to fix up the unit test for this. Soccer, can
you take a look?


_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH] nvdimm: Remove minimum size requirement

[Soccer Liu]

Hi: 
  As part of processing in setting up the environment for running unitests, I was able to work through the instrcutions in https://github.com/pmem/ndctl/tree/0a628fdf4fe58a283b16c1bbaa49bb28b1842bf9all the way until I hit the followingbuild error (Segmentation fault) when buiding libnvdimm.o.
Anyone hit this before?
root@ubuntu:/home/soccerl/nvdimm#  make M=tools/testing/nvdimm  AR      tools/testing/nvdimm/built-in.o  CC [M]  tools/testing/nvdimm/../../../drivers/nvdimm/core.o  CC [M]  tools/testing/nvdimm/../../../drivers/nvdimm/bus.o  CC [M]  tools/testing/nvdimm/../../../drivers/nvdimm/dimm_devs.o  CC [M]  tools/testing/nvdimm/../../../drivers/nvdimm/dimm.o  CC [M]  tools/testing/nvdimm/../../../drivers/nvdimm/region_devs.o  CC [M]  tools/testing/nvdimm/../../../drivers/nvdimm/region.o  CC [M]  tools/testing/nvdimm/../../../drivers/nvdimm/namespace_devs.o  CC [M]  tools/testing/nvdimm/../../../drivers/nvdimm/label.o  CC [M]  tools/testing/nvdimm/../../../drivers/nvdimm/claim.o  CC [M]  tools/testing/nvdimm/../../../drivers/nvdimm/btt_devs.o  CC [M]  tools/testing/nvdimm/../../../drivers/nvdimm/pfn_devs.o  CC [M]  tools/testing/nvdimm/../../../drivers/nvdimm/dax_devs.o  CC [M]  tools/testing/nvdimm/config_check.o  LD [M]  tools/testing/nvdimm/libnvdimm.oSegmentation faultscripts/Makefile.build:548: recipe for target 'tools/testing/nvdimm/libnvdimm.o' failedmake[1]: *** [tools/testing/nvdimm/libnvdimm.o] Error 139Makefile:1511: recipe for target '_module_tools/testing/nvdimm' failedmake: *** [_module_tools/testing/nvdimm] Error 2
My devbox has 4.13 Linux in it.I am not sure whether it has anything to do with fact that I didnt do anything with ndctl/ndctl.spec.in (because I am not sure how to apply those dependendies to my testbox)
 Any idea? 

ThanksCheng-mean
    On Thursday, August 31, 2017 3:31 PM, Dan Williams <dan.j.williams@intel.com> wrote:
 

 On Mon, Aug 7, 2017 at 11:13 AM, Dan Williams <dan.j.williams@intel.com> wrote:
> On Mon, Aug 7, 2017 at 11:09 AM, Cheng-mean Liu (SOCCER)
> <soccerl@microsoft.com> wrote:
>> Hi Dan:
>>
>>    I am wondering if failing on those unittests is still an issue for this minimum size requirement change.
>
> Yes, I just haven't had a chance to circle back and get this fixed up.
>
> You can reproduce by running:
>
>    make TESTS=dpa-alloc check
>
> ...in a checkout of the ndctl project: https://github.com/pmem/ndctl
>
> If you attempt that, note the required setup of the nfit_test modules
> documented in README.md in that same repository.

I have not had any time to fix up the unit test for this. Soccer, can
you take a look?


   
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm