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

[Lukas Straub]

On Fri, 8 May 2020 06:50:39 +0000
"Zhang, Chen" <address@hidden> wrote:

> > -----Original Message-----
> > From: Lukas Straub <address@hidden>
> > Sent: Friday, May 8, 2020 2:11 PM
> > To: Zhang, Chen <address@hidden>
> > Cc: qemu-devel <address@hidden>; Li Zhijian
> > <address@hidden>; Jason Wang <address@hidden>; Marc-
> > André Lureau <address@hidden>; Paolo Bonzini
> > <address@hidden>
> > 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" <address@hidden> wrote:
> >   
> > > > -----Original Message-----
> > > > From: Lukas Straub <address@hidden>
> > > > Sent: Thursday, May 7, 2020 11:54 PM
> > > > To: Zhang, Chen <address@hidden>
> > > > Cc: qemu-devel <address@hidden>; Li Zhijian
> > > > <address@hidden>; Jason Wang <address@hidden>; Marc-
> > > > André Lureau <address@hidden>; Paolo Bonzini
> > > > <address@hidden>
> > > > 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" <address@hidden> wrote:
> > > >  
> > > > > > -----Original Message-----
> > > > > > From: Lukas Straub <address@hidden>
> > > > > > Sent: Monday, May 4, 2020 6:28 PM
> > > > > > To: qemu-devel <address@hidden>
> > > > > > Cc: Zhang, Chen <address@hidden>; Li Zhijian
> > > > > > <address@hidden>; Jason Wang <address@hidden>;
> > > > > > Marc- André Lureau <address@hidden>; Paolo Bonzini
> > > > > > <address@hidden>
> > > > > > 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. 

Sure, but maybe the user doesn't need (reliable) network on failover. Also 
proper network support with periodic mode can easily be implemented by 
modifying filter-buffer to buffer packets until checkpoint. Being able to use 
colo without colo-compare gives more flexibility to the user.

Regards,
Lukas Straub

> 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 <address@hidden>
> > > > > > ---
> > > > > >  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  
> > > > >  
> > >  
>

pgp7H5Jt9p6I9.pgp
Description: OpenPGP digital signature