On Wed, Nov 9, 2016 at 4:14 PM, Vishal Verma <[email protected]> wrote: > On 11/09, Dave Jiang wrote: >> We need to clear any poison when we are writing to pmem. The granularity >> will be sector size. If it's less then we can't do anything about it >> barring corruption. >> >> Signed-off-by: Dave Jiang <[email protected]> >> --- >> drivers/nvdimm/claim.c | 20 ++++++++++++++++++-- >> 1 file changed, 18 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/nvdimm/claim.c b/drivers/nvdimm/claim.c >> index d5dc80c..2078d25 100644 >> --- a/drivers/nvdimm/claim.c >> +++ b/drivers/nvdimm/claim.c >> @@ -226,6 +226,7 @@ static int nsio_rw_bytes(struct nd_namespace_common >> *ndns, >> resource_size_t offset, void *buf, size_t size, int rw) >> { >> struct nd_namespace_io *nsio = to_nd_namespace_io(&ndns->dev); >> + unsigned int sz_align = ALIGN(size + (offset & (512 - 1)), 512); >> >> if (unlikely(offset + size > nsio->size)) { >> dev_WARN_ONCE(&ndns->dev, 1, "request out of range\n"); >> @@ -233,12 +234,27 @@ static int nsio_rw_bytes(struct nd_namespace_common >> *ndns, >> } >> >> if (rw == READ) { >> - unsigned int sz_align = ALIGN(size + (offset & (512 - 1)), >> 512); >> - >> if (unlikely(is_bad_pmem(&nsio->bb, offset / 512, sz_align))) >> return -EIO; >> return memcpy_from_pmem(buf, nsio->addr + offset, size); >> } else { >> + sector_t sector = offset / 512; > > Small nit, but if you move the above line to the top of the function, > you can reuse 'sector' int he rw == READ case's is_bad_pmem call too, > making it read the same as the one below. > >> + >> + if (unlikely(is_bad_pmem(&nsio->bb, sector, sz_align))) { >> + if (IS_ALIGNED(offset, 512) && >> + IS_ALIGNED(size, 512) && (size >= 512)) { > > Having the size >= 512 check here feels a bit odd.. If you can refactor > this to have something near the top to simply exit out for size == 0 > (that should never happen anyway), then you can simply remove this last > part of the condition. If size > 0, and if it is aligned to 512 > automatically implies it will at least be 512. > >> + int rc; >> + >> + rc = nvdimm_clear_poison(&ndns->dev, offset, >> + size); > > nvdimm_clear_poison returns a long, rc is an int.. While at it, since > this is the only palce where 'rc' is being used, perhaps rename it to > something like 'cleared' (like in pmem_clear_poison), so that the usage > is better described. > >> + if (rc != size) >> + return -EIO; > > I'm not sure of this myself - perhaps a question to Dan - > Is it safe to return EIO here? We have potentially cleared some poison, > and lost some data, but subsequent reads will not indicate any errors, > returning either 0s or something unpredictable. Would it be better to > simply go ahead with the write even if we weren't able to clear the > whole range?
Good point, the interface allows partial completions, so it's reasonable to go write what we were able to clear. However that only shrinks the exposure not eliminate it since we could immediately crash for some other reason before the write completes. We would an error clearing interface that had deterministic data after clearing to do something better, but I can get on board with writing new data after a partial completion. _______________________________________________ Linux-nvdimm mailing list [email protected] https://lists.01.org/mailman/listinfo/linux-nvdimm
