Re: [PATCH] migration: Don't assert() if MIGRATE_DATA comes before attaching the agent

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



This is very close to the last iteration I sent, except that you moved
the dead code path my patch had to a more appropriate place ;) ACK,
though would be nice to have a reproducer in the commit log.

Christophe

On Wed, Jul 23, 2014 at 12:16:56AM +0300, Uri Lublin wrote:
> During seamless migration, after switching host, if a client was connected
> during the migration, it will have data to send back to the new
> qemu/spice-server instance. This is handled through MIGRATE_DATA messages.
> SPICE char devices use such MIGRATE_DATA messages to restore their state.
> 
> However, the MIGRATE_DATA message can arrive any time after the new qemu
> instance has started, this can happen before or after the SPICE char
> devices have been created. In order to handle this, if the migrate data
> arrives early, it's stored in reds->agent_state.mig_data, and
> attach_to_red_agent() will restore the agent state as appropriate.
> 
> Unfortunately this does not work as expected, for main
> channel (agent messages).
> If attach_to_red_agent() is called before the MIGRATE_DATA
> message reaches the server, all goes well,
> but if MIGRATE_DATA reaches the server before
> attach_to_red_agent() gets called, then some assert() gets
> triggered in spice_char_device_state_restore():
> 
> ((null):32507): Spice-ERROR **: char_device.c:937:spice_char_device_state_restore: assertion `dev->num_clients == 1 && dev->wait_for_migrate_data' failed
> Thread 3 (Thread 0x7f406b543700 (LWP 32543)):
> Thread 2 (Thread 0x7f40697ff700 (LWP 32586)):
> Thread 1 (Thread 0x7f4079b45a40 (LWP 32507)):
> 
> When restoring state, a client must already be added to the
> spice-char-device.
> What happens is that a client is not being added to the char-device
> when when MIGRATE_DATA arrives first, which leaves both
> dev->num_clients and dev->wait_for_migrate_data value at 0.
> 
> This commit changes the logic in spice_server_char_device_add_interface(),
> such that if there is migrate data pending in reds->agent_state.mig_data
> but no client was added to the spice-char-device yet,
> then first the client is added to the device by calling
> spice_char_device_client_add(), and only then the state is restored.
> 
> This fixes https://bugzilla.redhat.com/show_bug.cgi?id=1035184
> 
> Based-on-a-patch-by: Christophe Fergeau <cfergeau@xxxxxxxxxx>
> ---
>  server/reds.c | 39 ++++++++++++++++++++++++++++-----------
>  1 file changed, 28 insertions(+), 11 deletions(-)
> 
> diff --git a/server/reds.c b/server/reds.c
> index 2c437ac..ed142ec 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -1274,6 +1274,7 @@ int reds_handle_migrate_data(MainChannelClient *mcc, SpiceMigrateDataMain *mig_d
>  {
>      VDIPortState *agent_state = &reds->agent_state;
> 
> +    spice_debug("main-channel: got migrate data");
>      /*
>       * Now that the client has switched to the target server, if main_channel
>       * controls the mm-time, we update the client's mm-time.
> @@ -1295,15 +1296,18 @@ int reds_handle_migrate_data(MainChannelClient *mcc, SpiceMigrateDataMain *mig_d
>                      main_channel_push_agent_disconnected(reds->main_channel);
>                      main_channel_push_agent_connected(reds->main_channel);
>                  } else {
> +                    spice_debug("restoring state from mig_data");
>                      return reds_agent_state_restore(mig_data);
>                  }
>              }
>          } else {
>              /* restore agent starte when the agent gets attached */
> +            spice_debug("saving mig_data");
>              spice_assert(agent_state->plug_generation == 0);
>              agent_state->mig_data = spice_memdup(mig_data, size);
>          }
>      } else {
> +        spice_debug("agent was not attached on the source host");
>          if (vdagent) {
>              /* spice_char_device_client_remove disables waiting for migration data */
>              spice_char_device_client_remove(agent_state->base,
> @@ -2857,17 +2861,15 @@ static SpiceCharDeviceState *attach_to_red_agent(SpiceCharDeviceInstance *sin)
>      state->read_filter.discard_all = FALSE;
>      reds->agent_state.plug_generation++;
> 
> -    if (reds->agent_state.mig_data) {
> -        spice_assert(reds->agent_state.plug_generation == 1);
> -        reds_agent_state_restore(reds->agent_state.mig_data);
> -        free(reds->agent_state.mig_data);
> -        reds->agent_state.mig_data = NULL;
> -    } else if (!red_channel_waits_for_migrate_data(&reds->main_channel->base)) {
> -        /* we will assoicate the client with the char device, upon reds_on_main_agent_start,
> -         * in response to MSGC_AGENT_START */
> -        main_channel_push_agent_connected(reds->main_channel);
> -    } else {
> -       spice_debug("waiting for migration data");
> +    if (reds->agent_state.mig_data ||
> +        red_channel_waits_for_migrate_data(&reds->main_channel->base)) {
> +        /* Migration in progress (code is running on the destination host):
> +         * 1.  Add the client to spice char device, if it was not already added.
> +         * 2.a If this (qemu-kvm state load side of migration) happens first
> +         *     then wait for spice migration data to arrive. Otherwise
> +         * 2.b If this happens second ==> we already have spice migrate data
> +         *     then restore state
> +         */
>          if (!spice_char_device_client_exists(reds->agent_state.base, reds_get_client())) {
>              int client_added;
> 
> @@ -2883,9 +2885,24 @@ static SpiceCharDeviceState *attach_to_red_agent(SpiceCharDeviceInstance *sin)
>                  spice_warning("failed to add client to agent");
>                  reds_disconnect();
>              }
> +        }
> 
> +        if (reds->agent_state.mig_data) {
> +            spice_debug("restoring state from stored migration data");
> +            spice_assert(reds->agent_state.plug_generation == 1);
> +            reds_agent_state_restore(reds->agent_state.mig_data);
> +            free(reds->agent_state.mig_data);
> +            reds->agent_state.mig_data = NULL;
> +        }
> +        else {
> +            spice_debug("waiting for migration data");
>          }
> +    } else {
> +        /* we will associate the client with the char device, upon reds_on_main_agent_start,
> +         * in response to MSGC_AGENT_START */
> +        main_channel_push_agent_connected(reds->main_channel);
>      }
> +
>      return state->base;
>  }
> 
> -- 
> 1.9.3
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/spice-devel

Attachment: pgpwWJIMsYeOA.pgp
Description: PGP signature

_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel

[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]