Message ID | 551D0888.3000705@cn.fujitsu.com |
---|---|
State | New |
Headers | show |
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.
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. > . >
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 --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;