Re: [PATCH v5 2/4] mm/nvdimm: Add page size and struct page size to pfn superblock

Dan Williams
2019-08-14T21:08:40Z

On Fri, Aug 9, 2019 at 9:21 PM Aneesh Kumar K.V
<aneesh.kumar@linux.ibm.com> wrote:
>
> "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:
>
> case PFN_MODE_PMEM:
> > @@ -475,6 +484,20 @@ int nd_pfn_validate(struct nd_pfn *nd_pfn, const char *sig)
> > align = 1UL << ilog2(offset);
> > mode = le32_to_cpu(pfn_sb->mode);
> >
> > + if (le32_to_cpu(pfn_sb->page_size) != PAGE_SIZE) {

I think this is too strict. It's only a potential problem in the
"pfn_sb->page_size > PAGE_SIZE" case, because only then might the
existing reservation for the memmap be too small. Otherwise, unless
I'm missing something, there should be no ill effects for
under-utilizing the reservation.

> > + dev_err(&nd_pfn->dev,
> > + "init failed, page size mismatch %d\n",
> > + le32_to_cpu(pfn_sb->page_size));
> > + return -EOPNOTSUPP;
> > + }
> > +
> > + if (le16_to_cpu(pfn_sb->page_struct_size) < sizeof(struct page)) {
> > + dev_err(&nd_pfn->dev,
> > + "init failed, struct page size mismatch %d\n",
> > + le16_to_cpu(pfn_sb->page_struct_size));
> > + return -EOPNOTSUPP;
> > + }
> > +
>
> We need this here?

Yes, both ->page_struct_size and ->page_size are only relevant in the
PFN_MODE_PMEM case because PFN_MODE_RAM assumes all pages are
allocated dynamically and the size does not matter.

>
> From 9885b2f9ed81a2438fc81507cfcdbdb1aeab756c Mon Sep 17 00:00:00 2001
> From: "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com>
> Date: Fri, 9 Aug 2019 22:10:08 +0530
> Subject: [PATCH] nvdimm: check struct page size only if pfn node is PMEM
>
> We should do the check only with PFN_MODE_PMEM. If we use
> memory for backing vmemmap, we should be able to enable
> the namespace even if struct page size change.

Same as the other patches please drop the usage of "we" in the changelog.