Re: [Qemu-block] [RFC PATCH COLO v2 04/13] Add new block driver interfaces to control block replication

2015-03-26 Thread Wen Congyang 
On 03/26/2015 03:12 PM, Fam Zheng wrote:
 On Wed, 03/25 17:36, Wen Congyang wrote:
 Signed-off-by: Wen Congyang we...@cn.fujitsu.com
 Signed-off-by: zhanghailiang zhang.zhanghaili...@huawei.com
 Signed-off-by: Gonglei arei.gong...@huawei.com
 Cc: Luiz Capitulino lcapitul...@redhat.com
 Cc: Michael Roth mdr...@linux.vnet.ibm.com
 ---
  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)

Hmm, do you mean that don't use QERR_UNSUPPORTED here?

 
 +}
 +}
 +
 +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.

No special reason.

 
 +/* 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?

I will check it.

Thanks
Wen Congyang

 
 Fam
 
 +
 +##
  # @BlockdevSnapshotInternal
  #
  # @device: the name of the device to generate the snapshot from
 -- 
 2.1.0

 .

[Qemu-block] [RFC PATCH COLO v2 04/13] Add new block driver interfaces to control block replication

2015-03-25 Thread Wen Congyang 
Signed-off-by: Wen Congyang we...@cn.fujitsu.com
Signed-off-by: zhanghailiang zhang.zhanghaili...@huawei.com
Signed-off-by: Gonglei arei.gong...@huawei.com
Cc: Luiz Capitulino lcapitul...@redhat.com
Cc: Michael Roth mdr...@linux.vnet.ibm.com
---
 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);
+}
+}
+
+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);
+/* 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']}
+
+##
 # @BlockdevSnapshotInternal
 #
 # @device: the name of the device to generate the snapshot from
-- 
2.1.0

Re: [Qemu-block] [RFC PATCH COLO v2 04/13] Add new block driver interfaces to control block replication

2015-03-25 Thread Paolo Bonzini 


On 25/03/2015 10:36, Wen Congyang wrote:
 Signed-off-by: Wen Congyang we...@cn.fujitsu.com
 Signed-off-by: zhanghailiang zhang.zhanghaili...@huawei.com
 Signed-off-by: Gonglei arei.gong...@huawei.com
 Cc: Luiz Capitulino lcapitul...@redhat.com
 Cc: Michael Roth mdr...@linux.vnet.ibm.com
 ---
  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);
 +}
 +}
 +
 +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);
 +/* 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']}

Perhaps replace COLOMode with ReplicationMode or FaultToleranceMode?

 +##
  # @BlockdevSnapshotInternal
  #
  # @device: the name of the device to generate the snapshot from
 

Apart from this,

Reviewed-by: Paolo Bonzini pbonz...@redhat.com