On Feb 10 20:14, Minwoo Im wrote: > On 21-02-10 08:06:46, Klaus Jensen wrote: > > From: Gollu Appalanaidu <anaidu.go...@samsung.com> > > > > Add support for marking blocks invalid with the Write Uncorrectable > > command. Block status is tracked in a (non-persistent) bitmap that is > > checked on all reads and written to on all writes. This is potentially > > expensive, so keep Write Uncorrectable disabled by default. > > > > Signed-off-by: Gollu Appalanaidu <anaidu.go...@samsung.com> > > Signed-off-by: Klaus Jensen <k.jen...@samsung.com> > > --- > > docs/specs/nvme.txt | 3 ++ > > hw/block/nvme-ns.h | 2 ++ > > hw/block/nvme.h | 1 + > > hw/block/nvme-ns.c | 2 ++ > > hw/block/nvme.c | 65 +++++++++++++++++++++++++++++++++++++------ > > hw/block/trace-events | 1 + > > 6 files changed, 66 insertions(+), 8 deletions(-) > > > > diff --git a/docs/specs/nvme.txt b/docs/specs/nvme.txt > > index 56d393884e7a..88f9cc278d4c 100644 > > --- a/docs/specs/nvme.txt > > +++ b/docs/specs/nvme.txt > > @@ -19,5 +19,8 @@ Known issues > > > > * The accounting numbers in the SMART/Health are reset across power cycles > > > > +* Marking blocks invalid with the Write Uncorrectable is not persisted > > across > > + power cycles. > > + > > * Interrupt Coalescing is not supported and is disabled by default in > > volation > > of the specification. > > diff --git a/hw/block/nvme-ns.h b/hw/block/nvme-ns.h > > index 7af6884862b5..15fa422ded03 100644 > > --- a/hw/block/nvme-ns.h > > +++ b/hw/block/nvme-ns.h > > @@ -72,6 +72,8 @@ typedef struct NvmeNamespace { > > struct { > > uint32_t err_rec; > > } features; > > + > > + unsigned long *uncorrectable; > > } NvmeNamespace; > > > > static inline uint32_t nvme_nsid(NvmeNamespace *ns) > > diff --git a/hw/block/nvme.h b/hw/block/nvme.h > > index 98082b2dfba3..9b8f85b9cf16 100644 > > --- a/hw/block/nvme.h > > +++ b/hw/block/nvme.h > > @@ -68,6 +68,7 @@ static inline const char *nvme_io_opc_str(uint8_t opc) > > case NVME_CMD_FLUSH: return "NVME_NVM_CMD_FLUSH"; > > case NVME_CMD_WRITE: return "NVME_NVM_CMD_WRITE"; > > case NVME_CMD_READ: return "NVME_NVM_CMD_READ"; > > + case NVME_CMD_WRITE_UNCOR: return "NVME_CMD_WRITE_UNCOR"; > > case NVME_CMD_COMPARE: return "NVME_NVM_CMD_COMPARE"; > > case NVME_CMD_WRITE_ZEROES: return "NVME_NVM_CMD_WRITE_ZEROES"; > > case NVME_CMD_DSM: return "NVME_NVM_CMD_DSM"; > > diff --git a/hw/block/nvme-ns.c b/hw/block/nvme-ns.c > > index ade46e2f3739..742bbc4b4b62 100644 > > --- a/hw/block/nvme-ns.c > > +++ b/hw/block/nvme-ns.c > > @@ -72,6 +72,8 @@ static int nvme_ns_init(NvmeNamespace *ns, Error **errp) > > id_ns->mcl = cpu_to_le32(ns->params.mcl); > > id_ns->msrc = ns->params.msrc; > > > > + ns->uncorrectable = bitmap_new(id_ns->nsze); > > + > > return 0; > > } > > > > diff --git a/hw/block/nvme.c b/hw/block/nvme.c > > index e5f6666725d7..56048046c193 100644 > > --- a/hw/block/nvme.c > > +++ b/hw/block/nvme.c > > @@ -1112,6 +1112,20 @@ static uint16_t nvme_check_dulbe(NvmeNamespace *ns, > > uint64_t slba, > > return NVME_SUCCESS; > > } > > > > +static inline uint16_t nvme_check_uncor(NvmeNamespace *ns, uint64_t slba, > > + uint32_t nlb) > > +{ > > + uint64_t elba = nlb + slba; > > + > > + if (ns->uncorrectable) { > > + if (find_next_bit(ns->uncorrectable, elba, slba) < elba) { > > + return NVME_UNRECOVERED_READ | NVME_DNR; > > + } > > + } > > + > > + return NVME_SUCCESS; > > +} > > + > > static void nvme_aio_err(NvmeRequest *req, int ret) > > { > > uint16_t status = NVME_SUCCESS; > > @@ -1423,14 +1437,24 @@ static void nvme_rw_cb(void *opaque, int ret) > > BlockAcctCookie *acct = &req->acct; > > BlockAcctStats *stats = blk_get_stats(blk); > > > > + bool is_write = nvme_is_write(req); > > + > > trace_pci_nvme_rw_cb(nvme_cid(req), blk_name(blk)); > > > > - if (ns->params.zoned && nvme_is_write(req)) { > > + if (ns->params.zoned && is_write) { > > nvme_finalize_zoned_write(ns, req); > > } > > > > if (!ret) { > > block_acct_done(stats, acct); > > + > > + if (is_write) { > > + NvmeRwCmd *rw = (NvmeRwCmd *)&req->cmd; > > + uint64_t slba = le64_to_cpu(rw->slba); > > + uint32_t nlb = le16_to_cpu(rw->nlb) + 1; > > + > > + bitmap_clear(ns->uncorrectable, slba, nlb); > > It might be nitpick, 'nlb' would easily represent the value which is > defined itself in the spec which is zero-based. Can we have this like: > > uint32_t nlb = le16_to_cpu(rw->nlb); > > bitmap_clear(ns->uncorrectable, slba, nlb + 1); >
I do not disagree, but the `uint32_t nlb = le16_to_cpu(rw->nlb) + 1;` pattern is already used in several places. > Otherwise, it looks good to me. > > Reviewed-by: Minwoo Im <minwoo.im....@gmail.com> -- One of us - No more doubt, silence or taboo about mental illness.
signature.asc
Description: PGP signature