[Qemu-devel] [PATCH v9 0/3] qapi: child add/delete support

[Qemu-devel] [PATCH v9 0/3] qapi: child add/delete support

[Changlong Xie]

If quorum's child is broken, we can use mirror job to replace it.
But sometimes, the user only need to remove the broken child, and
add it later when the problem is fixed.

ChangLog:
v9:
1. Rebase to the newest codes
2. Remove redundant codes in quorum_add_child() and quorum_del_child()
3. Fix typos and in qmp-commands.hx 
v8:
1. Rebase to the newest codes
2. Address the comments from Eric Blake
v7:
1. Remove the qmp command x-blockdev-change's parameter operation according
   to Kevin's comments.
2. Remove the hmp command.
v6:
1. Use a single qmp command x-blockdev-change to replace x-blockdev-child-add
   and x-blockdev-child-delete
v5:
1. Address Eric Blake's comments
v4:
1. drop nbd driver's implementation. We can use human-monitor-command
   to do it.
2. Rename the command name.
v3:
1. Don't open BDS in bdrv_add_child(). Use the existing BDS which is
   created by the QMP command blockdev-add.
2. The driver NBD can support filename, path, host:port now.
v2:
1. Use bdrv_get_device_or_node_name() instead of new function
   bdrv_get_id_or_node_name()
2. Update the error message
3. Update the documents in block-core.json

Wen Congyang (3):
  Add new block driver interface to add/delete a BDS's child
  quorum: implement bdrv_add_child() and bdrv_del_child()
  qmp: add monitor command to add/remove a child

 block.c                   |  58 ++++++++++++++++++++--
 block/quorum.c            | 122 +++++++++++++++++++++++++++++++++++++++++++++-
 blockdev.c                |  54 ++++++++++++++++++++
 include/block/block.h     |   9 ++++
 include/block/block_int.h |   5 ++
 qapi/block-core.json      |  23 +++++++++
 qmp-commands.hx           |  47 ++++++++++++++++++
 7 files changed, 312 insertions(+), 6 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH v9 1/3] Add new block driver interface to add/delete a BDS's child

[Changlong Xie]

From: Wen Congyang <wency@cn.fujitsu.com>

In some cases, we want to take a quorum child offline, and take
another child online.

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
Signed-off-by: Gonglei <arei.gonglei@huawei.com>
Signed-off-by: Changlong Xie <xiecl.fnst@cn.fujitsu.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
 block.c                   | 50 +++++++++++++++++++++++++++++++++++++++++++++++
 include/block/block.h     |  5 +++++
 include/block/block_int.h |  5 +++++
 3 files changed, 60 insertions(+)

diff --git a/block.c b/block.c
index 411edbf..a347008 100644
--- a/block.c
+++ b/block.c
@@ -4320,3 +4320,53 @@ void bdrv_refresh_filename(BlockDriverState *bs)
         QDECREF(json);
     }
 }
+
+/*
+ * Hot add/remove a BDS's child. So the user can take a child offline when
+ * it is broken and take a new child online
+ */
+void bdrv_add_child(BlockDriverState *parent_bs, BlockDriverState *child_bs,
+                    Error **errp)
+{
+
+    if (!parent_bs->drv || !parent_bs->drv->bdrv_add_child) {
+        error_setg(errp, "The node %s doesn't support adding a child",
+                   bdrv_get_device_or_node_name(parent_bs));
+        return;
+    }
+
+    if (!QLIST_EMPTY(&child_bs->parents)) {
+        error_setg(errp, "The node %s already has a parent",
+                   child_bs->node_name);
+        return;
+    }
+
+    parent_bs->drv->bdrv_add_child(parent_bs, child_bs, errp);
+}
+
+void bdrv_del_child(BlockDriverState *parent_bs, BlockDriverState *child_bs,
+                    Error **errp)
+{
+    BdrvChild *child;
+
+    if (!parent_bs->drv || !parent_bs->drv->bdrv_del_child) {
+        error_setg(errp, "The node %s doesn't support removing a child",
+                   bdrv_get_device_or_node_name(parent_bs));
+        return;
+    }
+
+    QLIST_FOREACH(child, &parent_bs->children, next) {
+        if (child->bs == child_bs) {
+            break;
+        }
+    }
+
+    if (!child) {
+        error_setg(errp, "The node %s is not a child of %s",
+                   bdrv_get_device_or_node_name(child_bs),
+                   bdrv_get_device_or_node_name(parent_bs));
+        return;
+    }
+
+    parent_bs->drv->bdrv_del_child(parent_bs, child_bs, errp);
+}
diff --git a/include/block/block.h b/include/block/block.h
index db8e096..863a7c8 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -578,4 +578,9 @@ void bdrv_drained_begin(BlockDriverState *bs);
  */
 void bdrv_drained_end(BlockDriverState *bs);
 
+void bdrv_add_child(BlockDriverState *parent, BlockDriverState *child,
+                    Error **errp);
+void bdrv_del_child(BlockDriverState *parent, BlockDriverState *child,
+                    Error **errp);
+
 #endif
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 256609d..ebe8b1e 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -303,6 +303,11 @@ struct BlockDriver {
      */
     void (*bdrv_drain)(BlockDriverState *bs);
 
+    void (*bdrv_add_child)(BlockDriverState *parent, BlockDriverState *child,
+                           Error **errp);
+    void (*bdrv_del_child)(BlockDriverState *parent, BlockDriverState *child,
+                           Error **errp);
+
     QLIST_ENTRY(BlockDriver) list;
 };
 
-- 
1.9.3

[Qemu-devel] [PATCH v9 2/3] quorum: implement bdrv_add_child() and bdrv_del_child()

[Changlong Xie]

