From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45663) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yb1yN-0004d9-VO for qemu-devel@nongnu.org; Thu, 26 Mar 2015 03:13:01 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Yb1yK-00055l-O5 for qemu-devel@nongnu.org; Thu, 26 Mar 2015 03:12:59 -0400 Date: Thu, 26 Mar 2015 15:12:46 +0800 From: Fam Zheng Message-ID: <20150326071246.GA1035@ad.nay.redhat.com> References: <1427276174-9130-1-git-send-email-wency@cn.fujitsu.com> <1427276174-9130-5-git-send-email-wency@cn.fujitsu.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1427276174-9130-5-git-send-email-wency@cn.fujitsu.com> Subject: Re: [Qemu-devel] [RFC PATCH COLO v2 04/13] Add new block driver interfaces to control block replication List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Wen Congyang Cc: Kevin Wolf , Yang Hongyang , Lai Jiangshan , qemu block , Jiang Yunhong , Dong Eddie , "Dr. David Alan Gilbert" , qemu devel , Gonglei , Stefan Hajnoczi , Luiz Capitulino , Paolo Bonzini , Max Reitz , Michael Roth , zhanghailiang On Wed, 03/25 17:36, Wen Congyang wrote: > Signed-off-by: Wen Congyang > Signed-off-by: zhanghailiang > Signed-off-by: Gonglei > Cc: Luiz Capitulino > Cc: Michael Roth > --- > block.c | 39 +++++++++++++++++++++++++++++++++++++++ > include/block/block.h | 4 ++++ > include/block/block_int.h | 11 +++++++++++ > qapi/block.json | 16 ++++++++++++++++ > 4 files changed, 70 insertions(+) > > diff --git a/block.c b/block.c > index 0fe97de..0ff5cf8 100644 > --- a/block.c > +++ b/block.c > @@ -6196,3 +6196,42 @@ BlockAcctStats *bdrv_get_stats(BlockDriverState *bs) > { > return &bs->stats; > } > + > +void bdrv_start_replication(BlockDriverState *bs, COLOMode mode, Error **errp) > +{ > + BlockDriver *drv = bs->drv; > + > + if (drv && drv->bdrv_start_replication) { > + drv->bdrv_start_replication(bs, mode, errp); > + } else if (bs->file) { > + bdrv_start_replication(bs->file, mode, errp); > + } else { > + error_set(errp, QERR_UNSUPPORTED); I think we should use error_setg in new code? (The same to following ones) > + } > +} > + > +void bdrv_do_checkpoint(BlockDriverState *bs, Error **errp) > +{ > + BlockDriver *drv = bs->drv; > + > + if (drv && drv->bdrv_do_checkpoint) { > + drv->bdrv_do_checkpoint(bs, errp); > + } else if (bs->file) { > + bdrv_do_checkpoint(bs->file, errp); > + } else { > + error_set(errp, QERR_UNSUPPORTED); > + } > +} > + > +void bdrv_stop_replication(BlockDriverState *bs, Error **errp) > +{ > + BlockDriver *drv = bs->drv; > + > + if (drv && drv->bdrv_stop_replication) { > + drv->bdrv_stop_replication(bs, errp); > + } else if (bs->file) { > + bdrv_stop_replication(bs->file, errp); > + } else { > + error_set(errp, QERR_UNSUPPORTED); > + } > +} > diff --git a/include/block/block.h b/include/block/block.h > index 4c57d63..68f3b1a 100644 > --- a/include/block/block.h > +++ b/include/block/block.h > @@ -569,4 +569,8 @@ void bdrv_flush_io_queue(BlockDriverState *bs); > > BlockAcctStats *bdrv_get_stats(BlockDriverState *bs); > > +void bdrv_start_replication(BlockDriverState *bs, COLOMode mode, Error **errp); > +void bdrv_do_checkpoint(BlockDriverState *bs, Error **errp); > +void bdrv_stop_replication(BlockDriverState *bs, Error **errp); > + > #endif > diff --git a/include/block/block_int.h b/include/block/block_int.h > index dccb092..08dd8ba 100644 > --- a/include/block/block_int.h > +++ b/include/block/block_int.h > @@ -290,6 +290,17 @@ struct BlockDriver { > */ > int (*bdrv_probe_geometry)(BlockDriverState *bs, HDGeometry *geo); > > + > + void (*bdrv_start_replication)(BlockDriverState *bs, COLOMode mode, > + Error **errp); Need some documentation, but I have a generic question: Why is a single interface with modes better than different functions for each mode (bdrv_start_replication_{primary,secondary}? Asking because the behavior is very different between them, and I don't see much sharing -- you implement primary operation in quorum, and secondary in qcow2+colo. > + /* Drop Disk buffer when doing checkpoint. */ > + void (*bdrv_do_checkpoint)(BlockDriverState *bs, Error **errp); > + /* > + * After failover, we should flush Disk buffer into secondary disk > + * and stop block replication. > + */ > + void (*bdrv_stop_replication)(BlockDriverState *bs, Error **errp); > + > QLIST_ENTRY(BlockDriver) list; > }; > > diff --git a/qapi/block.json b/qapi/block.json > index e313465..e640566 100644 > --- a/qapi/block.json > +++ b/qapi/block.json > @@ -40,6 +40,22 @@ > 'data': ['auto', 'none', 'lba', 'large', 'rechs']} > > ## > +# @COLOMode > +# > +# An enumeration of COLO mode. > +# > +# @unprotected: COLO is not started or after failover > +# > +# @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.4 > +## > +{ 'enum' : 'COLOMode', > + 'data' : ['unprotected', 'primary', 'secondary']} If split bdrv_start_replication, do we still need an enum? I can't find the usage in QMP interface, is it in some other series? Fam > + > +## > # @BlockdevSnapshotInternal > # > # @device: the name of the device to generate the snapshot from > -- > 2.1.0 >