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

[Fam Zheng]

On Thu, 04/02 16:51, Paolo Bonzini wrote:
> 
> 
> On 02/04/2015 16:39, Fam Zheng wrote:
> > 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?
> 
> You would still have to add more code to differentiate reads and
> writes---I think.

Yeah, but the extra field will not be needed.

Fam

> 
> Paolo
> 
> > 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
> >>
> >>