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

[Klaus Jensen <its@irrelevant.dk>]

[-- Attachment #1: Type: text/plain, Size: 5213 bytes --]

On Feb 10 20:14, Minwoo Im wrote:
> 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);
> 


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.dev@gmail.com>

-- 
One of us - No more doubt, silence or taboo about mental illness.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]