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);
Otherwise, it looks good to me.
Reviewed-by: Minwoo Im <minwoo.im....@gmail.com>
