From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:32795) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZyCit-00044h-Io for qemu-devel@nongnu.org; Mon, 16 Nov 2015 00:53:04 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZyCis-0002DC-Pn for qemu-devel@nongnu.org; Mon, 16 Nov 2015 00:53:03 -0500 Date: Mon, 16 Nov 2015 13:52:53 +0800 From: Fam Zheng Message-ID: <20151116055253.GA20672@ad.usersys.redhat.com> References: <1447407140-24055-1-git-send-email-famz@redhat.com> <5645E2FE.3090803@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5645E2FE.3090803@redhat.com> Subject: Re: [Qemu-devel] [PATCH] virtio-blk: Fix double completion for werror=stop List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: Kevin Wolf , lvivier@redhat.com, qemu-block@nongnu.org, pl@kamp.de, qemu-devel@nongnu.org, qemu-stable@nongnu.org, Stefan Hajnoczi , dgibson@redhat.com 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-stable@nongnu.org > > Signed-off-by: Fam Zheng > > --- > > 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