mbox series

[5.4-stable,0/7] libnvdimm: Cross-arch compatible namespace alignment

Message ID 159010426294.1062454.8853083370975871627.stgit@dwillia2-desk3.amr.corp.intel.com (mailing list archive)
Headers show
Series libnvdimm: Cross-arch compatible namespace alignment | expand

Message

Dan Williams May 21, 2020, 11:37 p.m. UTC
Hello stable team,

These patches have been shipping in mainline since v5.7-rc1 with no
reported issues. They address long standing problems in libnvdimm's
handling of namespace provisioning relative to alignment constraints
including crashes trying to even load the driver on some PowerPC
configurations.

I did fold one build fix [1] into "libnvdimm/region: Introduce an 'align'
attribute" so as to not convey the bisection breakage to -stable.

Please consider them for v5.4-stable. They do pass the latest
version of the ndctl unit tests.

[1]: 04ff4863e126 libnvdimm/region: Fix build error


---

Original cover letter for the series:

Aneesh reports that PowerPC requires 16MiB alignment for the address
range passed to devm_memremap_pages(), and Jeff reports that it is
possible to create a misaligned namespace which blocks future namespace
creation in that region. Both of these issues require namespace
alignment to be managed at the region level rather than padding at the
namespace level which has been a broken approach to date.

Introduce memremap_compat_align() to indicate the hard requirements of
an arch's memremap_pages() implementation. Use the maximum known
memremap_compat_align() to set the default namespace alignment for
libnvdimm. Consult that alignment when allocating free space. Finally,
allow the default region alignment to be overridden to maintain the same
namespace creation capability as previous kernels (modulo dax operation
not being supported with a non-zero start_pad).

---

Aneesh Kumar K.V (1):
      libnvdimm/pfn_dev: Don't clear device memmap area during generic namespace probe

Dan Williams (6):
      mm/memremap_pages: Kill unused __devm_memremap_pages()
      mm/memremap_pages: Introduce memremap_compat_align()
      libnvdimm/pfn: Prevent raw mode fallback if pfn-infoblock valid
      libnvdimm/namespace: Enforce memremap_compat_align()
      libnvdimm/region: Introduce NDD_LABELING
      libnvdimm/region: Introduce an 'align' attribute


 arch/powerpc/Kconfig                      |    1 
 arch/powerpc/mm/ioremap.c                 |   21 +++++
 arch/powerpc/platforms/pseries/papr_scm.c |    2 
 drivers/acpi/nfit/core.c                  |    4 +
 drivers/nvdimm/dimm.c                     |    2 
 drivers/nvdimm/dimm_devs.c                |   95 +++++++++++++++++----
 drivers/nvdimm/namespace_devs.c           |   28 ++++++
 drivers/nvdimm/nd.h                       |    3 -
 drivers/nvdimm/pfn.h                      |   12 +++
 drivers/nvdimm/pfn_devs.c                 |   48 +++++++++--
 drivers/nvdimm/region_devs.c              |  130 ++++++++++++++++++++++++++---
 include/linux/io.h                        |    2 
 include/linux/libnvdimm.h                 |    2 
 include/linux/memremap.h                  |    8 ++
 include/linux/mmzone.h                    |    1 
 lib/Kconfig                               |    3 +
 mm/memremap.c                             |   23 +++++
 17 files changed, 335 insertions(+), 50 deletions(-)

base-commit: 1cdaf895c99d319c0007d0b62818cf85fc4b087f

Comments

Greg Kroah-Hartman May 22, 2020, 11:58 a.m. UTC | #1
On Thu, May 21, 2020 at 04:37:43PM -0700, Dan Williams wrote:
> Hello stable team,
> 
> These patches have been shipping in mainline since v5.7-rc1 with no
> reported issues. They address long standing problems in libnvdimm's
> handling of namespace provisioning relative to alignment constraints
> including crashes trying to even load the driver on some PowerPC
> configurations.
> 
> I did fold one build fix [1] into "libnvdimm/region: Introduce an 'align'
> attribute" so as to not convey the bisection breakage to -stable.
> 
> Please consider them for v5.4-stable. They do pass the latest
> version of the ndctl unit tests.

