diff mbox

[Migration,Bug?] Occasionally, the content of VM's memory is inconsistent between Source and Destination of migration

Message ID 551D0888.3000705@cn.fujitsu.com
State New
Headers show

Commit Message

Wen Congyang April 2, 2015, 9:14 a.m. UTC 
On 03/26/2015 06:29 PM, Juan Quintela wrote:
> Wen Congyang <wency@cn.fujitsu.com> wrote:
>> On 03/25/2015 05:50 PM, Juan Quintela wrote:
>>> zhanghailiang <zhang.zhanghailiang@huawei.com> wrote:
>>>> Hi all,
>>>>
>>>> We found that, sometimes, the content of VM's memory is
>>>> inconsistent between Source side and Destination side
>>>> when we check it just after finishing migration but before VM continue to Run.
>>>>
>>>> We use a patch like bellow to find this issue, you can find it from affix,
>>>> and Steps to reprduce:
>>>>
>>>> (1) Compile QEMU:
>>>>  ./configure --target-list=x86_64-softmmu  --extra-ldflags="-lssl" && make
>>>>
>>>> (2) Command and output:
>>>> SRC: # x86_64-softmmu/qemu-system-x86_64 -enable-kvm -cpu
>>>> qemu64,-kvmclock -netdev tap,id=hn0-device
>>>> virtio-net-pci,id=net-pci0,netdev=hn0 -boot c -drive
>>>> file=/mnt/sdb/pure_IMG/sles/sles11_sp3.img,if=none,id=drive-virtio-disk0,cache=unsafe
>>>> -device
>>>> virtio-blk-pci,bus=pci.0,addr=0x4,drive=drive-virtio-disk0,id=virtio-disk0
>>>> -vnc :7 -m 2048 -smp 2 -device piix3-usb-uhci -device usb-tablet
>>>> -monitor stdio
>>>
>>> Could you try to reproduce:
>>> - without vhost
>>> - without virtio-net
>>> - cache=unsafe is going to give you trouble, but trouble should only
>>>   happen after migration of pages have finished.
>>
>> If I use ide disk, it doesn't happen.
>> Even if I use virtio-net with vhost=on, it still doesn't happen. I guess
>> it is because I migrate the guest when it is booting. The virtio net
>> device is not used in this case.
> 
> Kevin, Stefan, Michael, any great idea?

The following patch can fix this problem(vhost=off):

From ebc024702dd3147e0cbdfd173c599103dc87796c Mon Sep 17 00:00:00 2001
From: Wen Congyang <wency@cn.fujitsu.com>
Date: Thu, 2 Apr 2015 16:28:17 +0800
Subject: [PATCH] fix qiov size

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
---
 hw/block/virtio-blk.c          | 15 +++++++++++++++
 include/hw/virtio/virtio-blk.h |  1 +
 2 files changed, 16 insertions(+)

Comments

Paolo Bonzini April 2, 2015, 1:17 p.m. UTC | #1 
On 02/04/2015 11:14, Wen Congyang wrote:
> From ebc024702dd3147e0cbdfd173c599103dc87796c Mon Sep 17 00:00:00 2001
> From: Wen Congyang <wency@cn.fujitsu.com>
> Date: Thu, 2 Apr 2015 16:28:17 +0800
> Subject: [PATCH] fix qiov size
> 
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> ---
>  hw/block/virtio-blk.c          | 15 +++++++++++++++
>  include/hw/virtio/virtio-blk.h |  1 +
>  2 files changed, 16 insertions(+)
> 
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index 000c38d..13967bc 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->size = 0;
>      req->next = NULL;
>      req->mr_next = NULL;
>      return req;
> @@ -97,12 +98,20 @@ static void virtio_blk_rw_complete(void *opaque, int ret)
>               * external iovec. It was allocated in submit_merged_requests
>               * to be able to merge requests. */
>              qemu_iovec_destroy(&req->qiov);
> +
> +            /* Restore qiov->size here */
> +            req->qiov.size = req->size;

At this point the iovec has been destroyed.

It's clearer, I think, to just use req->size in
virtio_blk_complete_request (as you suggest in the comment below!).  For
dataplane you'll need to do the same in complete_request_vring.

I'll send a patch to the mailing list, please test it---and then the
block device people can merge it.

Paolo

