| Message ID | 20190809074520.27115-2-aneesh.kumar@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Mark the namespace disabled on pfn superblock mismatch | expand |
Hi Aneesh, logic looks correct but there are some cleanups I'd like to
see and a lead-in patch that I attached.
I've started prefixing nvdimm patches with:
libnvdimm/$component:
...since this patch mostly impacts the pmem driver lets prefix it
"libnvdimm/pmem: "
On Fri, Aug 9, 2019 at 12:45 AM Aneesh Kumar K.V
<aneesh.kumar@linux.ibm.com> wrote:
>
> This patch add -EOPNOTSUPP as return from probe callback to
s/This patch add/Add/
No need to say "this patch" it's obviously a patch.
> indicate we were not able to initialize a namespace due to pfn superblock
> feature/version mismatch. We want to consider this a probe success so that
> we can create new namesapce seed and there by avoid marking the failed
> namespace as the seed namespace.
Please replace usage of "we" with the exact agent involved as which
"we" is being referred to gets confusing for the reader.
i.e. "indicate that the pmem driver was not..." "The nvdimm core wants
to consider this...".
>
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> ---
> drivers/nvdimm/bus.c | 2 +-
> drivers/nvdimm/pmem.c | 26 ++++++++++++++++++++++----
> 2 files changed, 23 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
> index 798c5c4aea9c..16c35e6446a7 100644
> --- a/drivers/nvdimm/bus.c
> +++ b/drivers/nvdimm/bus.c
> @@ -95,7 +95,7 @@ static int nvdimm_bus_probe(struct device *dev)
> rc = nd_drv->probe(dev);
> debug_nvdimm_unlock(dev);
>
> - if (rc == 0)
> + if (rc == 0 || rc == -EOPNOTSUPP)
> nd_region_probe_success(nvdimm_bus, dev);
This now makes the nd_region_probe_success() helper obviously misnamed
since it now wants to take actions on non-probe success. I attached a
lead-in cleanup that you can pull into your series that renames that
routine to nd_region_advance_seeds().
When you rebase this needs a comment about why EOPNOTSUPP has special handling.
> else
> nd_region_disable(nvdimm_bus, dev);
> diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
> index 4c121dd03dd9..3f498881dd28 100644
> --- a/drivers/nvdimm/pmem.c
> +++ b/drivers/nvdimm/pmem.c
> @@ -490,6 +490,7 @@ static int pmem_attach_disk(struct device *dev,
>
> static int nd_pmem_probe(struct device *dev)
> {
> + int ret;
> struct nd_namespace_common *ndns;
>
> ndns = nvdimm_namespace_common_probe(dev);
> @@ -505,12 +506,29 @@ static int nd_pmem_probe(struct device *dev)
> if (is_nd_pfn(dev))
> return pmem_attach_disk(dev, ndns);
>
> - /* if we find a valid info-block we'll come back as that personality */
> - if (nd_btt_probe(dev, ndns) == 0 || nd_pfn_probe(dev, ndns) == 0
> - || nd_dax_probe(dev, ndns) == 0)
Similar need for an updated comment here to explain the special
translation of error codes.
> + ret = nd_btt_probe(dev, ndns);
> + if (ret == 0)
> return -ENXIO;
> + else if (ret == -EOPNOTSUPP)
Are there cases where the btt driver needs to return EOPNOTSUPP? I'd
otherwise like to keep this special casing constrained to the pfn /
dax info block cases.
On Tue, Aug 13, 2019 at 9:22 PM Dan Williams <dan.j.williams@intel.com> wrote: > > Hi Aneesh, logic looks correct but there are some cleanups I'd like to > see and a lead-in patch that I attached. > > I've started prefixing nvdimm patches with: > > libnvdimm/$component: > > ...since this patch mostly impacts the pmem driver lets prefix it > "libnvdimm/pmem: " > > On Fri, Aug 9, 2019 at 12:45 AM Aneesh Kumar K.V > <aneesh.kumar@linux.ibm.com> wrote: > > > > This patch add -EOPNOTSUPP as return from probe callback to > > s/This patch add/Add/ > > No need to say "this patch" it's obviously a patch. > > > indicate we were not able to initialize a namespace due to pfn superblock > > feature/version mismatch. We want to consider this a probe success so that > > we can create new namesapce seed and there by avoid marking the failed > > namespace as the seed namespace. > > Please replace usage of "we" with the exact agent involved as which > "we" is being referred to gets confusing for the reader. > > i.e. "indicate that the pmem driver was not..." "The nvdimm core wants > to consider this...". > > > > > Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> > > --- > > drivers/nvdimm/bus.c | 2 +- > > drivers/nvdimm/pmem.c | 26 ++++++++++++++++++++++---- > > 2 files changed, 23 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c > > index 798c5c4aea9c..16c35e6446a7 100644 > > --- a/drivers/nvdimm/bus.c > > +++ b/drivers/nvdimm/bus.c > > @@ -95,7 +95,7 @@ static int nvdimm_bus_probe(struct device *dev) > > rc = nd_drv->probe(dev); > > debug_nvdimm_unlock(dev); > > > > - if (rc == 0) > > + if (rc == 0 || rc == -EOPNOTSUPP) > > nd_region_probe_success(nvdimm_bus, dev); > > This now makes the nd_region_probe_success() helper obviously misnamed > since it now wants to take actions on non-probe success. I attached a > lead-in cleanup that you can pull into your series that renames that > routine to nd_region_advance_seeds(). > > When you rebase this needs a comment about why EOPNOTSUPP has special handling. > > > else > > nd_region_disable(nvdimm_bus, dev); > > diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c > > index 4c121dd03dd9..3f498881dd28 100644 > > --- a/drivers/nvdimm/pmem.c > > +++ b/drivers/nvdimm/pmem.c > > @@ -490,6 +490,7 @@ static int pmem_attach_disk(struct device *dev, > > > > static int nd_pmem_probe(struct device *dev) > > { > > + int ret; > > struct nd_namespace_common *ndns; > > > > ndns = nvdimm_namespace_common_probe(dev); > > @@ -505,12 +506,29 @@ static int nd_pmem_probe(struct device *dev) > > if (is_nd_pfn(dev)) > > return pmem_attach_disk(dev, ndns); > > > > - /* if we find a valid info-block we'll come back as that personality */ > > - if (nd_btt_probe(dev, ndns) == 0 || nd_pfn_probe(dev, ndns) == 0 > > - || nd_dax_probe(dev, ndns) == 0) > > Similar need for an updated comment here to explain the special > translation of error codes. > > > + ret = nd_btt_probe(dev, ndns); > > + if (ret == 0) > > return -ENXIO; > > + else if (ret == -EOPNOTSUPP) > > Are there cases where the btt driver needs to return EOPNOTSUPP? I'd > otherwise like to keep this special casing constrained to the pfn / > dax info block cases. In fact I think EOPNOTSUPP is only something that the device-dax case would be concerned with because that's the only interface that attempts to guarantee a given mapping granularity.
Dan Williams <dan.j.williams@intel.com> writes: > On Tue, Aug 13, 2019 at 9:22 PM Dan Williams <dan.j.williams@intel.com> wrote: >> >> Hi Aneesh, logic looks correct but there are some cleanups I'd like to >> see and a lead-in patch that I attached. >> >> I've started prefixing nvdimm patches with: >> >> libnvdimm/$component: >> >> ...since this patch mostly impacts the pmem driver lets prefix it >> "libnvdimm/pmem: " >> >> On Fri, Aug 9, 2019 at 12:45 AM Aneesh Kumar K.V >> <aneesh.kumar@linux.ibm.com> wrote: >> > >> > This patch add -EOPNOTSUPP as return from probe callback to >> >> s/This patch add/Add/ >> >> No need to say "this patch" it's obviously a patch. >> >> > indicate we were not able to initialize a namespace due to pfn superblock >> > feature/version mismatch. We want to consider this a probe success so that >> > we can create new namesapce seed and there by avoid marking the failed >> > namespace as the seed namespace. >> >> Please replace usage of "we" with the exact agent involved as which >> "we" is being referred to gets confusing for the reader. >> >> i.e. "indicate that the pmem driver was not..." "The nvdimm core wants >> to consider this...". >> >> > >> > Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> >> > --- >> > drivers/nvdimm/bus.c | 2 +- >> > drivers/nvdimm/pmem.c | 26 ++++++++++++++++++++++---- >> > 2 files changed, 23 insertions(+), 5 deletions(-) >> > >> > diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c >> > index 798c5c4aea9c..16c35e6446a7 100644 >> > --- a/drivers/nvdimm/bus.c >> > +++ b/drivers/nvdimm/bus.c >> > @@ -95,7 +95,7 @@ static int nvdimm_bus_probe(struct device *dev) >> > rc = nd_drv->probe(dev); >> > debug_nvdimm_unlock(dev); >> > >> > - if (rc == 0) >> > + if (rc == 0 || rc == -EOPNOTSUPP) >> > nd_region_probe_success(nvdimm_bus, dev); >> >> This now makes the nd_region_probe_success() helper obviously misnamed >> since it now wants to take actions on non-probe success. I attached a >> lead-in cleanup that you can pull into your series that renames that >> routine to nd_region_advance_seeds(). >> >> When you rebase this needs a comment about why EOPNOTSUPP has special handling. >> >> > else >> > nd_region_disable(nvdimm_bus, dev); >> > diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c >> > index 4c121dd03dd9..3f498881dd28 100644 >> > --- a/drivers/nvdimm/pmem.c >> > +++ b/drivers/nvdimm/pmem.c >> > @@ -490,6 +490,7 @@ static int pmem_attach_disk(struct device *dev, >> > >> > static int nd_pmem_probe(struct device *dev) >> > { >> > + int ret; >> > struct nd_namespace_common *ndns; >> > >> > ndns = nvdimm_namespace_common_probe(dev); >> > @@ -505,12 +506,29 @@ static int nd_pmem_probe(struct device *dev) >> > if (is_nd_pfn(dev)) >> > return pmem_attach_disk(dev, ndns); >> > >> > - /* if we find a valid info-block we'll come back as that personality */ >> > - if (nd_btt_probe(dev, ndns) == 0 || nd_pfn_probe(dev, ndns) == 0 >> > - || nd_dax_probe(dev, ndns) == 0) >> >> Similar need for an updated comment here to explain the special >> translation of error codes. >> >> > + ret = nd_btt_probe(dev, ndns); >> > + if (ret == 0) >> > return -ENXIO; >> > + else if (ret == -EOPNOTSUPP) >> >> Are there cases where the btt driver needs to return EOPNOTSUPP? I'd >> otherwise like to keep this special casing constrained to the pfn / >> dax info block cases. > > In fact I think EOPNOTSUPP is only something that the device-dax case > would be concerned with because that's the only interface that > attempts to guarantee a given mapping granularity. We need to do similar error handling w.r.t fsdax when the pfn superblock indicates different PAGE_SIZE and struct page size? I don't think btt needs to support EOPNOTSUPP. But we can keep it for consistency? -aneesh
On Mon, Aug 19, 2019 at 12:07 AM Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> wrote: > > Dan Williams <dan.j.williams@intel.com> writes: > > > On Tue, Aug 13, 2019 at 9:22 PM Dan Williams <dan.j.williams@intel.com> wrote: > >> > >> Hi Aneesh, logic looks correct but there are some cleanups I'd like to > >> see and a lead-in patch that I attached. > >> > >> I've started prefixing nvdimm patches with: > >> > >> libnvdimm/$component: > >> > >> ...since this patch mostly impacts the pmem driver lets prefix it > >> "libnvdimm/pmem: " > >> > >> On Fri, Aug 9, 2019 at 12:45 AM Aneesh Kumar K.V > >> <aneesh.kumar@linux.ibm.com> wrote: > >> > > >> > This patch add -EOPNOTSUPP as return from probe callback to > >> > >> s/This patch add/Add/ > >> > >> No need to say "this patch" it's obviously a patch. > >> > >> > indicate we were not able to initialize a namespace due to pfn superblock > >> > feature/version mismatch. We want to consider this a probe success so that > >> > we can create new namesapce seed and there by avoid marking the failed > >> > namespace as the seed namespace. > >> > >> Please replace usage of "we" with the exact agent involved as which > >> "we" is being referred to gets confusing for the reader. > >> > >> i.e. "indicate that the pmem driver was not..." "The nvdimm core wants > >> to consider this...". > >> > >> > > >> > Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> > >> > --- > >> > drivers/nvdimm/bus.c | 2 +- > >> > drivers/nvdimm/pmem.c | 26 ++++++++++++++++++++++---- > >> > 2 files changed, 23 insertions(+), 5 deletions(-) > >> > > >> > diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c > >> > index 798c5c4aea9c..16c35e6446a7 100644 > >> > --- a/drivers/nvdimm/bus.c > >> > +++ b/drivers/nvdimm/bus.c > >> > @@ -95,7 +95,7 @@ static int nvdimm_bus_probe(struct device *dev) > >> > rc = nd_drv->probe(dev); > >> > debug_nvdimm_unlock(dev); > >> > > >> > - if (rc == 0) > >> > + if (rc == 0 || rc == -EOPNOTSUPP) > >> > nd_region_probe_success(nvdimm_bus, dev); > >> > >> This now makes the nd_region_probe_success() helper obviously misnamed > >> since it now wants to take actions on non-probe success. I attached a > >> lead-in cleanup that you can pull into your series that renames that > >> routine to nd_region_advance_seeds(). > >> > >> When you rebase this needs a comment about why EOPNOTSUPP has special handling. > >> > >> > else > >> > nd_region_disable(nvdimm_bus, dev); > >> > diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c > >> > index 4c121dd03dd9..3f498881dd28 100644 > >> > --- a/drivers/nvdimm/pmem.c > >> > +++ b/drivers/nvdimm/pmem.c > >> > @@ -490,6 +490,7 @@ static int pmem_attach_disk(struct device *dev, > >> > > >> > static int nd_pmem_probe(struct device *dev) > >> > { > >> > + int ret; > >> > struct nd_namespace_common *ndns; > >> > > >> > ndns = nvdimm_namespace_common_probe(dev); > >> > @@ -505,12 +506,29 @@ static int nd_pmem_probe(struct device *dev) > >> > if (is_nd_pfn(dev)) > >> > return pmem_attach_disk(dev, ndns); > >> > > >> > - /* if we find a valid info-block we'll come back as that personality */ > >> > - if (nd_btt_probe(dev, ndns) == 0 || nd_pfn_probe(dev, ndns) == 0 > >> > - || nd_dax_probe(dev, ndns) == 0) > >> > >> Similar need for an updated comment here to explain the special > >> translation of error codes. > >> > >> > + ret = nd_btt_probe(dev, ndns); > >> > + if (ret == 0) > >> > return -ENXIO; > >> > + else if (ret == -EOPNOTSUPP) > >> > >> Are there cases where the btt driver needs to return EOPNOTSUPP? I'd > >> otherwise like to keep this special casing constrained to the pfn / > >> dax info block cases. > > > > In fact I think EOPNOTSUPP is only something that the device-dax case > > would be concerned with because that's the only interface that > > attempts to guarantee a given mapping granularity. > > We need to do similar error handling w.r.t fsdax when the pfn superblock > indicates different PAGE_SIZE and struct page size? Only in the case where PAGE_SIZE is less than the pfn superblock page size, the memmap is stored on pmem, and the reservation is too small. Otherwise the PAGE_SIZE difference does not matter in practice for the fsdax case... unless I'm overlooking another failure case? > I don't think btt > needs to support EOPNOTSUPP. But we can keep it for consistency? That's not a sufficient argument in my mind. The comment about why EOPNOTSUPP is treated specially should have a note about the known usages, and since there is no BTT case for it lets leave it out.
diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c index 798c5c4aea9c..16c35e6446a7 100644 --- a/drivers/nvdimm/bus.c +++ b/drivers/nvdimm/bus.c @@ -95,7 +95,7 @@ static int nvdimm_bus_probe(struct device *dev) rc = nd_drv->probe(dev); debug_nvdimm_unlock(dev); - if (rc == 0) + if (rc == 0 || rc == -EOPNOTSUPP) nd_region_probe_success(nvdimm_bus, dev); else nd_region_disable(nvdimm_bus, dev); diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c index 4c121dd03dd9..3f498881dd28 100644 --- a/drivers/nvdimm/pmem.c +++ b/drivers/nvdimm/pmem.c @@ -490,6 +490,7 @@ static int pmem_attach_disk(struct device *dev, static int nd_pmem_probe(struct device *dev) { + int ret; struct nd_namespace_common *ndns; ndns = nvdimm_namespace_common_probe(dev); @@ -505,12 +506,29 @@ static int nd_pmem_probe(struct device *dev) if (is_nd_pfn(dev)) return pmem_attach_disk(dev, ndns); - /* if we find a valid info-block we'll come back as that personality */ - if (nd_btt_probe(dev, ndns) == 0 || nd_pfn_probe(dev, ndns) == 0 - || nd_dax_probe(dev, ndns) == 0) + ret = nd_btt_probe(dev, ndns); + if (ret == 0) return -ENXIO; + else if (ret == -EOPNOTSUPP) + return ret; - /* ...otherwise we're just a raw pmem device */ + ret = nd_pfn_probe(dev, ndns); + if (ret == 0) + return -ENXIO; + else if (ret == -EOPNOTSUPP) + return ret; + + ret = nd_dax_probe(dev, ndns); + if (ret == 0) + return -ENXIO; + else if (ret == -EOPNOTSUPP) + return ret; + /* + * We have two failure conditions here, there is no + * info reserver block or we found a valid info reserve block + * but failed to initialize the pfn superblock. + * Don't create a raw pmem disk for the second case. + */ return pmem_attach_disk(dev, ndns); }
This patch add -EOPNOTSUPP as return from probe callback to indicate we were not able to initialize a namespace due to pfn superblock feature/version mismatch. We want to consider this a probe success so that we can create new namesapce seed and there by avoid marking the failed namespace as the seed namespace. Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> --- drivers/nvdimm/bus.c | 2 +- drivers/nvdimm/pmem.c | 26 ++++++++++++++++++++++---- 2 files changed, 23 insertions(+), 5 deletions(-)