| Message ID | 20210402092555.208590-1-vaibhav@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | libnvdimm/region: Update nvdimm_has_flush() to handle ND_REGION_ASYNC | expand |
Vaibhav Jain <vaibhav@linux.ibm.com> writes: > In case a platform doesn't provide explicit flush-hints but provides an > explicit flush callback via ND_REGION_ASYNC region flag, then > nvdimm_has_flush() still returns '0' indicating that writes do not > require flushing. This happens on PPC64 with patch at [1] applied, > where 'deep_flush' of a region was denied even though an explicit > flush function was provided. > > Fix this by adding a condition to nvdimm_has_flush() to test for the > ND_REGION_ASYNC flag on the region and see if a 'region->flush' > callback is assigned. > May be this should have Fixes: c5d4355d10d4 ("libnvdimm: nd_region flush callback support") Without this we will mark the pmem disk not having FUA support? > References: > [1] "powerpc/papr_scm: Implement support for H_SCM_FLUSH hcall" > https://lore.kernel.org/linux-nvdimm/161703936121.36.7260632399 582101498.stgit@e1fbed493c87 > > Reported-by: Shivaprasad G Bhat <sbhat@linux.ibm.com> > Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com> > --- > drivers/nvdimm/region_devs.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/drivers/nvdimm/region_devs.c b/drivers/nvdimm/region_devs.c > index ef23119db574..e05cc9f8a9fd 100644 > --- a/drivers/nvdimm/region_devs.c > +++ b/drivers/nvdimm/region_devs.c > @@ -1239,6 +1239,11 @@ int nvdimm_has_flush(struct nd_region *nd_region) > || !IS_ENABLED(CONFIG_ARCH_HAS_PMEM_API)) > return -ENXIO; > > + /* Test if an explicit flush function is defined */ > + if (test_bit(ND_REGION_ASYNC, &nd_region->flags) && nd_region->flush) > + return 1; > + > + /* Test if any flush hints for the region are available */ > for (i = 0; i < nd_region->ndr_mappings; i++) { > struct nd_mapping *nd_mapping = &nd_region->mapping[i]; > struct nvdimm *nvdimm = nd_mapping->nvdimm; > @@ -1249,8 +1254,8 @@ int nvdimm_has_flush(struct nd_region *nd_region) > } > > /* > - * The platform defines dimm devices without hints, assume > - * platform persistence mechanism like ADR > + * The platform defines dimm devices without hints nor explicit flush, > + * assume platform persistence mechanism like ADR > */ > return 0; > } > -- > 2.30.2 > _______________________________________________ > Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org > To unsubscribe send an email to linux-nvdimm-leave@lists.01.org
On Fri, Apr 2, 2021 at 2:26 AM Vaibhav Jain <vaibhav@linux.ibm.com> wrote: > > In case a platform doesn't provide explicit flush-hints but provides an > explicit flush callback via ND_REGION_ASYNC region flag, then > nvdimm_has_flush() still returns '0' indicating that writes do not > require flushing. This happens on PPC64 with patch at [1] applied, > where 'deep_flush' of a region was denied even though an explicit > flush function was provided. > > Fix this by adding a condition to nvdimm_has_flush() to test for the > ND_REGION_ASYNC flag on the region and see if a 'region->flush' > callback is assigned. Looks good. > > References: > [1] "powerpc/papr_scm: Implement support for H_SCM_FLUSH hcall" > https://lore.kernel.org/linux-nvdimm/161703936121.36.7260632399 582101498.stgit@e1fbed493c87 Looks like a typo happened in that link, I can fix that up. I'll also change this to the canonical "Link:" tag for references. > > Reported-by: Shivaprasad G Bhat <sbhat@linux.ibm.com> > Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com> > --- > drivers/nvdimm/region_devs.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/drivers/nvdimm/region_devs.c b/drivers/nvdimm/region_devs.c > index ef23119db574..e05cc9f8a9fd 100644 > --- a/drivers/nvdimm/region_devs.c > +++ b/drivers/nvdimm/region_devs.c > @@ -1239,6 +1239,11 @@ int nvdimm_has_flush(struct nd_region *nd_region) > || !IS_ENABLED(CONFIG_ARCH_HAS_PMEM_API)) > return -ENXIO; > > + /* Test if an explicit flush function is defined */ > + if (test_bit(ND_REGION_ASYNC, &nd_region->flags) && nd_region->flush) > + return 1; > + > + /* Test if any flush hints for the region are available */ > for (i = 0; i < nd_region->ndr_mappings; i++) { > struct nd_mapping *nd_mapping = &nd_region->mapping[i]; > struct nvdimm *nvdimm = nd_mapping->nvdimm; > @@ -1249,8 +1254,8 @@ int nvdimm_has_flush(struct nd_region *nd_region) > } > > /* > - * The platform defines dimm devices without hints, assume > - * platform persistence mechanism like ADR > + * The platform defines dimm devices without hints nor explicit flush, > + * assume platform persistence mechanism like ADR > */ > return 0; > } > -- > 2.30.2 >
On Mon, Apr 5, 2021 at 4:31 AM Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> wrote: > > Vaibhav Jain <vaibhav@linux.ibm.com> writes: > > > In case a platform doesn't provide explicit flush-hints but provides an > > explicit flush callback via ND_REGION_ASYNC region flag, then > > nvdimm_has_flush() still returns '0' indicating that writes do not > > require flushing. This happens on PPC64 with patch at [1] applied, > > where 'deep_flush' of a region was denied even though an explicit > > flush function was provided. > > > > Fix this by adding a condition to nvdimm_has_flush() to test for the > > ND_REGION_ASYNC flag on the region and see if a 'region->flush' > > callback is assigned. > > > > May be this should have > Fixes: c5d4355d10d4 ("libnvdimm: nd_region flush callback support") Yes, thanks for that. > > Without this we will mark the pmem disk not having FUA support? Yes. > > > > References: > > [1] "powerpc/papr_scm: Implement support for H_SCM_FLUSH hcall" > > https://lore.kernel.org/linux-nvdimm/161703936121.36.7260632399 582101498.stgit@e1fbed493c87 > > > > Reported-by: Shivaprasad G Bhat <sbhat@linux.ibm.com> > > Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com> > > --- > > drivers/nvdimm/region_devs.c | 9 +++++++-- > > 1 file changed, 7 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/nvdimm/region_devs.c b/drivers/nvdimm/region_devs.c > > index ef23119db574..e05cc9f8a9fd 100644 > > --- a/drivers/nvdimm/region_devs.c > > +++ b/drivers/nvdimm/region_devs.c > > @@ -1239,6 +1239,11 @@ int nvdimm_has_flush(struct nd_region *nd_region) > > || !IS_ENABLED(CONFIG_ARCH_HAS_PMEM_API)) > > return -ENXIO; > > > > + /* Test if an explicit flush function is defined */ > > + if (test_bit(ND_REGION_ASYNC, &nd_region->flags) && nd_region->flush) > > + return 1; > > > + > > + /* Test if any flush hints for the region are available */ > > for (i = 0; i < nd_region->ndr_mappings; i++) { > > struct nd_mapping *nd_mapping = &nd_region->mapping[i]; > > struct nvdimm *nvdimm = nd_mapping->nvdimm; > > @@ -1249,8 +1254,8 @@ int nvdimm_has_flush(struct nd_region *nd_region) > > } > > > > /* > > - * The platform defines dimm devices without hints, assume > > - * platform persistence mechanism like ADR > > + * The platform defines dimm devices without hints nor explicit flush, > > + * assume platform persistence mechanism like ADR > > */ > > return 0; > > } > > -- > > 2.30.2 > > _______________________________________________ > > Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org > > To unsubscribe send an email to linux-nvdimm-leave@lists.01.org
diff --git a/drivers/nvdimm/region_devs.c b/drivers/nvdimm/region_devs.c index ef23119db574..e05cc9f8a9fd 100644 --- a/drivers/nvdimm/region_devs.c +++ b/drivers/nvdimm/region_devs.c @@ -1239,6 +1239,11 @@ int nvdimm_has_flush(struct nd_region *nd_region) || !IS_ENABLED(CONFIG_ARCH_HAS_PMEM_API)) return -ENXIO; + /* Test if an explicit flush function is defined */ + if (test_bit(ND_REGION_ASYNC, &nd_region->flags) && nd_region->flush) + return 1; + + /* Test if any flush hints for the region are available */ for (i = 0; i < nd_region->ndr_mappings; i++) { struct nd_mapping *nd_mapping = &nd_region->mapping[i]; struct nvdimm *nvdimm = nd_mapping->nvdimm; @@ -1249,8 +1254,8 @@ int nvdimm_has_flush(struct nd_region *nd_region) } /* - * The platform defines dimm devices without hints, assume - * platform persistence mechanism like ADR + * The platform defines dimm devices without hints nor explicit flush, + * assume platform persistence mechanism like ADR */ return 0; }
In case a platform doesn't provide explicit flush-hints but provides an explicit flush callback via ND_REGION_ASYNC region flag, then nvdimm_has_flush() still returns '0' indicating that writes do not require flushing. This happens on PPC64 with patch at [1] applied, where 'deep_flush' of a region was denied even though an explicit flush function was provided. Fix this by adding a condition to nvdimm_has_flush() to test for the ND_REGION_ASYNC flag on the region and see if a 'region->flush' callback is assigned. References: [1] "powerpc/papr_scm: Implement support for H_SCM_FLUSH hcall" https://lore.kernel.org/linux-nvdimm/161703936121.36.7260632399 582101498.stgit@e1fbed493c87 Reported-by: Shivaprasad G Bhat <sbhat@linux.ibm.com> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com> --- drivers/nvdimm/region_devs.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)