diff mbox

[RFC,05/14] quorom: implement block driver interfaces for block replication

Message ID 1423710438-14377-6-git-send-email-wency@cn.fujitsu.com
State New
Headers show

Commit Message

Wen Congyang Feb. 12, 2015, 3:07 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>
---
 block/quorum.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 69 insertions(+)

Comments

Max Reitz Feb. 23, 2015, 9:22 p.m. UTC | #1
On 2015-02-11 at 22:07, Wen Congyang wrote:
> 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>
> ---
>   block/quorum.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 69 insertions(+)
>
> diff --git a/block/quorum.c b/block/quorum.c
> index e6aff5f..c8479b4 100644
> --- a/block/quorum.c
> +++ b/block/quorum.c
> @@ -1070,6 +1070,71 @@ static void quorum_refresh_filename(BlockDriverState *bs)
>       bs->full_open_options = opts;
>   }
>   
> +static int quorum_stop_replication(BlockDriverState *bs);
> +static int quorum_start_replication(BlockDriverState *bs, int mode)
> +{
> +    BDRVQuorumState *s = bs->opaque;
> +    int ret = -1, i;

Again, I'd prefer it if you used -errno (or the Error API).

> +
> +    /*
> +     * TODO: support COLO_SECONDARY_MODE if we allow secondary
> +     * QEMU becoming primary QEMU.
> +     */
> +    if (mode != COLO_PRIMARY_MODE) {
> +        return -1;
> +    }
> +
> +    if (s->read_pattern != QUORUM_READ_PATTERN_FIRST) {
> +        return -1;
> +    }
> +
> +    /* NBD client should not be the first child */
> +    if (bdrv_start_replication(s->bs[0], mode) == 0) {

If you allow the NBD client to be the first child you can probably drop 
this block (and start from "i = 0" in the for loop).

> +        bdrv_stop_replication(s->bs[0]);
> +        return -1;
> +    }
> +
> +    for (i = 1; i < s->num_children; i++) {
> +        if (bdrv_start_replication(s->bs[i], mode) == 0) {
> +            ret++;
> +        }
> +    }
> +
> +    if (ret > 0) {
> +        quorum_stop_replication(bs);
> +    }

I think it would be easier to read if you had an additional "count" 
variable which is set to 0 before the for loop and then incremented 
(instead of ret). This would then be "if (count > 1)".

> +
> +    return ret ? -1 : 0;

And this would be "return count == 0 ? 0 : -ENOTSUP" or something.

But apart from that, what's so bad about having multiple children which 
support bdrv_start_replication()? I mean, other than "It's not what we 
intended".

Max

> +}
> +
> +static int quorum_do_checkpoint(BlockDriverState *bs)
> +{
> +    BDRVQuorumState *s = bs->opaque;
> +    int i;
> +
> +    for (i = 1; i < s->num_children; i++) {
> +        if (bdrv_do_checkpoint(s->bs[i]) == 0) {
> +            return 0;
> +        }
> +    }
> +
> +    return -1;
> +}
> +
> +static int quorum_stop_replication(BlockDriverState *bs)
> +{
> +    BDRVQuorumState *s = bs->opaque;
> +    int ret = -1, i;
> +
> +    for (i = 0; i < s->num_children; i++) {
> +        if (bdrv_stop_replication(s->bs[i]) == 0) {
> +            ret++;
> +        }
> +    }
> +
> +    return ret ? -1 : 0;
> +}
> +
>   static BlockDriver bdrv_quorum = {
>       .format_name                        = "quorum",
>       .protocol_name                      = "quorum",
> @@ -1093,6 +1158,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)
diff mbox

Patch

diff --git a/block/quorum.c b/block/quorum.c
index e6aff5f..c8479b4 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -1070,6 +1070,71 @@  static void quorum_refresh_filename(BlockDriverState *bs)
     bs->full_open_options = opts;
 }
 
+static int quorum_stop_replication(BlockDriverState *bs);
+static int quorum_start_replication(BlockDriverState *bs, int mode)
+{
+    BDRVQuorumState *s = bs->opaque;
+    int ret = -1, i;
+
+    /*
+     * TODO: support COLO_SECONDARY_MODE if we allow secondary
+     * QEMU becoming primary QEMU.
+     */
+    if (mode != COLO_PRIMARY_MODE) {
+        return -1;
+    }
+
+    if (s->read_pattern != QUORUM_READ_PATTERN_FIRST) {
+        return -1;
+    }
+
+    /* NBD client should not be the first child */
+    if (bdrv_start_replication(s->bs[0], mode) == 0) {
+        bdrv_stop_replication(s->bs[0]);
+        return -1;
+    }
+
+    for (i = 1; i < s->num_children; i++) {
+        if (bdrv_start_replication(s->bs[i], mode) == 0) {
+            ret++;
+        }
+    }
+
+    if (ret > 0) {
+        quorum_stop_replication(bs);
+    }
+
+    return ret ? -1 : 0;
+}
+
+static int quorum_do_checkpoint(BlockDriverState *bs)
+{
+    BDRVQuorumState *s = bs->opaque;
+    int i;
+
+    for (i = 1; i < s->num_children; i++) {
+        if (bdrv_do_checkpoint(s->bs[i]) == 0) {
+            return 0;
+        }
+    }
+
+    return -1;
+}
+
+static int quorum_stop_replication(BlockDriverState *bs)
+{
+    BDRVQuorumState *s = bs->opaque;
+    int ret = -1, i;
+
+    for (i = 0; i < s->num_children; i++) {
+        if (bdrv_stop_replication(s->bs[i]) == 0) {
+            ret++;
+        }
+    }
+
+    return ret ? -1 : 0;
+}
+
 static BlockDriver bdrv_quorum = {
     .format_name                        = "quorum",
     .protocol_name                      = "quorum",
@@ -1093,6 +1158,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)