| Message ID | 20190809074520.27115-4-aneesh.kumar@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Mark the namespace disabled on pfn superblock mismatch | expand |
On Fri, Aug 9, 2019 at 12:45 AM Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> wrote: > > Use PAGE_SIZE instead of SZ_4K and sizeof(struct page) instead of 64. > If we have a kernel built with different struct page size the previous > patch should handle marking the namespace disabled. Each of these changes carry independent non-overlapping regression risk, so lets split them into separate patches. Others might > Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> > --- > drivers/nvdimm/label.c | 2 +- > drivers/nvdimm/namespace_devs.c | 6 +++--- > drivers/nvdimm/pfn_devs.c | 3 ++- > drivers/nvdimm/region_devs.c | 8 ++++---- > 4 files changed, 10 insertions(+), 9 deletions(-) > > diff --git a/drivers/nvdimm/label.c b/drivers/nvdimm/label.c > index 73e197babc2f..7ee037063be7 100644 > --- a/drivers/nvdimm/label.c > +++ b/drivers/nvdimm/label.c > @@ -355,7 +355,7 @@ static bool slot_valid(struct nvdimm_drvdata *ndd, > > /* check that DPA allocations are page aligned */ > if ((__le64_to_cpu(nd_label->dpa) > - | __le64_to_cpu(nd_label->rawsize)) % SZ_4K) > + | __le64_to_cpu(nd_label->rawsize)) % PAGE_SIZE) The UEFI label specification has no concept of PAGE_SIZE, so this check is a pure Linux-ism. There's no strict requirement why slot_valid() needs to check for page alignment and it would seem to actively hurt cross-page-size compatibility, so let's delete the check and rely on checksum validation. > return false; > > /* check checksum */ > diff --git a/drivers/nvdimm/namespace_devs.c b/drivers/nvdimm/namespace_devs.c > index a16e52251a30..a9c76df12cb9 100644 > --- a/drivers/nvdimm/namespace_devs.c > +++ b/drivers/nvdimm/namespace_devs.c > @@ -1006,10 +1006,10 @@ static ssize_t __size_store(struct device *dev, unsigned long long val) > return -ENXIO; > } > > - div_u64_rem(val, SZ_4K * nd_region->ndr_mappings, &remainder); > + div_u64_rem(val, PAGE_SIZE * nd_region->ndr_mappings, &remainder); > if (remainder) { > - dev_dbg(dev, "%llu is not %dK aligned\n", val, > - (SZ_4K * nd_region->ndr_mappings) / SZ_1K); > + dev_dbg(dev, "%llu is not %ldK aligned\n", val, > + (PAGE_SIZE * nd_region->ndr_mappings) / SZ_1K); > return -EINVAL; Yes, looks good, but this deserves its own independent patch. > } > > diff --git a/drivers/nvdimm/pfn_devs.c b/drivers/nvdimm/pfn_devs.c > index 37e96811c2fc..c1d9be609322 100644 > --- a/drivers/nvdimm/pfn_devs.c > +++ b/drivers/nvdimm/pfn_devs.c > @@ -725,7 +725,8 @@ static int nd_pfn_init(struct nd_pfn *nd_pfn) > * when populating the vmemmap. This *should* be equal to > * PMD_SIZE for most architectures. > */ > - offset = ALIGN(start + SZ_8K + 64 * npfns, align) - start; > + offset = ALIGN(start + SZ_8K + sizeof(struct page) * npfns, I'd prefer if this was not dynamic and was instead set to the maximum size of 'struct page' across all archs just to enhance cross-arch compatibility. I think that answer is '64'. > + align) - start; > } else if (nd_pfn->mode == PFN_MODE_RAM) > offset = ALIGN(start + SZ_8K, align) - start; > else > diff --git a/drivers/nvdimm/region_devs.c b/drivers/nvdimm/region_devs.c > index af30cbe7a8ea..20e265a534f8 100644 > --- a/drivers/nvdimm/region_devs.c > +++ b/drivers/nvdimm/region_devs.c > @@ -992,10 +992,10 @@ static struct nd_region *nd_region_create(struct nvdimm_bus *nvdimm_bus, > struct nd_mapping_desc *mapping = &ndr_desc->mapping[i]; > struct nvdimm *nvdimm = mapping->nvdimm; > > - if ((mapping->start | mapping->size) % SZ_4K) { > - dev_err(&nvdimm_bus->dev, "%s: %s mapping%d is not 4K aligned\n", > - caller, dev_name(&nvdimm->dev), i); > - > + if ((mapping->start | mapping->size) % PAGE_SIZE) { > + dev_err(&nvdimm_bus->dev, > + "%s: %s mapping%d is not %ld aligned\n", > + caller, dev_name(&nvdimm->dev), i, PAGE_SIZE); > return NULL; > } > > -- > 2.21.0 >
Dan Williams <dan.j.williams@intel.com> writes: > On Fri, Aug 9, 2019 at 12:45 AM Aneesh Kumar K.V > <aneesh.kumar@linux.ibm.com> wrote: >> >> Use PAGE_SIZE instead of SZ_4K and sizeof(struct page) instead of 64. >> If we have a kernel built with different struct page size the previous >> patch should handle marking the namespace disabled. > > Each of these changes carry independent non-overlapping regression > risk, so lets split them into separate patches. Others might > >> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> >> --- >> drivers/nvdimm/label.c | 2 +- >> drivers/nvdimm/namespace_devs.c | 6 +++--- >> drivers/nvdimm/pfn_devs.c | 3 ++- >> drivers/nvdimm/region_devs.c | 8 ++++---- >> 4 files changed, 10 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/nvdimm/label.c b/drivers/nvdimm/label.c >> index 73e197babc2f..7ee037063be7 100644 >> --- a/drivers/nvdimm/label.c >> +++ b/drivers/nvdimm/label.c >> @@ -355,7 +355,7 @@ static bool slot_valid(struct nvdimm_drvdata *ndd, >> >> /* check that DPA allocations are page aligned */ >> if ((__le64_to_cpu(nd_label->dpa) >> - | __le64_to_cpu(nd_label->rawsize)) % SZ_4K) >> + | __le64_to_cpu(nd_label->rawsize)) % PAGE_SIZE) > > The UEFI label specification has no concept of PAGE_SIZE, so this > check is a pure Linux-ism. There's no strict requirement why > slot_valid() needs to check for page alignment and it would seem to > actively hurt cross-page-size compatibility, so let's delete the check > and rely on checksum validation. Will do a separate patch to drop that check. > >> return false; >> >> /* check checksum */ >> diff --git a/drivers/nvdimm/namespace_devs.c b/drivers/nvdimm/namespace_devs.c >> index a16e52251a30..a9c76df12cb9 100644 >> --- a/drivers/nvdimm/namespace_devs.c >> +++ b/drivers/nvdimm/namespace_devs.c >> @@ -1006,10 +1006,10 @@ static ssize_t __size_store(struct device *dev, unsigned long long val) >> return -ENXIO; >> } >> >> - div_u64_rem(val, SZ_4K * nd_region->ndr_mappings, &remainder); >> + div_u64_rem(val, PAGE_SIZE * nd_region->ndr_mappings, &remainder); >> if (remainder) { >> - dev_dbg(dev, "%llu is not %dK aligned\n", val, >> - (SZ_4K * nd_region->ndr_mappings) / SZ_1K); >> + dev_dbg(dev, "%llu is not %ldK aligned\n", val, >> + (PAGE_SIZE * nd_region->ndr_mappings) / SZ_1K); >> return -EINVAL; > > Yes, looks good, but this deserves its own independent patch. > >> } >> >> diff --git a/drivers/nvdimm/pfn_devs.c b/drivers/nvdimm/pfn_devs.c >> index 37e96811c2fc..c1d9be609322 100644 >> --- a/drivers/nvdimm/pfn_devs.c >> +++ b/drivers/nvdimm/pfn_devs.c >> @@ -725,7 +725,8 @@ static int nd_pfn_init(struct nd_pfn *nd_pfn) >> * when populating the vmemmap. This *should* be equal to >> * PMD_SIZE for most architectures. >> */ >> - offset = ALIGN(start + SZ_8K + 64 * npfns, align) - start; >> + offset = ALIGN(start + SZ_8K + sizeof(struct page) * npfns, > > I'd prefer if this was not dynamic and was instead set to the maximum > size of 'struct page' across all archs just to enhance cross-arch > compatibility. I think that answer is '64'. That still doesn't take care of the case where we add new elements to struct page later. If we have struct page size changing across architectures, we should still be ok as long as new size is less than what is stored in pfn superblock? I understand the desire to keep it non-dynamic. But we also need to make sure we don't reserve less space when creating a new namespace on a config that got struct page size > 64? >> + align) - start; >> } else if (nd_pfn->mode == PFN_MODE_RAM) >> offset = ALIGN(start + SZ_8K, align) - start; >> else >> diff --git a/drivers/nvdimm/region_devs.c b/drivers/nvdimm/region_devs.c >> index af30cbe7a8ea..20e265a534f8 100644 >> --- a/drivers/nvdimm/region_devs.c >> +++ b/drivers/nvdimm/region_devs.c >> @@ -992,10 +992,10 @@ static struct nd_region *nd_region_create(struct nvdimm_bus *nvdimm_bus, >> struct nd_mapping_desc *mapping = &ndr_desc->mapping[i]; >> struct nvdimm *nvdimm = mapping->nvdimm; >> >> - if ((mapping->start | mapping->size) % SZ_4K) { >> - dev_err(&nvdimm_bus->dev, "%s: %s mapping%d is not 4K aligned\n", >> - caller, dev_name(&nvdimm->dev), i); >> - >> + if ((mapping->start | mapping->size) % PAGE_SIZE) { >> + dev_err(&nvdimm_bus->dev, >> + "%s: %s mapping%d is not %ld aligned\n", >> + caller, dev_name(&nvdimm->dev), i, PAGE_SIZE); >> return NULL; >> } >> >> -- >> 2.21.0 >>
Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> writes: > Dan Williams <dan.j.williams@intel.com> writes: > >> On Fri, Aug 9, 2019 at 12:45 AM Aneesh Kumar K.V >> <aneesh.kumar@linux.ibm.com> wrote: >>> >> ... >>> diff --git a/drivers/nvdimm/pfn_devs.c b/drivers/nvdimm/pfn_devs.c >>> index 37e96811c2fc..c1d9be609322 100644 >>> --- a/drivers/nvdimm/pfn_devs.c >>> +++ b/drivers/nvdimm/pfn_devs.c >>> @@ -725,7 +725,8 @@ static int nd_pfn_init(struct nd_pfn *nd_pfn) >>> * when populating the vmemmap. This *should* be equal to >>> * PMD_SIZE for most architectures. >>> */ >>> - offset = ALIGN(start + SZ_8K + 64 * npfns, align) - start; >>> + offset = ALIGN(start + SZ_8K + sizeof(struct page) * npfns, >> >> I'd prefer if this was not dynamic and was instead set to the maximum >> size of 'struct page' across all archs just to enhance cross-arch >> compatibility. I think that answer is '64'. > > > That still doesn't take care of the case where we add new elements to > struct page later. If we have struct page size changing across > architectures, we should still be ok as long as new size is less than what is > stored in pfn superblock? I understand the desire to keep it > non-dynamic. But we also need to make sure we don't reserve less space > when creating a new namespace on a config that got struct page size > > 64? How about libnvdimm/pfn_dev: Add a build check to make sure we notice when struct page size change When namespace is created with map device as pmem device, struct page is stored in the reserve block area. We need to make sure we account for the right struct page size while doing this. Instead of directly depending on sizeof(struct page) which can change based on different kernel config option, use the max struct page size (64) while calculating the reserve block area. This makes sure pmem device can be used across kernels built with different configs. If the above assumption of max struct page size change, we need to update the reserve block allocation space for new namespaces created. Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> 1 file changed, 7 insertions(+) drivers/nvdimm/pfn_devs.c | 7 +++++++ modified drivers/nvdimm/pfn_devs.c @@ -722,7 +722,14 @@ static int nd_pfn_init(struct nd_pfn *nd_pfn) * The altmap should be padded out to the block size used * when populating the vmemmap. This *should* be equal to * PMD_SIZE for most architectures. + * + * Also make sure size of struct page is less than 64. We + * want to make sure we use large enough size here so that + * we don't have a dynamic reserve space depending on + * struct page size. But we also want to make sure we notice + * if we end up adding new elements to struct page. */ + BUILD_BUG_ON(64 < sizeof(struct page)); offset = ALIGN(start + SZ_8K + 64 * npfns, align) - start; } else if (nd_pfn->mode == PFN_MODE_RAM) offset = ALIGN(start + SZ_8K, align) - start; -aneesh
On Mon, Aug 19, 2019 at 2:32 AM Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> wrote: > > Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> writes: > > > Dan Williams <dan.j.williams@intel.com> writes: > > > >> On Fri, Aug 9, 2019 at 12:45 AM Aneesh Kumar K.V > >> <aneesh.kumar@linux.ibm.com> wrote: > >>> > >> > > ... > > >>> diff --git a/drivers/nvdimm/pfn_devs.c b/drivers/nvdimm/pfn_devs.c > >>> index 37e96811c2fc..c1d9be609322 100644 > >>> --- a/drivers/nvdimm/pfn_devs.c > >>> +++ b/drivers/nvdimm/pfn_devs.c > >>> @@ -725,7 +725,8 @@ static int nd_pfn_init(struct nd_pfn *nd_pfn) > >>> * when populating the vmemmap. This *should* be equal to > >>> * PMD_SIZE for most architectures. > >>> */ > >>> - offset = ALIGN(start + SZ_8K + 64 * npfns, align) - start; > >>> + offset = ALIGN(start + SZ_8K + sizeof(struct page) * npfns, > >> > >> I'd prefer if this was not dynamic and was instead set to the maximum > >> size of 'struct page' across all archs just to enhance cross-arch > >> compatibility. I think that answer is '64'. > > > > > > That still doesn't take care of the case where we add new elements to > > struct page later. If we have struct page size changing across > > architectures, we should still be ok as long as new size is less than what is > > stored in pfn superblock? I understand the desire to keep it > > non-dynamic. But we also need to make sure we don't reserve less space > > when creating a new namespace on a config that got struct page size > > > 64? > > > How about > > libnvdimm/pfn_dev: Add a build check to make sure we notice when struct page size change > > When namespace is created with map device as pmem device, struct page is stored in the > reserve block area. We need to make sure we account for the right struct page > size while doing this. Instead of directly depending on sizeof(struct page) > which can change based on different kernel config option, use the max struct > page size (64) while calculating the reserve block area. This makes sure pmem > device can be used across kernels built with different configs. > > If the above assumption of max struct page size change, we need to update the > reserve block allocation space for new namespaces created. > > Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> > > 1 file changed, 7 insertions(+) > drivers/nvdimm/pfn_devs.c | 7 +++++++ > > modified drivers/nvdimm/pfn_devs.c > @@ -722,7 +722,14 @@ static int nd_pfn_init(struct nd_pfn *nd_pfn) > * The altmap should be padded out to the block size used > * when populating the vmemmap. This *should* be equal to > * PMD_SIZE for most architectures. > + * > + * Also make sure size of struct page is less than 64. We > + * want to make sure we use large enough size here so that > + * we don't have a dynamic reserve space depending on > + * struct page size. But we also want to make sure we notice > + * if we end up adding new elements to struct page. > */ > + BUILD_BUG_ON(64 < sizeof(struct page)); Looks ok to me. There are ongoing heroic efforts to make sure 'struct page' does not grown beyond the size of cacheline. The fact that 'struct page_ext' is allocated out of line makes it safe to assume that 'struct page' will not be growing larger in the foreseeable future.
diff --git a/drivers/nvdimm/label.c b/drivers/nvdimm/label.c index 73e197babc2f..7ee037063be7 100644 --- a/drivers/nvdimm/label.c +++ b/drivers/nvdimm/label.c @@ -355,7 +355,7 @@ static bool slot_valid(struct nvdimm_drvdata *ndd, /* check that DPA allocations are page aligned */ if ((__le64_to_cpu(nd_label->dpa) - | __le64_to_cpu(nd_label->rawsize)) % SZ_4K) + | __le64_to_cpu(nd_label->rawsize)) % PAGE_SIZE) return false; /* check checksum */ diff --git a/drivers/nvdimm/namespace_devs.c b/drivers/nvdimm/namespace_devs.c index a16e52251a30..a9c76df12cb9 100644 --- a/drivers/nvdimm/namespace_devs.c +++ b/drivers/nvdimm/namespace_devs.c @@ -1006,10 +1006,10 @@ static ssize_t __size_store(struct device *dev, unsigned long long val) return -ENXIO; } - div_u64_rem(val, SZ_4K * nd_region->ndr_mappings, &remainder); + div_u64_rem(val, PAGE_SIZE * nd_region->ndr_mappings, &remainder); if (remainder) { - dev_dbg(dev, "%llu is not %dK aligned\n", val, - (SZ_4K * nd_region->ndr_mappings) / SZ_1K); + dev_dbg(dev, "%llu is not %ldK aligned\n", val, + (PAGE_SIZE * nd_region->ndr_mappings) / SZ_1K); return -EINVAL; } diff --git a/drivers/nvdimm/pfn_devs.c b/drivers/nvdimm/pfn_devs.c index 37e96811c2fc..c1d9be609322 100644 --- a/drivers/nvdimm/pfn_devs.c +++ b/drivers/nvdimm/pfn_devs.c @@ -725,7 +725,8 @@ static int nd_pfn_init(struct nd_pfn *nd_pfn) * when populating the vmemmap. This *should* be equal to * PMD_SIZE for most architectures. */ - offset = ALIGN(start + SZ_8K + 64 * npfns, align) - start; + offset = ALIGN(start + SZ_8K + sizeof(struct page) * npfns, + align) - start; } else if (nd_pfn->mode == PFN_MODE_RAM) offset = ALIGN(start + SZ_8K, align) - start; else diff --git a/drivers/nvdimm/region_devs.c b/drivers/nvdimm/region_devs.c index af30cbe7a8ea..20e265a534f8 100644 --- a/drivers/nvdimm/region_devs.c +++ b/drivers/nvdimm/region_devs.c @@ -992,10 +992,10 @@ static struct nd_region *nd_region_create(struct nvdimm_bus *nvdimm_bus, struct nd_mapping_desc *mapping = &ndr_desc->mapping[i]; struct nvdimm *nvdimm = mapping->nvdimm; - if ((mapping->start | mapping->size) % SZ_4K) { - dev_err(&nvdimm_bus->dev, "%s: %s mapping%d is not 4K aligned\n", - caller, dev_name(&nvdimm->dev), i); - + if ((mapping->start | mapping->size) % PAGE_SIZE) { + dev_err(&nvdimm_bus->dev, + "%s: %s mapping%d is not %ld aligned\n", + caller, dev_name(&nvdimm->dev), i, PAGE_SIZE); return NULL; }
Use PAGE_SIZE instead of SZ_4K and sizeof(struct page) instead of 64. If we have a kernel built with different struct page size the previous patch should handle marking the namespace disabled. Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> --- drivers/nvdimm/label.c | 2 +- drivers/nvdimm/namespace_devs.c | 6 +++--- drivers/nvdimm/pfn_devs.c | 3 ++- drivers/nvdimm/region_devs.c | 8 ++++---- 4 files changed, 10 insertions(+), 9 deletions(-)