[Qemu-devel] [PATCH v2 0/3] colo: Add support for continious replication

[Qemu-devel] [PATCH v2 0/3] colo: Add support for continious replication

[Lukas Straub]

Hello Everyone,
These Patches add support for continious replication to colo.
Please review.

Regards,
Lukas Straub

v2:
 - fix email formating
 - fix checkpatch.pl warnings
 - fix patchew error
 - clearer commit messages

Lukas Straub (3):
  Replication: Ignore requests after failover
  net/filter.c: Add Options to insert filters anywhere in the filter
    list
  Update Documentation

 block/replication.c  |  38 ++++++++-
 docs/COLO-FT.txt     | 185 ++++++++++++++++++++++++++++++++-----------
 include/net/filter.h |   2 +
 net/filter.c         |  71 ++++++++++++++++-
 qemu-options.hx      |  10 +--
 5 files changed, 250 insertions(+), 56 deletions(-)

--
2.20.1

[Qemu-devel] [PATCH v2 1/3] Replication: Ignore requests after failover

[Lukas Straub]

After failover the Secondary side of replication shouldn't change state, because
it now functions as our primary disk.

In replication_start, replication_do_checkpoint, replication_stop, ignore
the request if current state is BLOCK_REPLICATION_DONE (sucessful failover) or
BLOCK_REPLICATION_FAILOVER (failover in progres i.e. currently merging active
and hidden images into the base image).

Signed-off-by: Lukas Straub <lukasstraub2@web.de>
---
 block/replication.c | 38 +++++++++++++++++++++++++++++++++++---
 1 file changed, 35 insertions(+), 3 deletions(-)

diff --git a/block/replication.c b/block/replication.c
index 3d4dedddfc..97cc65c0cf 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -454,6 +454,17 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode,
     aio_context_acquire(aio_context);
     s = bs->opaque;

+    if (s->stage == BLOCK_REPLICATION_DONE ||
+        s->stage == BLOCK_REPLICATION_FAILOVER) {
+        /*
+         * This case happens when a secondary is promoted to primary.
+         * Ignore the request because the secondary side of replication
+         * doesn't have to do anything anymore.
+         */
+        aio_context_release(aio_context);
+        return;
+    }
+
     if (s->stage != BLOCK_REPLICATION_NONE) {
         error_setg(errp, "Block replication is running or done");
         aio_context_release(aio_context);
@@ -529,8 +540,7 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode,
                    "Block device is in use by internal backup job");

         top_bs = bdrv_lookup_bs(s->top_id, s->top_id, NULL);
-        if (!top_bs || !bdrv_is_root_node(top_bs) ||
-            !check_top_bs(top_bs, bs)) {
+        if (!top_bs || !check_top_bs(top_bs, bs)) {
             error_setg(errp, "No top_bs or it is invalid");
             reopen_backing_file(bs, false, NULL);
             aio_context_release(aio_context);
@@ -577,6 +587,17 @@ static void replication_do_checkpoint(ReplicationState *rs, Error **errp)
     aio_context_acquire(aio_context);
     s = bs->opaque;

+    if (s->stage == BLOCK_REPLICATION_DONE ||
+        s->stage == BLOCK_REPLICATION_FAILOVER) {
+        /*
+         * This case happens when a secondary was promoted to primary.
+         * Ignore the request because the secondary side of replication
+         * doesn't have to do anything anymore.
+         */
+        aio_context_release(aio_context);
+        return;
+    }
+
     if (s->mode == REPLICATION_MODE_SECONDARY) {
         secondary_do_checkpoint(s, errp);
     }
@@ -593,7 +614,7 @@ static void replication_get_error(ReplicationState *rs, Error **errp)
     aio_context_acquire(aio_context);
     s = bs->opaque;

-    if (s->stage != BLOCK_REPLICATION_RUNNING) {
+    if (s->stage == BLOCK_REPLICATION_NONE) {
         error_setg(errp, "Block replication is not running");
         aio_context_release(aio_context);
         return;
@@ -635,6 +656,17 @@ static void replication_stop(ReplicationState *rs, bool failover, Error **errp)
     aio_context_acquire(aio_context);
     s = bs->opaque;

+    if (s->stage == BLOCK_REPLICATION_DONE ||
+        s->stage == BLOCK_REPLICATION_FAILOVER) {
+        /*
+         * This case happens when a secondary was promoted to primary.
+         * Ignore the request because the secondary side of replication
+         * doesn't have to do anything anymore.
+         */
+        aio_context_release(aio_context);
+        return;
+    }
+
     if (s->stage != BLOCK_REPLICATION_RUNNING) {
         error_setg(errp, "Block replication is not running");
         aio_context_release(aio_context);
--
2.20.1

[Qemu-devel] [PATCH v2 2/3] net/filter.c: Add Options to insert filters anywhere in the filter list

[Lukas Straub]

To switch the Secondary to Primary, we need to insert new filters
before the filter-rewriter.

Add the options insert= and position= to be able to insert filters
anywhere in the filter list.

position should be either "head", "tail" or the id of another filter.
insert should be either "before" or "after" to specify where to
insert the new filter relative to the one specified with position.

Signed-off-by: Lukas Straub <lukasstraub2@web.de>
---
 include/net/filter.h |  2 ++
 net/filter.c         | 71 +++++++++++++++++++++++++++++++++++++++++++-
 qemu-options.hx      | 10 +++----
 3 files changed, 77 insertions(+), 6 deletions(-)

diff --git a/include/net/filter.h b/include/net/filter.h
index 49da666ac0..355c178f75 100644
--- a/include/net/filter.h
+++ b/include/net/filter.h
@@ -62,6 +62,8 @@ struct NetFilterState {
     NetClientState *netdev;
     NetFilterDirection direction;
     bool on;
+    char *position;
+    bool insert_before;
     QTAILQ_ENTRY(NetFilterState) next;
 };

diff --git a/net/filter.c b/net/filter.c
index 28d1930db7..309fd778df 100644
--- a/net/filter.c
+++ b/net/filter.c
@@ -171,11 +171,47 @@ static void netfilter_set_status(Object *obj, const char *str, Error **errp)
     }
 }

+static char *netfilter_get_position(Object *obj, Error **errp)
+{
+    NetFilterState *nf = NETFILTER(obj);
+
+    return g_strdup(nf->position);
+}
+
+static void netfilter_set_position(Object *obj, const char *str, Error **errp)
+{
+    NetFilterState *nf = NETFILTER(obj);
+
+    nf->position = g_strdup(str);
+}
+
+static char *netfilter_get_insert(Object *obj, Error **errp)
+{
+    NetFilterState *nf = NETFILTER(obj);
+
+    return nf->insert_before ? g_strdup("before") : g_strdup("after");
+}
+
+static void netfilter_set_insert(Object *obj, const char *str, Error **errp)
+{
+    NetFilterState *nf = NETFILTER(obj);
+
+    if (strcmp(str, "before") && strcmp(str, "after")) {
+        error_setg(errp, "Invalid value for netfilter insert, "
+                         "should be 'head' or 'tail'");
+        return;
+    }
+
+    nf->insert_before = !strcmp(str, "before");
+}
+
 static void netfilter_init(Object *obj)
 {
     NetFilterState *nf = NETFILTER(obj);

     nf->on = true;
+    nf->insert_before = false;
+    nf->position = g_strdup("tail");

     object_property_add_str(obj, "netdev",
                             netfilter_get_netdev_id, netfilter_set_netdev_id,
@@ -187,11 +223,18 @@ static void netfilter_init(Object *obj)
     object_property_add_str(obj, "status",
                             netfilter_get_status, netfilter_set_status,
                             NULL);
+    object_property_add_str(obj, "position",
+                            netfilter_get_position, netfilter_set_position,
+                            NULL);
+    object_property_add_str(obj, "insert",
+                            netfilter_get_insert, netfilter_set_insert,
+                            NULL);
 }

 static void netfilter_complete(UserCreatable *uc, Error **errp)
 {
     NetFilterState *nf = NETFILTER(uc);
+    NetFilterState *position = NULL;
     NetClientState *ncs[MAX_QUEUE_NUM];
     NetFilterClass *nfc = NETFILTER_GET_CLASS(uc);
     int queues;
@@ -219,6 +262,20 @@ static void netfilter_complete(UserCreatable *uc, Error **errp)
         return;
     }

+    if (strcmp(nf->position, "head") && strcmp(nf->position, "tail")) {
+        /* Search for the position to insert before/after */
+        Object *container;
+        Object *obj;
+
+        container = object_get_objects_root();
+        obj = object_resolve_path_component(container, nf->position);
+        if (!obj) {
+            error_setg(errp, "filter '%s' not found", nf->position);
+            return;
+        }
+        position = NETFILTER(obj);
+    }
+
     nf->netdev = ncs[0];

     if (nfc->setup) {
@@ -228,7 +285,18 @@ static void netfilter_complete(UserCreatable *uc, Error **errp)
             return;
         }
     }
-    QTAILQ_INSERT_TAIL(&nf->netdev->filters, nf, next);
+
+    if (position) {
+        if (nf->insert_before) {
+            QTAILQ_INSERT_BEFORE(position, nf, next);
+        } else {
+            QTAILQ_INSERT_AFTER(&nf->netdev->filters, position, nf, next);
+        }
+    } else if (!strcmp(nf->position, "head")) {
+        QTAILQ_INSERT_HEAD(&nf->netdev->filters, nf, next);
+    } else if (!strcmp(nf->position, "tail")) {
+        QTAILQ_INSERT_TAIL(&nf->netdev->filters, nf, next);
+    }
 }

 static void netfilter_finalize(Object *obj)
@@ -245,6 +313,7 @@ static void netfilter_finalize(Object *obj)
         QTAILQ_REMOVE(&nf->netdev->filters, nf, next);
     }
     g_free(nf->netdev_id);
+    g_free(nf->position);
 }

 static void default_handle_event(NetFilterState *nf, int event, Error **errp)
