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

[Paolo Bonzini]

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.

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