RE: [PATCH v4 5/6] net/colo-compare.c, softmmu/vl.c: Check that colo-compare is active

["Zhang, Chen" <chen.zhang@intel.com>]

> -----Original Message-----
> From: Lukas Straub <lukasstraub2@web.de>
> Sent: Friday, May 8, 2020 2:11 PM
> To: Zhang, Chen <chen.zhang@intel.com>
> Cc: qemu-devel <qemu-devel@nongnu.org>; Li Zhijian
> <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; Marc-
> André Lureau <marcandre.lureau@redhat.com>; Paolo Bonzini
> <pbonzini@redhat.com>
> Subject: Re: [PATCH v4 5/6] net/colo-compare.c, softmmu/vl.c: Check that
> colo-compare is active
> 
> On Fri, 8 May 2020 02:26:21 +0000
> "Zhang, Chen" <chen.zhang@intel.com> wrote:
> 
> > > -----Original Message-----
> > > From: Lukas Straub <lukasstraub2@web.de>
> > > Sent: Thursday, May 7, 2020 11:54 PM
> > > To: Zhang, Chen <chen.zhang@intel.com>
> > > Cc: qemu-devel <qemu-devel@nongnu.org>; Li Zhijian
> > > <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; Marc-
> > > André Lureau <marcandre.lureau@redhat.com>; Paolo Bonzini
> > > <pbonzini@redhat.com>
> > > Subject: Re: [PATCH v4 5/6] net/colo-compare.c, softmmu/vl.c: Check
> > > that colo-compare is active
> > >
> > > On Thu, 7 May 2020 11:38:04 +0000
> > > "Zhang, Chen" <chen.zhang@intel.com> wrote:
> > >
> > > > > -----Original Message-----
> > > > > From: Lukas Straub <lukasstraub2@web.de>
> > > > > Sent: Monday, May 4, 2020 6:28 PM
> > > > > To: qemu-devel <qemu-devel@nongnu.org>
> > > > > Cc: Zhang, Chen <chen.zhang@intel.com>; Li Zhijian
> > > > > <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>;
> > > > > Marc- André Lureau <marcandre.lureau@redhat.com>; Paolo Bonzini
> > > > > <pbonzini@redhat.com>
> > > > > Subject: [PATCH v4 5/6] net/colo-compare.c, softmmu/vl.c: Check
> > > > > that
> > > > > colo- compare is active
> > > > >
> > > > > If the colo-compare object is removed before failover and a
> > > > > checkpoint happens, qemu crashes because it tries to lock the
> > > > > destroyed event_mtx in colo_notify_compares_event.
> > > > >
> > > > > Fix this by checking if everything is initialized by introducing
> > > > > a new variable colo_compare_active which is protected by a new
> > > > > mutex colo_compare_mutex. The new mutex also protects against
> > > > > concurrent access of the net_compares list and makes sure that
> > > > > colo_notify_compares_event isn't active while we destroy
> > > > > event_mtx and event_complete_cond.
> > > > >
> > > > > With this it also is again possible to use colo without
> > > > > colo-compare (periodic
> > > > > mode) and to use multiple colo-compare for multiple network
> interfaces.
> > > > >
> > > >
> > > > Hi Lukas,
> > > >
> > > > For this case I think we don't need to touch vl.c code, we can
> > > > solve this
> > > issue from another perspective:
> > > > How to remove colo-compare?
> > > > User will use qemu-monitor or QMP command to disable an object, so
> > > > we just need return operation failed When user try to remove
> > > > colo-compare
> > > object while COLO is running.
> > >
> > > Yeah, but that still leaves the other problem that colo can't be
> > > used without colo-compare (qemu crashes then).
> >
> > Yes, the COLO-compare is necessary module in COLO original design.
> > At most cases, user need it do dynamic sync.
> > For rare cases, maybe we can add a new colo-compare parameter to
> bypass all the network workload.
> 
> I think such an parameter would only be a workaround instead of a real
> solution like this patch.

