From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52518) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aVsJ2-0007tc-6n for qemu-devel@nongnu.org; Tue, 16 Feb 2016 21:57:33 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aVsJ1-0005OQ-Bk for qemu-devel@nongnu.org; Tue, 16 Feb 2016 21:57:32 -0500 Date: Wed, 17 Feb 2016 10:57:22 +0800 From: Fam Zheng Message-ID: <20160217025722.GC30207@ad.usersys.redhat.com> References: <1455638000-18051-1-git-send-email-pbonzini@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1455638000-18051-1-git-send-email-pbonzini@redhat.com> Subject: Re: [Qemu-devel] [PATCH] qed: fix bdrv_qed_drain List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: stefanha@redhat.com, qemu-devel@nongnu.org, qemu-block@nongnu.org, qemu-stable@nongnu.org On Tue, 02/16 16:53, Paolo Bonzini wrote: > The current implementation of bdrv_qed_drain can cause a double > completion of a request. > > The problem is that bdrv_qed_drain calls qed_plug_allocating_write_reqs > unconditionally, but this is not correct if an allocating write > is queued. In this case, qed_unplug_allocating_write_reqs will > restart the allocating write and possibly cause it to complete. > The aiocb however is still in use for the L2/L1 table writes, > and will then be completed again as soon as the table writes > are stable. > > The fix is to only call qed_plug_allocating_write_reqs and > bdrv_aio_flush (which is the same as the timer callback---the patch > makes this explicit) only if the timer was scheduled in the first > place. This fixes qemu-iotests test 011. > > Cc: qemu-stable@nongnu.org > Cc: qemu-block@nongnu.org > Cc: Stefan Hajnoczi > Signed-off-by: Paolo Bonzini > --- > block/qed.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/block/qed.c b/block/qed.c > index 404be1e..ebba220 100644 > --- a/block/qed.c > +++ b/block/qed.c > @@ -380,12 +380,13 @@ static void bdrv_qed_drain(BlockDriverState *bs) > { > BDRVQEDState *s = bs->opaque; > > - /* Cancel timer and start doing I/O that were meant to happen as if it > - * fired, that way we get bdrv_drain() taking care of the ongoing requests > - * correctly. */ > - qed_cancel_need_check_timer(s); > - qed_plug_allocating_write_reqs(s); > - bdrv_aio_flush(s->bs, qed_clear_need_check, s); > + /* Fire the timer immediately in order to start doing I/O as soon as the > + * header is flushed. > + */ > + if (s->need_check_timer && timer_pending(s->need_check_timer)) { We can assert(s->need_check_timer); > + qed_cancel_need_check_timer(s); > + qed_need_check_timer_cb(s); > + } What if an allocating write is queued (the else branch case)? Its completion will be in bdrv_drain and it could arm the need_check_timer which is wrong. We need to drain the allocating_write_reqs queue before checking the timer. Fam