From: Wen Congyang <wency@cn.fujitsu.com>

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
Signed-off-by: Gonglei <arei.gonglei@huawei.com>
Signed-off-by: Changlong Xie <xiecl.fnst@cn.fujitsu.com>
---
 block.c               |   8 ++--
 block/quorum.c        | 122 +++++++++++++++++++++++++++++++++++++++++++++++++-
 include/block/block.h |   4 ++
 3 files changed, 128 insertions(+), 6 deletions(-)

diff --git a/block.c b/block.c
index a347008..b9e99da 100644
--- a/block.c
+++ b/block.c
@@ -1196,10 +1196,10 @@ static int bdrv_fill_options(QDict **options, const char *filename,
     return 0;
 }
 
-static BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
-                                    BlockDriverState *child_bs,
-                                    const char *child_name,
-                                    const BdrvChildRole *child_role)
+BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
+                             BlockDriverState *child_bs,
+                             const char *child_name,
+                             const BdrvChildRole *child_role)
 {
     BdrvChild *child = g_new(BdrvChild, 1);
     *child = (BdrvChild) {
diff --git a/block/quorum.c b/block/quorum.c
index 6793f12..e73418c 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -23,6 +23,7 @@
 #include "qapi/qmp/qstring.h"
 #include "qapi-event.h"
 #include "crypto/hash.h"
+#include "qemu/bitmap.h"
 
 #define HASH_LENGTH 32
 
@@ -80,6 +81,8 @@ typedef struct BDRVQuorumState {
     bool rewrite_corrupted;/* true if the driver must rewrite-on-read corrupted
                             * block if Quorum is reached.
                             */
+    unsigned long *index_bitmap;
+    int bsize;
 
     QuorumReadPattern read_pattern;
 } BDRVQuorumState;
@@ -875,9 +878,9 @@ static int quorum_open(BlockDriverState *bs, QDict *options, int flags,
         ret = -EINVAL;
         goto exit;
     }
-    if (s->num_children < 2) {
+    if (s->num_children < 1) {
         error_setg(&local_err,
-                   "Number of provided children must be greater than 1");
+                   "Number of provided children must be 1 or more");
         ret = -EINVAL;
         goto exit;
     }
@@ -926,6 +929,7 @@ static int quorum_open(BlockDriverState *bs, QDict *options, int flags,
     /* allocate the children array */
     s->children = g_new0(BdrvChild *, s->num_children);
     opened = g_new0(bool, s->num_children);
+    s->index_bitmap = bitmap_new(s->num_children);
 
     for (i = 0; i < s->num_children; i++) {
         char indexstr[32];
@@ -941,6 +945,8 @@ static int quorum_open(BlockDriverState *bs, QDict *options, int flags,
 
         opened[i] = true;
     }
+    bitmap_set(s->index_bitmap, 0, s->num_children);
+    s->bsize = s->num_children;
 
     g_free(opened);
     goto exit;
@@ -997,6 +1003,115 @@ static void quorum_attach_aio_context(BlockDriverState *bs,
     }
 }
 
+static int get_new_child_index(BDRVQuorumState *s)
+{
+    int index;
+
+    index = find_next_zero_bit(s->index_bitmap, s->bsize, 0);
+    if (index < s->bsize) {
+        return index;
+    }
+
+    if ((s->bsize % BITS_PER_LONG) == 0) {
+        s->index_bitmap = bitmap_zero_extend(s->index_bitmap, s->bsize,
+                                             s->bsize + 1);
+    }
+
+    return s->bsize++;
+}
+
+static void remove_child_index(BDRVQuorumState *s, int index)
+{
+    int last_index;
+    long new_len;
+
+    assert(index < s->bsize);
+
+    clear_bit(index, s->index_bitmap);
+    if (index < s->bsize - 1) {
+        /*
+         * The last bit is always set, and we don't clear
+         * the last bit.
+         */
+        return;
+    }
+
+    last_index = find_last_bit(s->index_bitmap, s->bsize);
+    s->bsize = last_index + 1;
+    if (BITS_TO_LONGS(last_index + 1) == BITS_TO_LONGS(s->bsize)) {
+        return;
+    }
+
+    new_len = BITS_TO_LONGS(last_index + 1) * sizeof(unsigned long);
+    s->index_bitmap = g_realloc(s->index_bitmap, new_len);
+}
+
+static void quorum_add_child(BlockDriverState *bs, BlockDriverState *child_bs,
+                             Error **errp)
+{
+    BDRVQuorumState *s = bs->opaque;
+    BdrvChild *child;
+    char indexstr[32];
+    int index, ret;
+
+    index = get_new_child_index(s);
+    ret = snprintf(indexstr, 32, "children.%d", index);
+    if (ret < 0 || ret >= 32) {
+        error_setg(errp, "cannot generate child name");
+        return;
+    }
+
+    bdrv_drain(bs);
+
+    assert(s->num_children <= INT_MAX / sizeof(BdrvChild *));
+    if (s->num_children == INT_MAX / sizeof(BdrvChild *)) {
+        error_setg(errp, "Too many children");
+        return;
+    }
+    s->children = g_renew(BdrvChild *, s->children, s->num_children + 1);
+
+    bdrv_ref(child_bs);
+    child = bdrv_attach_child(bs, child_bs, indexstr, &child_format);
+    s->children[s->num_children++] = child;
+    set_bit(index, s->index_bitmap);
+}
+
+static void quorum_del_child(BlockDriverState *bs, BlockDriverState *child_bs,
+                             Error **errp)
+{
+    BDRVQuorumState *s = bs->opaque;
+    BdrvChild *child;
+    int i, index;
+
+    for (i = 0; i < s->num_children; i++) {
+        if (s->children[i]->bs == child_bs) {
+            break;
+        }
+    }
+
+    /* we have checked it in bdrv_del_child() */
+    assert(i < s->num_children);
+    child = s->children[i];
+
+    if (s->num_children <= s->threshold) {
+        error_setg(errp,
+            "The number of children cannot be lower than the vote threshold %d",
+            s->threshold);
+        return;
+    }
+
+    /* child->name is "children.%d" */
+    index = atoi(child->name + 9);
+
+    bdrv_drain(bs);
+    /* We can safely remove this child now */
+    memmove(&s->children[i], &s->children[i + 1],
+            (s->num_children - i - 1) * sizeof(void *));
+    s->children = g_renew(BdrvChild *, s->children, --s->num_children);
+    remove_child_index(s, index);
+    bdrv_unref_child(bs, child);
+}
+
 static void quorum_refresh_filename(BlockDriverState *bs, QDict *options)
 {
     BDRVQuorumState *s = bs->opaque;
@@ -1052,6 +1167,9 @@ static BlockDriver bdrv_quorum = {
     .bdrv_detach_aio_context            = quorum_detach_aio_context,
     .bdrv_attach_aio_context            = quorum_attach_aio_context,
 
+    .bdrv_add_child                     = quorum_add_child,
+    .bdrv_del_child                     = quorum_del_child,
+
     .is_filter                          = true,
     .bdrv_recurse_is_first_non_filter   = quorum_recurse_is_first_non_filter,
 };
diff --git a/include/block/block.h b/include/block/block.h
index 863a7c8..6c7e54b 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -513,6 +513,10 @@ void bdrv_disable_copy_on_read(BlockDriverState *bs);
 void bdrv_ref(BlockDriverState *bs);
 void bdrv_unref(BlockDriverState *bs);
 void bdrv_unref_child(BlockDriverState *parent, BdrvChild *child);
+BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
+                             BlockDriverState *child_bs,
+                             const char *child_name,
+                             const BdrvChildRole *child_role);
 
 bool bdrv_op_is_blocked(BlockDriverState *bs, BlockOpType op, Error **errp);
 void bdrv_op_block(BlockDriverState *bs, BlockOpType op, Error *reason);
-- 
1.9.3

[Qemu-devel] [PATCH v9 3/3] qmp: add monitor command to add/remove a child

[Changlong Xie]

From: Wen Congyang <wency@cn.fujitsu.com>

The new QMP command name is x-blockdev-change. It's just for adding/removing
quorum's child now, and doesn't support all kinds of children, all kinds of
operations, nor all block drivers. So it is experimental now.

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
Signed-off-by: Gonglei <arei.gonglei@huawei.com>
Signed-off-by: Changlong Xie <xiecl.fnst@cn.fujitsu.com>
---
 blockdev.c           | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 qapi/block-core.json | 23 ++++++++++++++++++++++
 qmp-commands.hx      | 47 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 124 insertions(+)

diff --git a/blockdev.c b/blockdev.c
index 64dbfeb..4e62fdf 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3836,6 +3836,60 @@ out:
     aio_context_release(aio_context);
 }
 
+static BlockDriverState *bdrv_find_child(BlockDriverState *parent_bs,
+                                         const char *child_name)
+{
+    BdrvChild *child;
+
+    QLIST_FOREACH(child, &parent_bs->children, next) {
+        if (strcmp(child->name, child_name) == 0) {
+            return child->bs;
+        }
+    }
+
+    return NULL;
+}
+
+void qmp_x_blockdev_change(const char *parent, bool has_child,
+                           const char *child, bool has_node,
+                           const char *node, Error **errp)
+{
+    BlockDriverState *parent_bs, *child_bs = NULL, *new_bs = NULL;
+
+    parent_bs = bdrv_lookup_bs(parent, parent, errp);
+    if (!parent_bs) {
+        return;
+    }
+
+    if (has_child == has_node) {
+        if (has_child) {
+            error_setg(errp, "The paramter child and node is conflict");
+        } else {
+            error_setg(errp, "Either child or node should be specified");
+        }
+        return;
+    }
+
+    if (has_child) {
+        child_bs = bdrv_find_child(parent_bs, child);
+        if (!child_bs) {
+            error_setg(errp, "Node '%s' doesn't have child %s",
+                       parent, child);
+            return;
+        }
+        bdrv_del_child(parent_bs, child_bs, errp);
+    }
+
+    if (has_node) {
+        new_bs = bdrv_find_node(node);
+        if (!new_bs) {
+            error_setg(errp, "Node '%s' not found", node);
+            return;
+        }
+        bdrv_add_child(parent_bs, new_bs, errp);
+    }
+}
+
 BlockJobInfoList *qmp_query_block_jobs(Error **errp)
 {
     BlockJobInfoList *head = NULL, **p_next = &head;
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 1a5d9ce..fe63c6d 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2408,3 +2408,26 @@
 ##
 { 'command': 'block-set-write-threshold',
   'data': { 'node-name': 'str', 'write-threshold': 'uint64' } }
+
+##
+# @x-blockdev-change
+#
+# Dynamically reconfigure the block driver state graph. It can be used
+# to add, remove, insert or replace a block driver state. Currently only
+# the Quorum driver implements this feature to add or remove its child.
+# This is useful to fix a broken quorum child.
+#
+# @parent: the id or name of the node that will be changed.
+#
+# @child: #optional the name of the child that will be deleted.
+#
+# @node: #optional the name of the node will be added.
+#
+# Note: this command is experimental, and its API is not stable.
+#
+# Since: 2.6
+##
+{ 'command': 'x-blockdev-change',
+  'data' : { 'parent': 'str',
+             '*child': 'str',
+             '*node': 'str' } }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 7b235ee..efee0ca 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -4293,6 +4293,53 @@ Example:
 EQMP
 
     {
+        .name       = "x-blockdev-change",
+        .args_type  = "parent:B,child:B?,node:B?",
+        .mhandler.cmd_new = qmp_marshal_x_blockdev_change,
+    },
+
+SQMP
+x-blockdev-change
+-----------------
+
+Dynamically reconfigure the block driver state graph. It can be used to
+add, remove, insert, or replace a block driver state. Currently only
+the Quorum driver implements this feature to add and remove its child.
+This is useful to fix a broken quorum child.
+
+Arguments:
+- "parent": the id or node name of which node will be changed (json-string)
+- "child": the child name which will be deleted (json-string, optional)
+- "node": the new node-name which will be added (json-string, optional)
+
+Note: this command is experimental, and not a stable API. It doesn't
+support all kinds of operations, all kinds of children, nor all block
+drivers.
+
+Example:
+
+Add a new node to a quorum
+-> { "execute": "blockdev-add",
+    "arguments": { "options": { "driver": "raw",
+                                "node-name": "new_node",
+                                "id": "test_new_node",
+                                "file": { "driver": "file",
+                                          "filename": "test.raw" } } } }
+<- { "return": {} }
+-> { "execute": "x-blockdev-change",
+    "arguments": { "parent": "disk1",
+                   "node": "new_node" } }
+<- { "return": {} }
+
+Delete a quorum's node
+-> { "execute": "x-blockdev-change",
+    "arguments": { "parent": "disk1",
+                   "child": "children.1" } }
+<- { "return": {} }
+
+EQMP
+
+    {
         .name       = "query-named-block-nodes",
         .args_type  = "",
         .mhandler.cmd_new = qmp_marshal_query_named_block_nodes,
-- 
1.9.3

Re: [Qemu-devel] [PATCH v9 0/3] qapi: child add/delete support

[Wen Congyang]

Ping...

On 12/25/2015 05:22 PM, Changlong Xie wrote:
> If quorum's child is broken, we can use mirror job to replace it.
> But sometimes, the user only need to remove the broken child, and
> add it later when the problem is fixed.
> 
> ChangLog:
> v9:
> 1. Rebase to the newest codes
> 2. Remove redundant codes in quorum_add_child() and quorum_del_child()
> 3. Fix typos and in qmp-commands.hx 
> v8:
> 1. Rebase to the newest codes
> 2. Address the comments from Eric Blake
> v7:
> 1. Remove the qmp command x-blockdev-change's parameter operation according
>    to Kevin's comments.
> 2. Remove the hmp command.
> v6:
> 1. Use a single qmp command x-blockdev-change to replace x-blockdev-child-add
>    and x-blockdev-child-delete
> v5:
> 1. Address Eric Blake's comments
> v4:
> 1. drop nbd driver's implementation. We can use human-monitor-command
>    to do it.
> 2. Rename the command name.
> v3:
> 1. Don't open BDS in bdrv_add_child(). Use the existing BDS which is
>    created by the QMP command blockdev-add.
> 2. The driver NBD can support filename, path, host:port now.
> v2:
> 1. Use bdrv_get_device_or_node_name() instead of new function
>    bdrv_get_id_or_node_name()
> 2. Update the error message
> 3. Update the documents in block-core.json
> 
> Wen Congyang (3):
>   Add new block driver interface to add/delete a BDS's child
>   quorum: implement bdrv_add_child() and bdrv_del_child()
>   qmp: add monitor command to add/remove a child
> 
>  block.c                   |  58 ++++++++++++++++++++--
>  block/quorum.c            | 122 +++++++++++++++++++++++++++++++++++++++++++++-
>  blockdev.c                |  54 ++++++++++++++++++++
>  include/block/block.h     |   9 ++++
>  include/block/block_int.h |   5 ++
>  qapi/block-core.json      |  23 +++++++++
>  qmp-commands.hx           |  47 ++++++++++++++++++
>  7 files changed, 312 insertions(+), 6 deletions(-)
> 

Re: [Qemu-devel] [PATCH v9 2/3] quorum: implement bdrv_add_child() and bdrv_del_child()

[Alberto Garcia]

On Fri 25 Dec 2015 10:22:55 AM CET, Changlong Xie wrote:
> @@ -875,9 +878,9 @@ static int quorum_open(BlockDriverState *bs, QDict *options, int flags,
>          ret = -EINVAL;
>          goto exit;
>      }
> -    if (s->num_children < 2) {
> +    if (s->num_children < 1) {
>          error_setg(&local_err,
> -                   "Number of provided children must be greater than 1");
> +                   "Number of provided children must be 1 or more");
>          ret = -EINVAL;
>          goto exit;
>      }

I have a question: if you have a Quorum with just one member and you add
a new one, how do you know if it has the same data as the existing one?

In general, what do you do to make sure that the data in a new Quorum
child is consistent with that of the rest of the array?

Berto

Re: [Qemu-devel] [PATCH v9 2/3] quorum: implement bdrv_add_child() and bdrv_del_child()

[Wen Congyang]

On 01/20/2016 11:43 PM, Alberto Garcia wrote:
> On Fri 25 Dec 2015 10:22:55 AM CET, Changlong Xie wrote:
>> @@ -875,9 +878,9 @@ static int quorum_open(BlockDriverState *bs, QDict *options, int flags,
>>          ret = -EINVAL;
>>          goto exit;
>>      }
>> -    if (s->num_children < 2) {
>> +    if (s->num_children < 1) {
>>          error_setg(&local_err,
>> -                   "Number of provided children must be greater than 1");
>> +                   "Number of provided children must be 1 or more");
>>          ret = -EINVAL;
>>          goto exit;
>>      }
> 
> I have a question: if you have a Quorum with just one member and you add
> a new one, how do you know if it has the same data as the existing one?
> 
> In general, what do you do to make sure that the data in a new Quorum
> child is consistent with that of the rest of the array?

Quorum can have more than one child when it starts. But we don't do the
similar check. So I don't think we should do such check here.

Thanks
Wen Congyang

> 
> Berto
> 
> 
> .
> 

Re: [Qemu-devel] [PATCH v9 2/3] quorum: implement bdrv_add_child() and bdrv_del_child()

[Alberto Garcia]

On Thu 21 Jan 2016 02:54:10 AM CET, Wen Congyang <wency@cn.fujitsu.com> wrote:

>>> @@ -875,9 +878,9 @@ static int quorum_open(BlockDriverState *bs, QDict *options, int flags,
>>>          ret = -EINVAL;
>>>          goto exit;
>>>      }
>>> -    if (s->num_children < 2) {
>>> +    if (s->num_children < 1) {
>>>          error_setg(&local_err,
>>> -                   "Number of provided children must be greater than 1");
>>> +                   "Number of provided children must be 1 or more");
>>>          ret = -EINVAL;
>>>          goto exit;
>>>      }
>> 
>> I have a question: if you have a Quorum with just one member and you
>> add a new one, how do you know if it has the same data as the
>> existing one?
>> 
>> In general, what do you do to make sure that the data in a new Quorum
>> child is consistent with that of the rest of the array?
>
> Quorum can have more than one child when it starts. But we don't do
> the similar check. So I don't think we should do such check here.

Yes, but when you start a VM you can verify in advance that all members
of the Quorum have the same data. If you do that on a running VM how can
you know if the new disk is consistent with the others?

Berto

Re: [Qemu-devel] [PATCH v9 2/3] quorum: implement bdrv_add_child() and bdrv_del_child()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1655 bytes --]

On 01/21/2016 06:05 AM, Alberto Garcia wrote:
> On Thu 21 Jan 2016 02:54:10 AM CET, Wen Congyang <wency@cn.fujitsu.com> wrote:
> 
>>>> @@ -875,9 +878,9 @@ static int quorum_open(BlockDriverState *bs, QDict *options, int flags,
>>>>          ret = -EINVAL;
>>>>          goto exit;
>>>>      }
>>>> -    if (s->num_children < 2) {
>>>> +    if (s->num_children < 1) {
>>>>          error_setg(&local_err,
>>>> -                   "Number of provided children must be greater than 1");
>>>> +                   "Number of provided children must be 1 or more");
>>>>          ret = -EINVAL;
>>>>          goto exit;
>>>>      }
>>>
>>> I have a question: if you have a Quorum with just one member and you
>>> add a new one, how do you know if it has the same data as the
>>> existing one?
>>>
>>> In general, what do you do to make sure that the data in a new Quorum
>>> child is consistent with that of the rest of the array?
>>
>> Quorum can have more than one child when it starts. But we don't do
>> the similar check. So I don't think we should do such check here.
> 
> Yes, but when you start a VM you can verify in advance that all members
> of the Quorum have the same data. If you do that on a running VM how can
> you know if the new disk is consistent with the others?

User error if it is not.  Just the same as it is user error if you
request a shallow drive-mirror but the destination is not the same
contents as the backing file.  I don't think qemu has to protect us from
user error in this case.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v9 2/3] quorum: implement bdrv_add_child() and bdrv_del_child()

[Alberto Garcia]

On Thu 21 Jan 2016 05:58:42 PM CET, Eric Blake <eblake@redhat.com> wrote:
>>>> In general, what do you do to make sure that the data in a new Quorum
>>>> child is consistent with that of the rest of the array?
>>>
>>> Quorum can have more than one child when it starts. But we don't do
>>> the similar check. So I don't think we should do such check here.
>> 
>> Yes, but when you start a VM you can verify in advance that all
>> members of the Quorum have the same data. If you do that on a running
>> VM how can you know if the new disk is consistent with the others?
>
> User error if it is not.  Just the same as it is user error if you
> request a shallow drive-mirror but the destination is not the same
> contents as the backing file.  I don't think qemu has to protect us
> from user error in this case.

But the backing file is read-only so the user can guarantee that the
destination has the same data before the shallow mirror. How do you do
that in this case?

Berto

Re: [Qemu-devel] [PATCH v9 2/3] quorum: implement bdrv_add_child() and bdrv_del_child()

[Dr. David Alan Gilbert]

* Alberto Garcia (berto@igalia.com) wrote:
> On Thu 21 Jan 2016 05:58:42 PM CET, Eric Blake <eblake@redhat.com> wrote:
> >>>> In general, what do you do to make sure that the data in a new Quorum
> >>>> child is consistent with that of the rest of the array?
> >>>
> >>> Quorum can have more than one child when it starts. But we don't do
> >>> the similar check. So I don't think we should do such check here.
> >> 
> >> Yes, but when you start a VM you can verify in advance that all
> >> members of the Quorum have the same data. If you do that on a running
> >> VM how can you know if the new disk is consistent with the others?
> >
> > User error if it is not.  Just the same as it is user error if you
> > request a shallow drive-mirror but the destination is not the same
> > contents as the backing file.  I don't think qemu has to protect us
> > from user error in this case.
> 
> But the backing file is read-only so the user can guarantee that the
> destination has the same data before the shallow mirror. How do you do
> that in this case?

I think in the colo case they're relying on doing a block migrate
to synchronise the remote disk prior to switching into colo mode.

Dave

> Berto
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v9 2/3] quorum: implement bdrv_add_child() and bdrv_del_child()

[Wen Congyang]

On 01/23/2016 04:02 AM, Dr. David Alan Gilbert wrote:
> * Alberto Garcia (berto@igalia.com) wrote:
>> On Thu 21 Jan 2016 05:58:42 PM CET, Eric Blake <eblake@redhat.com> wrote:
>>>>>> In general, what do you do to make sure that the data in a new Quorum
>>>>>> child is consistent with that of the rest of the array?
>>>>>
>>>>> Quorum can have more than one child when it starts. But we don't do
>>>>> the similar check. So I don't think we should do such check here.
>>>>
>>>> Yes, but when you start a VM you can verify in advance that all
>>>> members of the Quorum have the same data. If you do that on a running
>>>> VM how can you know if the new disk is consistent with the others?
>>>
>>> User error if it is not.  Just the same as it is user error if you
>>> request a shallow drive-mirror but the destination is not the same
>>> contents as the backing file.  I don't think qemu has to protect us
>>> from user error in this case.
>>
>> But the backing file is read-only so the user can guarantee that the
>> destination has the same data before the shallow mirror. How do you do
>> that in this case?
> 
> I think in the colo case they're relying on doing a block migrate
> to synchronise the remote disk prior to switching into colo mode.

Yes, we can do a block migration to sync the disk. After the migration finished,
we stop block migration before starting colo.

Thanks
Wen Congyang

> 
> Dave
> 
>> Berto
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 
> 
> .
> 

Re: [Qemu-devel] [PATCH v9 2/3] quorum: implement bdrv_add_child() and bdrv_del_child()

[Alberto Garcia]

On Fri 22 Jan 2016 09:02:10 PM CET, "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:

>> >>>> In general, what do you do to make sure that the data in a new
>> >>>> Quorum child is consistent with that of the rest of the array?
>> >>>
>> >>> Quorum can have more than one child when it starts. But we don't
>> >>> do the similar check. So I don't think we should do such check
>> >>> here.
>> >> 
>> >> Yes, but when you start a VM you can verify in advance that all
>> >> members of the Quorum have the same data. If you do that on a
>> >> running VM how can you know if the new disk is consistent with the
>> >> others?
>> >
>> > User error if it is not.  Just the same as it is user error if you
>> > request a shallow drive-mirror but the destination is not the same
>> > contents as the backing file.  I don't think qemu has to protect us
>> > from user error in this case.
>> 
>> But the backing file is read-only so the user can guarantee that the
>> destination has the same data before the shallow mirror. How do you
>> do that in this case?
>
> I think in the colo case they're relying on doing a block migrate to
> synchronise the remote disk prior to switching into colo mode.

Yes but this is a general API that can be used independently from
COLO. I'd say if we want to allow that we should at least place a big
warning in the documentation.

Berto

Re: [Qemu-devel] [Qemu-block] [PATCH v9 3/3] qmp: add monitor command to add/remove a child

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 7366 bytes --]

