On Fri, 11/13 14:17, Paolo Bonzini wrote:
>
>
> On 13/11/2015 10:32, Fam Zheng wrote:
> > When a request R is absorbed by request M, it is appended to the
> > "mr_next" queue led by M, and is completed together with the completion
> > of M, in virtio_blk_rw_complete.
> >
> > With error policy equals stop, if M has an I/O error, now R also gets
> > prepended to the per device DMA restart queue, which will be retried
> > when VM resumes. It leads to a double completion (in symptoms of memory
> > corruption or use after free).
> >
> > Adding R to the queue is superfluous, only M needs to be in the queue.
> >
> > Fix this by marking request R as "merged" and skipping it in
> > virtio_blk_handle_rw_error.
> >
> > Cc: qemu-sta...@nongnu.org
> > Signed-off-by: Fam Zheng <f...@redhat.com>
> > ---
> > hw/block/virtio-blk.c | 9 +++++++--
> > include/hw/virtio/virtio-blk.h | 1 +
> > 2 files changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> > index e70fccf..4871f2a 100644
> > --- a/hw/block/virtio-blk.c
> > +++ b/hw/block/virtio-blk.c
> > @@ -36,6 +36,7 @@ VirtIOBlockReq *virtio_blk_alloc_request(VirtIOBlock *s)
> > req->in_len = 0;
> > req->next = NULL;
> > req->mr_next = NULL;
> > + req->merged = false;
> > return req;
> > }
> >
> > @@ -72,8 +73,11 @@ static int virtio_blk_handle_rw_error(VirtIOBlockReq
> > *req, int error,
> > VirtIOBlock *s = req->dev;
> >
> > if (action == BLOCK_ERROR_ACTION_STOP) {
> > - req->next = s->rq;
> > - s->rq = req;
> > + /* Merged requests don't need to be restarted. */
> > + if (!req->merged) {
> > + req->next = s->rq;
> > + s->rq = req;
> > + }
>
> I think it still needs to be queued, so that the request is migrated
> correctly. Should req->merged be checked in virtio_blk_dma_restart_bh
> instead?
Yes you're right.
Thanks,
Fam
