On 02/04/2015 19:54, Michael S. Tsirkin wrote:
> On Thu, Apr 02, 2015 at 07:50:44PM +0200, 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).
>>
>> Save the size in virtio_blk_handle_request, and use it when the request
>> is completed.
>>
>> Based on a patch by Wen Congyang.
>>
>> Signed-off-by: Wen Congyang <we...@cn.fujitsu.com>
>> Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>
>
> If you are touching this code anyway, maybe it makes
> sense to merge Rusty's virtio len patches?
> I didn't want them in 2.3 since they aren't ciritical,
> but we are changing these lines anyway, maybe make
> them correct?
My patch is a strict superset of Rusty's patch 2/2. In fact the first
version was very similar to his, but neither my v1 nor his patch covers
SCSI or flush or get-serial requests.
Rusty's patch 1/2 only adds an assertion, I think. It's not critical
for 2.3.
Paolo
>> ---
>> hw/block/dataplane/virtio-blk.c | 3 +--
>> hw/block/virtio-blk.c | 13 ++++++++++++-
>> include/hw/virtio/virtio-blk.h | 1 +
>> 3 files changed, 14 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/block/dataplane/virtio-blk.c
>> b/hw/block/dataplane/virtio-blk.c
>> index cd41478..3db139b 100644
>> --- a/hw/block/dataplane/virtio-blk.c
>> +++ b/hw/block/dataplane/virtio-blk.c
>> @@ -77,8 +77,7 @@ static void complete_request_vring(VirtIOBlockReq *req,
>> unsigned char status)
>> VirtIOBlockDataPlane *s = req->dev->dataplane;
>> stb_p(&req->in->status, status);
>>
>> - vring_push(s->vdev, &req->dev->dataplane->vring, &req->elem,
>> - req->qiov.size + sizeof(*req->in));
>> + vring_push(s->vdev, &req->dev->dataplane->vring, &req->elem,
>> req->in_len);
>>
>> /* 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..9546fd2 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->in_len = 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->in_len);
>> 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;
>> }
>> @@ -496,6 +505,8 @@ void virtio_blk_handle_request(VirtIOBlockReq *req,
>> MultiReqBuffer *mrb)
>> exit(1);
>> }
>>
>> + /* We always touch the last byte, so just see how big in_iov is. */
>> + req->in_len = iov_size(in_iov, in_num);
>> req->in = (void *)in_iov[in_num - 1].iov_base
>> + in_iov[in_num - 1].iov_len
>> - sizeof(struct virtio_blk_inhdr);
>> diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
>> index b3ffcd9..6bf5905 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 in_len;
>> struct VirtIOBlockReq *next;
>> struct VirtIOBlockReq *mr_next;
>> BlockAcctCookie acct;
>> --
>> 2.3.4
