diff mbox

[COLO-BLOCK,v7,16/17] quorum: implement block driver interfaces for block replication

Message ID 1435635285-5804-17-git-send-email-wency@cn.fujitsu.com
State New
Headers show

Commit Message

Wen Congyang June 30, 2015, 3:34 a.m. UTC 
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>
Cc: Alberto Garcia <berto@igalia.com>
---
 block/quorum.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 78 insertions(+)

Comments

Alberto Garcia July 3, 2015, 12:21 p.m. UTC | #1 
On Tue 30 Jun 2015 05:34:44 AM CEST, Wen Congyang wrote:

> +static void quorum_start_replication(BlockDriverState *bs, ReplicationMode mode,
> +                                     Error **errp)
> +{
> +    BDRVQuorumState *s = bs->opaque;
> +    int count = 0, i, index;
> +    Error *local_err = NULL;
> +
> +    /*
> +     * TODO: support REPLICATION_MODE_SECONDARY if we allow secondary
> +     * QEMU becoming primary QEMU.
> +     */
> +    if (mode != REPLICATION_MODE_PRIMARY) {
> +        error_setg(errp, "Invalid parameter 'mode'");
> +        return;
> +    }
> +
> +    if (s->read_pattern != QUORUM_READ_PATTERN_FIFO) {
> +        error_setg(errp, "Invalid parameter 'read pattern'");
> +        return;
> +    }

Those error messages seem a bit confusing. As I understand it, what's
wrong is the value of the parameter, not the parameter itself.

A more descriptive error message would be something along the lines of
"The only supported replication mode for this operation is 'primary'",
"The only supported read pattern for this operation is 'fifo'".

It doesn't need to be those exact words, but the idea is that the
message explains what's wrong.

> +    if (count == 0) {
> +        /* No child supports block replication */
> +        error_setg(errp, "this feature or command is not currently supported");
> +    } else if (count > 1) {
> +        for (i = 0; i < s->num_children; i++) {
> +            bdrv_stop_replication(s->bs[i], false, NULL);
> +        }
> +        error_setg(errp, "too many children support block replication");
> +    } else {
> +        s->replication_index = index;
> +    }
> +}

Not very important, but the previous error messages start with uppercase
and these are all lowercase.

> +        error_setg(errp, "Block replication is not started");

It sounds a bit odd to me, any native speaker can confirm?

Comments about the messages aside, the code looks good to me, hence

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto
diff mbox

Patch

diff --git a/block/quorum.c b/block/quorum.c
index 6774d4c..bafa774 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -88,6 +88,8 @@  typedef struct BDRVQuorumState {
                             */
 
     QuorumReadPattern read_pattern;
+
+    int replication_index; /* store which child supports block replication */
 } BDRVQuorumState;
 
 typedef struct QuorumAIOCB QuorumAIOCB;
@@ -1019,6 +1021,7 @@  static int quorum_open(BlockDriverState *bs, QDict *options, int flags,
     }
 
     g_free(opened);
+    s->replication_index = -1;
     goto exit;
 
 close_exit:
@@ -1178,6 +1181,77 @@  static void quorum_refresh_filename(BlockDriverState *bs)
     bs->full_open_options = opts;
 }
 
+static void quorum_start_replication(BlockDriverState *bs, ReplicationMode mode,
+                                     Error **errp)
+{
+    BDRVQuorumState *s = bs->opaque;
+    int count = 0, i, index;
+    Error *local_err = NULL;
+
+    /*
+     * TODO: support REPLICATION_MODE_SECONDARY if we allow secondary
+     * QEMU becoming primary QEMU.
+     */
+    if (mode != REPLICATION_MODE_PRIMARY) {
+        error_setg(errp, "Invalid parameter 'mode'");
+        return;
+    }
+
+    if (s->read_pattern != QUORUM_READ_PATTERN_FIFO) {
+        error_setg(errp, "Invalid parameter 'read pattern'");
+        return;
+    }
+
+    for (i = 0; i < s->num_children; i++) {
+        bdrv_start_replication(s->bs[i], mode, &local_err);
+        if (local_err) {
+            error_free(local_err);
+            local_err = NULL;
+        } else {
+            count++;
+            index = i;
+        }
+    }
+
+    if (count == 0) {
+        /* No child supports block replication */
+        error_setg(errp, "this feature or command is not currently supported");
+    } else if (count > 1) {
+        for (i = 0; i < s->num_children; i++) {
+            bdrv_stop_replication(s->bs[i], false, NULL);
+        }
+        error_setg(errp, "too many children support block replication");
+    } else {
+        s->replication_index = index;
+    }
+}
+
+static void quorum_do_checkpoint(BlockDriverState *bs, Error **errp)
+{
+    BDRVQuorumState *s = bs->opaque;
+
+    if (s->replication_index < 0) {
+        error_setg(errp, "Block replication is not started");
+        return;
+    }
+
+    bdrv_do_checkpoint(s->bs[s->replication_index], errp);
+}
+
+static void quorum_stop_replication(BlockDriverState *bs, bool failover,
+                                    Error **errp)
+{
+    BDRVQuorumState *s = bs->opaque;
+
+    if (s->replication_index < 0) {
+        error_setg(errp, "Block replication is not started");
+        return;
+    }
+
+    bdrv_stop_replication(s->bs[s->replication_index], failover, errp);
+    s->replication_index = -1;
+}
+
 static BlockDriver bdrv_quorum = {
     .format_name                        = "quorum",
     .protocol_name                      = "quorum",
@@ -1204,6 +1278,10 @@  static BlockDriver bdrv_quorum = {
 
     .is_filter                          = true,
     .bdrv_recurse_is_first_non_filter   = quorum_recurse_is_first_non_filter,
+
+    .bdrv_start_replication             = quorum_start_replication,
+    .bdrv_do_checkpoint                 = quorum_do_checkpoint,
+    .bdrv_stop_replication              = quorum_stop_replication,
 };
 
 static void bdrv_quorum_init(void)