From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54027) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YQ0lS-00033b-Df for qemu-devel@nongnu.org; Mon, 23 Feb 2015 16:42:12 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YQ0lM-0006tR-Ga for qemu-devel@nongnu.org; Mon, 23 Feb 2015 16:42:06 -0500 Received: from mx1.redhat.com ([209.132.183.28]:58729) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YQ0lM-0006tK-8k for qemu-devel@nongnu.org; Mon, 23 Feb 2015 16:42:00 -0500 Message-ID: <54EB9E8B.8070300@redhat.com> Date: Mon, 23 Feb 2015 16:41:31 -0500 From: Max Reitz MIME-Version: 1.0 References: <1423710438-14377-1-git-send-email-wency@cn.fujitsu.com> <1423710438-14377-8-git-send-email-wency@cn.fujitsu.com> In-Reply-To: <1423710438-14377-8-git-send-email-wency@cn.fujitsu.com> Content-Type: text/plain; charset=iso-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RFC PATCH 07/14] NBD client: implement block driver interfaces for block replication List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Wen Congyang , qemu devel , Kevin Wolf , Stefan Hajnoczi , Paolo Bonzini Cc: Lai Jiangshan , Jiang Yunhong , Dong Eddie , "Dr. David Alan Gilbert" , Gonglei , Yang Hongyang , zhanghailiang On 2015-02-11 at 22:07, Wen Congyang wrote: > Signed-off-by: Wen Congyang > Signed-off-by: zhanghailiang > Signed-off-by: Gonglei > --- > block/nbd.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 55 insertions(+) So by now to me it looks like you're using bdrv_start_replication() on the primary VM to start transferring data through the NBD connection to the secondary VM. I guess this is what you were discussing with Fam and John, whether there'd be a better way to do it by using functionality that is already (or is about to become) part of qemu, right? > diff --git a/block/nbd.c b/block/nbd.c > index 19b9200..1ff6ecf 100644 > --- a/block/nbd.c > +++ b/block/nbd.c > @@ -445,6 +445,58 @@ static void nbd_refresh_filename(BlockDriverState *bs) > bs->full_open_options = opts; > } > > +static int nbd_start_replication(BlockDriverState *bs, int mode) > +{ > + BDRVNBDState *s = bs->opaque; > + Error *local_err = NULL; > + int ret; > + > + /* > + * TODO: support COLO_SECONDARY_MODE if we allow secondary > + * QEMU becoming primary QEMU. > + */ > + if (mode != COLO_PRIMARY_MODE) { > + return -1; Once again, I'd like -ENOTSUP more (or -EINVAL or whatever you prefer). > + } > + > + if (s->connected) { > + return -1; > + } > + > + /* TODO: NBD client should be one child of quorum, how to verify it? */ Again, why would you care about that? Other than "It's how it's intended to be used". > + ret = nbd_connect_server(bs, &local_err); > + if (local_err) { > + error_free(local_err); > + } If you'd use the Error API you'd be able to propagate the error. Max > + > + return ret; > +} > + > +static int nbd_do_checkpoint(BlockDriverState *bs) > +{ > + BDRVNBDState *s = bs->opaque; > + > + if (!s->connected) { > + return -1; > + } > + > + return 0; > +} > + > +static int nbd_stop_replication(BlockDriverState *bs) > +{ > + BDRVNBDState *s = bs->opaque; > + > + if (!s->connected) { > + return -1; > + } > + > + nbd_client_session_close(&s->client); > + s->connected = false; > + > + return 0; > +} > + > static BlockDriver bdrv_nbd = { > .format_name = "nbd", > .protocol_name = "nbd", > @@ -514,6 +566,9 @@ static BlockDriver bdrv_nbd_colo = { > .bdrv_detach_aio_context = nbd_detach_aio_context, > .bdrv_attach_aio_context = nbd_attach_aio_context, > .bdrv_refresh_filename = nbd_refresh_filename, > + .bdrv_start_replication = nbd_start_replication, > + .bdrv_do_checkpoint = nbd_do_checkpoint, > + .bdrv_stop_replication = nbd_stop_replication, > > .has_variable_length = true, > };