Re: [PATCH 3/6] hw/nvme: Fix block accounting in nvme_copy()

On Jul 24 17:23, Hanna Czenczek wrote:
> In nvme_copy(), we start accounting for each of the copied ranges, but
> only end it once (in nvme_copy_done()). We should end it after each
> read/write operation is done instead, so they are properly accounted
> for.
> 
> We should also actually pass the number of bytes we are reading/writing
> (instead of 0), and probably account for the metadata operations also,
> as they are block operations we execute.
> 
> Signed-off-by: Hanna Czenczek <[email protected]>

Thanks for this,

Reviewed-by: Klaus Jensen <[email protected]>

> ---
>  hw/nvme/ctrl.c | 53 +++++++++++++++++++++++++++++++-------------------
>  1 file changed, 33 insertions(+), 20 deletions(-)
> 
> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> index a67e1598891..32e78881234 100644
> --- a/hw/nvme/ctrl.c
> +++ b/hw/nvme/ctrl.c
> @@ -2801,8 +2801,6 @@ static const AIOCBInfo nvme_copy_aiocb_info = {
>  static void nvme_copy_done(NvmeCopyAIOCB *iocb)
>  {
>      NvmeRequest *req = iocb->req;
> -    NvmeNamespace *ns = req->ns;
> -    BlockAcctStats *stats = blk_get_stats(ns->blkconf.blk);
>  
>      if (iocb->idx != iocb->nr) {
>          req->cqe.result = cpu_to_le32(iocb->idx);
> @@ -2811,14 +2809,6 @@ static void nvme_copy_done(NvmeCopyAIOCB *iocb)
>      qemu_iovec_destroy(&iocb->iov);
>      g_free(iocb->bounce);
>  
> -    if (iocb->ret < 0) {
> -        block_acct_failed(stats, &iocb->acct.read);
> -        block_acct_failed(stats, &iocb->acct.write);
> -    } else {
> -        block_acct_done(stats, &iocb->acct.read);
> -        block_acct_done(stats, &iocb->acct.write);
> -    }
> -
>      iocb->common.cb(iocb->common.opaque, iocb->ret);
>      qemu_aio_unref(iocb);
>  }
> @@ -2949,6 +2939,7 @@ static void nvme_copy_out_completed_cb(void *opaque, int ret)
>      NvmeCopyAIOCB *iocb = opaque;
>      NvmeRequest *req = iocb->req;
>      NvmeNamespace *dns = req->ns;
> +    BlockAcctStats *stats = blk_get_stats(dns->blkconf.blk);
>      uint32_t nlb;
>  
>      nvme_copy_source_range_parse(iocb->ranges, iocb->idx, iocb->format, NULL,
> @@ -2957,10 +2948,12 @@ static void nvme_copy_out_completed_cb(void *opaque, int ret)
>      if (ret < 0) {
>          iocb->ret = ret;
>          req->status = NVME_WRITE_FAULT;
> -        goto out;
> -    } else if (iocb->ret < 0) {
> +    }
> +    if (iocb->ret < 0) {
> +        block_acct_failed(stats, &iocb->acct.write);
>          goto out;
>      }
> +    block_acct_done(stats, &iocb->acct.write);
>  
>      if (dns->params.zoned) {
>          nvme_advance_zone_wp(dns, iocb->zone, nlb);
> @@ -2977,11 +2970,18 @@ static void nvme_copy_out_cb(void *opaque, int ret)
>      NvmeCopyAIOCB *iocb = opaque;
>      NvmeRequest *req = iocb->req;
>      NvmeNamespace *dns = req->ns;
> +    BlockAcctStats *stats = blk_get_stats(dns->blkconf.blk);
>      uint32_t nlb;
>      size_t mlen;
>      uint8_t *mbounce;
>  
> -    if (ret < 0 || iocb->ret < 0 || !dns->lbaf.ms) {
> +    if (ret < 0 || iocb->ret < 0) {
> +        block_acct_failed(stats, &iocb->acct.write);
> +        goto out;
> +    }
> +    block_acct_done(stats, &iocb->acct.write);
> +
> +    if (!dns->lbaf.ms) {
>          goto out;
>      }
>  
> @@ -2994,6 +2994,7 @@ static void nvme_copy_out_cb(void *opaque, int ret)
>      qemu_iovec_reset(&iocb->iov);
>      qemu_iovec_add(&iocb->iov, mbounce, mlen);
>  
> +    block_acct_start(stats, &iocb->acct.write, mlen, BLOCK_ACCT_WRITE);
>      iocb->aiocb = blk_aio_pwritev(dns->blkconf.blk, nvme_moff(dns, iocb->slba),
>                                    &iocb->iov, 0, nvme_copy_out_completed_cb,
>                                    iocb);
> @@ -3010,6 +3011,7 @@ static void nvme_copy_in_completed_cb(void *opaque, int ret)
>      NvmeRequest *req = iocb->req;
>      NvmeNamespace *sns = iocb->sns;
>      NvmeNamespace *dns = req->ns;
> +    BlockAcctStats *sstats = blk_get_stats(sns->blkconf.blk);
>      NvmeCopyCmd *copy = NULL;
>      uint8_t *mbounce = NULL;
>      uint32_t nlb;
> @@ -3022,10 +3024,12 @@ static void nvme_copy_in_completed_cb(void *opaque, int ret)
>      if (ret < 0) {
>          iocb->ret = ret;
>          req->status = NVME_UNRECOVERED_READ;
> -        goto out;
> -    } else if (iocb->ret < 0) {
> +    }
> +    if (iocb->ret < 0) {
> +        block_acct_failed(sstats, &iocb->acct.read);
>          goto out;
>      }
> +    block_acct_done(sstats, &iocb->acct.read);
>  
>      nvme_copy_source_range_parse(iocb->ranges, iocb->idx, iocb->format, &slba,
>                                   &nlb, NULL, &apptag, &appmask, &reftag);
> @@ -3100,7 +3104,7 @@ static void nvme_copy_in_completed_cb(void *opaque, int ret)
>      qemu_iovec_reset(&iocb->iov);
>      qemu_iovec_add(&iocb->iov, iocb->bounce, len);
>  
> -    block_acct_start(blk_get_stats(dns->blkconf.blk), &iocb->acct.write, 0,
> +    block_acct_start(blk_get_stats(dns->blkconf.blk), &iocb->acct.write, len,
>                       BLOCK_ACCT_WRITE);
>  
>      iocb->aiocb = blk_aio_pwritev(dns->blkconf.blk, nvme_l2b(dns, iocb->slba),
> @@ -3119,20 +3123,29 @@ static void nvme_copy_in_cb(void *opaque, int ret)
>  {
>      NvmeCopyAIOCB *iocb = opaque;
>      NvmeNamespace *sns = iocb->sns;
> +    BlockAcctStats *stats = blk_get_stats(sns->blkconf.blk);
>      uint64_t slba;
>      uint32_t nlb;
> +    size_t mlen;
> +
> +    if (ret < 0 || iocb->ret < 0) {
> +        block_acct_failed(stats, &iocb->acct.read);
> +        goto out;
> +    }
> +    block_acct_done(stats, &iocb->acct.read);
>  
> -    if (ret < 0 || iocb->ret < 0 || !sns->lbaf.ms) {
> +    if (!sns->lbaf.ms) {
>          goto out;
>      }
>  
>      nvme_copy_source_range_parse(iocb->ranges, iocb->idx, iocb->format, &slba,
>                                   &nlb, NULL, NULL, NULL, NULL);
>  
> +    mlen = nvme_m2b(sns, nlb);
>      qemu_iovec_reset(&iocb->iov);
> -    qemu_iovec_add(&iocb->iov, iocb->bounce + nvme_l2b(sns, nlb),
> -                   nvme_m2b(sns, nlb));
> +    qemu_iovec_add(&iocb->iov, iocb->bounce + nvme_l2b(sns, nlb), mlen);
>  
> +    block_acct_start(stats, &iocb->acct.read, mlen, BLOCK_ACCT_READ);
>      iocb->aiocb = blk_aio_preadv(sns->blkconf.blk, nvme_moff(sns, slba),
>                                   &iocb->iov, 0, nvme_copy_in_completed_cb,
>                                   iocb);
> @@ -3337,7 +3350,7 @@ static void nvme_do_copy(NvmeCopyAIOCB *iocb)
>      qemu_iovec_reset(&iocb->iov);
>      qemu_iovec_add(&iocb->iov, iocb->bounce, len);
>  
> -    block_acct_start(blk_get_stats(sns->blkconf.blk), &iocb->acct.read, 0,
> +    block_acct_start(blk_get_stats(sns->blkconf.blk), &iocb->acct.read, len,
>                       BLOCK_ACCT_READ);
>  
>      iocb->aiocb = blk_aio_preadv(sns->blkconf.blk, nvme_l2b(sns, slba),
> -- 
> 2.55.0
> 
>

-----BEGIN PGP SIGNATURE-----

iQEzBAABCgAdFiEEUigzqnXi3OaiR2bATeGvMW1PDekFAmpp9XYACgkQTeGvMW1P
DenurQf+PlOSgZU+RxY4yqEBkWpJJ4Mg8MomITg/ZVskMuMxFAONxNvwctF5xh7G
D5BYI4YXJ104iyWszylUTCMrz3LkI8NDMyxNZ+qYkcDcMuv4cMheBUXwhuR8pAuX
LefoUYRym9ZBEWa6QoFtKbLYzlgpJBf2Y1Yi+CQfiVb8ptsSJNj+0zM15RMyVn1F
ThqLsdcslPBwwgM0VsF2NTX5WxHSFIyTZeCzeYelTBh3SXFcz+kfkABZqqD//t72
JqcbqVRxD9MkbAssMvJjJxOSjbEa8z7cmqqXh5Pgpl7oxgECBdzfyYZlPtlQdmt5
1IwUUq0i1WGtK59Dmfe854ueMYy4uw==
=7Mml
-----END PGP SIGNATURE-----