On 25.12.2015 10:22, Changlong Xie wrote:
> From: Wen Congyang <wency@cn.fujitsu.com>
> 
> The new QMP command name is x-blockdev-change. It's just for adding/removing
> quorum's child now, and doesn't support all kinds of children, all kinds of
> operations, nor all block drivers. So it is experimental now.
> 
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> Signed-off-by: Changlong Xie <xiecl.fnst@cn.fujitsu.com>
> ---
>  blockdev.c           | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qapi/block-core.json | 23 ++++++++++++++++++++++
>  qmp-commands.hx      | 47 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 124 insertions(+)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 64dbfeb..4e62fdf 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3836,6 +3836,60 @@ out:
>      aio_context_release(aio_context);
>  }
>  
> +static BlockDriverState *bdrv_find_child(BlockDriverState *parent_bs,
> +                                         const char *child_name)
> +{
> +    BdrvChild *child;
> +
> +    QLIST_FOREACH(child, &parent_bs->children, next) {
> +        if (strcmp(child->name, child_name) == 0) {
> +            return child->bs;
> +        }
> +    }
> +
> +    return NULL;
> +}
> +
> +void qmp_x_blockdev_change(const char *parent, bool has_child,
> +                           const char *child, bool has_node,
> +                           const char *node, Error **errp)
> +{
> +    BlockDriverState *parent_bs, *child_bs = NULL, *new_bs = NULL;
> +
> +    parent_bs = bdrv_lookup_bs(parent, parent, errp);
> +    if (!parent_bs) {
> +        return;
> +    }
> +
> +    if (has_child == has_node) {
> +        if (has_child) {
> +            error_setg(errp, "The paramter child and node is conflict");

"The parameters child and node are in conflict"

Or, more naturally:

"child and node may not be specified at the same time"

> +        } else {
> +            error_setg(errp, "Either child or node should be specified");

s/should/must/

> +        }
> +        return;
> +    }
> +
> +    if (has_child) {
> +        child_bs = bdrv_find_child(parent_bs, child);
> +        if (!child_bs) {
> +            error_setg(errp, "Node '%s' doesn't have child %s",

s/doesn't/does not/

(This is a personal opinion, but a pretty strong one.)

Also, if you put quotes around the node name, maybe you should do the
same around the child name.

> +                       parent, child);
> +            return;
> +        }
> +        bdrv_del_child(parent_bs, child_bs, errp);
> +    }
> +
> +    if (has_node) {
> +        new_bs = bdrv_find_node(node);
> +        if (!new_bs) {
> +            error_setg(errp, "Node '%s' not found", node);
> +            return;
> +        }
> +        bdrv_add_child(parent_bs, new_bs, errp);
> +    }
> +}
> +
>  BlockJobInfoList *qmp_query_block_jobs(Error **errp)
>  {
>      BlockJobInfoList *head = NULL, **p_next = &head;
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 1a5d9ce..fe63c6d 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -2408,3 +2408,26 @@
>  ##
>  { 'command': 'block-set-write-threshold',
>    'data': { 'node-name': 'str', 'write-threshold': 'uint64' } }
> +
> +##
> +# @x-blockdev-change
> +#
> +# Dynamically reconfigure the block driver state graph. It can be used
> +# to add, remove, insert or replace a block driver state. Currently only

I'd prefer "graph node" over BDS in this line.

> +# the Quorum driver implements this feature to add or remove its child.
> +# This is useful to fix a broken quorum child.
> +#

I'd like a list here what this command does depending on the parameters
given. For instance:

If @(new-)node is specified, it will be inserted under @parent. @child
may not be specified in this case.

If both @parent and @child are specified but @(new-)node is not, @child
will be detached from @parent.

> +# @parent: the id or name of the node that will be changed.

I don't know. The parent will actually not be changed, it's an edge that
will be changed; and the parent is the parent node of that edge. Just put

@parent: the id or name of the parent node

here.

> +#
> +# @child: #optional the name of the child that will be deleted.

For now. But maybe that will change in the future. Generally, it is just
the child node of the edge that will be changed. So just putting

@child: #optional the name of a child under the given parent node

(Let's hope this is clear enough that people know that this is not a
node name.)

> +#
> +# @node: #optional the name of the node will be added.

Maybe this should be named new-node instead.

Also: "...of the node that will be added."

> +#
> +# Note: this command is experimental, and its API is not stable.
> +#
> +# Since: 2.6
> +##
> +{ 'command': 'x-blockdev-change',
> +  'data' : { 'parent': 'str',
> +             '*child': 'str',
> +             '*node': 'str' } }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 7b235ee..efee0ca 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -4293,6 +4293,53 @@ Example:
>  EQMP
>  
>      {
> +        .name       = "x-blockdev-change",
> +        .args_type  = "parent:B,child:B?,node:B?",
> +        .mhandler.cmd_new = qmp_marshal_x_blockdev_change,
> +    },
> +
> +SQMP
> +x-blockdev-change
> +-----------------
> +
> +Dynamically reconfigure the block driver state graph. It can be used to
> +add, remove, insert, or replace a block driver state. Currently only
> +the Quorum driver implements this feature to add and remove its child.
> +This is useful to fix a broken quorum child.
> +
> +Arguments:
> +- "parent": the id or node name of which node will be changed (json-string)
> +- "child": the child name which will be deleted (json-string, optional)
> +- "node": the new node-name which will be added (json-string, optional)
> +
> +Note: this command is experimental, and not a stable API. It doesn't
> +support all kinds of operations, all kinds of children, nor all block
> +drivers.

I think this second sentence should be present in qapi/block-core.json
as well.

Max

> +
> +Example:
> +
> +Add a new node to a quorum
> +-> { "execute": "blockdev-add",
> +    "arguments": { "options": { "driver": "raw",
> +                                "node-name": "new_node",
> +                                "id": "test_new_node",
> +                                "file": { "driver": "file",
> +                                          "filename": "test.raw" } } } }
> +<- { "return": {} }
> +-> { "execute": "x-blockdev-change",
> +    "arguments": { "parent": "disk1",
> +                   "node": "new_node" } }
> +<- { "return": {} }
> +
> +Delete a quorum's node
> +-> { "execute": "x-blockdev-change",
> +    "arguments": { "parent": "disk1",
> +                   "child": "children.1" } }
> +<- { "return": {} }
> +
> +EQMP
> +
> +    {
>          .name       = "query-named-block-nodes",
>          .args_type  = "",
>          .mhandler.cmd_new = qmp_marshal_query_named_block_nodes,
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH v9 3/3] qmp: add monitor command to add/remove a child

[Changlong Xie]

On 02/11/2016 02:02 AM, Max Reitz wrote:
> On 25.12.2015 10:22, Changlong Xie wrote:
>> From: Wen Congyang <wency@cn.fujitsu.com>
>>
>> The new QMP command name is x-blockdev-change. It's just for adding/removing
>> quorum's child now, and doesn't support all kinds of children, all kinds of
>> operations, nor all block drivers. So it is experimental now.
>>
>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>> Signed-off-by: Changlong Xie <xiecl.fnst@cn.fujitsu.com>
>> ---
>>   blockdev.c           | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   qapi/block-core.json | 23 ++++++++++++++++++++++
>>   qmp-commands.hx      | 47 +++++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 124 insertions(+)
>>
>> diff --git a/blockdev.c b/blockdev.c
>> index 64dbfeb..4e62fdf 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -3836,6 +3836,60 @@ out:
>>       aio_context_release(aio_context);
>>   }
>>
>> +static BlockDriverState *bdrv_find_child(BlockDriverState *parent_bs,
>> +                                         const char *child_name)
>> +{
>> +    BdrvChild *child;
>> +
>> +    QLIST_FOREACH(child, &parent_bs->children, next) {
>> +        if (strcmp(child->name, child_name) == 0) {
>> +            return child->bs;
>> +        }
>> +    }
>> +
>> +    return NULL;
>> +}
>> +
>> +void qmp_x_blockdev_change(const char *parent, bool has_child,
>> +                           const char *child, bool has_node,
>> +                           const char *node, Error **errp)
>> +{
>> +    BlockDriverState *parent_bs, *child_bs = NULL, *new_bs = NULL;
>> +
>> +    parent_bs = bdrv_lookup_bs(parent, parent, errp);
>> +    if (!parent_bs) {
>> +        return;
>> +    }
>> +
>> +    if (has_child == has_node) {
>> +        if (has_child) {
>> +            error_setg(errp, "The paramter child and node is conflict");
>
> "The parameters child and node are in conflict"
>
> Or, more naturally:
>
> "child and node may not be specified at the same time"
>

OK

>> +        } else {
>> +            error_setg(errp, "Either child or node should be specified");
>
> s/should/must/

Ok

>
>> +        }
>> +        return;
>> +    }
>> +
>> +    if (has_child) {
>> +        child_bs = bdrv_find_child(parent_bs, child);
>> +        if (!child_bs) {
>> +            error_setg(errp, "Node '%s' doesn't have child %s",
>
> s/doesn't/does not/
>
> (This is a personal opinion, but a pretty strong one.)
>
> Also, if you put quotes around the node name, maybe you should do the
> same around the child name.

OK

>
>> +                       parent, child);
>> +            return;
>> +        }
>> +        bdrv_del_child(parent_bs, child_bs, errp);
>> +    }
>> +
>> +    if (has_node) {
>> +        new_bs = bdrv_find_node(node);
>> +        if (!new_bs) {
>> +            error_setg(errp, "Node '%s' not found", node);
>> +            return;
>> +        }
>> +        bdrv_add_child(parent_bs, new_bs, errp);
>> +    }
>> +}
>> +
>>   BlockJobInfoList *qmp_query_block_jobs(Error **errp)
>>   {
>>       BlockJobInfoList *head = NULL, **p_next = &head;
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index 1a5d9ce..fe63c6d 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -2408,3 +2408,26 @@
>>   ##
>>   { 'command': 'block-set-write-threshold',
>>     'data': { 'node-name': 'str', 'write-threshold': 'uint64' } }
>> +
>> +##
>> +# @x-blockdev-change
>> +#
>> +# Dynamically reconfigure the block driver state graph. It can be used
>> +# to add, remove, insert or replace a block driver state. Currently only
>
> I'd prefer "graph node" over BDS in this line.

ok

>
>> +# the Quorum driver implements this feature to add or remove its child.
>> +# This is useful to fix a broken quorum child.
>> +#
>
> I'd like a list here what this command does depending on the parameters
> given. For instance:
>
> If @(new-)node is specified, it will be inserted under @parent. @child
> may not be specified in this case.
>
> If both @parent and @child are specified but @(new-)node is not, @child
> will be detached from @parent.
>

ok

>> +# @parent: the id or name of the node that will be changed.
>
> I don't know. The parent will actually not be changed, it's an edge that
> will be changed; and the parent is the parent node of that edge. Just put
>
> @parent: the id or name of the parent node
>
> here.
>

Yes, you are right here.

>> +#
>> +# @child: #optional the name of the child that will be deleted.
>
> For now. But maybe that will change in the future. Generally, it is just
> the child node of the edge that will be changed. So just putting
>
> @child: #optional the name of a child under the given parent node
>
> (Let's hope this is clear enough that people know that this is not a
> node name.)
>

ok

>> +#
>> +# @node: #optional the name of the node will be added.
>
> Maybe this should be named new-node instead.
>
> Also: "...of the node that will be added."

Yes, it's more clear.

>
>> +#
>> +# Note: this command is experimental, and its API is not stable.
>> +#
>> +# Since: 2.6
>> +##
>> +{ 'command': 'x-blockdev-change',
>> +  'data' : { 'parent': 'str',
>> +             '*child': 'str',
>> +             '*node': 'str' } }
>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>> index 7b235ee..efee0ca 100644
>> --- a/qmp-commands.hx
>> +++ b/qmp-commands.hx
>> @@ -4293,6 +4293,53 @@ Example:
>>   EQMP
>>
>>       {
>> +        .name       = "x-blockdev-change",
>> +        .args_type  = "parent:B,child:B?,node:B?",
>> +        .mhandler.cmd_new = qmp_marshal_x_blockdev_change,
>> +    },
>> +
>> +SQMP
>> +x-blockdev-change
>> +-----------------
>> +
>> +Dynamically reconfigure the block driver state graph. It can be used to
>> +add, remove, insert, or replace a block driver state. Currently only
>> +the Quorum driver implements this feature to add and remove its child.
>> +This is useful to fix a broken quorum child.
>> +
>> +Arguments:
>> +- "parent": the id or node name of which node will be changed (json-string)
>> +- "child": the child name which will be deleted (json-string, optional)
>> +- "node": the new node-name which will be added (json-string, optional)
>> +
>> +Note: this command is experimental, and not a stable API. It doesn't
>> +support all kinds of operations, all kinds of children, nor all block
>> +drivers.
>
> I think this second sentence should be present in qapi/block-core.json
> as well.

Will *highlight* it in qapi/block-core.json. Thanks for your review.

Thanks
	-Xie
>
> Max
>
>> +
>> +Example:
>> +
>> +Add a new node to a quorum
>> +-> { "execute": "blockdev-add",
>> +    "arguments": { "options": { "driver": "raw",
>> +                                "node-name": "new_node",
>> +                                "id": "test_new_node",
>> +                                "file": { "driver": "file",
>> +                                          "filename": "test.raw" } } } }
>> +<- { "return": {} }
>> +-> { "execute": "x-blockdev-change",
>> +    "arguments": { "parent": "disk1",
>> +                   "node": "new_node" } }
>> +<- { "return": {} }
>> +
>> +Delete a quorum's node
>> +-> { "execute": "x-blockdev-change",
>> +    "arguments": { "parent": "disk1",
>> +                   "child": "children.1" } }
>> +<- { "return": {} }
>> +
>> +EQMP
>> +
>> +    {
>>           .name       = "query-named-block-nodes",
>>           .args_type  = "",
>>           .mhandler.cmd_new = qmp_marshal_query_named_block_nodes,
>>
>
>

Re: [Qemu-devel] [PATCH v9 2/3] quorum: implement bdrv_add_child() and bdrv_del_child()

[Changlong Xie]

On 02/09/2016 01:06 AM, Alberto Garcia wrote:
> On Fri 22 Jan 2016 09:02:10 PM CET, "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
>
>>>>>>> In general, what do you do to make sure that the data in a new
>>>>>>> Quorum child is consistent with that of the rest of the array?
>>>>>>
>>>>>> Quorum can have more than one child when it starts. But we don't
>>>>>> do the similar check. So I don't think we should do such check
>>>>>> here.
>>>>>
>>>>> Yes, but when you start a VM you can verify in advance that all
>>>>> members of the Quorum have the same data. If you do that on a
>>>>> running VM how can you know if the new disk is consistent with the
>>>>> others?
>>>>
>>>> User error if it is not.  Just the same as it is user error if you
>>>> request a shallow drive-mirror but the destination is not the same
>>>> contents as the backing file.  I don't think qemu has to protect us
>>>> from user error in this case.
>>>
>>> But the backing file is read-only so the user can guarantee that the
>>> destination has the same data before the shallow mirror. How do you
>>> do that in this case?
>>
>> I think in the colo case they're relying on doing a block migrate to
>> synchronise the remote disk prior to switching into colo mode.
>
> Yes but this is a general API that can be used independently from
> COLO. I'd say if we want to allow that we should at least place a big
> warning in the documentation.
>

Ok, that's fair enough. Will add in next version.

Thanks
	-Xie
> Berto
>
>
> .
>