From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:40573) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TxGHg-0000wQ-Ju for qemu-devel@nongnu.org; Mon, 21 Jan 2013 07:15:30 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TxGHe-0002Uq-6Z for qemu-devel@nongnu.org; Mon, 21 Jan 2013 07:15:28 -0500 Received: from mx1.redhat.com ([209.132.183.28]:23767) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TxGHd-0002Ui-Uh for qemu-devel@nongnu.org; Mon, 21 Jan 2013 07:15:26 -0500 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r0LCFNWc005560 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Mon, 21 Jan 2013 07:15:25 -0500 Message-ID: <50FD3153.2080607@redhat.com> Date: Mon, 21 Jan 2013 13:15:15 +0100 From: Kevin Wolf MIME-Version: 1.0 References: <1358357479-7912-1-git-send-email-pbonzini@redhat.com> <1358357479-7912-10-git-send-email-pbonzini@redhat.com> <50FD28D7.4080004@redhat.com> <50FD2FDC.5080906@redhat.com> In-Reply-To: <50FD2FDC.5080906@redhat.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 09/12] mirror: switch mirror_iteration to AIO List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: qemu-devel@nongnu.org, stefanha@redhat.com Am 21.01.2013 13:09, schrieb Paolo Bonzini: > Il 21/01/2013 12:39, Kevin Wolf ha scritto: >> Am 16.01.2013 18:31, schrieb Paolo Bonzini: >>> There is really no change in the behavior of the job here, since >>> there is still a maximum of one in-flight I/O operation between >>> the source and the target. However, this patch already introduces >>> the AIO callbacks (which are unmodified in the next patch) >>> and some of the logic to count in-flight operations and only >>> complete the job when there is none. >>> >>> Signed-off-by: Paolo Bonzini >>> --- >>> block/mirror.c | 155 ++++++++++++++++++++++++++++++++++++++++++-------------- >>> trace-events | 2 + >>> 2 files changed, 119 insertions(+), 38 deletions(-) >>> >>> diff --git a/block/mirror.c b/block/mirror.c >>> index ab41340..75c550a 100644 >>> --- a/block/mirror.c >>> +++ b/block/mirror.c >>> @@ -33,8 +33,19 @@ typedef struct MirrorBlockJob { >>> unsigned long *cow_bitmap; >>> HBitmapIter hbi; >>> uint8_t *buf; >>> + >>> + int in_flight; >>> + int ret; >>> } MirrorBlockJob; >>> >>> +typedef struct MirrorOp { >>> + MirrorBlockJob *s; >>> + QEMUIOVector qiov; >>> + struct iovec iov; >>> + int64_t sector_num; >>> + int nb_sectors; >>> +} MirrorOp; >>> + >>> static BlockErrorAction mirror_error_action(MirrorBlockJob *s, bool read, >>> int error) >>> { >>> @@ -48,15 +59,60 @@ static BlockErrorAction mirror_error_action(MirrorBlockJob *s, bool read, >>> } >>> } >>> >>> -static int coroutine_fn mirror_iteration(MirrorBlockJob *s, >>> - BlockErrorAction *p_action) >>> +static void mirror_iteration_done(MirrorOp *op) >>> +{ >>> + MirrorBlockJob *s = op->s; >>> + >>> + s->in_flight--; >>> + trace_mirror_iteration_done(s, op->sector_num, op->nb_sectors); >>> + g_slice_free(MirrorOp, op); >>> + qemu_coroutine_enter(s->common.co, NULL); >> >> This doesn't check if the job coroutine is actually in a state where >> it's valid to reenter. >> >> Technically it might even be okay because reentering during a sleep is >> allowed and as good as reentering during the new yield, and bdrv_flush() >> is only called if s->in_flight == 0. Most other calls _should_ be okay >> as well, but I'm not so sure about bdrv_drain_all(), especially once >> .bdrv_drain exists. > > bdrv_drain_all is also called only if s->in_flight == 0 too, but I see > your point. It is indeed quite subtle, but it's okay. Ah, yes, that's the part I missed. Looks correct indeed. >> As you can see, this is becoming very subtle, so I would prefer adding >> some explicit bool s->may_reenter or something like that. > > The right boolean to test is already there, it's job->busy. I can add a > new API block_job_yield/block_job_enter (where block_job_yield > resets/sets busy across the yield, and block_job_enter only enters if > !job->busy), but that would be a separate series IMO. Please put it on your todo list then. I think I can accept the current state if I know that it will be improved soon, even though I'm not very comfortable with it. Kevin