Re: [PATCH 2/2] hw/block/nvme: add write uncorrectable command

[Minwoo Im <minwoo.im.dev@gmail.com>]

On 21-02-10 08:06:46, Klaus Jensen wrote:
> From: Gollu Appalanaidu <anaidu.gollu@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.gollu@samsung.com>
> Signed-off-by: Klaus Jensen <k.jensen@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.dev@gmail.com>