Re: [PATCH v8 3/5] hw/block/nvme: add dulbe support

[Minwoo Im]

On 11/12 20:59, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
> 
> Add support for reporting the Deallocated or Unwritten Logical Block
> Error (DULBE).
> 
> Rely on the block status flags reported by the block layer and consider
> any block with the BDRV_BLOCK_ZERO flag to be deallocated.
> 
> Multiple factors affect when a Write Zeroes command result in
> deallocation of blocks.
> 
>   * the underlying file system block size
>   * the blockdev format
>   * the 'discard' and 'logical_block_size' parameters
> 
>      format | discard | wz (512B)  wz (4KiB)  wz (64KiB)
>     -----------------------------------------------------
>       qcow2    ignore   n          n          y
>       qcow2    unmap    n          n          y
>       raw      ignore   n          y          y
>       raw      unmap    n          y          y
> 
> So, this works best with an image in raw format and 4KiB LBAs, since
> holes can then be punched on a per-block basis (this assumes a file
> system with a 4kb block size, YMMV). A qcow2 image, uses a cluster size
> of 64KiB by default and blocks will only be marked deallocated if a full
> cluster is zeroed or discarded. However, this *is* consistent with the
> spec since Write Zeroes "should" deallocate the block if the Deallocate
> attribute is set and "may" deallocate if the Deallocate attribute is not
> set. Thus, we always try to deallocate (the BDRV_REQ_MAY_UNMAP flag is
> always set).
> 
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> ---
>  hw/block/nvme-ns.h    |  4 ++
>  include/block/nvme.h  |  5 +++
>  hw/block/nvme-ns.c    |  8 ++--
>  hw/block/nvme.c       | 91 ++++++++++++++++++++++++++++++++++++++++++-
>  hw/block/trace-events |  4 ++
>  5 files changed, 107 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/block/nvme-ns.h b/hw/block/nvme-ns.h
> index 83734f4606e1..44bf6271b744 100644
> --- a/hw/block/nvme-ns.h
> +++ b/hw/block/nvme-ns.h
> @@ -31,6 +31,10 @@ typedef struct NvmeNamespace {
>      NvmeIdNs     id_ns;
>  
>      NvmeNamespaceParams params;
> +
> +    struct {
> +        uint32_t err_rec;
> +    } features;
>  } NvmeNamespace;
>  
>  static inline uint32_t nvme_nsid(NvmeNamespace *ns)
> diff --git a/include/block/nvme.h b/include/block/nvme.h
> index 8a46d9cf015f..966c3bb304bd 100644
> --- a/include/block/nvme.h
> +++ b/include/block/nvme.h
> @@ -687,6 +687,7 @@ enum NvmeStatusCodes {
>      NVME_E2E_REF_ERROR          = 0x0284,
>      NVME_CMP_FAILURE            = 0x0285,
>      NVME_ACCESS_DENIED          = 0x0286,
> +    NVME_DULB                   = 0x0287,
>      NVME_MORE                   = 0x2000,
>      NVME_DNR                    = 0x4000,
>      NVME_NO_COMPLETE            = 0xffff,
> @@ -903,6 +904,9 @@ enum NvmeIdCtrlLpa {
>  #define NVME_AEC_NS_ATTR(aec)       ((aec >> 8) & 0x1)
>  #define NVME_AEC_FW_ACTIVATION(aec) ((aec >> 9) & 0x1)
>  
> +#define NVME_ERR_REC_TLER(err_rec)  (err_rec & 0xffff)
> +#define NVME_ERR_REC_DULBE(err_rec) (err_rec & 0x10000)
> +
>  enum NvmeFeatureIds {
>      NVME_ARBITRATION                = 0x1,
>      NVME_POWER_MANAGEMENT           = 0x2,
> @@ -1023,6 +1027,7 @@ enum NvmeNsIdentifierType {
>  
>  
>  #define NVME_ID_NS_NSFEAT_THIN(nsfeat)      ((nsfeat & 0x1))
> +#define NVME_ID_NS_NSFEAT_DULBE(nsfeat)     ((nsfeat >> 2) & 0x1)
>  #define NVME_ID_NS_FLBAS_EXTENDED(flbas)    ((flbas >> 4) & 0x1)
>  #define NVME_ID_NS_FLBAS_INDEX(flbas)       ((flbas & 0xf))
>  #define NVME_ID_NS_MC_SEPARATE(mc)          ((mc >> 1) & 0x1)
> diff --git a/hw/block/nvme-ns.c b/hw/block/nvme-ns.c
> index 31c80cdf5b5f..f1cc734c60f5 100644
> --- a/hw/block/nvme-ns.c
> +++ b/hw/block/nvme-ns.c
> @@ -33,9 +33,7 @@ static void nvme_ns_init(NvmeNamespace *ns)
>      NvmeIdNs *id_ns = &ns->id_ns;
>      int lba_index = NVME_ID_NS_FLBAS_INDEX(ns->id_ns.flbas);
>  
> -    if (blk_get_flags(ns->blkconf.blk) & BDRV_O_UNMAP) {
> -        ns->id_ns.dlfeat = 0x9;
> -    }
> +    ns->id_ns.dlfeat = 0x9;
>  
>      id_ns->lbaf[lba_index].ds = 31 - clz32(ns->blkconf.logical_block_size);
>  
> @@ -44,6 +42,9 @@ static void nvme_ns_init(NvmeNamespace *ns)
>      /* no thin provisioning */
>      id_ns->ncap = id_ns->nsze;
>      id_ns->nuse = id_ns->ncap;
> +
> +    /* support DULBE */
> +    id_ns->nsfeat |= 0x4;
>  }
>  
>  static int nvme_ns_init_blk(NvmeCtrl *n, NvmeNamespace *ns, Error **errp)
> @@ -92,6 +93,7 @@ int nvme_ns_setup(NvmeCtrl *n, NvmeNamespace *ns, Error 
> **errp)
>      }
>  
>      nvme_ns_init(ns);
> +
>      if (nvme_register_namespace(n, ns, errp)) {
>          return -1;
>      }
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index a96a996ddc0d..8d75a89fd872 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -105,6 +105,7 @@ static const bool nvme_feature_support[NVME_FID_MAX] = {
>  
>  static const uint32_t nvme_feature_cap[NVME_FID_MAX] = {
>      [NVME_TEMPERATURE_THRESHOLD]    = NVME_FEAT_CAP_CHANGE,
> +    [NVME_ERROR_RECOVERY]           = NVME_FEAT_CAP_CHANGE | 
> NVME_FEAT_CAP_NS,
>      [NVME_VOLATILE_WRITE_CACHE]     = NVME_FEAT_CAP_CHANGE,
>      [NVME_NUMBER_OF_QUEUES]         = NVME_FEAT_CAP_CHANGE,
>      [NVME_ASYNCHRONOUS_EVENT_CONF]  = NVME_FEAT_CAP_CHANGE,
> @@ -878,6 +879,49 @@ static inline uint16_t nvme_check_bounds(NvmeNamespace 
> *ns, uint64_t slba,
>      return NVME_SUCCESS;
>  }
>  
> +static uint16_t nvme_check_dulbe(NvmeNamespace *ns, uint64_t slba,
> +                                 uint32_t nlb)
> +{
> +    BlockDriverState *bs = blk_bs(ns->blkconf.blk);
> +
> +    int64_t pnum = 0, bytes = nvme_l2b(ns, nlb);
> +    int64_t offset = nvme_l2b(ns, slba);
> +    bool zeroed;
> +    int ret;
> +
> +    Error *local_err = NULL;
> +
> +    /*
> +     * `pnum` holds the number of bytes after offset that shares the same
> +     * allocation status as the byte at offset. If `pnum` is different from
> +     * `bytes`, we should check the allocation status of the next range and
> +     * continue this until all bytes have been checked.
> +     */
> +    do {
> +        bytes -= pnum;
> +
> +        ret = bdrv_block_status(bs, offset, bytes, &pnum, NULL, NULL);
> +        if (ret < 0) {
> +            error_setg_errno(&local_err, -ret, "unable to get block status");
> +            error_report_err(local_err);
> +
> +            return NVME_INTERNAL_DEV_ERROR;
> +        }
> +
> +        zeroed = !!(ret & BDRV_BLOCK_ZERO);
> +
> +        trace_pci_nvme_block_status(offset, bytes, pnum, ret, zeroed);
> +
> +        if (zeroed) {
> +            return NVME_DULB;
> +        }
> +
> +        offset += pnum;
> +    } while (pnum != bytes);
> +
> +    return NVME_SUCCESS;
> +}
> +
>  static void nvme_aio_err(NvmeRequest *req, int ret)
>  {
>      uint16_t status = NVME_SUCCESS;
> @@ -996,6 +1040,15 @@ static uint16_t nvme_rw(NvmeCtrl *n, NvmeRequest *req)
>          goto invalid;
>      }
>  
> +    if (acct == BLOCK_ACCT_READ) {
> +        if (NVME_ERR_REC_DULBE(ns->features.err_rec)) {
> +            status = nvme_check_dulbe(ns, slba, nlb);
> +            if (status) {
> +                goto invalid;
> +            }
> +        }
> +    }
> +
>      status = nvme_map_dptr(n, data_size, req);
>      if (status) {
>          goto invalid;
> @@ -1643,6 +1696,7 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, 
> NvmeRequest *req)
>      uint8_t fid = NVME_GETSETFEAT_FID(dw10);
>      NvmeGetFeatureSelect sel = NVME_GETFEAT_SELECT(dw10);
>      uint16_t iv;
> +    NvmeNamespace *ns;
>  
>      static const uint32_t nvme_feature_default[NVME_FID_MAX] = {
>          [NVME_ARBITRATION] = NVME_ARB_AB_NOLIMIT,
> @@ -1705,6 +1759,18 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, 
> NvmeRequest *req)
>          }
>  
>          return NVME_INVALID_FIELD | NVME_DNR;
> +    case NVME_ERROR_RECOVERY:
> +        if (!nvme_nsid_valid(n, nsid)) {
> +            return NVME_INVALID_NSID | NVME_DNR;
> +        }
> +
> +        ns = nvme_ns(n, nsid);
> +        if (unlikely(!ns)) {
> +            return NVME_INVALID_FIELD | NVME_DNR;
> +        }
> +
> +        result = ns->features.err_rec;
> +        goto out;
>      case NVME_VOLATILE_WRITE_CACHE:
>          result = n->features.vwc;
>          trace_pci_nvme_getfeat_vwcache(result ? "enabled" : "disabled");
> @@ -1777,7 +1843,7 @@ static uint16_t nvme_set_feature_timestamp(NvmeCtrl *n, 
> NvmeRequest *req)
>  
>  static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeRequest *req)
>  {
> -    NvmeNamespace *ns;
> +    NvmeNamespace *ns = NULL;
>  
>      NvmeCmd *cmd = &req->cmd;
>      uint32_t dw10 = le32_to_cpu(cmd->cdw10);
> @@ -1785,6 +1851,7 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, 
> NvmeRequest *req)
>      uint32_t nsid = le32_to_cpu(cmd->nsid);
>      uint8_t fid = NVME_GETSETFEAT_FID(dw10);
>      uint8_t save = NVME_SETFEAT_SAVE(dw10);
> +    int i;
>  
>      trace_pci_nvme_setfeat(nvme_cid(req), nsid, fid, save, dw11);
>  
> @@ -1844,11 +1911,31 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, 
> NvmeRequest *req)
>                                 NVME_LOG_SMART_INFO);
>          }
>  
> +        break;
> +    case NVME_ERROR_RECOVERY:
> +        if (nsid == NVME_NSID_BROADCAST) {
> +            for (i = 1; i <= n->num_namespaces; i++) {
> +                ns = nvme_ns(n, i);
> +
> +                if (!ns) {
> +                    continue;
> +                }
> +
> +                if (NVME_ID_NS_NSFEAT_DULBE(ns->id_ns.nsfeat)) {
> +                    ns->features.err_rec = dw11;
> +                }
> +            }
> +
> +            break;
> +        }
> +
> +        assert(ns);

Klaus,

Sorry, but can we have assert for the failure that might happen due to
user's mis-behavior?  Why don't we have NVME_INVALID_NSID for this case?