From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40429) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1buzoJ-0004Lb-WB for qemu-devel@nongnu.org; Fri, 14 Oct 2016 06:33:57 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1buzoI-0000U5-QB for qemu-devel@nongnu.org; Fri, 14 Oct 2016 06:33:55 -0400 Date: Fri, 14 Oct 2016 18:33:46 +0800 From: Fam Zheng Message-ID: <20161014103346.GD14830@lemon> References: <1476380062-18001-1-git-send-email-pbonzini@redhat.com> <1476380062-18001-7-git-send-email-pbonzini@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1476380062-18001-7-git-send-email-pbonzini@redhat.com> Subject: Re: [Qemu-devel] [PATCH 06/18] qed: Implement .bdrv_drain List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: qemu-devel@nongnu.org, qemu-block@nongnu.org, kwolf@redhat.com, stefanha@redhat.com On Thu, 10/13 19:34, Paolo Bonzini wrote: > From: Fam Zheng > > The "need_check_timer" is used to clear the "NEED_CHECK" flag in the > image header after a grace period once metadata update has finished. To > comply with the bdrv_drain semantics, we should make sure it remains > deleted once .bdrv_drain is called. > > The change to qed_need_check_timer_cb is needed because bdrv_qed_drain > is called after s->bs has been drained, and should not operate on it; > instead it should operate on the BdrvChild-ren exclusively. Doing so > is easy because QED does not have a bdrv_co_flush_to_os callback, hence > all that is needed to flush it is to ensure writes have reached the disk. > > Based on commit df9a681dc9a Much is changed so you should already take the authorship, otherwise I cannot rev-by it. :) > (which however included some unrelated > hunks, possibly due to a merge failure or an overlooked squash). > The patch was reverted because at the time bdrv_qed_drain could call > qed_plug_allocating_write_reqs while an allocating write was queued. > This however is not possible anymore after the previous patch; > .bdrv_drain is only called after all writes have completed at the > QED level, and its purpose is to trigger metadata writes in bs->file. > > Signed-off-by: Fam Zheng > Signed-off-by: Paolo Bonzini > --- > block/qed.c | 16 +++++++++++++++- > 1 file changed, 15 insertions(+), 1 deletion(-) > > diff --git a/block/qed.c b/block/qed.c > index 3ee879b..1a7ef0a 100644 > --- a/block/qed.c > +++ b/block/qed.c > @@ -336,7 +336,7 @@ static void qed_need_check_timer_cb(void *opaque) > qed_plug_allocating_write_reqs(s); > > /* Ensure writes are on disk before clearing flag */ > - bdrv_aio_flush(s->bs, qed_clear_need_check, s); > + bdrv_aio_flush(s->bs->file->bs, qed_clear_need_check, s); If this one has to change, what about the other bdrv_aio_flush(s->bs, ...) down in this call path: qed_need_check_timer_cb qed_clear_need_check qed_write_header qed_flush_after_clear_need_check ? > } > > static void qed_start_need_check_timer(BDRVQEDState *s) > @@ -378,6 +378,19 @@ static void bdrv_qed_attach_aio_context(BlockDriverState *bs, > } > } > > +static void bdrv_qed_drain(BlockDriverState *bs) > +{ > + BDRVQEDState *s = bs->opaque; > + > + /* 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)) { > + qed_cancel_need_check_timer(s); > + qed_need_check_timer_cb(s); > + } > +} > + > static int bdrv_qed_open(BlockDriverState *bs, QDict *options, int flags, > Error **errp) > { > @@ -1668,6 +1681,7 @@ static BlockDriver bdrv_qed = { > .bdrv_check = bdrv_qed_check, > .bdrv_detach_aio_context = bdrv_qed_detach_aio_context, > .bdrv_attach_aio_context = bdrv_qed_attach_aio_context, > + .bdrv_drain = bdrv_qed_drain, > }; > > static void bdrv_qed_init(void) > -- > 2.7.4 > >