The root problem is why COLO-compare is necessary.
Yes, maybe someone want to use pure periodic synchronization mode,
But it means it will lost all guest network support(without colo-compare/filter-mirror/filter-redirector/filter-rewriter).
The secondary guest just a solid backup for the primary guest, when occur failover the new build stateful connection (like TCP)
will crashed, need userspace to handle this status. It lost the original meaning for COLO FT/HA solution, no need use do HA in application layer.
it looks like normal/remote periodic VM snapshot here. 
Dave or Jason have any comments here? 

Thanks
Zhang Chen

> 
> Regards,
> Lukas Straub
> 
> > Thanks
> > Zhang Chen
> >
> > >
> > > Regards,
> > > Lukas Straub
> > >
> > > > Thanks
> > > > Zhang Chen
> > > >
> > > > > Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> > > > > ---
> > > > >  net/colo-compare.c | 35 +++++++++++++++++++++++++++++------
> > > > >  net/colo-compare.h |  1 +
> > > > >  softmmu/vl.c       |  2 ++
> > > > >  3 files changed, 32 insertions(+), 6 deletions(-)
> > > > >
> > > > > diff --git a/net/colo-compare.c b/net/colo-compare.c index
> > > > > 56db3d3bfc..c7572d75e9 100644
> > > > > --- a/net/colo-compare.c
> > > > > +++ b/net/colo-compare.c
> > > > > @@ -54,6 +54,8 @@ static NotifierList colo_compare_notifiers =
> > > > > #define REGULAR_PACKET_CHECK_MS 3000  #define
> > > DEFAULT_TIME_OUT_MS
> > > > > 3000
> > > > >
> > > > > +static QemuMutex colo_compare_mutex; static bool
> > > > > +colo_compare_active;
> > > > >  static QemuMutex event_mtx;
> > > > >  static QemuCond event_complete_cond;  static int
> > > > > event_unhandled_count; @@ -906,6 +908,12 @@ static void
> > > > > check_old_packet_regular(void *opaque) void
> > > > > colo_notify_compares_event(void *opaque, int event, Error **errp)
> {
> > > > >      CompareState *s;
> > > > > +    qemu_mutex_lock(&colo_compare_mutex);
> > > > > +
> > > > > +    if (!colo_compare_active) {
> > > > > +        qemu_mutex_unlock(&colo_compare_mutex);
> > > > > +        return;
> > > > > +    }
> > > > >
> > > > >      qemu_mutex_lock(&event_mtx);
> > > > >      QTAILQ_FOREACH(s, &net_compares, next) { @@ -919,6 +927,7
> > > > > @@ void colo_notify_compares_event(void *opaque, int event,
> Error **errp)
> > > > >      }
> > > > >
> > > > >      qemu_mutex_unlock(&event_mtx);
> > > > > +    qemu_mutex_unlock(&colo_compare_mutex);
> > > > >  }
> > > > >
> > > > >  static void colo_compare_timer_init(CompareState *s) @@ -1274,7
> > > > > +1283,14 @@ static void colo_compare_complete(UserCreatable *uc,
> > > Error **errp)
> > > > >                             s->vnet_hdr);
> > > > >      }
> > > > >
> > > > > +    qemu_mutex_lock(&colo_compare_mutex);
> > > > > +    if (!colo_compare_active) {
> > > > > +        qemu_mutex_init(&event_mtx);
> > > > > +        qemu_cond_init(&event_complete_cond);
> > > > > +        colo_compare_active = true;
> > > > > +    }
> > > > >      QTAILQ_INSERT_TAIL(&net_compares, s, next);
> > > > > +    qemu_mutex_unlock(&colo_compare_mutex);
> > > > >
> > > > >      s->out_sendco.s = s;
> > > > >      s->out_sendco.chr = &s->chr_out; @@ -1290,9 +1306,6 @@
> > > > > static void colo_compare_complete(UserCreatable
> > > > > *uc, Error **errp)
> > > > >
> > > > >      g_queue_init(&s->conn_list);
> > > > >
> > > > > -    qemu_mutex_init(&event_mtx);
> > > > > -    qemu_cond_init(&event_complete_cond);
> > > > > -
> > > > >      s->connection_track_table =
> > > > > g_hash_table_new_full(connection_key_hash,
> > > > >                                                        connection_key_equal,
> > > > >                                                        g_free,
> > > > > @@
> > > > > -1384,12 +1397,19 @@ static void colo_compare_finalize(Object
> > > > > *obj)
> > > > >
> > > > >      qemu_bh_delete(s->event_bh);
> > > > >
> > > > > +    qemu_mutex_lock(&colo_compare_mutex);
> > > > >      QTAILQ_FOREACH(tmp, &net_compares, next) {
> > > > >          if (tmp == s) {
> > > > >              QTAILQ_REMOVE(&net_compares, s, next);
> > > > >              break;
> > > > >          }
> > > > >      }
> > > > > +    if (QTAILQ_EMPTY(&net_compares)) {
> > > > > +        colo_compare_active = false;
> > > > > +        qemu_mutex_destroy(&event_mtx);
> > > > > +        qemu_cond_destroy(&event_complete_cond);
> > > > > +    }
> > > > > +    qemu_mutex_unlock(&colo_compare_mutex);
> > > > >
> > > > >      AioContext *ctx = iothread_get_aio_context(s->iothread);
> > > > >      aio_context_acquire(ctx);
> > > > > @@ -1413,15 +1433,18 @@ static void colo_compare_finalize(Object
> *obj)
> > > > >          object_unref(OBJECT(s->iothread));
> > > > >      }
> > > > >
> > > > > -    qemu_mutex_destroy(&event_mtx);
> > > > > -    qemu_cond_destroy(&event_complete_cond);
> > > > > -
> > > > >      g_free(s->pri_indev);
> > > > >      g_free(s->sec_indev);
> > > > >      g_free(s->outdev);
> > > > >      g_free(s->notify_dev);
> > > > >  }
> > > > >
> > > > > +void colo_compare_init_globals(void) {
> > > > > +    colo_compare_active = false;
> > > > > +    qemu_mutex_init(&colo_compare_mutex);
> > > > > +}
> > > > > +
> > > > >  static const TypeInfo colo_compare_info = {
> > > > >      .name = TYPE_COLO_COMPARE,
> > > > >      .parent = TYPE_OBJECT,
> > > > > diff --git a/net/colo-compare.h b/net/colo-compare.h index
> > > > > 22ddd512e2..eb483ac586 100644
> > > > > --- a/net/colo-compare.h
> > > > > +++ b/net/colo-compare.h
> > > > > @@ -17,6 +17,7 @@
> > > > >  #ifndef QEMU_COLO_COMPARE_H
> > > > >  #define QEMU_COLO_COMPARE_H
> > > > >
> > > > > +void colo_compare_init_globals(void);
> > > > >  void colo_notify_compares_event(void *opaque, int event, Error
> > > > > **errp); void colo_compare_register_notifier(Notifier *notify);
> > > > > void colo_compare_unregister_notifier(Notifier *notify); diff
> > > > > --git a/softmmu/vl.c b/softmmu/vl.c index 32c0047889..a913ed5469
> > > > > 100644
> > > > > --- a/softmmu/vl.c
> > > > > +++ b/softmmu/vl.c
> > > > > @@ -112,6 +112,7 @@
> > > > >  #include "qapi/qmp/qerror.h"
> > > > >  #include "sysemu/iothread.h"
> > > > >  #include "qemu/guest-random.h"
> > > > > +#include "net/colo-compare.h"
> > > > >
> > > > >  #define MAX_VIRTIO_CONSOLES 1
> > > > >
> > > > > @@ -2906,6 +2907,7 @@ void qemu_init(int argc, char **argv, char
> > > **envp)
> > > > >      precopy_infrastructure_init();
> > > > >      postcopy_infrastructure_init();
> > > > >      monitor_init_globals();
> > > > > +    colo_compare_init_globals();
> > > > >
> > > > >      if (qcrypto_init(&err) < 0) {
> > > > >          error_reportf_err(err, "cannot initialize crypto: ");
> > > > > --
> > > > > 2.20.1
> > > >
> >