Re: [Qemu-devel] [PATCH] virtio-blk: correctly dirty guest memory

[Fam Zheng]

On Thu, 04/02 15:37, Paolo Bonzini wrote:
> After qemu_iovec_destroy, the QEMUIOVector's size is zeroed and
> the zero size ultimately is used to compute virtqueue_push's len
> argument.  Therefore, reads from virtio-blk devices did not
> migrate their results correctly.  (Writes were okay).

Can't we move qemu_iovec_destroy to virtio_blk_free_request?

Fam

> 
> Save the size in submit_requests, and use it when the request is
> completed.
> 
> Based on a patch by Wen Congyang.
> 
> Signed-off-by: Wen Congyang <address@hidden>
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
>  hw/block/dataplane/virtio-blk.c |  2 +-
>  hw/block/virtio-blk.c           | 16 +++++++++++++++-
>  include/hw/virtio/virtio-blk.h  |  1 +
>  3 files changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
> index cd41478..b37ede3 100644
> --- a/hw/block/dataplane/virtio-blk.c
> +++ b/hw/block/dataplane/virtio-blk.c
> @@ -78,7 +78,7 @@ static void complete_request_vring(VirtIOBlockReq *req, 
> unsigned char status)
>      stb_p(&req->in->status, status);
>  
>      vring_push(s->vdev, &req->dev->dataplane->vring, &req->elem,
> -               req->qiov.size + sizeof(*req->in));
> +               req->read_size + sizeof(*req->in));
>  
>      /* Suppress notification to guest by BH and its scheduled
>       * flag because requests are completed as a batch after io
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index 000c38d..2f00dc4 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -33,6 +33,7 @@ VirtIOBlockReq *virtio_blk_alloc_request(VirtIOBlock *s)
>      VirtIOBlockReq *req = g_slice_new(VirtIOBlockReq);
>      req->dev = s;
>      req->qiov.size = 0;
> +    req->read_size = 0;
>      req->next = NULL;
>      req->mr_next = NULL;
>      return req;
> @@ -54,7 +55,7 @@ static void virtio_blk_complete_request(VirtIOBlockReq *req,
>      trace_virtio_blk_req_complete(req, status);
>  
>      stb_p(&req->in->status, status);
> -    virtqueue_push(s->vq, &req->elem, req->qiov.size + sizeof(*req->in));
> +    virtqueue_push(s->vq, &req->elem, req->read_size + sizeof(*req->in));
>      virtio_notify(vdev, s->vq);
>  }
>  
> @@ -102,6 +103,14 @@ static void virtio_blk_rw_complete(void *opaque, int ret)
>          if (ret) {
>              int p = virtio_ldl_p(VIRTIO_DEVICE(req->dev), &req->out.type);
>              bool is_read = !(p & VIRTIO_BLK_T_OUT);
> +            /* Note that memory may be dirtied on read failure.  If the
> +             * virtio request is not completed here, as is the case for
> +             * BLOCK_ERROR_ACTION_STOP, the memory may not be copied
> +             * correctly during live migration.  While this is ugly,
> +             * it is acceptable because the device is free to write to
> +             * the memory until the request is completed (which will
> +             * happen on the other side of the migration).
> +             */
>              if (virtio_blk_handle_rw_error(req, -ret, is_read)) {
>                  continue;
>              }
> @@ -348,9 +357,14 @@ static inline void submit_requests(BlockBackend *blk, 
> MultiReqBuffer *mrb,
>      }
>  
>      if (is_write) {
> +        mrb->reqs[start]->read_size = 0;
>          blk_aio_writev(blk, sector_num, qiov, nb_sectors,
>                         virtio_blk_rw_complete, mrb->reqs[start]);
>      } else {
> +        /* Save old qiov->size, which will be used in
> +         * virtio_blk_complete_request()
> +         */
> +        mrb->reqs[start]->read_size = qiov->size;
>          blk_aio_readv(blk, sector_num, qiov, nb_sectors,
>                        virtio_blk_rw_complete, mrb->reqs[start]);
>      }
> diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
> index b3ffcd9..d73ec06 100644
> --- a/include/hw/virtio/virtio-blk.h
> +++ b/include/hw/virtio/virtio-blk.h
> @@ -67,6 +67,7 @@ typedef struct VirtIOBlockReq {
>      struct virtio_blk_inhdr *in;
>      struct virtio_blk_outhdr out;
>      QEMUIOVector qiov;
> +    size_t read_size;
>      struct VirtIOBlockReq *next;
>      struct VirtIOBlockReq *mr_next;
>      BlockAcctCookie acct;
> -- 
> 2.3.4
> 
>