Hi Aneesh, Thanks for looking into this patch. "Aneesh Kumar K.V" <aneesh.kumar@xxxxxxxxxxxxx> writes: > Vaibhav Jain <vaibhav@xxxxxxxxxxxxx> 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. >> >> Similar problem is also seen with virtio-pmem where the 'deep_flush' >> sysfs attribute is not visible as in absence of any registered nvdimm, >> 'nd_region->ndr_mappings == 0'. >> >> Fix this by updating nvdimm_has_flush() adding a condition to >> nvdimm_has_flush() testing for ND_REGION_ASYNC flag on the region >> and see if a 'region->flush' callback is assigned. Also remove >> explicit test for 'nd_region->ndr_mapping' since regions can be marked >> 'ND_REGION_SYNC' without any explicit mappings as in case of >> virtio-pmem. > > Do we need to check for ND_REGION_ASYNC? What if the backend wants to > provide a synchronous dax region but with different deep flush semantic > than writing to wpq flush address? > ie, For a synchronous dax region, writes arent expected to require any flushing (or deep-flush) so this function should ideally return '0' in such a case. Hence I had added the test for ND_REGION_ASYNC region flag. > >> >> References: >> [1] "powerpc/papr_scm: Implement support for H_SCM_FLUSH hcall" >> https://lore.kernel.org/linux-nvdimm/161703936121.36.7260632399 582101498.stgit@e1fbed493c87 >> >> Cc: <stable@xxxxxxxxxxxxxxx> >> Fixes: c5d4355d10d4 ("libnvdimm: nd_region flush callback support") >> Reported-by: Shivaprasad G Bhat <sbhat@xxxxxxxxxxxxx> >> Signed-off-by: Vaibhav Jain <vaibhav@xxxxxxxxxxxxx> >> --- >> Changelog: >> >> v2: >> * Added the fixes tag and addressed the patch to stable tree [ Aneesh ] >> * Updated patch description to address the virtio-pmem case. >> * Removed test for 'nd_region->ndr_mappings' from beginning of >> nvdimm_has_flush() to handle the virtio-pmem case. >> --- >> drivers/nvdimm/region_devs.c | 14 +++++++++----- >> 1 file changed, 9 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/nvdimm/region_devs.c b/drivers/nvdimm/region_devs.c >> index ef23119db574..cdf5eb6fa425 100644 >> --- a/drivers/nvdimm/region_devs.c >> +++ b/drivers/nvdimm/region_devs.c >> @@ -1234,11 +1234,15 @@ int nvdimm_has_flush(struct nd_region *nd_region) >> { >> int i; >> >> - /* no nvdimm or pmem api == flushing capability unknown */ >> - if (nd_region->ndr_mappings == 0 >> - || !IS_ENABLED(CONFIG_ARCH_HAS_PMEM_API)) >> + /* no pmem api == flushing capability unknown */ >> + if (!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; >> + > > > if (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 +1253,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@xxxxxxxxxxxx >> To unsubscribe send an email to linux-nvdimm-leave@xxxxxxxxxxxx > _______________________________________________ > Linux-nvdimm mailing list -- linux-nvdimm@xxxxxxxxxxxx > To unsubscribe send an email to linux-nvdimm-leave@xxxxxxxxxxxx -- Cheers ~ Vaibhav