>          }
>  
>          if (ret) {
>              int p = virtio_ldl_p(VIRTIO_DEVICE(req->dev), &req->out.type);
>              bool is_read = !(p & VIRTIO_BLK_T_OUT);
>              if (virtio_blk_handle_rw_error(req, -ret, is_read)) {
> +                /*
> +                 * FIXME:
> +                 *  The memory may be dirtied on read failure, it will
> +                 *  break live migration.
> +                 */
>                  continue;
>              }
>          }
> @@ -323,6 +332,12 @@ static inline void submit_requests(BlockBackend *blk, MultiReqBuffer *mrb,
>          struct iovec *tmp_iov = qiov->iov;
>          int tmp_niov = qiov->niov;
>  
> +        /*
> +         * Save old qiov->size, which will used in
> +         * virtio_blk_complete_request()
> +         */
> +        mrb->reqs[start]->size = qiov->size;
> +
>          /* mrb->reqs[start]->qiov was initialized from external so we can't
>           * modifiy it here. We need to initialize it locally and then add the
>           * external iovecs. */
> diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
> index b3ffcd9..7d47310 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 size;
>      struct VirtIOBlockReq *next;
>      struct VirtIOBlockReq *mr_next;
>      BlockAcctCookie acct;
> -- 2.1.0 PS: I don't check if virtio-scsi, virtio-net... has the similar
> problem. If vhost=on, we can also reproduce this problem.
Wen Congyang April 3, 2015, 1:29 a.m. UTC | #2 
On 04/02/2015 09:17 PM, Paolo Bonzini wrote:
> 
> 
> On 02/04/2015 11:14, Wen Congyang wrote:
>> From ebc024702dd3147e0cbdfd173c599103dc87796c Mon Sep 17 00:00:00 2001
>> From: Wen Congyang <wency@cn.fujitsu.com>
>> Date: Thu, 2 Apr 2015 16:28:17 +0800
>> Subject: [PATCH] fix qiov size
>>
>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>> ---
>>  hw/block/virtio-blk.c          | 15 +++++++++++++++
>>  include/hw/virtio/virtio-blk.h |  1 +
>>  2 files changed, 16 insertions(+)
>>
>> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
>> index 000c38d..13967bc 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->size = 0;
>>      req->next = NULL;
>>      req->mr_next = NULL;
>>      return req;
>> @@ -97,12 +98,20 @@ static void virtio_blk_rw_complete(void *opaque, int ret)
>>               * external iovec. It was allocated in submit_merged_requests
>>               * to be able to merge requests. */
>>              qemu_iovec_destroy(&req->qiov);
>> +
>> +            /* Restore qiov->size here */
>> +            req->qiov.size = req->size;
> 
> At this point the iovec has been destroyed.
> 
> It's clearer, I think, to just use req->size in
> virtio_blk_complete_request (as you suggest in the comment below!).  For
> dataplane you'll need to do the same in complete_request_vring.

Dataplane will be disabled before migration. But it should be also
fixed if it has the same problem.

Thanks
Wen Congyang

> 
> I'll send a patch to the mailing list, please test it---and then the
> block device people can merge it.
> 
> Paolo
> 
>>          }
>>  
>>          if (ret) {
>>              int p = virtio_ldl_p(VIRTIO_DEVICE(req->dev), &req->out.type);
>>              bool is_read = !(p & VIRTIO_BLK_T_OUT);
>>              if (virtio_blk_handle_rw_error(req, -ret, is_read)) {
>> +                /*
>> +                 * FIXME:
>> +                 *  The memory may be dirtied on read failure, it will
>> +                 *  break live migration.
>> +                 */
>>                  continue;
>>              }
>>          }
>> @@ -323,6 +332,12 @@ static inline void submit_requests(BlockBackend *blk, MultiReqBuffer *mrb,
>>          struct iovec *tmp_iov = qiov->iov;
>>          int tmp_niov = qiov->niov;
>>  
>> +        /*
>> +         * Save old qiov->size, which will used in
>> +         * virtio_blk_complete_request()
>> +         */
>> +        mrb->reqs[start]->size = qiov->size;
>> +
>>          /* mrb->reqs[start]->qiov was initialized from external so we can't
>>           * modifiy it here. We need to initialize it locally and then add the
>>           * external iovecs. */
>> diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
>> index b3ffcd9..7d47310 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 size;
>>      struct VirtIOBlockReq *next;
>>      struct VirtIOBlockReq *mr_next;
>>      BlockAcctCookie acct;
>> -- 2.1.0 PS: I don't check if virtio-scsi, virtio-net... has the similar
>> problem. If vhost=on, we can also reproduce this problem.
> .
>
Paolo Bonzini April 3, 2015, 10:56 a.m. UTC | #3 
On 03/04/2015 03:29, Wen Congyang wrote:
>> > At this point the iovec has been destroyed.
>> > 
>> > It's clearer, I think, to just use req->size in
>> > virtio_blk_complete_request (as you suggest in the comment below!).  For
>> > dataplane you'll need to do the same in complete_request_vring.
> Dataplane will be disabled before migration. But it should be also
> fixed if it has the same problem.

Yes, it does.

Paolo
diff mbox

Patch

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 000c38d..13967bc 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->size = 0;
     req->next = NULL;
     req->mr_next = NULL;
     return req;
@@ -97,12 +98,20 @@  static void virtio_blk_rw_complete(void *opaque, int ret)
              * external iovec. It was allocated in submit_merged_requests
              * to be able to merge requests. */
             qemu_iovec_destroy(&req->qiov);
+
+            /* Restore qiov->size here */
+            req->qiov.size = req->size;
         }
 
         if (ret) {
             int p = virtio_ldl_p(VIRTIO_DEVICE(req->dev), &req->out.type);
             bool is_read = !(p & VIRTIO_BLK_T_OUT);
             if (virtio_blk_handle_rw_error(req, -ret, is_read)) {
+                /*
+                 * FIXME:
+                 *  The memory may be dirtied on read failure, it will
+                 *  break live migration.
+                 */
                 continue;
             }
         }
@@ -323,6 +332,12 @@  static inline void submit_requests(BlockBackend *blk, MultiReqBuffer *mrb,
         struct iovec *tmp_iov = qiov->iov;
         int tmp_niov = qiov->niov;
 
+        /*
+         * Save old qiov->size, which will used in
+         * virtio_blk_complete_request()
+         */
+        mrb->reqs[start]->size = qiov->size;
+
         /* mrb->reqs[start]->qiov was initialized from external so we can't
          * modifiy it here. We need to initialize it locally and then add the
          * external iovecs. */
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index b3ffcd9..7d47310 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 size;
     struct VirtIOBlockReq *next;
     struct VirtIOBlockReq *mr_next;
     BlockAcctCookie acct;