diff --git a/qemu-options.hx b/qemu-options.hx
index 08749a3391..f0a47a0746 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -4368,7 +4368,7 @@ applications, they can do this through this parameter. Its format is
 a gnutls priority string as described at
 @url{https://gnutls.org/manual/html_node/Priority-Strings.html}.

-@item -object filter-buffer,id=@var{id},netdev=@var{netdevid},interval=@var{t}[,queue=@var{all|rx|tx}][,status=@var{on|off}]
+@item -object filter-buffer,id=@var{id},netdev=@var{netdevid},interval=@var{t}[,queue=@var{all|rx|tx}][,status=@var{on|off}][,position=@var{head|tail|id}][,insert=@var{after|before}]

 Interval @var{t} can't be 0, this filter batches the packet delivery: all
 packets arriving in a given interval on netdev @var{netdevid} are delayed
@@ -4387,11 +4387,11 @@ queue @var{all|rx|tx} is an option that can be applied to any netfilter.
 @option{tx}: the filter is attached to the transmit queue of the netdev,
              where it will receive packets sent by the netdev.

-@item -object filter-mirror,id=@var{id},netdev=@var{netdevid},outdev=@var{chardevid},queue=@var{all|rx|tx}[,vnet_hdr_support]
+@item -object filter-mirror,id=@var{id},netdev=@var{netdevid},outdev=@var{chardevid},queue=@var{all|rx|tx}[,vnet_hdr_support][,position=@var{head|tail|id}][,insert=@var{after|before}]

 filter-mirror on netdev @var{netdevid},mirror net packet to chardev@var{chardevid}, if it has the vnet_hdr_support flag, filter-mirror will mirror packet with vnet_hdr_len.

-@item -object filter-redirector,id=@var{id},netdev=@var{netdevid},indev=@var{chardevid},outdev=@var{chardevid},queue=@var{all|rx|tx}[,vnet_hdr_support]
+@item -object filter-redirector,id=@var{id},netdev=@var{netdevid},indev=@var{chardevid},outdev=@var{chardevid},queue=@var{all|rx|tx}[,vnet_hdr_support][,position=@var{head|tail|id}][,insert=@var{after|before}]

 filter-redirector on netdev @var{netdevid},redirect filter's net packet to chardev
 @var{chardevid},and redirect indev's packet to filter.if it has the vnet_hdr_support flag,
@@ -4400,7 +4400,7 @@ Create a filter-redirector we need to differ outdev id from indev id, id can not
 be the same. we can just use indev or outdev, but at least one of indev or outdev
 need to be specified.

-@item -object filter-rewriter,id=@var{id},netdev=@var{netdevid},queue=@var{all|rx|tx},[vnet_hdr_support]
+@item -object filter-rewriter,id=@var{id},netdev=@var{netdevid},queue=@var{all|rx|tx},[vnet_hdr_support][,position=@var{head|tail|id}][,insert=@var{after|before}]

 Filter-rewriter is a part of COLO project.It will rewrite tcp packet to
 secondary from primary to keep secondary tcp connection,and rewrite
@@ -4413,7 +4413,7 @@ colo secondary:
 -object filter-redirector,id=f2,netdev=hn0,queue=rx,outdev=red1
 -object filter-rewriter,id=rew0,netdev=hn0,queue=all

-@item -object filter-dump,id=@var{id},netdev=@var{dev}[,file=@var{filename}][,maxlen=@var{len}]
+@item -object filter-dump,id=@var{id},netdev=@var{dev}[,file=@var{filename}][,maxlen=@var{len}][,position=@var{head|tail|id}][,insert=@var{after|before}]

 Dump the network traffic on netdev @var{dev} to the file specified by
 @var{filename}. At most @var{len} bytes (64k by default) per packet are stored.
--
2.20.1

Re: [Qemu-devel] [PATCH v2 2/3] net/filter.c: Add Options to insert filters anywhere in the filter list

[Zhang, Chen]

> -----Original Message-----
> From: Lukas Straub [mailto:lukasstraub2@web.de]
> Sent: Friday, August 16, 2019 2:49 AM
> To: qemu-devel <qemu-devel@nongnu.org>
> Cc: Zhang, Chen <chen.zhang@intel.com>; Jason Wang
> <jasowang@redhat.com>; Wen Congyang <wencongyang2@huawei.com>; Xie
> Changlong <xiechanglong.d@gmail.com>
> Subject: [PATCH v2 2/3] net/filter.c: Add Options to insert filters anywhere in
> the filter list
> 
> To switch the Secondary to Primary, we need to insert new filters before the
> filter-rewriter.
> 
> Add the options insert= and position= to be able to insert filters anywhere in the
> filter list.
> 
> position should be either "head", "tail" or the id of another filter.
> insert should be either "before" or "after" to specify where to insert the new
> filter relative to the one specified with position.
> 

Hi Lukas,

It looks no need to add the "insert = xxx" for this operation.
For example:

We have 3 net-filters, the running order like that:

Fiter1   ---------->   Filter2 ------------> Filter3

If we want to add another filter between filter1 and filter2.
The "Position = head, insert = after" always seam with "position = filter2 id, insert = before". It seems the "insert" is a redundant args.
So I think it is enough with the "position", we can make the "insert" always equal "after" except the "head".


Thanks
Zhang Chen

> Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> ---
>  include/net/filter.h |  2 ++
>  net/filter.c         | 71 +++++++++++++++++++++++++++++++++++++++++++-
>  qemu-options.hx      | 10 +++----
>  3 files changed, 77 insertions(+), 6 deletions(-)
> 
> diff --git a/include/net/filter.h b/include/net/filter.h index
> 49da666ac0..355c178f75 100644
> --- a/include/net/filter.h
> +++ b/include/net/filter.h
> @@ -62,6 +62,8 @@ struct NetFilterState {
>      NetClientState *netdev;
>      NetFilterDirection direction;
>      bool on;
> +    char *position;
> +    bool insert_before;
>      QTAILQ_ENTRY(NetFilterState) next;
>  };
> 
> diff --git a/net/filter.c b/net/filter.c index 28d1930db7..309fd778df 100644
> --- a/net/filter.c
> +++ b/net/filter.c
> @@ -171,11 +171,47 @@ static void netfilter_set_status(Object *obj, const
> char *str, Error **errp)
>      }
>  }
> 
> +static char *netfilter_get_position(Object *obj, Error **errp) {
> +    NetFilterState *nf = NETFILTER(obj);
> +
> +    return g_strdup(nf->position);
> +}
> +
> +static void netfilter_set_position(Object *obj, const char *str, Error
> +**errp) {
> +    NetFilterState *nf = NETFILTER(obj);
> +
> +    nf->position = g_strdup(str);
> +}
> +
> +static char *netfilter_get_insert(Object *obj, Error **errp) {
> +    NetFilterState *nf = NETFILTER(obj);
> +
> +    return nf->insert_before ? g_strdup("before") : g_strdup("after");
> +}
> +
> +static void netfilter_set_insert(Object *obj, const char *str, Error
> +**errp) {
> +    NetFilterState *nf = NETFILTER(obj);
> +
> +    if (strcmp(str, "before") && strcmp(str, "after")) {
> +        error_setg(errp, "Invalid value for netfilter insert, "
> +                         "should be 'head' or 'tail'");
> +        return;
> +    }
> +
> +    nf->insert_before = !strcmp(str, "before"); }
> +
>  static void netfilter_init(Object *obj)  {
>      NetFilterState *nf = NETFILTER(obj);
> 
>      nf->on = true;
> +    nf->insert_before = false;
> +    nf->position = g_strdup("tail");
> 
>      object_property_add_str(obj, "netdev",
>                              netfilter_get_netdev_id, netfilter_set_netdev_id, @@ -187,11
> +223,18 @@ static void netfilter_init(Object *obj)
>      object_property_add_str(obj, "status",
>                              netfilter_get_status, netfilter_set_status,
>                              NULL);
> +    object_property_add_str(obj, "position",
> +                            netfilter_get_position, netfilter_set_position,
> +                            NULL);
> +    object_property_add_str(obj, "insert",
> +                            netfilter_get_insert, netfilter_set_insert,
> +                            NULL);
>  }
> 
>  static void netfilter_complete(UserCreatable *uc, Error **errp)  {
>      NetFilterState *nf = NETFILTER(uc);
> +    NetFilterState *position = NULL;
>      NetClientState *ncs[MAX_QUEUE_NUM];
>      NetFilterClass *nfc = NETFILTER_GET_CLASS(uc);
>      int queues;
> @@ -219,6 +262,20 @@ static void netfilter_complete(UserCreatable *uc,
> Error **errp)
>          return;
>      }
> 
> +    if (strcmp(nf->position, "head") && strcmp(nf->position, "tail")) {
> +        /* Search for the position to insert before/after */
> +        Object *container;
> +        Object *obj;
> +
> +        container = object_get_objects_root();
> +        obj = object_resolve_path_component(container, nf->position);
> +        if (!obj) {
> +            error_setg(errp, "filter '%s' not found", nf->position);
> +            return;
> +        }
> +        position = NETFILTER(obj);
> +    }
> +
>      nf->netdev = ncs[0];
> 
>      if (nfc->setup) {
> @@ -228,7 +285,18 @@ static void netfilter_complete(UserCreatable *uc,
> Error **errp)
>              return;
>          }
>      }
> -    QTAILQ_INSERT_TAIL(&nf->netdev->filters, nf, next);
> +
> +    if (position) {
> +        if (nf->insert_before) {
> +            QTAILQ_INSERT_BEFORE(position, nf, next);
> +        } else {
> +            QTAILQ_INSERT_AFTER(&nf->netdev->filters, position, nf, next);
> +        }
> +    } else if (!strcmp(nf->position, "head")) {
> +        QTAILQ_INSERT_HEAD(&nf->netdev->filters, nf, next);
> +    } else if (!strcmp(nf->position, "tail")) {
> +        QTAILQ_INSERT_TAIL(&nf->netdev->filters, nf, next);
> +    }
>  }
> 
>  static void netfilter_finalize(Object *obj) @@ -245,6 +313,7 @@ static void
> netfilter_finalize(Object *obj)
>          QTAILQ_REMOVE(&nf->netdev->filters, nf, next);
>      }
>      g_free(nf->netdev_id);
> +    g_free(nf->position);
>  }
> 
>  static void default_handle_event(NetFilterState *nf, int event, Error **errp)
> diff --git a/qemu-options.hx b/qemu-options.hx index 08749a3391..f0a47a0746
> 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -4368,7 +4368,7 @@ applications, they can do this through this parameter.
> Its format is  a gnutls priority string as described at
> @url{https://gnutls.org/manual/html_node/Priority-Strings.html}.
> 
> -@item -object filter-
> buffer,id=@var{id},netdev=@var{netdevid},interval=@var{t}[,queue=@var{all|r
> x|tx}][,status=@var{on|off}]
> +@item -object
> +filter-buffer,id=@var{id},netdev=@var{netdevid},interval=@var{t}[,queue
> +=@var{all|rx|tx}][,status=@var{on|off}][,position=@var{head|tail|id}][,
> +insert=@var{after|before}]
> 
>  Interval @var{t} can't be 0, this filter batches the packet delivery: all  packets
> arriving in a given interval on netdev @var{netdevid} are delayed @@ -4387,11
> +4387,11 @@ queue @var{all|rx|tx} is an option that can be applied to any
> netfilter.
>  @option{tx}: the filter is attached to the transmit queue of the netdev,
>               where it will receive packets sent by the netdev.
> 
> -@item -object filter-
> mirror,id=@var{id},netdev=@var{netdevid},outdev=@var{chardevid},queue=@v
> ar{all|rx|tx}[,vnet_hdr_support]
> +@item -object
> +filter-mirror,id=@var{id},netdev=@var{netdevid},outdev=@var{chardevid},
> +queue=@var{all|rx|tx}[,vnet_hdr_support][,position=@var{head|tail|id}][
> +,insert=@var{after|before}]
> 
>  filter-mirror on netdev @var{netdevid},mirror net packet to
> chardev@var{chardevid}, if it has the vnet_hdr_support flag, filter-mirror will
> mirror packet with vnet_hdr_len.
> 
> -@item -object filter-
> redirector,id=@var{id},netdev=@var{netdevid},indev=@var{chardevid},outdev=
> @var{chardevid},queue=@var{all|rx|tx}[,vnet_hdr_support]
> +@item -object
> +filter-redirector,id=@var{id},netdev=@var{netdevid},indev=@var{chardevi
> +d},outdev=@var{chardevid},queue=@var{all|rx|tx}[,vnet_hdr_support][,pos
> +ition=@var{head|tail|id}][,insert=@var{after|before}]
> 
>  filter-redirector on netdev @var{netdevid},redirect filter's net packet to
> chardev  @var{chardevid},and redirect indev's packet to filter.if it has the
> vnet_hdr_support flag, @@ -4400,7 +4400,7 @@ Create a filter-redirector we
> need to differ outdev id from indev id, id can not  be the same. we can just use
> indev or outdev, but at least one of indev or outdev  need to be specified.
> 
> -@item -object filter-
> rewriter,id=@var{id},netdev=@var{netdevid},queue=@var{all|rx|tx},[vnet_hdr_
> support]
> +@item -object
> +filter-rewriter,id=@var{id},netdev=@var{netdevid},queue=@var{all|rx|tx}
> +,[vnet_hdr_support][,position=@var{head|tail|id}][,insert=@var{after|be
> +fore}]
> 
>  Filter-rewriter is a part of COLO project.It will rewrite tcp packet to  secondary
> from primary to keep secondary tcp connection,and rewrite @@ -4413,7
> +4413,7 @@ colo secondary:
>  -object filter-redirector,id=f2,netdev=hn0,queue=rx,outdev=red1
>  -object filter-rewriter,id=rew0,netdev=hn0,queue=all
> 
> -@item -object filter-
> dump,id=@var{id},netdev=@var{dev}[,file=@var{filename}][,maxlen=@var{len}]
> +@item -object
> +filter-dump,id=@var{id},netdev=@var{dev}[,file=@var{filename}][,maxlen=
> +@var{len}][,position=@var{head|tail|id}][,insert=@var{after|before}]
> 
>  Dump the network traffic on netdev @var{dev} to the file specified by
> @var{filename}. At most @var{len} bytes (64k by default) per packet are stored.
> --
> 2.20.1

Re: [Qemu-devel] [PATCH v2 2/3] net/filter.c: Add Options to insert filters anywhere in the filter list

[Lukas Straub]

On Fri, 23 Aug 2019 03:24:02 +0000
"Zhang, Chen" <chen.zhang@intel.com> wrote:

> > -----Original Message-----
> > From: Lukas Straub [mailto:lukasstraub2@web.de]
> > Sent: Friday, August 16, 2019 2:49 AM
> > To: qemu-devel <qemu-devel@nongnu.org>
> > Cc: Zhang, Chen <chen.zhang@intel.com>; Jason Wang
> > <jasowang@redhat.com>; Wen Congyang <wencongyang2@huawei.com>; Xie
> > Changlong <xiechanglong.d@gmail.com>
> > Subject: [PATCH v2 2/3] net/filter.c: Add Options to insert filters anywhere in
> > the filter list
> >
> > To switch the Secondary to Primary, we need to insert new filters before the
> > filter-rewriter.
> >
> > Add the options insert= and position= to be able to insert filters anywhere in the
> > filter list.
> >
> > position should be either "head", "tail" or the id of another filter.
> > insert should be either "before" or "after" to specify where to insert the new
> > filter relative to the one specified with position.
> >
>
> Hi Lukas,
>
> It looks no need to add the "insert = xxx" for this operation.
> For example:
>
> We have 3 net-filters, the running order like that:
>
> Fiter1   ---------->   Filter2 ------------> Filter3
>
> If we want to add another filter between filter1 and filter2.
> The "Position = head, insert = after" always seam with "position = filter2 id, insert = before".

Hi Zhang,
The insert= parameter is ignored if position=head or tail. It always Inserts at the head (before Filter1) or the tail (after Filter3) of the List in these cases.

> It seems the "insert" is a redundant args.
> So I think it is enough with the "position", we can make the "insert" always equal "after" except the "head".

Yes, we still could do it without it, but its more convenient with the insert= parameter. For example our Case with inserting before the rewriter:

'filter-mirror', 'id': 'm0', 'props': { 'insert': 'before', 'position': 'rew0', 'netdev': 'hn0', 'queue': 'tx', 'outdev': 'mirror0' }
'filter-redirector', 'id': 'redire0', 'props': { 'insert': 'before', 'position': 'rew0', 'netdev': 'hn0', 'queue': 'rx', 'indev': 'compare_out' }
'filter-redirector', 'id': 'redire1', 'props': { 'insert': 'before', 'position': 'rew0', 'netdev': 'hn0', 'queue': 'rx', 'outdev': 'compare0' }

You see directly that here 3 Filters are inserted before the rewriter.

would have to become:

'filter-mirror', 'id': 'm0', 'props': { 'position': 'head', 'netdev': 'hn0', 'queue': 'tx', 'outdev': 'mirror0' }
'filter-redirector', 'id': 'redire0', 'props': { 'position': 'm0', 'netdev': 'hn0', 'queue': 'rx', 'indev': 'compare_out' }
'filter-redirector', 'id': 'redire1', 'props': { 'position': 'redire0', 'netdev': 'hn0', 'queue': 'rx', 'outdev': 'compare0' }

Which is less obvious.

Regards,
Lukas Straub

>
> Thanks
> Zhang Chen
>
> > Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> > ---
> >  include/net/filter.h |  2 ++
> >  net/filter.c         | 71 +++++++++++++++++++++++++++++++++++++++++++-
> >  qemu-options.hx      | 10 +++----
> >  3 files changed, 77 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/net/filter.h b/include/net/filter.h index
> > 49da666ac0..355c178f75 100644
> > --- a/include/net/filter.h
> > +++ b/include/net/filter.h
> > @@ -62,6 +62,8 @@ struct NetFilterState {
> >      NetClientState *netdev;
> >      NetFilterDirection direction;
> >      bool on;
> > +    char *position;
> > +    bool insert_before;
> >      QTAILQ_ENTRY(NetFilterState) next;
> >  };
> >
> > diff --git a/net/filter.c b/net/filter.c index 28d1930db7..309fd778df 100644
> > --- a/net/filter.c
> > +++ b/net/filter.c
> > @@ -171,11 +171,47 @@ static void netfilter_set_status(Object *obj, const
> > char *str, Error **errp)
> >      }
> >  }
> >
> > +static char *netfilter_get_position(Object *obj, Error **errp) {
> > +    NetFilterState *nf = NETFILTER(obj);
> > +
> > +    return g_strdup(nf->position);
> > +}
> > +
> > +static void netfilter_set_position(Object *obj, const char *str, Error
> > +**errp) {
> > +    NetFilterState *nf = NETFILTER(obj);
> > +
> > +    nf->position = g_strdup(str);
> > +}
> > +
> > +static char *netfilter_get_insert(Object *obj, Error **errp) {
> > +    NetFilterState *nf = NETFILTER(obj);
> > +
> > +    return nf->insert_before ? g_strdup("before") : g_strdup("after");
> > +}
> > +
> > +static void netfilter_set_insert(Object *obj, const char *str, Error
> > +**errp) {
> > +    NetFilterState *nf = NETFILTER(obj);
> > +
> > +    if (strcmp(str, "before") && strcmp(str, "after")) {
> > +        error_setg(errp, "Invalid value for netfilter insert, "
> > +                         "should be 'head' or 'tail'");
> > +        return;
> > +    }
> > +
> > +    nf->insert_before = !strcmp(str, "before"); }
> > +
> >  static void netfilter_init(Object *obj)  {
> >      NetFilterState *nf = NETFILTER(obj);
> >
> >      nf->on = true;
> > +    nf->insert_before = false;
> > +    nf->position = g_strdup("tail");
> >
> >      object_property_add_str(obj, "netdev",
> >                              netfilter_get_netdev_id, netfilter_set_netdev_id, @@ -187,11
> > +223,18 @@ static void netfilter_init(Object *obj)
> >      object_property_add_str(obj, "status",
> >                              netfilter_get_status, netfilter_set_status,
> >                              NULL);
> > +    object_property_add_str(obj, "position",
> > +                            netfilter_get_position, netfilter_set_position,
> > +                            NULL);
> > +    object_property_add_str(obj, "insert",
> > +                            netfilter_get_insert, netfilter_set_insert,
> > +                            NULL);
> >  }
> >
> >  static void netfilter_complete(UserCreatable *uc, Error **errp)  {
> >      NetFilterState *nf = NETFILTER(uc);
> > +    NetFilterState *position = NULL;
> >      NetClientState *ncs[MAX_QUEUE_NUM];
> >      NetFilterClass *nfc = NETFILTER_GET_CLASS(uc);
> >      int queues;
> > @@ -219,6 +262,20 @@ static void netfilter_complete(UserCreatable *uc,
> > Error **errp)
> >          return;
> >      }
> >
> > +    if (strcmp(nf->position, "head") && strcmp(nf->position, "tail")) {
> > +        /* Search for the position to insert before/after */
> > +        Object *container;
> > +        Object *obj;
> > +
> > +        container = object_get_objects_root();
> > +        obj = object_resolve_path_component(container, nf->position);
> > +        if (!obj) {
> > +            error_setg(errp, "filter '%s' not found", nf->position);
> > +            return;
> > +        }
> > +        position = NETFILTER(obj);
> > +    }
> > +
> >      nf->netdev = ncs[0];
> >
> >      if (nfc->setup) {
> > @@ -228,7 +285,18 @@ static void netfilter_complete(UserCreatable *uc,
> > Error **errp)
> >              return;
> >          }
> >      }
> > -    QTAILQ_INSERT_TAIL(&nf->netdev->filters, nf, next);
> > +
> > +    if (position) {
> > +        if (nf->insert_before) {
> > +            QTAILQ_INSERT_BEFORE(position, nf, next);
> > +        } else {
> > +            QTAILQ_INSERT_AFTER(&nf->netdev->filters, position, nf, next);
> > +        }
> > +    } else if (!strcmp(nf->position, "head")) {
> > +        QTAILQ_INSERT_HEAD(&nf->netdev->filters, nf, next);
> > +    } else if (!strcmp(nf->position, "tail")) {
> > +        QTAILQ_INSERT_TAIL(&nf->netdev->filters, nf, next);
> > +    }
> >  }
> >
> >  static void netfilter_finalize(Object *obj) @@ -245,6 +313,7 @@ static void
> > netfilter_finalize(Object *obj)
> >          QTAILQ_REMOVE(&nf->netdev->filters, nf, next);
> >      }
> >      g_free(nf->netdev_id);
> > +    g_free(nf->position);
> >  }
> >
> >  static void default_handle_event(NetFilterState *nf, int event, Error **errp)
> > diff --git a/qemu-options.hx b/qemu-options.hx index 08749a3391..f0a47a0746
> > 100644
> > --- a/qemu-options.hx
> > +++ b/qemu-options.hx
> > @@ -4368,7 +4368,7 @@ applications, they can do this through this parameter.
> > Its format is  a gnutls priority string as described at
> > @url{https://gnutls.org/manual/html_node/Priority-Strings.html}.
> >
> > -@item -object filter-
> > buffer,id=@var{id},netdev=@var{netdevid},interval=@var{t}[,queue=@var{all|r
> > x|tx}][,status=@var{on|off}]
> > +@item -object
> > +filter-buffer,id=@var{id},netdev=@var{netdevid},interval=@var{t}[,queue
> > +=@var{all|rx|tx}][,status=@var{on|off}][,position=@var{head|tail|id}][,
> > +insert=@var{after|before}]
> >
> >  Interval @var{t} can't be 0, this filter batches the packet delivery: all  packets
> > arriving in a given interval on netdev @var{netdevid} are delayed @@ -4387,11
> > +4387,11 @@ queue @var{all|rx|tx} is an option that can be applied to any
> > netfilter.
> >  @option{tx}: the filter is attached to the transmit queue of the netdev,
> >               where it will receive packets sent by the netdev.
> >
> > -@item -object filter-
> > mirror,id=@var{id},netdev=@var{netdevid},outdev=@var{chardevid},queue=@v
> > ar{all|rx|tx}[,vnet_hdr_support]
> > +@item -object
> > +filter-mirror,id=@var{id},netdev=@var{netdevid},outdev=@var{chardevid},
> > +queue=@var{all|rx|tx}[,vnet_hdr_support][,position=@var{head|tail|id}][
> > +,insert=@var{after|before}]
> >
> >  filter-mirror on netdev @var{netdevid},mirror net packet to
> > chardev@var{chardevid}, if it has the vnet_hdr_support flag, filter-mirror will
> > mirror packet with vnet_hdr_len.
> >
> > -@item -object filter-
> > redirector,id=@var{id},netdev=@var{netdevid},indev=@var{chardevid},outdev=
> > @var{chardevid},queue=@var{all|rx|tx}[,vnet_hdr_support]
> > +@item -object
> > +filter-redirector,id=@var{id},netdev=@var{netdevid},indev=@var{chardevi
> > +d},outdev=@var{chardevid},queue=@var{all|rx|tx}[,vnet_hdr_support][,pos
> > +ition=@var{head|tail|id}][,insert=@var{after|before}]
> >
> >  filter-redirector on netdev @var{netdevid},redirect filter's net packet to
> > chardev  @var{chardevid},and redirect indev's packet to filter.if it has the
> > vnet_hdr_support flag, @@ -4400,7 +4400,7 @@ Create a filter-redirector we
> > need to differ outdev id from indev id, id can not  be the same. we can just use
> > indev or outdev, but at least one of indev or outdev  need to be specified.
> >
> > -@item -object filter-
> > rewriter,id=@var{id},netdev=@var{netdevid},queue=@var{all|rx|tx},[vnet_hdr_
> > support]
> > +@item -object
> > +filter-rewriter,id=@var{id},netdev=@var{netdevid},queue=@var{all|rx|tx}
> > +,[vnet_hdr_support][,position=@var{head|tail|id}][,insert=@var{after|be
> > +fore}]
> >
> >  Filter-rewriter is a part of COLO project.It will rewrite tcp packet to  secondary
> > from primary to keep secondary tcp connection,and rewrite @@ -4413,7
> > +4413,7 @@ colo secondary:
> >  -object filter-redirector,id=f2,netdev=hn0,queue=rx,outdev=red1
> >  -object filter-rewriter,id=rew0,netdev=hn0,queue=all
> >
> > -@item -object filter-
> > dump,id=@var{id},netdev=@var{dev}[,file=@var{filename}][,maxlen=@var{len}]
> > +@item -object
> > +filter-dump,id=@var{id},netdev=@var{dev}[,file=@var{filename}][,maxlen=
> > +@var{len}][,position=@var{head|tail|id}][,insert=@var{after|before}]
> >
> >  Dump the network traffic on netdev @var{dev} to the file specified by
> > @var{filename}. At most @var{len} bytes (64k by default) per packet are stored.
> > --
> > 2.20.1
>

Re: [Qemu-devel] [PATCH v2 2/3] net/filter.c: Add Options to insert filters anywhere in the filter list

[Zhang, Chen]

> -----Original Message-----
> From: Lukas Straub <lukasstraub2@web.de>
> Sent: Friday, August 23, 2019 2:21 PM
> To: Zhang, Chen <chen.zhang@intel.com>
> Cc: qemu-devel <qemu-devel@nongnu.org>; Jason Wang
> <jasowang@redhat.com>; Wen Congyang <wencongyang2@huawei.com>;
> Xie Changlong <xiechanglong.d@gmail.com>
> Subject: Re: [PATCH v2 2/3] net/filter.c: Add Options to insert filters
> anywhere in the filter list
> 
> On Fri, 23 Aug 2019 03:24:02 +0000
> "Zhang, Chen" <chen.zhang@intel.com> wrote:
> 
> > > -----Original Message-----
> > > From: Lukas Straub [mailto:lukasstraub2@web.de]
> > > Sent: Friday, August 16, 2019 2:49 AM
> > > To: qemu-devel <qemu-devel@nongnu.org>
> > > Cc: Zhang, Chen <chen.zhang@intel.com>; Jason Wang
> > > <jasowang@redhat.com>; Wen Congyang
> <wencongyang2@huawei.com>; Xie
> > > Changlong <xiechanglong.d@gmail.com>
> > > Subject: [PATCH v2 2/3] net/filter.c: Add Options to insert filters
> > > anywhere in the filter list
> > >
> > > To switch the Secondary to Primary, we need to insert new filters
> > > before the filter-rewriter.
> > >
> > > Add the options insert= and position= to be able to insert filters
> > > anywhere in the filter list.
> > >
> > > position should be either "head", "tail" or the id of another filter.
> > > insert should be either "before" or "after" to specify where to
> > > insert the new filter relative to the one specified with position.
> > >
> >
> > Hi Lukas,
> >
> > It looks no need to add the "insert = xxx" for this operation.
> > For example:
> >
> > We have 3 net-filters, the running order like that:
> >
> > Fiter1   ---------->   Filter2 ------------> Filter3
> >
> > If we want to add another filter between filter1 and filter2.
> > The "Position = head, insert = after" always seam with "position = filter2 id,
> insert = before".
> 
> Hi Zhang,
> The insert= parameter is ignored if position=head or tail. It always Inserts at
> the head (before Filter1) or the tail (after Filter3) of the List in these cases.
> 
> > It seems the "insert" is a redundant args.
> > So I think it is enough with the "position", we can make the "insert" always
> equal "after" except the "head".
> 
> Yes, we still could do it without it, but its more convenient with the insert=
> parameter. For example our Case with inserting before the rewriter:
> 
> 'filter-mirror', 'id': 'm0', 'props': { 'insert': 'before', 'position': 'rew0', 'netdev':
> 'hn0', 'queue': 'tx', 'outdev': 'mirror0' } 'filter-redirector', 'id': 'redire0', 'props':
> { 'insert': 'before', 'position': 'rew0', 'netdev': 'hn0', 'queue': 'rx', 'indev':
> 'compare_out' } 'filter-redirector', 'id': 'redire1', 'props': { 'insert': 'before',
> 'position': 'rew0', 'netdev': 'hn0', 'queue': 'rx', 'outdev': 'compare0' }
> 
> You see directly that here 3 Filters are inserted before the rewriter.
> 
> would have to become:
> 
> 'filter-mirror', 'id': 'm0', 'props': { 'position': 'head', 'netdev': 'hn0', 'queue': 'tx',
> 'outdev': 'mirror0' } 'filter-redirector', 'id': 'redire0', 'props': { 'position': 'm0',
> 'netdev': 'hn0', 'queue': 'rx', 'indev': 'compare_out' } 'filter-redirector', 'id':
> 'redire1', 'props': { 'position': 'redire0', 'netdev': 'hn0', 'queue': 'rx', 'outdev':
> 'compare0' }
> 
> Which is less obvious.

OK, It is fine for me.
But in the code have some other issues like that:

+
+static void netfilter_set_insert(Object *obj, const char *str, Error **errp)
+{
+    NetFilterState *nf = NETFILTER(obj);
+
+    if (strcmp(str, "before") && strcmp(str, "after")) {                                
+        error_setg(errp, "Invalid value for netfilter insert, " 
+                         "should be 'head' or 'tail'");                                  -------------------------------->>> I think you should change the "head/tail"  to "before/after".
+        return;
+    }
+
+    nf->insert_before = !strcmp(str, "before");
+}


And I think the "front/behind" is better than "before/after" in this status.
At the same time the name of the "insert_before" change to "front_flag" is better.


Thanks
Zhang Chen

> 
> Regards,
> Lukas Straub
> 
> >
> > Thanks
> > Zhang Chen
> >
> > > Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> > > ---
> > >  include/net/filter.h |  2 ++
> > >  net/filter.c         | 71
> +++++++++++++++++++++++++++++++++++++++++++-
> > >  qemu-options.hx      | 10 +++----
> > >  3 files changed, 77 insertions(+), 6 deletions(-)

Re: [Qemu-devel] [PATCH v2 2/3] net/filter.c: Add Options to insert filters anywhere in the filter list

[Lukas Straub]

On Mon, 2 Sep 2019 11:43:57 +0000
"Zhang, Chen" <chen.zhang@intel.com> wrote:

> > -----Original Message-----
> > From: Lukas Straub <lukasstraub2@web.de>
> > Sent: Friday, August 23, 2019 2:21 PM
> > To: Zhang, Chen <chen.zhang@intel.com>
> > Cc: qemu-devel <qemu-devel@nongnu.org>; Jason Wang
> > <jasowang@redhat.com>; Wen Congyang <wencongyang2@huawei.com>;
> > Xie Changlong <xiechanglong.d@gmail.com>
> > Subject: Re: [PATCH v2 2/3] net/filter.c: Add Options to insert filters
> > anywhere in the filter list
> >
> > On Fri, 23 Aug 2019 03:24:02 +0000
> > "Zhang, Chen" <chen.zhang@intel.com> wrote:
> >
> > > > -----Original Message-----
> > > > From: Lukas Straub [mailto:lukasstraub2@web.de]
> > > > Sent: Friday, August 16, 2019 2:49 AM
> > > > To: qemu-devel <qemu-devel@nongnu.org>
> > > > Cc: Zhang, Chen <chen.zhang@intel.com>; Jason Wang
> > > > <jasowang@redhat.com>; Wen Congyang
> > <wencongyang2@huawei.com>; Xie
> > > > Changlong <xiechanglong.d@gmail.com>
> > > > Subject: [PATCH v2 2/3] net/filter.c: Add Options to insert filters
> > > > anywhere in the filter list
> > > >
> > > > To switch the Secondary to Primary, we need to insert new filters
> > > > before the filter-rewriter.
> > > >
> > > > Add the options insert= and position= to be able to insert filters
> > > > anywhere in the filter list.
> > > >
> > > > position should be either "head", "tail" or the id of another filter.
> > > > insert should be either "before" or "after" to specify where to
> > > > insert the new filter relative to the one specified with position.
> > > >
> > >
> > > Hi Lukas,
> > >
> > > It looks no need to add the "insert = xxx" for this operation.
> > > For example:
> > >
> > > We have 3 net-filters, the running order like that:
> > >
> > > Fiter1   ---------->   Filter2 ------------> Filter3
> > >
> > > If we want to add another filter between filter1 and filter2.
> > > The "Position = head, insert = after" always seam with "position = filter2 id,
> > insert = before".
> >
> > Hi Zhang,
> > The insert= parameter is ignored if position=head or tail. It always Inserts at
> > the head (before Filter1) or the tail (after Filter3) of the List in these cases.
> >
> > > It seems the "insert" is a redundant args.
> > > So I think it is enough with the "position", we can make the "insert" always
> > equal "after" except the "head".
> >
> > Yes, we still could do it without it, but its more convenient with the insert=
> > parameter. For example our Case with inserting before the rewriter:
> >
> > 'filter-mirror', 'id': 'm0', 'props': { 'insert': 'before', 'position': 'rew0', 'netdev':
> > 'hn0', 'queue': 'tx', 'outdev': 'mirror0' } 'filter-redirector', 'id': 'redire0', 'props':
> > { 'insert': 'before', 'position': 'rew0', 'netdev': 'hn0', 'queue': 'rx', 'indev':
> > 'compare_out' } 'filter-redirector', 'id': 'redire1', 'props': { 'insert': 'before',
> > 'position': 'rew0', 'netdev': 'hn0', 'queue': 'rx', 'outdev': 'compare0' }
> >
> > You see directly that here 3 Filters are inserted before the rewriter.
> >
> > would have to become:
> >
> > 'filter-mirror', 'id': 'm0', 'props': { 'position': 'head', 'netdev': 'hn0', 'queue': 'tx',
> > 'outdev': 'mirror0' } 'filter-redirector', 'id': 'redire0', 'props': { 'position': 'm0',
> > 'netdev': 'hn0', 'queue': 'rx', 'indev': 'compare_out' } 'filter-redirector', 'id':
> > 'redire1', 'props': { 'position': 'redire0', 'netdev': 'hn0', 'queue': 'rx', 'outdev':
> > 'compare0' }
> >
> > Which is less obvious.
>
> OK, It is fine for me.
> But in the code have some other issues like that:
>
> +
> +static void netfilter_set_insert(Object *obj, const char *str, Error **errp)
> +{
> +    NetFilterState *nf = NETFILTER(obj);
> +
> +    if (strcmp(str, "before") && strcmp(str, "after")) {
> +        error_setg(errp, "Invalid value for netfilter insert, "
> +                         "should be 'head' or 'tail'");                                  -------------------------------->>> I think you should change the "head/tail"  to "before/after".

Oops, that was a typo.

> +        return;
> +    }
> +
> +    nf->insert_before = !strcmp(str, "before");
> +}
>
>
> And I think the "front/behind" is better than "before/after" in this status.
> At the same time the name of the "insert_before" change to "front_flag" is better.

What I like about the "before" is that it sounds more like an English sentence.
insert: before, position: rew0
vs.
insert: front, position: rew0

But I agree, "behind" is more clear than "after".

What do you think about "first/last" instead of "head/tail"?

Regards,
Lukas Straub

>
> Thanks
> Zhang Chen
>
> >
> > Regards,
> > Lukas Straub
> >
> > >
> > > Thanks
> > > Zhang Chen
> > >
> > > > Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> > > > ---
> > > >  include/net/filter.h |  2 ++
> > > >  net/filter.c         | 71
> > +++++++++++++++++++++++++++++++++++++++++++-
> > > >  qemu-options.hx      | 10 +++----
> > > >  3 files changed, 77 insertions(+), 6 deletions(-)
>

Re: [Qemu-devel] [PATCH v2 2/3] net/filter.c: Add Options to insert filters anywhere in the filter list

[Zhang, Chen]

> -----Original Message-----
> From: Lukas Straub <lukasstraub2@web.de>
> Sent: Tuesday, September 3, 2019 2:51 AM
> To: Zhang, Chen <chen.zhang@intel.com>
> Cc: qemu-devel <qemu-devel@nongnu.org>; Jason Wang
> <jasowang@redhat.com>; Wen Congyang <wencongyang2@huawei.com>;
> Xie Changlong <xiechanglong.d@gmail.com>
> Subject: Re: [PATCH v2 2/3] net/filter.c: Add Options to insert filters
> anywhere in the filter list
> 
> On Mon, 2 Sep 2019 11:43:57 +0000
> "Zhang, Chen" <chen.zhang@intel.com> wrote:
> 
> > > -----Original Message-----
> > > From: Lukas Straub <lukasstraub2@web.de>
> > > Sent: Friday, August 23, 2019 2:21 PM
> > > To: Zhang, Chen <chen.zhang@intel.com>
> > > Cc: qemu-devel <qemu-devel@nongnu.org>; Jason Wang
> > > <jasowang@redhat.com>; Wen Congyang
> <wencongyang2@huawei.com>; Xie
> > > Changlong <xiechanglong.d@gmail.com>
> > > Subject: Re: [PATCH v2 2/3] net/filter.c: Add Options to insert
> > > filters anywhere in the filter list
> > >
> > > On Fri, 23 Aug 2019 03:24:02 +0000
> > > "Zhang, Chen" <chen.zhang@intel.com> wrote:
> > >
> > > > > -----Original Message-----
> > > > > From: Lukas Straub [mailto:lukasstraub2@web.de]
> > > > > Sent: Friday, August 16, 2019 2:49 AM
> > > > > To: qemu-devel <qemu-devel@nongnu.org>
> > > > > Cc: Zhang, Chen <chen.zhang@intel.com>; Jason Wang
> > > > > <jasowang@redhat.com>; Wen Congyang
> > > <wencongyang2@huawei.com>; Xie
> > > > > Changlong <xiechanglong.d@gmail.com>
> > > > > Subject: [PATCH v2 2/3] net/filter.c: Add Options to insert
> > > > > filters anywhere in the filter list
> > > > >
> > > > > To switch the Secondary to Primary, we need to insert new
> > > > > filters before the filter-rewriter.
> > > > >
> > > > > Add the options insert= and position= to be able to insert
> > > > > filters anywhere in the filter list.
> > > > >
> > > > > position should be either "head", "tail" or the id of another filter.
> > > > > insert should be either "before" or "after" to specify where to
> > > > > insert the new filter relative to the one specified with position.
> > > > >
> > > >
> > > > Hi Lukas,
> > > >
> > > > It looks no need to add the "insert = xxx" for this operation.
> > > > For example:
> > > >
> > > > We have 3 net-filters, the running order like that:
> > > >
> > > > Fiter1   ---------->   Filter2 ------------> Filter3
> > > >
> > > > If we want to add another filter between filter1 and filter2.
> > > > The "Position = head, insert = after" always seam with "position =
> > > > filter2 id,
> > > insert = before".
> > >
> > > Hi Zhang,
> > > The insert= parameter is ignored if position=head or tail. It always
> > > Inserts at the head (before Filter1) or the tail (after Filter3) of the List in
> these cases.
> > >
> > > > It seems the "insert" is a redundant args.
> > > > So I think it is enough with the "position", we can make the
> > > > "insert" always
> > > equal "after" except the "head".
> > >
> > > Yes, we still could do it without it, but its more convenient with
> > > the insert= parameter. For example our Case with inserting before the
> rewriter:
> > >
> > > 'filter-mirror', 'id': 'm0', 'props': { 'insert': 'before', 'position': 'rew0',
> 'netdev':
> > > 'hn0', 'queue': 'tx', 'outdev': 'mirror0' } 'filter-redirector', 'id': 'redire0',
> 'props':
> > > { 'insert': 'before', 'position': 'rew0', 'netdev': 'hn0', 'queue': 'rx', 'indev':
> > > 'compare_out' } 'filter-redirector', 'id': 'redire1', 'props': {
> > > 'insert': 'before',
> > > 'position': 'rew0', 'netdev': 'hn0', 'queue': 'rx', 'outdev':
> > > 'compare0' }
> > >
> > > You see directly that here 3 Filters are inserted before the rewriter.
> > >
> > > would have to become:
> > >
> > > 'filter-mirror', 'id': 'm0', 'props': { 'position': 'head',
> > > 'netdev': 'hn0', 'queue': 'tx',
> > > 'outdev': 'mirror0' } 'filter-redirector', 'id': 'redire0', 'props':
> > > { 'position': 'm0',
> > > 'netdev': 'hn0', 'queue': 'rx', 'indev': 'compare_out' } 'filter-redirector', 'id':
> > > 'redire1', 'props': { 'position': 'redire0', 'netdev': 'hn0', 'queue': 'rx',
> 'outdev':
> > > 'compare0' }
> > >
> > > Which is less obvious.
> >
> > OK, It is fine for me.
> > But in the code have some other issues like that:
> >
> > +
> > +static void netfilter_set_insert(Object *obj, const char *str, Error
> > +**errp) {
> > +    NetFilterState *nf = NETFILTER(obj);
> > +
> > +    if (strcmp(str, "before") && strcmp(str, "after")) {
> > +        error_setg(errp, "Invalid value for netfilter insert, "
> > +                         "should be 'head' or 'tail'");                                  ----------------------
> ---------->>> I think you should change the "head/tail"  to "before/after".
> 
> Oops, that was a typo.
> 
> > +        return;
> > +    }
> > +
> > +    nf->insert_before = !strcmp(str, "before"); }
> >
> >
> > And I think the "front/behind" is better than "before/after" in this status.
> > At the same time the name of the "insert_before" change to "front_flag" is
> better.
> 
> What I like about the "before" is that it sounds more like an English sentence.
> insert: before, position: rew0
> vs.
> insert: front, position: rew0
> 
> But I agree, "behind" is more clear than "after".
> 
> What do you think about "first/last" instead of "head/tail"?

I'm not a English native speaker, but I think head/tail is enough same as the QTAILQ.

Thanks
Zhang Chen

> 
> Regards,
> Lukas Straub
> 
> >
> > Thanks
> > Zhang Chen
> >
> > >
> > > Regards,
> > > Lukas Straub
> > >
> > > >
> > > > Thanks
> > > > Zhang Chen
> > > >
> > > > > Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> > > > > ---
> > > > >  include/net/filter.h |  2 ++
> > > > >  net/filter.c         | 71
> > > +++++++++++++++++++++++++++++++++++++++++++-
> > > > >  qemu-options.hx      | 10 +++----
> > > > >  3 files changed, 77 insertions(+), 6 deletions(-)
> >

[Qemu-devel] [PATCH v2 3/3] Update Documentation

[Lukas Straub]

Document the qemu command-line and qmp commands for continious replication

Signed-off-by: Lukas Straub <lukasstraub2@web.de>
---
 docs/COLO-FT.txt | 185 +++++++++++++++++++++++++++++++++++------------
 1 file changed, 138 insertions(+), 47 deletions(-)

diff --git a/docs/COLO-FT.txt b/docs/COLO-FT.txt
index ad24680d13..c08bfbd3a8 100644
--- a/docs/COLO-FT.txt
+++ b/docs/COLO-FT.txt
@@ -145,35 +145,64 @@ The diagram just shows the main qmp command, you can get the detail
 in test procedure.
 
 == Test procedure ==
+Note: Here we are running both instances on the same Machine for testing, 
+change the IP Addresses if you want to run it on two Hosts
+
 1. Startup qemu
 Primary:
-# qemu-system-x86_64 -accel kvm -m 2048 -smp 2 -qmp stdio -name primary \
-  -device piix3-usb-uhci -vnc :7 \
-  -device usb-tablet -netdev tap,id=hn0,vhost=off \
-  -device virtio-net-pci,id=net-pci0,netdev=hn0 \
-  -drive if=virtio,id=primary-disk0,driver=quorum,read-pattern=fifo,vote-threshold=1,\
-         children.0.file.filename=1.raw,\
-         children.0.driver=raw -S
+# imagefolder="/mnt/vms/colo-test"
+
+# cp --reflink=auto $imagefolder/primary.qcow2 $imagefolder/primary-copy.qcow2
+
+# qemu-system-x86_64 -enable-kvm -cpu qemu64,+kvmclock -m 512 -smp 1 -qmp stdio \
+   -vnc :0 -k de -device piix3-usb-uhci -device usb-tablet -name primary \
+   -netdev tap,id=hn0,vhost=off,helper=/usr/lib/qemu/qemu-bridge-helper \
+   -device rtl8139,id=e0,netdev=hn0 \
+   -chardev socket,id=mirror0,host=127.0.0.1,port=9003,server,nowait \
+   -chardev socket,id=compare1,host=127.0.0.1,port=9004,server,wait \
+   -chardev socket,id=compare0,host=127.0.0.1,port=9001,server,nowait \
+   -chardev socket,id=compare0-0,host=127.0.0.1,port=9001 \
+   -chardev socket,id=compare_out,host=127.0.0.1,port=9005,server,nowait \
+   -chardev socket,id=compare_out0,host=127.0.0.1,port=9005 \
+   -object filter-mirror,id=m0,netdev=hn0,queue=tx,outdev=mirror0 \
+   -object filter-redirector,netdev=hn0,id=redire0,queue=rx,indev=compare_out \
+   -object filter-redirector,netdev=hn0,id=redire1,queue=rx,outdev=compare0 \
+   -object iothread,id=iothread1 \
+   -object colo-compare,id=comp0,primary_in=compare0-0,secondary_in=compare1,\
+outdev=compare_out0,iothread=iothread1 \
+   -drive if=ide,id=colo-disk0,driver=quorum,read-pattern=fifo,vote-threshold=1,\
+children.0.file.filename=$imagefolder/primary.qcow2,children.0.driver=qcow2 -S
+
 Secondary:
-# qemu-system-x86_64 -accel kvm -m 2048 -smp 2 -qmp stdio -name secondary \
-  -device piix3-usb-uhci -vnc :7 \
-  -device usb-tablet -netdev tap,id=hn0,vhost=off \
-  -device virtio-net-pci,id=net-pci0,netdev=hn0 \
-  -drive if=none,id=secondary-disk0,file.filename=1.raw,driver=raw,node-name=node0 \
-  -drive if=virtio,id=active-disk0,driver=replication,mode=secondary,\
-         file.driver=qcow2,top-id=active-disk0,\
-         file.file.filename=/mnt/ramfs/active_disk.img,\
-         file.backing.driver=qcow2,\
-         file.backing.file.filename=/mnt/ramfs/hidden_disk.img,\
-         file.backing.backing=secondary-disk0 \
-  -incoming tcp:0:8888
+# imagefolder="/mnt/vms/colo-test"
+
+# qemu-img create -f qcow2 $imagefolder/secondary-active.qcow2 10G
+
+# qemu-img create -f qcow2 $imagefolder/secondary-hidden.qcow2 10G
+
+# qemu-system-x86_64 -enable-kvm -cpu qemu64,+kvmclock -m 512 -smp 1 -qmp stdio \
+   -vnc :1 -k de -device piix3-usb-uhci -device usb-tablet -name secondary \
+   -netdev tap,id=hn0,vhost=off,helper=/usr/lib/qemu/qemu-bridge-helper \
+   -device rtl8139,id=e0,netdev=hn0 \
+   -chardev socket,id=red0,host=127.0.0.1,port=9003,reconnect=1 \
+   -chardev socket,id=red1,host=127.0.0.1,port=9004,reconnect=1 \
+   -object filter-redirector,id=f1,netdev=hn0,queue=tx,indev=red0 \
+   -object filter-redirector,id=f2,netdev=hn0,queue=rx,outdev=red1 \
+   -object filter-rewriter,id=rew0,netdev=hn0,queue=all \
+   -drive if=none,id=parent0,file.filename=$imagefolder/primary-copy.qcow2,driver=qcow2 \
+   -drive if=none,id=childs0,driver=replication,mode=secondary,file.driver=qcow2,\
+top-id=childs0,file.file.filename=$imagefolder/secondary-active.qcow2,\
+file.backing.driver=qcow2,file.backing.file.filename=$imagefolder/secondary-hidden.qcow2,\
+file.backing.backing=parent0 \
+   -drive if=ide,id=colo-disk0,driver=quorum,read-pattern=fifo,vote-threshold=1,\
+children.0.file=childs0,children.0.driver=raw \
+   -incoming tcp:0:9998
+
 
 2. On Secondary VM's QEMU monitor, issue command
 {'execute':'qmp_capabilities'}
-{ 'execute': 'nbd-server-start',
-  'arguments': {'addr': {'type': 'inet', 'data': {'host': 'xx.xx.xx.xx', 'port': '8889'} } }
-}
-{'execute': 'nbd-server-add', 'arguments': {'device': 'secondary-disk0', 'writable': true } }
+{'execute': 'nbd-server-start', 'arguments': {'addr': {'type': 'inet', 'data': {'host': '127.0.0.1', 'port': '9999'} } } }
+{'execute': 'nbd-server-add', 'arguments': {'device': 'parent0', 'writable': true } }
 
 Note:
   a. The qmp command nbd-server-start and nbd-server-add must be run
@@ -184,12 +213,10 @@ Note:
 
 3. On Primary VM's QEMU monitor, issue command:
 {'execute':'qmp_capabilities'}
-{ 'execute': 'human-monitor-command',
-  'arguments': {'command-line': 'drive_add -n buddy driver=replication,mode=primary,file.driver=nbd,file.host=xx.xx.xx.xx,file.port=8889,file.export=secondary-disk0,node-name=nbd_client0'}}
-{ 'execute':'x-blockdev-change', 'arguments':{'parent': 'primary-disk0', 'node': 'nbd_client0' } }
-{ 'execute': 'migrate-set-capabilities',
-      'arguments': {'capabilities': [ {'capability': 'x-colo', 'state': true } ] } }
-{ 'execute': 'migrate', 'arguments': {'uri': 'tcp:xx.xx.xx.xx:8888' } }
+{'execute': 'human-monitor-command', 'arguments': {'command-line': 'drive_add -n buddy driver=replication,mode=primary,file.driver=nbd,file.host=127.0.0.1,file.port=9999,file.export=parent0,node-name=replication0'}}
+{'execute': 'x-blockdev-change', 'arguments':{'parent': 'colo-disk0', 'node': 'replication0' } }
+{'execute': 'migrate-set-capabilities', 'arguments': {'capabilities': [ {'capability': 'x-colo', 'state': true } ] } }
+{'execute': 'migrate', 'arguments': {'uri': 'tcp:127.0.0.1:9998' } }
 
   Note:
   a. There should be only one NBD Client for each primary disk.
@@ -199,27 +226,91 @@ Note:
 
 4. After the above steps, you will see, whenever you make changes to PVM, SVM will be synced.
 You can issue command '{ "execute": "migrate-set-parameters" , "arguments":{ "x-checkpoint-delay": 2000 } }'
-to change the checkpoint period time
+to change the idle checkpoint period time
 
 5. Failover test
-You can kill Primary VM and run 'x_colo_lost_heartbeat' in Secondary VM's
-monitor at the same time, then SVM will failover and client will not detect this
-change.
+You can kill one of the VMs and Failover on the surviving VM: 
 
-Before issuing '{ "execute": "x-colo-lost-heartbeat" }' command, we have to
-issue block related command to stop block replication.
-Primary:
-  Remove the nbd child from the quorum:
-  { 'execute': 'x-blockdev-change', 'arguments': {'parent': 'colo-disk0', 'child': 'children.1'}}
-  { 'execute': 'human-monitor-command','arguments': {'command-line': 'drive_del blk-buddy0'}}
-  Note: there is no qmp command to remove the blockdev now
+== Primary Failover ==
+The Secondary died, resume on the Primary
 
-Secondary:
-  The primary host is down, so we should do the following thing:
-  { 'execute': 'nbd-server-stop' }
+{'execute': 'x-blockdev-change', 'arguments':{ 'parent': 'colo-disk0', 'child': 'children.1'} }
+{'execute': 'human-monitor-command', 'arguments':{ 'command-line': 'drive_del replication0' } }
+{'execute': 'object-del', 'arguments':{ 'id': 'comp0' } }
+{'execute': 'object-del', 'arguments':{ 'id': 'iothread1' } }
+{'execute': 'object-del', 'arguments':{ 'id': 'm0' } }
+{'execute': 'object-del', 'arguments':{ 'id': 'redire0' } }
+{'execute': 'object-del', 'arguments':{ 'id': 'redire1' } }
+{'execute': 'x-colo-lost-heartbeat' }
+
+== Secondary Failover ==
+The Primary died, resume on the Secondary and prepare to become the new Primary
+
+{'execute': 'nbd-server-stop'}
+{'execute': 'x-colo-lost-heartbeat'}
+
+{'execute': 'object-del', 'arguments':{ 'id': 'f2' } }
+{'execute': 'object-del', 'arguments':{ 'id': 'f1' } }
+{'execute': 'chardev-remove', 'arguments':{ 'id': 'red1' } }
+{'execute': 'chardev-remove', 'arguments':{ 'id': 'red0' } }
+
+{'execute': 'chardev-add', 'arguments':{ 'id': 'mirror0', 'backend': {'type': 'socket', 'data': {'addr': { 'type': 'inet', 'data': { 'host': '127.0.0.1', 'port': '9003' } }, 'server': true } } } }
+{'execute': 'chardev-add', 'arguments':{ 'id': 'compare1', 'backend': {'type': 'socket', 'data': {'addr': { 'type': 'inet', 'data': { 'host': '127.0.0.1', 'port': '9004' } }, 'server': true } } } }
+{'execute': 'chardev-add', 'arguments':{ 'id': 'compare0', 'backend': {'type': 'socket', 'data': {'addr': { 'type': 'inet', 'data': { 'host': '127.0.0.1', 'port': '9001' } }, 'server': true } } } }
+{'execute': 'chardev-add', 'arguments':{ 'id': 'compare0-0', 'backend': {'type': 'socket', 'data': {'addr': { 'type': 'inet', 'data': { 'host': '127.0.0.1', 'port': '9001' } }, 'server': false } } } }
+{'execute': 'chardev-add', 'arguments':{ 'id': 'compare_out', 'backend': {'type': 'socket', 'data': {'addr': { 'type': 'inet', 'data': { 'host': '127.0.0.1', 'port': '9005' } }, 'server': true } } } }
+{'execute': 'chardev-add', 'arguments':{ 'id': 'compare_out0', 'backend': {'type': 'socket', 'data': {'addr': { 'type': 'inet', 'data': { 'host': '127.0.0.1', 'port': '9005' } }, 'server': false } } } }
+
+== Primary resume replication ==
+Resume replication after new Secondary is up.
+
+Start the new Secondary, then:
+{'execute': 'drive-mirror', 'arguments':{ 'device': 'colo-disk0', 'job-id': 'resync', 'target': 'nbd://127.0.0.1:9999/parent0', 'mode': 'existing', 'format': 'raw', 'sync': 'full'} }
+
+Wait until disk is synced, then:
+{'execute': 'stop'}
+{'execute': 'block-job-cancel', 'arguments':{ 'device': 'resync'} }
+
+{'execute': 'human-monitor-command', 'arguments':{ 'command-line': 'drive_add -n buddy driver=replication,mode=primary,file.driver=nbd,file.host=127.0.0.1,file.port=9999,file.export=parent0,node-name=replication0'}}
+{'execute': 'x-blockdev-change', 'arguments':{ 'parent': 'colo-disk0', 'node': 'replication0' } }
+
+{'execute': 'object-add', 'arguments':{ 'qom-type': 'filter-mirror', 'id': 'm0', 'props': { 'netdev': 'hn0', 'queue': 'tx', 'outdev': 'mirror0' } } }
+{'execute': 'object-add', 'arguments':{ 'qom-type': 'filter-redirector', 'id': 'redire0', 'props': { 'netdev': 'hn0', 'queue': 'rx', 'indev': 'compare_out' } } }
+{'execute': 'object-add', 'arguments':{ 'qom-type': 'filter-redirector', 'id': 'redire1', 'props': { 'netdev': 'hn0', 'queue': 'rx', 'outdev': 'compare0' } } }
+{'execute': 'object-add', 'arguments':{ 'qom-type': 'iothread', 'id': 'iothread1' } }
+{'execute': 'object-add', 'arguments':{ 'qom-type': 'colo-compare', 'id': 'comp0', 'props': { 'primary_in': 'compare0-0', 'secondary_in': 'compare1', 'outdev': 'compare_out0', 'iothread': 'iothread1' } } }
+
+{'execute': 'migrate-set-capabilities', 'arguments':{ 'capabilities': [ {'capability': 'x-colo', 'state': true } ] } }
+{'execute': 'migrate', 'arguments':{ 'uri': 'tcp:127.0.0.1:9998' } }
+
+Note:
+If this Primary previously was a Secondary, then we need to insert the 
+filters before the filter-rewriter by using the 
+"'insert': 'before', 'position': 'rew0'" Options. See below.
+
+== Secondary resume replication ==
+Become Primary and resume replication after new Secondary is up.
+
+Start the new Secondary, then:
+{'execute': 'drive-mirror', 'arguments':{ 'device': 'colo-disk0', 'job-id': 'resync', 'target': 'nbd://127.0.0.1:9999/parent0', 'mode': 'existing', 'format': 'raw', 'sync': 'full'} }
+
+Wait until disk is synced, then:
+{'execute': 'stop'}
+{'execute': 'block-job-cancel', 'arguments':{ 'device': 'resync' } }
+
+{'execute': 'human-monitor-command', 'arguments':{ 'command-line': 'drive_add -n buddy driver=replication,mode=primary,file.driver=nbd,file.host=127.0.0.1,file.port=9999,file.export=parent0,node-name=replication0'}}
+{'execute': 'x-blockdev-change', 'arguments':{ 'parent': 'colo-disk0', 'node': 'replication0' } }
+
+{'execute': 'object-add', 'arguments':{ 'qom-type': 'filter-mirror', 'id': 'm0', 'props': { 'insert': 'before', 'position': 'rew0', 'netdev': 'hn0', 'queue': 'tx', 'outdev': 'mirror0' } } }
+{'execute': 'object-add', 'arguments':{ 'qom-type': 'filter-redirector', 'id': 'redire0', 'props': { 'insert': 'before', 'position': 'rew0', 'netdev': 'hn0', 'queue': 'rx', 'indev': 'compare_out' } } }
+{'execute': 'object-add', 'arguments':{ 'qom-type': 'filter-redirector', 'id': 'redire1', 'props': { 'insert': 'before', 'position': 'rew0', 'netdev': 'hn0', 'queue': 'rx', 'outdev': 'compare0' } } }
+{'execute': 'object-add', 'arguments':{ 'qom-type': 'iothread', 'id': 'iothread1' } }
+{'execute': 'object-add', 'arguments':{ 'qom-type': 'colo-compare', 'id': 'comp0', 'props': { 'primary_in': 'compare0-0', 'secondary_in': 'compare1', 'outdev': 'compare_out0', 'iothread': 'iothread1' } } }
+
+{'execute': 'migrate-set-capabilities', 'arguments':{ 'capabilities': [ {'capability': 'x-colo', 'state': true } ] } }
+{'execute': 'migrate', 'arguments':{ 'uri': 'tcp:127.0.0.1:9998' } }
 
 == TODO ==
-1. Support continuous VM replication.
-2. Support shared storage.
-3. Develop the heartbeat part.
-4. Reduce checkpoint VM’s downtime while doing checkpoint.
+1. Support shared storage.
+2. Develop the heartbeat part.
+3. Reduce checkpoint VM’s downtime while doing checkpoint.
-- 
2.20.1

Re: [Qemu-devel] [PATCH v2 3/3] Update Documentation

[Markus Armbruster]

Subject "Update Documentation" is too vague, because (1) it lacks a
subsystem prefix, and (2) it fails to hint at what it is updated for.
Suggest something like "colo: Update documentation for FIXME", or maybe
"docs/COLO-FT.txt: Update for FIXME", with a suitable replacement for
FIXME.

Re: [Qemu-devel] [PATCH v2 3/3] Update Documentation

[Zhang, Chen]

Hi Lukas,

Please address Markus's comments change this patch's name in next version.
And I wrote some comments behind, please check it.
Firstly, Please remove all the trailing whitespace in this patch.


> -----Original Message-----
> From: Lukas Straub <lukasstraub2@web.de>
> Sent: Friday, August 16, 2019 2:49 AM
> To: qemu-devel <qemu-devel@nongnu.org>
> Cc: Zhang, Chen <chen.zhang@intel.com>; Jason Wang
> <jasowang@redhat.com>; Wen Congyang <wencongyang2@huawei.com>;
> Xie Changlong <xiechanglong.d@gmail.com>
> Subject: [PATCH v2 3/3] Update Documentation
> 
> Document the qemu command-line and qmp commands for continious
> replication
> 
> Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> ---
>  docs/COLO-FT.txt | 185 +++++++++++++++++++++++++++++++++++--------
> ----
>  1 file changed, 138 insertions(+), 47 deletions(-)
> 
> diff --git a/docs/COLO-FT.txt b/docs/COLO-FT.txt index
> ad24680d13..c08bfbd3a8 100644
> --- a/docs/COLO-FT.txt
> +++ b/docs/COLO-FT.txt
> @@ -145,35 +145,64 @@ The diagram just shows the main qmp command,
> you can get the detail  in test procedure.
> 
>  == Test procedure ==
> +Note: Here we are running both instances on the same Machine for
> +testing, change the IP Addresses if you want to run it on two Hosts
> +
>  1. Startup qemu
>  Primary:
> -# qemu-system-x86_64 -accel kvm -m 2048 -smp 2 -qmp stdio -name
> primary \
> -  -device piix3-usb-uhci -vnc :7 \
> -  -device usb-tablet -netdev tap,id=hn0,vhost=off \
> -  -device virtio-net-pci,id=net-pci0,netdev=hn0 \
> -  -drive if=virtio,id=primary-disk0,driver=quorum,read-pattern=fifo,vote-
> threshold=1,\
> -         children.0.file.filename=1.raw,\
> -         children.0.driver=raw -S
> +# imagefolder="/mnt/vms/colo-test"
> +
> +# cp --reflink=auto $imagefolder/primary.qcow2
> +$imagefolder/primary-copy.qcow2

I think you can tell other people here that we need two same disk image before COLO startup.
The name "primary and primary-copy" will make reader very confused.

> +
> +# qemu-system-x86_64 -enable-kvm -cpu qemu64,+kvmclock -m 512 -smp
> 1 -qmp stdio \
> +   -vnc :0 -k de -device piix3-usb-uhci -device usb-tablet -name primary \

What's mean of the "-k de" ?

> +   -netdev tap,id=hn0,vhost=off,helper=/usr/lib/qemu/qemu-bridge-helper
> \
> +   -device rtl8139,id=e0,netdev=hn0 \
> +   -chardev socket,id=mirror0,host=127.0.0.1,port=9003,server,nowait \
> +   -chardev socket,id=compare1,host=127.0.0.1,port=9004,server,wait \
> +   -chardev socket,id=compare0,host=127.0.0.1,port=9001,server,nowait \
> +   -chardev socket,id=compare0-0,host=127.0.0.1,port=9001 \
> +   -chardev socket,id=compare_out,host=127.0.0.1,port=9005,server,nowait
> \
> +   -chardev socket,id=compare_out0,host=127.0.0.1,port=9005 \
> +   -object filter-mirror,id=m0,netdev=hn0,queue=tx,outdev=mirror0 \
> +   -object filter-
> redirector,netdev=hn0,id=redire0,queue=rx,indev=compare_out \
> +   -object filter-
> redirector,netdev=hn0,id=redire1,queue=rx,outdev=compare0 \
> +   -object iothread,id=iothread1 \
> +   -object
> +colo-compare,id=comp0,primary_in=compare0-
> 0,secondary_in=compare1,\
> +outdev=compare_out0,iothread=iothread1 \

Please keep the space style.

> +   -drive
> +if=ide,id=colo-disk0,driver=quorum,read-pattern=fifo,vote-threshold=1,\
> +children.0.file.filename=$imagefolder/primary.qcow2,children.0.driver=q
> +cow2 -S
> +
>  Secondary:
> -# qemu-system-x86_64 -accel kvm -m 2048 -smp 2 -qmp stdio -name
> secondary \
> -  -device piix3-usb-uhci -vnc :7 \
> -  -device usb-tablet -netdev tap,id=hn0,vhost=off \
> -  -device virtio-net-pci,id=net-pci0,netdev=hn0 \
> -  -drive if=none,id=secondary-disk0,file.filename=1.raw,driver=raw,node-
> name=node0 \
> -  -drive if=virtio,id=active-disk0,driver=replication,mode=secondary,\
> -         file.driver=qcow2,top-id=active-disk0,\
> -         file.file.filename=/mnt/ramfs/active_disk.img,\
> -         file.backing.driver=qcow2,\
> -         file.backing.file.filename=/mnt/ramfs/hidden_disk.img,\
> -         file.backing.backing=secondary-disk0 \
> -  -incoming tcp:0:8888
> +# imagefolder="/mnt/vms/colo-test"
> +
> +# qemu-img create -f qcow2 $imagefolder/secondary-active.qcow2 10G
> +
> +# qemu-img create -f qcow2 $imagefolder/secondary-hidden.qcow2 10G
> +
> +# qemu-system-x86_64 -enable-kvm -cpu qemu64,+kvmclock -m 512 -smp
> 1 -qmp stdio \
> +   -vnc :1 -k de -device piix3-usb-uhci -device usb-tablet -name secondary \
> +   -netdev tap,id=hn0,vhost=off,helper=/usr/lib/qemu/qemu-bridge-helper
> \
> +   -device rtl8139,id=e0,netdev=hn0 \
> +   -chardev socket,id=red0,host=127.0.0.1,port=9003,reconnect=1 \
> +   -chardev socket,id=red1,host=127.0.0.1,port=9004,reconnect=1 \
> +   -object filter-redirector,id=f1,netdev=hn0,queue=tx,indev=red0 \
> +   -object filter-redirector,id=f2,netdev=hn0,queue=rx,outdev=red1 \
> +   -object filter-rewriter,id=rew0,netdev=hn0,queue=all \
> +   -drive if=none,id=parent0,file.filename=$imagefolder/primary-
> copy.qcow2,driver=qcow2 \
> +   -drive
> +if=none,id=childs0,driver=replication,mode=secondary,file.driver=qcow2,
> +\
> +top-id=childs0,file.file.filename=$imagefolder/secondary-active.qcow2,\
> +file.backing.driver=qcow2,file.backing.file.filename=$imagefolder/secon
> +dary-hidden.qcow2,\
> +file.backing.backing=parent0 \

Please keep the space style.

> +   -drive
> +if=ide,id=colo-disk0,driver=quorum,read-pattern=fifo,vote-threshold=1,\
> +children.0.file=childs0,children.0.driver=raw \
> +   -incoming tcp:0:9998
> +

Here, you changed the original block design, can you explain the reasons why we need to do this?
Please draw a ASCII diagram to describe COLO block detail.

> 
>  2. On Secondary VM's QEMU monitor, issue command
> {'execute':'qmp_capabilities'} -{ 'execute': 'nbd-server-start',
> -  'arguments': {'addr': {'type': 'inet', 'data': {'host': 'xx.xx.xx.xx', 'port':
> '8889'} } } -}
> -{'execute': 'nbd-server-add', 'arguments': {'device': 'secondary-disk0',
> 'writable': true } }
> +{'execute': 'nbd-server-start', 'arguments': {'addr': {'type': 'inet',
> +'data': {'host': '127.0.0.1', 'port': '9999'} } } }
> +{'execute': 'nbd-server-add', 'arguments': {'device': 'parent0',
> +'writable': true } }
> 
>  Note:
>    a. The qmp command nbd-server-start and nbd-server-add must be run
> @@ -184,12 +213,10 @@ Note:
> 
>  3. On Primary VM's QEMU monitor, issue command:
>  {'execute':'qmp_capabilities'}
> -{ 'execute': 'human-monitor-command',
> -  'arguments': {'command-line': 'drive_add -n buddy
> driver=replication,mode=primary,file.driver=nbd,file.host=xx.xx.xx.xx,file.p
> ort=8889,file.export=secondary-disk0,node-name=nbd_client0'}}
> -{ 'execute':'x-blockdev-change', 'arguments':{'parent': 'primary-disk0',
> 'node': 'nbd_client0' } } -{ 'execute': 'migrate-set-capabilities',
> -      'arguments': {'capabilities': [ {'capability': 'x-colo', 'state': true } ] } }
> -{ 'execute': 'migrate', 'arguments': {'uri': 'tcp:xx.xx.xx.xx:8888' } }
> +{'execute': 'human-monitor-command', 'arguments': {'command-line':
> +'drive_add -n buddy
> +driver=replication,mode=primary,file.driver=nbd,file.host=127.0.0.1,fil
> +e.port=9999,file.export=parent0,node-name=replication0'}}
> +{'execute': 'x-blockdev-change', 'arguments':{'parent': 'colo-disk0',
> +'node': 'replication0' } }
> +{'execute': 'migrate-set-capabilities', 'arguments': {'capabilities': [
> +{'capability': 'x-colo', 'state': true } ] } }
> +{'execute': 'migrate', 'arguments': {'uri': 'tcp:127.0.0.1:9998' } }
> 
>    Note:
>    a. There should be only one NBD Client for each primary disk.
> @@ -199,27 +226,91 @@ Note:
> 
>  4. After the above steps, you will see, whenever you make changes to PVM,
> SVM will be synced.
>  You can issue command '{ "execute": "migrate-set-parameters" ,
> "arguments":{ "x-checkpoint-delay": 2000 } }'
> -to change the checkpoint period time
> +to change the idle checkpoint period time
> 
>  5. Failover test
> -You can kill Primary VM and run 'x_colo_lost_heartbeat' in Secondary VM's -
> monitor at the same time, then SVM will failover and client will not detect
> this -change.
> +You can kill one of the VMs and Failover on the surviving VM:
> 
> -Before issuing '{ "execute": "x-colo-lost-heartbeat" }' command, we have to
> -issue block related command to stop block replication.
> -Primary:
> -  Remove the nbd child from the quorum:
> -  { 'execute': 'x-blockdev-change', 'arguments': {'parent': 'colo-disk0', 'child':
> 'children.1'}}
> -  { 'execute': 'human-monitor-command','arguments': {'command-line':
> 'drive_del blk-buddy0'}}
> -  Note: there is no qmp command to remove the blockdev now
> +== Primary Failover ==
> +The Secondary died, resume on the Primary
> 
> -Secondary:
> -  The primary host is down, so we should do the following thing:
> -  { 'execute': 'nbd-server-stop' }
> +{'execute': 'x-blockdev-change', 'arguments':{ 'parent': 'colo-disk0',
> +'child': 'children.1'} }
> +{'execute': 'human-monitor-command', 'arguments':{ 'command-line':
> +'drive_del replication0' } }
> +{'execute': 'object-del', 'arguments':{ 'id': 'comp0' } }
> +{'execute': 'object-del', 'arguments':{ 'id': 'iothread1' } }
> +{'execute': 'object-del', 'arguments':{ 'id': 'm0' } }
> +{'execute': 'object-del', 'arguments':{ 'id': 'redire0' } }
> +{'execute': 'object-del', 'arguments':{ 'id': 'redire1' } }
> +{'execute': 'x-colo-lost-heartbeat' }
> +
> +== Secondary Failover ==
> +The Primary died, resume on the Secondary and prepare to become the
> new
> +Primary
> +
> +{'execute': 'nbd-server-stop'}
> +{'execute': 'x-colo-lost-heartbeat'}
> +
> +{'execute': 'object-del', 'arguments':{ 'id': 'f2' } }
> +{'execute': 'object-del', 'arguments':{ 'id': 'f1' } }
> +{'execute': 'chardev-remove', 'arguments':{ 'id': 'red1' } }
> +{'execute': 'chardev-remove', 'arguments':{ 'id': 'red0' } }
> +
> +{'execute': 'chardev-add', 'arguments':{ 'id': 'mirror0', 'backend':
> +{'type': 'socket', 'data': {'addr': { 'type': 'inet', 'data': { 'host':
> +'127.0.0.1', 'port': '9003' } }, 'server': true } } } }
> +{'execute': 'chardev-add', 'arguments':{ 'id': 'compare1', 'backend':
> +{'type': 'socket', 'data': {'addr': { 'type': 'inet', 'data': { 'host':
> +'127.0.0.1', 'port': '9004' } }, 'server': true } } } }
> +{'execute': 'chardev-add', 'arguments':{ 'id': 'compare0', 'backend':
> +{'type': 'socket', 'data': {'addr': { 'type': 'inet', 'data': { 'host':
> +'127.0.0.1', 'port': '9001' } }, 'server': true } } } }
> +{'execute': 'chardev-add', 'arguments':{ 'id': 'compare0-0', 'backend':
> +{'type': 'socket', 'data': {'addr': { 'type': 'inet', 'data': { 'host':
> +'127.0.0.1', 'port': '9001' } }, 'server': false } } } }
> +{'execute': 'chardev-add', 'arguments':{ 'id': 'compare_out',
> +'backend': {'type': 'socket', 'data': {'addr': { 'type': 'inet',
> +'data': { 'host': '127.0.0.1', 'port': '9005' } }, 'server': true } } }
> +}
> +{'execute': 'chardev-add', 'arguments':{ 'id': 'compare_out0',
> +'backend': {'type': 'socket', 'data': {'addr': { 'type': 'inet',
> +'data': { 'host': '127.0.0.1', 'port': '9005' } }, 'server': false } }
> +} }
> +
> +== Primary resume replication ==
> +Resume replication after new Secondary is up.
> +
> +Start the new Secondary, then:
> +{'execute': 'drive-mirror', 'arguments':{ 'device': 'colo-disk0',
> +'job-id': 'resync', 'target': 'nbd://127.0.0.1:9999/parent0', 'mode':
> +'existing', 'format': 'raw', 'sync': 'full'} }

I think the original secondary node has stopped the nbd server in failover:
> +{'execute': 'nbd-server-stop'}
> +{'execute': 'x-colo-lost-heartbeat'}

Why you can connect to it?

And please add new Secondary node start up script here,
I think it is different with original secondary node script.
same issue in resume secondary.

> +
> +Wait until disk is synced, then:
> +{'execute': 'stop'}
> +{'execute': 'block-job-cancel', 'arguments':{ 'device': 'resync'} }
> +
> +{'execute': 'human-monitor-command', 'arguments':{ 'command-line':
> +'drive_add -n buddy
> +driver=replication,mode=primary,file.driver=nbd,file.host=127.0.0.1,fil
> +e.port=9999,file.export=parent0,node-name=replication0'}}
> +{'execute': 'x-blockdev-change', 'arguments':{ 'parent': 'colo-disk0',
> +'node': 'replication0' } }
> +
> +{'execute': 'object-add', 'arguments':{ 'qom-type': 'filter-mirror',
> +'id': 'm0', 'props': { 'netdev': 'hn0', 'queue': 'tx', 'outdev':
> +'mirror0' } } }
> +{'execute': 'object-add', 'arguments':{ 'qom-type':
> +'filter-redirector', 'id': 'redire0', 'props': { 'netdev': 'hn0',
> +'queue': 'rx', 'indev': 'compare_out' } } }
> +{'execute': 'object-add', 'arguments':{ 'qom-type':
> +'filter-redirector', 'id': 'redire1', 'props': { 'netdev': 'hn0',
> +'queue': 'rx', 'outdev': 'compare0' } } }
> +{'execute': 'object-add', 'arguments':{ 'qom-type': 'iothread', 'id':
> +'iothread1' } }
> +{'execute': 'object-add', 'arguments':{ 'qom-type': 'colo-compare',
> +'id': 'comp0', 'props': { 'primary_in': 'compare0-0', 'secondary_in':
> +'compare1', 'outdev': 'compare_out0', 'iothread': 'iothread1' } } }
> +
> +{'execute': 'migrate-set-capabilities', 'arguments':{ 'capabilities': [
> +{'capability': 'x-colo', 'state': true } ] } }
> +{'execute': 'migrate', 'arguments':{ 'uri': 'tcp:127.0.0.1:9998' } }
> +
> +Note:
> +If this Primary previously was a Secondary, then we need to insert the
> +filters before the filter-rewriter by using the
> +"'insert': 'before', 'position': 'rew0'" Options. See below.
> +
> +== Secondary resume replication ==
> +Become Primary and resume replication after new Secondary is up.
> +
> +Start the new Secondary, then:
> +{'execute': 'drive-mirror', 'arguments':{ 'device': 'colo-disk0',
> +'job-id': 'resync', 'target': 'nbd://127.0.0.1:9999/parent0', 'mode':
> +'existing', 'format': 'raw', 'sync': 'full'} }

Same issue as primary resume.

I want to re-tested the resume function but got stuck here.

Thanks
Zhang Chen

> +
> +Wait until disk is synced, then:
> +{'execute': 'stop'}
> +{'execute': 'block-job-cancel', 'arguments':{ 'device': 'resync' } }
> +
> +{'execute': 'human-monitor-command', 'arguments':{ 'command-line':
> +'drive_add -n buddy
> +driver=replication,mode=primary,file.driver=nbd,file.host=127.0.0.1,fil
> +e.port=9999,file.export=parent0,node-name=replication0'}}
> +{'execute': 'x-blockdev-change', 'arguments':{ 'parent': 'colo-disk0',
> +'node': 'replication0' } }
> +
> +{'execute': 'object-add', 'arguments':{ 'qom-type': 'filter-mirror',
> +'id': 'm0', 'props': { 'insert': 'before', 'position': 'rew0',
> +'netdev': 'hn0', 'queue': 'tx', 'outdev': 'mirror0' } } }
> +{'execute': 'object-add', 'arguments':{ 'qom-type':
> +'filter-redirector', 'id': 'redire0', 'props': { 'insert': 'before',
> +'position': 'rew0', 'netdev': 'hn0', 'queue': 'rx', 'indev':
> +'compare_out' } } }
> +{'execute': 'object-add', 'arguments':{ 'qom-type':
> +'filter-redirector', 'id': 'redire1', 'props': { 'insert': 'before',
> +'position': 'rew0', 'netdev': 'hn0', 'queue': 'rx', 'outdev':
> +'compare0' } } }
> +{'execute': 'object-add', 'arguments':{ 'qom-type': 'iothread', 'id':
> +'iothread1' } }
> +{'execute': 'object-add', 'arguments':{ 'qom-type': 'colo-compare',
> +'id': 'comp0', 'props': { 'primary_in': 'compare0-0', 'secondary_in':
> +'compare1', 'outdev': 'compare_out0', 'iothread': 'iothread1' } } }
> +
> +{'execute': 'migrate-set-capabilities', 'arguments':{ 'capabilities': [
> +{'capability': 'x-colo', 'state': true } ] } }
> +{'execute': 'migrate', 'arguments':{ 'uri': 'tcp:127.0.0.1:9998' } }
> 
>  == TODO ==
> -1. Support continuous VM replication.
> -2. Support shared storage.
> -3. Develop the heartbeat part.
> -4. Reduce checkpoint VM’s downtime while doing checkpoint.
> +1. Support shared storage.
> +2. Develop the heartbeat part.
> +3. Reduce checkpoint VM’s downtime while doing checkpoint.
> --
> 2.20.1

Re: [Qemu-devel] [PATCH v2 3/3] Update Documentation

[Lukas Straub]

On Mon, 2 Sep 2019 12:17:43 +0000
"Zhang, Chen" <chen.zhang@intel.com> wrote:

> Hi Lukas,
> 
> Please address Markus's comments change this patch's name in next version.
> And I wrote some comments behind, please check it.
> Firstly, Please remove all the trailing whitespace in this patch.
> 
> 
> > -----Original Message-----
> > From: Lukas Straub <lukasstraub2@web.de>
> > Sent: Friday, August 16, 2019 2:49 AM
> > To: qemu-devel <qemu-devel@nongnu.org>
> > Cc: Zhang, Chen <chen.zhang@intel.com>; Jason Wang
> > <jasowang@redhat.com>; Wen Congyang <wencongyang2@huawei.com>;
> > Xie Changlong <xiechanglong.d@gmail.com>
> > Subject: [PATCH v2 3/3] Update Documentation
> > 
> > Document the qemu command-line and qmp commands for continious
> > replication
> > 
> > Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> > ---
> >  docs/COLO-FT.txt | 185 +++++++++++++++++++++++++++++++++++--------
> > ----
> >  1 file changed, 138 insertions(+), 47 deletions(-)
> > 
> > diff --git a/docs/COLO-FT.txt b/docs/COLO-FT.txt index
> > ad24680d13..c08bfbd3a8 100644
> > --- a/docs/COLO-FT.txt
> > +++ b/docs/COLO-FT.txt
> > @@ -145,35 +145,64 @@ The diagram just shows the main qmp command,
> > you can get the detail  in test procedure.
> > 
> >  == Test procedure ==
> > +Note: Here we are running both instances on the same Machine for
> > +testing, change the IP Addresses if you want to run it on two Hosts
> > +
> >  1. Startup qemu
> >  Primary:
> > -# qemu-system-x86_64 -accel kvm -m 2048 -smp 2 -qmp stdio -name
> > primary \
> > -  -device piix3-usb-uhci -vnc :7 \
> > -  -device usb-tablet -netdev tap,id=hn0,vhost=off \
> > -  -device virtio-net-pci,id=net-pci0,netdev=hn0 \
> > -  -drive if=virtio,id=primary-disk0,driver=quorum,read-pattern=fifo,vote-
> > threshold=1,\
> > -         children.0.file.filename=1.raw,\
> > -         children.0.driver=raw -S
> > +# imagefolder="/mnt/vms/colo-test"
> > +
> > +# cp --reflink=auto $imagefolder/primary.qcow2
> > +$imagefolder/primary-copy.qcow2  
> 
> I think you can tell other people here that we need two same disk image before COLO startup.
> The name "primary and primary-copy" will make reader very confused.
> 
> > +
> > +# qemu-system-x86_64 -enable-kvm -cpu qemu64,+kvmclock -m 512 -smp
> > 1 -qmp stdio \
> > +   -vnc :0 -k de -device piix3-usb-uhci -device usb-tablet -name primary \  
> 
> What's mean of the "-k de" ?
Hi,

Oops, thats for German Keyboard layout, will remove.

> > +   -netdev tap,id=hn0,vhost=off,helper=/usr/lib/qemu/qemu-bridge-helper
> > \
> > +   -device rtl8139,id=e0,netdev=hn0 \
> > +   -chardev socket,id=mirror0,host=127.0.0.1,port=9003,server,nowait \
> > +   -chardev socket,id=compare1,host=127.0.0.1,port=9004,server,wait \
> > +   -chardev socket,id=compare0,host=127.0.0.1,port=9001,server,nowait \
> > +   -chardev socket,id=compare0-0,host=127.0.0.1,port=9001 \
> > +   -chardev socket,id=compare_out,host=127.0.0.1,port=9005,server,nowait
> > \
> > +   -chardev socket,id=compare_out0,host=127.0.0.1,port=9005 \
> > +   -object filter-mirror,id=m0,netdev=hn0,queue=tx,outdev=mirror0 \
> > +   -object filter-
> > redirector,netdev=hn0,id=redire0,queue=rx,indev=compare_out \
> > +   -object filter-
> > redirector,netdev=hn0,id=redire1,queue=rx,outdev=compare0 \
> > +   -object iothread,id=iothread1 \
> > +   -object
> > +colo-compare,id=comp0,primary_in=compare0-
> > 0,secondary_in=compare1,\
> > +outdev=compare_out0,iothread=iothread1 \  
> 
> Please keep the space style.
> 
> > +   -drive
> > +if=ide,id=colo-disk0,driver=quorum,read-pattern=fifo,vote-threshold=1,\
> > +children.0.file.filename=$imagefolder/primary.qcow2,children.0.driver=q
> > +cow2 -S
> > +
> >  Secondary:
> > -# qemu-system-x86_64 -accel kvm -m 2048 -smp 2 -qmp stdio -name
> > secondary \
> > -  -device piix3-usb-uhci -vnc :7 \
> > -  -device usb-tablet -netdev tap,id=hn0,vhost=off \
> > -  -device virtio-net-pci,id=net-pci0,netdev=hn0 \
> > -  -drive if=none,id=secondary-disk0,file.filename=1.raw,driver=raw,node-
> > name=node0 \
> > -  -drive if=virtio,id=active-disk0,driver=replication,mode=secondary,\
> > -         file.driver=qcow2,top-id=active-disk0,\
> > -         file.file.filename=/mnt/ramfs/active_disk.img,\
> > -         file.backing.driver=qcow2,\
> > -         file.backing.file.filename=/mnt/ramfs/hidden_disk.img,\
> > -         file.backing.backing=secondary-disk0 \
> > -  -incoming tcp:0:8888
> > +# imagefolder="/mnt/vms/colo-test"
> > +
> > +# qemu-img create -f qcow2 $imagefolder/secondary-active.qcow2 10G
> > +
> > +# qemu-img create -f qcow2 $imagefolder/secondary-hidden.qcow2 10G
> > +
> > +# qemu-system-x86_64 -enable-kvm -cpu qemu64,+kvmclock -m 512 -smp
> > 1 -qmp stdio \
> > +   -vnc :1 -k de -device piix3-usb-uhci -device usb-tablet -name secondary \
> > +   -netdev tap,id=hn0,vhost=off,helper=/usr/lib/qemu/qemu-bridge-helper
> > \
> > +   -device rtl8139,id=e0,netdev=hn0 \
> > +   -chardev socket,id=red0,host=127.0.0.1,port=9003,reconnect=1 \
> > +   -chardev socket,id=red1,host=127.0.0.1,port=9004,reconnect=1 \
> > +   -object filter-redirector,id=f1,netdev=hn0,queue=tx,indev=red0 \
> > +   -object filter-redirector,id=f2,netdev=hn0,queue=rx,outdev=red1 \
> > +   -object filter-rewriter,id=rew0,netdev=hn0,queue=all \
> > +   -drive if=none,id=parent0,file.filename=$imagefolder/primary-
> > copy.qcow2,driver=qcow2 \
> > +   -drive
> > +if=none,id=childs0,driver=replication,mode=secondary,file.driver=qcow2,
> > +\
> > +top-id=childs0,file.file.filename=$imagefolder/secondary-active.qcow2,\
> > +file.backing.driver=qcow2,file.backing.file.filename=$imagefolder/secon
> > +dary-hidden.qcow2,\
> > +file.backing.backing=parent0 \  
> 
> Please keep the space style.

The Idea here is that this can simply be copy-pasted into a Terminal and it will just 
work without having to edit it manually.

> > +   -drive
> > +if=ide,id=colo-disk0,driver=quorum,read-pattern=fifo,vote-threshold=1,\
> > +children.0.file=childs0,children.0.driver=raw \
> > +   -incoming tcp:0:9998
> > +  
> 
> Here, you changed the original block design, can you explain the reasons why we need to do this?
> Please draw a ASCII diagram to describe COLO block detail.

will do.

> > 
> >  2. On Secondary VM's QEMU monitor, issue command
> > {'execute':'qmp_capabilities'} -{ 'execute': 'nbd-server-start',
> > -  'arguments': {'addr': {'type': 'inet', 'data': {'host': 'xx.xx.xx.xx', 'port':
> > '8889'} } } -}
> > -{'execute': 'nbd-server-add', 'arguments': {'device': 'secondary-disk0',
> > 'writable': true } }
> > +{'execute': 'nbd-server-start', 'arguments': {'addr': {'type': 'inet',
> > +'data': {'host': '127.0.0.1', 'port': '9999'} } } }
> > +{'execute': 'nbd-server-add', 'arguments': {'device': 'parent0',
> > +'writable': true } }
> > 
> >  Note:
> >    a. The qmp command nbd-server-start and nbd-server-add must be run
> > @@ -184,12 +213,10 @@ Note:
> > 
> >  3. On Primary VM's QEMU monitor, issue command:
> >  {'execute':'qmp_capabilities'}
> > -{ 'execute': 'human-monitor-command',
> > -  'arguments': {'command-line': 'drive_add -n buddy
> > driver=replication,mode=primary,file.driver=nbd,file.host=xx.xx.xx.xx,file.p
> > ort=8889,file.export=secondary-disk0,node-name=nbd_client0'}}
> > -{ 'execute':'x-blockdev-change', 'arguments':{'parent': 'primary-disk0',
> > 'node': 'nbd_client0' } } -{ 'execute': 'migrate-set-capabilities',
> > -      'arguments': {'capabilities': [ {'capability': 'x-colo', 'state': true } ] } }
> > -{ 'execute': 'migrate', 'arguments': {'uri': 'tcp:xx.xx.xx.xx:8888' } }
> > +{'execute': 'human-monitor-command', 'arguments': {'command-line':
> > +'drive_add -n buddy
> > +driver=replication,mode=primary,file.driver=nbd,file.host=127.0.0.1,fil
> > +e.port=9999,file.export=parent0,node-name=replication0'}}
> > +{'execute': 'x-blockdev-change', 'arguments':{'parent': 'colo-disk0',
> > +'node': 'replication0' } }
> > +{'execute': 'migrate-set-capabilities', 'arguments': {'capabilities': [
> > +{'capability': 'x-colo', 'state': true } ] } }
> > +{'execute': 'migrate', 'arguments': {'uri': 'tcp:127.0.0.1:9998' } }
> > 
> >    Note:
> >    a. There should be only one NBD Client for each primary disk.
> > @@ -199,27 +226,91 @@ Note:
> > 
> >  4. After the above steps, you will see, whenever you make changes to PVM,
> > SVM will be synced.
> >  You can issue command '{ "execute": "migrate-set-parameters" ,
> > "arguments":{ "x-checkpoint-delay": 2000 } }'
> > -to change the checkpoint period time
> > +to change the idle checkpoint period time
> > 
> >  5. Failover test
> > -You can kill Primary VM and run 'x_colo_lost_heartbeat' in Secondary VM's -
> > monitor at the same time, then SVM will failover and client will not detect
> > this -change.
> > +You can kill one of the VMs and Failover on the surviving VM:
> > 
> > -Before issuing '{ "execute": "x-colo-lost-heartbeat" }' command, we have to
> > -issue block related command to stop block replication.
> > -Primary:
> > -  Remove the nbd child from the quorum:
> > -  { 'execute': 'x-blockdev-change', 'arguments': {'parent': 'colo-disk0', 'child':
> > 'children.1'}}
> > -  { 'execute': 'human-monitor-command','arguments': {'command-line':
> > 'drive_del blk-buddy0'}}
> > -  Note: there is no qmp command to remove the blockdev now
> > +== Primary Failover ==
> > +The Secondary died, resume on the Primary
> > 
> > -Secondary:
> > -  The primary host is down, so we should do the following thing:
> > -  { 'execute': 'nbd-server-stop' }
> > +{'execute': 'x-blockdev-change', 'arguments':{ 'parent': 'colo-disk0',
> > +'child': 'children.1'} }
> > +{'execute': 'human-monitor-command', 'arguments':{ 'command-line':
> > +'drive_del replication0' } }
> > +{'execute': 'object-del', 'arguments':{ 'id': 'comp0' } }
> > +{'execute': 'object-del', 'arguments':{ 'id': 'iothread1' } }
> > +{'execute': 'object-del', 'arguments':{ 'id': 'm0' } }
> > +{'execute': 'object-del', 'arguments':{ 'id': 'redire0' } }
> > +{'execute': 'object-del', 'arguments':{ 'id': 'redire1' } }
> > +{'execute': 'x-colo-lost-heartbeat' }
> > +
> > +== Secondary Failover ==
> > +The Primary died, resume on the Secondary and prepare to become the
> > new
> > +Primary
> > +
> > +{'execute': 'nbd-server-stop'}
> > +{'execute': 'x-colo-lost-heartbeat'}
> > +
> > +{'execute': 'object-del', 'arguments':{ 'id': 'f2' } }
> > +{'execute': 'object-del', 'arguments':{ 'id': 'f1' } }
> > +{'execute': 'chardev-remove', 'arguments':{ 'id': 'red1' } }
> > +{'execute': 'chardev-remove', 'arguments':{ 'id': 'red0' } }
> > +
> > +{'execute': 'chardev-add', 'arguments':{ 'id': 'mirror0', 'backend':
> > +{'type': 'socket', 'data': {'addr': { 'type': 'inet', 'data': { 'host':
> > +'127.0.0.1', 'port': '9003' } }, 'server': true } } } }
> > +{'execute': 'chardev-add', 'arguments':{ 'id': 'compare1', 'backend':
> > +{'type': 'socket', 'data': {'addr': { 'type': 'inet', 'data': { 'host':
> > +'127.0.0.1', 'port': '9004' } }, 'server': true } } } }
> > +{'execute': 'chardev-add', 'arguments':{ 'id': 'compare0', 'backend':
> > +{'type': 'socket', 'data': {'addr': { 'type': 'inet', 'data': { 'host':
> > +'127.0.0.1', 'port': '9001' } }, 'server': true } } } }
> > +{'execute': 'chardev-add', 'arguments':{ 'id': 'compare0-0', 'backend':
> > +{'type': 'socket', 'data': {'addr': { 'type': 'inet', 'data': { 'host':
> > +'127.0.0.1', 'port': '9001' } }, 'server': false } } } }
> > +{'execute': 'chardev-add', 'arguments':{ 'id': 'compare_out',
> > +'backend': {'type': 'socket', 'data': {'addr': { 'type': 'inet',
> > +'data': { 'host': '127.0.0.1', 'port': '9005' } }, 'server': true } } }
> > +}
> > +{'execute': 'chardev-add', 'arguments':{ 'id': 'compare_out0',
> > +'backend': {'type': 'socket', 'data': {'addr': { 'type': 'inet',
> > +'data': { 'host': '127.0.0.1', 'port': '9005' } }, 'server': false } }
> > +} }
> > +
> > +== Primary resume replication ==
> > +Resume replication after new Secondary is up.
> > +
> > +Start the new Secondary, then:
> > +{'execute': 'drive-mirror', 'arguments':{ 'device': 'colo-disk0',
> > +'job-id': 'resync', 'target': 'nbd://127.0.0.1:9999/parent0', 'mode':
> > +'existing', 'format': 'raw', 'sync': 'full'} }  
> 
> I think the original secondary node has stopped the nbd server in failover:
> > +{'execute': 'nbd-server-stop'}
> > +{'execute': 'x-colo-lost-heartbeat'}  
> 
> Why you can connect to it?

Oh, I meant it differently. You execute this only if you killed the Secondary.

Thats what I meant with "You can kill one of the VMs and Failover on 
the surviving VM" above.

If you killed the Secondary, then you start a new Secondary (using the Secondary
commandline above, start nbd server and add the disk to the nbd server ) 
then execute the commands here under "Primary resume replication". After that, 
if you want to resume the replication, follow "Primary resume replication"

I will try to clarify this.

> And please add new Secondary node start up script here,
> I think it is different with original secondary node script.
> same issue in resume secondary.

No, Just the initial Primary cmdline is different. The Secondary cmdline is
always the same as above. Except if you run both on the same node, then you
need to choose a different Image than primary-copy.qcow2, I will clarify this.

Regards,
Lukas Straub

> > +
> > +Wait until disk is synced, then:
> > +{'execute': 'stop'}
> > +{'execute': 'block-job-cancel', 'arguments':{ 'device': 'resync'} }
> > +
> > +{'execute': 'human-monitor-command', 'arguments':{ 'command-line':
> > +'drive_add -n buddy
> > +driver=replication,mode=primary,file.driver=nbd,file.host=127.0.0.1,fil
> > +e.port=9999,file.export=parent0,node-name=replication0'}}
> > +{'execute': 'x-blockdev-change', 'arguments':{ 'parent': 'colo-disk0',
> > +'node': 'replication0' } }
> > +
> > +{'execute': 'object-add', 'arguments':{ 'qom-type': 'filter-mirror',
> > +'id': 'm0', 'props': { 'netdev': 'hn0', 'queue': 'tx', 'outdev':
> > +'mirror0' } } }
> > +{'execute': 'object-add', 'arguments':{ 'qom-type':
> > +'filter-redirector', 'id': 'redire0', 'props': { 'netdev': 'hn0',
> > +'queue': 'rx', 'indev': 'compare_out' } } }
> > +{'execute': 'object-add', 'arguments':{ 'qom-type':
> > +'filter-redirector', 'id': 'redire1', 'props': { 'netdev': 'hn0',
> > +'queue': 'rx', 'outdev': 'compare0' } } }
> > +{'execute': 'object-add', 'arguments':{ 'qom-type': 'iothread', 'id':
> > +'iothread1' } }
> > +{'execute': 'object-add', 'arguments':{ 'qom-type': 'colo-compare',
> > +'id': 'comp0', 'props': { 'primary_in': 'compare0-0', 'secondary_in':
> > +'compare1', 'outdev': 'compare_out0', 'iothread': 'iothread1' } } }
> > +
> > +{'execute': 'migrate-set-capabilities', 'arguments':{ 'capabilities': [
> > +{'capability': 'x-colo', 'state': true } ] } }
> > +{'execute': 'migrate', 'arguments':{ 'uri': 'tcp:127.0.0.1:9998' } }
> > +
> > +Note:
> > +If this Primary previously was a Secondary, then we need to insert the
> > +filters before the filter-rewriter by using the
> > +"'insert': 'before', 'position': 'rew0'" Options. See below.
> > +
> > +== Secondary resume replication ==
> > +Become Primary and resume replication after new Secondary is up.
> > +
> > +Start the new Secondary, then:
> > +{'execute': 'drive-mirror', 'arguments':{ 'device': 'colo-disk0',
> > +'job-id': 'resync', 'target': 'nbd://127.0.0.1:9999/parent0', 'mode':
> > +'existing', 'format': 'raw', 'sync': 'full'} }  
> 
> Same issue as primary resume.
> 
> I want to re-tested the resume function but got stuck here.
> 
> Thanks
> Zhang Chen
> 
> > +
> > +Wait until disk is synced, then:
> > +{'execute': 'stop'}
> > +{'execute': 'block-job-cancel', 'arguments':{ 'device': 'resync' } }
> > +
> > +{'execute': 'human-monitor-command', 'arguments':{ 'command-line':
> > +'drive_add -n buddy
> > +driver=replication,mode=primary,file.driver=nbd,file.host=127.0.0.1,fil
> > +e.port=9999,file.export=parent0,node-name=replication0'}}
> > +{'execute': 'x-blockdev-change', 'arguments':{ 'parent': 'colo-disk0',
> > +'node': 'replication0' } }
> > +
> > +{'execute': 'object-add', 'arguments':{ 'qom-type': 'filter-mirror',
> > +'id': 'm0', 'props': { 'insert': 'before', 'position': 'rew0',
> > +'netdev': 'hn0', 'queue': 'tx', 'outdev': 'mirror0' } } }
> > +{'execute': 'object-add', 'arguments':{ 'qom-type':
> > +'filter-redirector', 'id': 'redire0', 'props': { 'insert': 'before',
> > +'position': 'rew0', 'netdev': 'hn0', 'queue': 'rx', 'indev':
> > +'compare_out' } } }
> > +{'execute': 'object-add', 'arguments':{ 'qom-type':
> > +'filter-redirector', 'id': 'redire1', 'props': { 'insert': 'before',
> > +'position': 'rew0', 'netdev': 'hn0', 'queue': 'rx', 'outdev':
> > +'compare0' } } }
> > +{'execute': 'object-add', 'arguments':{ 'qom-type': 'iothread', 'id':
> > +'iothread1' } }
> > +{'execute': 'object-add', 'arguments':{ 'qom-type': 'colo-compare',
> > +'id': 'comp0', 'props': { 'primary_in': 'compare0-0', 'secondary_in':
> > +'compare1', 'outdev': 'compare_out0', 'iothread': 'iothread1' } } }
> > +
> > +{'execute': 'migrate-set-capabilities', 'arguments':{ 'capabilities': [
> > +{'capability': 'x-colo', 'state': true } ] } }
> > +{'execute': 'migrate', 'arguments':{ 'uri': 'tcp:127.0.0.1:9998' } }
> > 
> >  == TODO ==
> > -1. Support continuous VM replication.
> > -2. Support shared storage.
> > -3. Develop the heartbeat part.
> > -4. Reduce checkpoint VM’s downtime while doing checkpoint.
> > +1. Support shared storage.
> > +2. Develop the heartbeat part.
> > +3. Reduce checkpoint VM’s downtime while doing checkpoint.
> > --
> > 2.20.1  

[Qemu-devel] [PATCH v2 1/3] Replication: Ignore requests after failover

[Lukas Straub]

After failover the Secondary side of replication shouldn't change state, because
it now functions as our primary disk.

In replication_start, replication_do_checkpoint, replication_stop, ignore
the request if current state is BLOCK_REPLICATION_DONE (sucessful failover) or
BLOCK_REPLICATION_FAILOVER (failover in progres i.e. currently merging active
and hidden images into the base image).

Signed-off-by: Lukas Straub <lukasstraub2@web.de>
---
 block/replication.c | 38 +++++++++++++++++++++++++++++++++++---
 1 file changed, 35 insertions(+), 3 deletions(-)

diff --git a/block/replication.c b/block/replication.c
index 3d4dedddfc..97cc65c0cf 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -454,6 +454,17 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode,
     aio_context_acquire(aio_context);
     s = bs->opaque;

+    if (s->stage == BLOCK_REPLICATION_DONE ||
+        s->stage == BLOCK_REPLICATION_FAILOVER) {
+        /*
+         * This case happens when a secondary is promoted to primary.
+         * Ignore the request because the secondary side of replication
+         * doesn't have to do anything anymore.
+         */
+        aio_context_release(aio_context);
+        return;
+    }
+
     if (s->stage != BLOCK_REPLICATION_NONE) {
         error_setg(errp, "Block replication is running or done");
         aio_context_release(aio_context);
@@ -529,8 +540,7 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode,
                    "Block device is in use by internal backup job");

         top_bs = bdrv_lookup_bs(s->top_id, s->top_id, NULL);
-        if (!top_bs || !bdrv_is_root_node(top_bs) ||
-            !check_top_bs(top_bs, bs)) {
+        if (!top_bs || !check_top_bs(top_bs, bs)) {
             error_setg(errp, "No top_bs or it is invalid");
             reopen_backing_file(bs, false, NULL);
             aio_context_release(aio_context);
@@ -577,6 +587,17 @@ static void replication_do_checkpoint(ReplicationState *rs, Error **errp)
     aio_context_acquire(aio_context);
     s = bs->opaque;

+    if (s->stage == BLOCK_REPLICATION_DONE ||
+        s->stage == BLOCK_REPLICATION_FAILOVER) {
+        /*
+         * This case happens when a secondary was promoted to primary.
+         * Ignore the request because the secondary side of replication
+         * doesn't have to do anything anymore.
+         */
+        aio_context_release(aio_context);
+        return;
+    }
+
     if (s->mode == REPLICATION_MODE_SECONDARY) {
         secondary_do_checkpoint(s, errp);
     }
@@ -593,7 +614,7 @@ static void replication_get_error(ReplicationState *rs, Error **errp)
     aio_context_acquire(aio_context);
     s = bs->opaque;

-    if (s->stage != BLOCK_REPLICATION_RUNNING) {
+    if (s->stage == BLOCK_REPLICATION_NONE) {
         error_setg(errp, "Block replication is not running");
         aio_context_release(aio_context);
         return;
@@ -635,6 +656,17 @@ static void replication_stop(ReplicationState *rs, bool failover, Error **errp)
     aio_context_acquire(aio_context);
     s = bs->opaque;

+    if (s->stage == BLOCK_REPLICATION_DONE ||
+        s->stage == BLOCK_REPLICATION_FAILOVER) {
+        /*
+         * This case happens when a secondary was promoted to primary.
+         * Ignore the request because the secondary side of replication
+         * doesn't have to do anything anymore.
+         */
+        aio_context_release(aio_context);
+        return;
+    }
+
     if (s->stage != BLOCK_REPLICATION_RUNNING) {
         error_setg(errp, "Block replication is not running");
         aio_context_release(aio_context);
--
2.20.1

Re: [Qemu-devel] [PATCH v2 1/3] Replication: Ignore requests after failover

[Dr. David Alan Gilbert]

* Lukas Straub (lukasstraub2@web.de) wrote:
> After failover the Secondary side of replication shouldn't change state, because
> it now functions as our primary disk.
> 
> In replication_start, replication_do_checkpoint, replication_stop, ignore
> the request if current state is BLOCK_REPLICATION_DONE (sucessful failover) or
> BLOCK_REPLICATION_FAILOVER (failover in progres i.e. currently merging active
> and hidden images into the base image).
> 
> Signed-off-by: Lukas Straub <lukasstraub2@web.de>

We should add some block people to this one to review it; cc'ing in
Kevin and Max.

Dave

> ---
>  block/replication.c | 38 +++++++++++++++++++++++++++++++++++---
>  1 file changed, 35 insertions(+), 3 deletions(-)
> 
> diff --git a/block/replication.c b/block/replication.c
> index 3d4dedddfc..97cc65c0cf 100644
> --- a/block/replication.c
> +++ b/block/replication.c
> @@ -454,6 +454,17 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode,
>      aio_context_acquire(aio_context);
>      s = bs->opaque;
> 
> +    if (s->stage == BLOCK_REPLICATION_DONE ||
> +        s->stage == BLOCK_REPLICATION_FAILOVER) {
> +        /*
> +         * This case happens when a secondary is promoted to primary.
> +         * Ignore the request because the secondary side of replication
> +         * doesn't have to do anything anymore.
> +         */
> +        aio_context_release(aio_context);
> +        return;
> +    }
> +
>      if (s->stage != BLOCK_REPLICATION_NONE) {
>          error_setg(errp, "Block replication is running or done");
>          aio_context_release(aio_context);
> @@ -529,8 +540,7 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode,
>                     "Block device is in use by internal backup job");
> 
>          top_bs = bdrv_lookup_bs(s->top_id, s->top_id, NULL);
> -        if (!top_bs || !bdrv_is_root_node(top_bs) ||
> -            !check_top_bs(top_bs, bs)) {
> +        if (!top_bs || !check_top_bs(top_bs, bs)) {
>              error_setg(errp, "No top_bs or it is invalid");
>              reopen_backing_file(bs, false, NULL);
>              aio_context_release(aio_context);
> @@ -577,6 +587,17 @@ static void replication_do_checkpoint(ReplicationState *rs, Error **errp)
>      aio_context_acquire(aio_context);
>      s = bs->opaque;
> 
> +    if (s->stage == BLOCK_REPLICATION_DONE ||
> +        s->stage == BLOCK_REPLICATION_FAILOVER) {
> +        /*
> +         * This case happens when a secondary was promoted to primary.
> +         * Ignore the request because the secondary side of replication
> +         * doesn't have to do anything anymore.
> +         */
> +        aio_context_release(aio_context);
> +        return;
> +    }
> +
>      if (s->mode == REPLICATION_MODE_SECONDARY) {
>          secondary_do_checkpoint(s, errp);
>      }
> @@ -593,7 +614,7 @@ static void replication_get_error(ReplicationState *rs, Error **errp)
>      aio_context_acquire(aio_context);
>      s = bs->opaque;
> 
> -    if (s->stage != BLOCK_REPLICATION_RUNNING) {
> +    if (s->stage == BLOCK_REPLICATION_NONE) {
>          error_setg(errp, "Block replication is not running");
>          aio_context_release(aio_context);
>          return;
> @@ -635,6 +656,17 @@ static void replication_stop(ReplicationState *rs, bool failover, Error **errp)
>      aio_context_acquire(aio_context);
>      s = bs->opaque;
> 
> +    if (s->stage == BLOCK_REPLICATION_DONE ||
> +        s->stage == BLOCK_REPLICATION_FAILOVER) {
> +        /*
> +         * This case happens when a secondary was promoted to primary.
> +         * Ignore the request because the secondary side of replication
> +         * doesn't have to do anything anymore.
> +         */
> +        aio_context_release(aio_context);
> +        return;
> +    }
> +
>      if (s->stage != BLOCK_REPLICATION_RUNNING) {
>          error_setg(errp, "Block replication is not running");
>          aio_context_release(aio_context);
> --
> 2.20.1
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK