From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52585) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aLnRu-00088C-6z for qemu-devel@nongnu.org; Wed, 20 Jan 2016 02:45:03 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aLnRp-0007Tu-VC for qemu-devel@nongnu.org; Wed, 20 Jan 2016 02:45:02 -0500 Received: from [59.151.112.132] (port=40157 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aLnRp-0007Tf-2U for qemu-devel@nongnu.org; Wed, 20 Jan 2016 02:44:57 -0500 Message-ID: <569F3B13.1020902@cn.fujitsu.com> Date: Wed, 20 Jan 2016 15:45:23 +0800 From: Changlong Xie MIME-Version: 1.0 References: <1452676712-24239-1-git-send-email-xiecl.fnst@cn.fujitsu.com> <1452676712-24239-8-git-send-email-xiecl.fnst@cn.fujitsu.com> <569ECF18.4070103@redhat.com> In-Reply-To: <569ECF18.4070103@redhat.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v14 7/8] Implement new driver for block replication List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , qemu devel , Fam Zheng , Max Reitz , Paolo Bonzini , Kevin Wolf , Stefan Hajnoczi Cc: Gonglei , zhanghailiang On 01/20/2016 08:04 AM, Eric Blake wrote: > On 01/13/2016 02:18 AM, Changlong Xie wrote: >> From: Wen Congyang >> >> Signed-off-by: Wen Congyang >> Signed-off-by: zhanghailiang >> Signed-off-by: Gonglei >> Signed-off-by: Changlong Xie >> --- >> block/Makefile.objs | 1 + >> block/replication-comm.c | 66 +++++ >> block/replication.c | 590 +++++++++++++++++++++++++++++++++++++++ >> include/block/replication-comm.h | 50 ++++ >> qapi/block-core.json | 13 + >> 5 files changed, 720 insertions(+) >> create mode 100644 block/replication-comm.c >> create mode 100644 block/replication.c >> create mode 100644 include/block/replication-comm.h >> > > Just a high-level overview, mainly on the user-visible interface and > things that caught my eye. Hi eric, thanks for your patience. > >> +++ b/block/replication-comm.c >> @@ -0,0 +1,66 @@ >> +/* >> + * Replication Block filter >> + * >> + * Copyright (c) 2015 HUAWEI TECHNOLOGIES CO., LTD. >> + * Copyright (c) 2015 Intel Corporation >> + * Copyright (c) 2015 FUJITSU LIMITED > > Do you want to claim 2016 in any of this? > > Will correct all Copyright issues in next version. >> + >> +enum { >> + BLOCK_REPLICATION_NONE, /* block replication is not started */ >> + BLOCK_REPLICATION_RUNNING, /* block replication is running */ >> + BLOCK_REPLICATION_FAILOVER, /* failover is running in background */ >> + BLOCK_REPLICATION_FAILOVER_FAILED, /* failover failed*/ > > Space before */ Will fix this space issue > >> + BLOCK_REPLICATION_DONE, /* block replication is done(after failover) */ >> +}; > > Should this be an enum type in a .json file? > Since this is just an internal enum that only used in block/replication.c, i think we don't need put it in *.json file. > >> + >> +static int replication_open(BlockDriverState *bs, QDict *options, >> + int flags, Error **errp) >> +{ > >> + >> +fail: >> + qemu_opts_del(opts); >> + /* propagate error */ >> + if (local_err) { >> + error_propagate(errp, local_err); >> + } > > It's safe to call error_propagate() unconditionally (you could drop the > 'if (local_err)'). You're right, i'll fix all relevant issue in my patchset. > > >> +static coroutine_fn int replication_co_discard(BlockDriverState *bs, >> + int64_t sector_num, >> + int nb_sectors) >> +{ >> + BDRVReplicationState *s = bs->opaque; >> + int ret; >> + >> + ret = replication_get_io_status(s); >> + if (ret < 0) { >> + return ret; >> + } >> + >> + if (ret == 1) { >> + /* It is secondary qemu and failover are running*/ > > Space before */ Will fix > >> +static void secondary_do_checkpoint(BDRVReplicationState *s, Error **errp) >> +{ >> + Error *local_err = NULL; >> + int ret; >> + >> + if (!s->secondary_disk->job) { >> + error_setg(errp, "Backup job is cancelled unexpectedly"); > > Maybe s/is/was/ > will fix >> +static void replication_start(BlockReplicationState *brs, ReplicationMode mode, >> + Error **errp) >> +{ >> + BlockDriverState *bs = brs->bs; >> + BDRVReplicationState *s = brs->bs->opaque; >> + int64_t active_length, hidden_length, disk_length; >> + AioContext *aio_context; >> + Error *local_err = NULL; >> + >> + if (s->replication_state != BLOCK_REPLICATION_NONE) { >> + error_setg(errp, "Block replication is running or done"); >> + return; >> + } >> + >> + if (s->mode != mode) { >> + error_setg(errp, "The parameter mode's value is invalid, needs %d," >> + " but receives %d", s->mode, mode); > > s/receives/got/ > will fix > >> +static void replication_do_checkpoint(BlockReplicationState *brs, Error **errp) >> +{ >> + BDRVReplicationState *s = brs->bs->opaque; >> + >> + if (s->replication_state != BLOCK_REPLICATION_RUNNING) { >> + error_setg(errp, "Block replication is not running"); >> + return; >> + } >> + >> + if (s->error) { >> + error_setg(errp, "I/O error occurs"); > > s/occurs/occurred/ > Will fix >> +++ b/qapi/block-core.json >> @@ -1975,6 +1975,19 @@ >> '*read-pattern': 'QuorumReadPattern' } } >> >> ## >> +# @ReplicationMode >> +# >> +# An enumeration of replication modes. >> +# >> +# @primary: Primary mode, the vm's state will be sent to secondary QEMU. >> +# >> +# @secondary: Secondary mode, receive the vm's state from primary QEMU. >> +# >> +# Since: 2.6 >> +## >> +{ 'enum' : 'ReplicationMode', 'data' : [ 'primary', 'secondary' ] } >> + > > Interface looks okay. Thanks -Xie >