From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50917) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bUZn0-0004Wx-8g for qemu-devel@nongnu.org; Tue, 02 Aug 2016 09:31:23 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bUZmw-00058a-6V for qemu-devel@nongnu.org; Tue, 02 Aug 2016 09:31:22 -0400 Received: from mx1.redhat.com ([209.132.183.28]:39888) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bUZmv-00058H-WB for qemu-devel@nongnu.org; Tue, 02 Aug 2016 09:31:18 -0400 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 88AC719FF1E for ; Tue, 2 Aug 2016 13:31:17 +0000 (UTC) References: <1470142856-742-1-git-send-email-famz@redhat.com> <1470142856-742-2-git-send-email-famz@redhat.com> From: Laszlo Ersek Message-ID: Date: Tue, 2 Aug 2016 15:31:15 +0200 MIME-Version: 1.0 In-Reply-To: <1470142856-742-2-git-send-email-famz@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 1/2] virtio-blk: Release s->rq queue at system_reset List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fam Zheng , qemu-devel@nongnu.org Cc: stefanha@redhat.com, pbonzini@redhat.com On 08/02/16 15:00, Fam Zheng wrote: > At system_reset, there is no point in retrying the queued request, > because the driver that issued the request won't be around any more. > > Analyzed-by: Laszlo Ersek > Reported-by: Laszlo Ersek > Signed-off-by: Fam Zheng > --- > hw/block/virtio-blk.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > index 475a822..12587d9 100644 > --- a/hw/block/virtio-blk.c > +++ b/hw/block/virtio-blk.c > @@ -654,6 +654,7 @@ static void virtio_blk_reset(VirtIODevice *vdev) > { > VirtIOBlock *s = VIRTIO_BLK(vdev); > AioContext *ctx; > + VirtIOBlockReq *req; > > /* > * This should cancel pending requests, but can't do nicely until there > @@ -662,6 +663,11 @@ static void virtio_blk_reset(VirtIODevice *vdev) > ctx = blk_get_aio_context(s->blk); > aio_context_acquire(ctx); > blk_drain(s->blk); > + while (s->rq) { > + req = s->rq; > + s->rq = req->next; > + virtio_blk_free_request(req); > + } > > if (s->dataplane) { > virtio_blk_data_plane_stop(s->dataplane); > I'd prefer if Paolo's remark (about blk_drain()'s ability to produce more failed requests, stashed in s->rq) were captured in either the commit message, or in a code comment. Something like: /* We drop queued requests after blk_drain() because blk_drain() * itself can produce them. */ What do you think? It's your call. I certainly lacked that bit of information. Reviewed-by: Laszlo Ersek Thanks! Laszlo