What about 5.6.y?  Any user upgrading from 5.4-stable to 5.6-stable
would hit a regression, right?

So can we get a series backported to 5.6.y as well?  I need that before
I can take this series.

thanks,

greg k-h
Greg Kroah-Hartman May 22, 2020, noon UTC | #2
On Fri, May 22, 2020 at 01:58:00PM +0200, Greg KH wrote:
> On Thu, May 21, 2020 at 04:37:43PM -0700, Dan Williams wrote:
> > Hello stable team,
> > 
> > These patches have been shipping in mainline since v5.7-rc1 with no
> > reported issues. They address long standing problems in libnvdimm's
> > handling of namespace provisioning relative to alignment constraints
> > including crashes trying to even load the driver on some PowerPC
> > configurations.
> > 
> > I did fold one build fix [1] into "libnvdimm/region: Introduce an 'align'
> > attribute" so as to not convey the bisection breakage to -stable.
> > 
> > Please consider them for v5.4-stable. They do pass the latest
> > version of the ndctl unit tests.
> 
> What about 5.6.y?  Any user upgrading from 5.4-stable to 5.6-stable
> would hit a regression, right?
> 
> So can we get a series backported to 5.6.y as well?  I need that before
> I can take this series.

Also, I really don't see the "bug" that this is fixing here.  If this
didn't work on PowerPC before, it can continue to just "not work" until
5.7, right?  What problems with 5.4.y and 5.6.y is this series fixing
that used to work before?

thanks,

greg k-h
Dan Williams May 26, 2020, 8:42 p.m. UTC | #3
On Fri, May 22, 2020 at 5:00 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Fri, May 22, 2020 at 01:58:00PM +0200, Greg KH wrote:
> > On Thu, May 21, 2020 at 04:37:43PM -0700, Dan Williams wrote:
> > > Hello stable team,
> > >
> > > These patches have been shipping in mainline since v5.7-rc1 with no
> > > reported issues. They address long standing problems in libnvdimm's
> > > handling of namespace provisioning relative to alignment constraints
> > > including crashes trying to even load the driver on some PowerPC
> > > configurations.
> > >
> > > I did fold one build fix [1] into "libnvdimm/region: Introduce an 'align'
> > > attribute" so as to not convey the bisection breakage to -stable.
> > >
> > > Please consider them for v5.4-stable. They do pass the latest
> > > version of the ndctl unit tests.
> >
> > What about 5.6.y?  Any user upgrading from 5.4-stable to 5.6-stable
> > would hit a regression, right?
> >
> > So can we get a series backported to 5.6.y as well?  I need that before
> > I can take this series.

Yes, should be the exact same set, but I will run the regression suite
to be sure.

> Also, I really don't see the "bug" that this is fixing here.  If this
> didn't work on PowerPC before, it can continue to just "not work" until
> 5.7, right?

There's a mix of "never worked" and "used to work" in this set. The
PowerPC case is indeed a "never worked", but I highlighted it as it
was the simplest to understand.

> What problems with 5.4.y and 5.6.y is this series fixing
> that used to work before?

The "used to work" bug fixed by this set is the fact that the kernel
used to force a 128MB (memory hotplug section size) alignment padding
on all persistent memory namespaces to enable DAX operation. The
support for sub-sections (2MB) dropped forced alignment padding, but
unfortunately introduced a regression for the case of trying to create
multiple unaligned namespaces. When that bug triggers namespace
creation for the region is disabled, iirc, previously that lockout
scenario was prevented.

Jeff, can you corroborate this?

I otherwise agree, if the above never worked then this can all wait
for v5.7 upgrades.
Jeff Moyer May 26, 2020, 8:49 p.m. UTC | #4
Dan Williams <dan.j.williams@intel.com> writes:

>> What problems with 5.4.y and 5.6.y is this series fixing
>> that used to work before?
>
> The "used to work" bug fixed by this set is the fact that the kernel
> used to force a 128MB (memory hotplug section size) alignment padding
> on all persistent memory namespaces to enable DAX operation. The
> support for sub-sections (2MB) dropped forced alignment padding, but
> unfortunately introduced a regression for the case of trying to create
> multiple unaligned namespaces. When that bug triggers namespace
> creation for the region is disabled, iirc, previously that lockout
> scenario was prevented.
>
> Jeff, can you corroborate this?

So, I don't pretend to remember the exact state of brokenness for each
iteration.  :)  As far as I can recall, though, the issue you describe
with a misaligned namespace preventing further namespace creation was
present in all kernels up until it was finally fixed.

> I otherwise agree, if the above never worked then this can all wait
> for v5.7 upgrades.

I can test specific kernel versions if that would help out.

Cheers,
Jeff
Dan Williams May 26, 2020, 8:52 p.m. UTC | #5
On Tue, May 26, 2020 at 1:49 PM Jeff Moyer <jmoyer@redhat.com> wrote:
>
> Dan Williams <dan.j.williams@intel.com> writes:
>
> >> What problems with 5.4.y and 5.6.y is this series fixing
> >> that used to work before?
> >
> > The "used to work" bug fixed by this set is the fact that the kernel
> > used to force a 128MB (memory hotplug section size) alignment padding
> > on all persistent memory namespaces to enable DAX operation. The
> > support for sub-sections (2MB) dropped forced alignment padding, but
> > unfortunately introduced a regression for the case of trying to create
> > multiple unaligned namespaces. When that bug triggers namespace
> > creation for the region is disabled, iirc, previously that lockout
> > scenario was prevented.
> >
> > Jeff, can you corroborate this?
>
> So, I don't pretend to remember the exact state of brokenness for each
> iteration.  :)  As far as I can recall, though, the issue you describe
> with a misaligned namespace preventing further namespace creation was
> present in all kernels up until it was finally fixed.

Well, if it was always there, then there is nothing to fix, and I
misremembered that we went backwards.

> > I otherwise agree, if the above never worked then this can all wait
> > for v5.7 upgrades.
>
> I can test specific kernel versions if that would help out.

Thanks for that offer, but outside of a clear regression I don't think
this meets -stable criteria.
Greg Kroah-Hartman June 1, 2020, 11:07 a.m. UTC | #6
On Tue, May 26, 2020 at 01:52:21PM -0700, Dan Williams wrote:
> On Tue, May 26, 2020 at 1:49 PM Jeff Moyer <jmoyer@redhat.com> wrote:
> >
> > Dan Williams <dan.j.williams@intel.com> writes:
> >
> > >> What problems with 5.4.y and 5.6.y is this series fixing
> > >> that used to work before?
> > >
> > > The "used to work" bug fixed by this set is the fact that the kernel
> > > used to force a 128MB (memory hotplug section size) alignment padding
> > > on all persistent memory namespaces to enable DAX operation. The
> > > support for sub-sections (2MB) dropped forced alignment padding, but
> > > unfortunately introduced a regression for the case of trying to create
> > > multiple unaligned namespaces. When that bug triggers namespace
> > > creation for the region is disabled, iirc, previously that lockout
> > > scenario was prevented.
> > >
> > > Jeff, can you corroborate this?
> >
> > So, I don't pretend to remember the exact state of brokenness for each
> > iteration.  :)  As far as I can recall, though, the issue you describe
> > with a misaligned namespace preventing further namespace creation was
> > present in all kernels up until it was finally fixed.
> 
> Well, if it was always there, then there is nothing to fix, and I
> misremembered that we went backwards.
> 
> > > I otherwise agree, if the above never worked then this can all wait
> > > for v5.7 upgrades.
> >
> > I can test specific kernel versions if that would help out.
> 
> Thanks for that offer, but outside of a clear regression I don't think
> this meets -stable criteria.

I agree, I'll drop this series from my pending-queue.

thanks,

greg k-h