Re: [Qemu-devel] [PATCH COLO-Frame v12 32/38] COLO: Split qemu_savevm_state_begin out of checkpoint process

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* zhanghailiang (zhang.zhanghailiang@huawei.com) wrote:
> It is unnecessary to call qemu_savevm_state_begin() in every checkponit process.
> It mainly sets up devices and does the first device state pass. These data will
> not change during the later checkpoint process. So, we split it out of
> colo_do_checkpoint_transaction(), in this way, we can reduce these data
> transferring in the later checkpoint.
> 
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
> ---
>  migration/colo.c | 51 +++++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 37 insertions(+), 14 deletions(-)
> 
> diff --git a/migration/colo.c b/migration/colo.c
> index d253d64..4571359 100644
> --- a/migration/colo.c
> +++ b/migration/colo.c
> @@ -276,15 +276,6 @@ static int colo_do_checkpoint_transaction(MigrationState *s,
>      if (ret < 0) {
>          goto out;
>      }
> -    /* Disable block migration */
> -    s->params.blk = 0;
> -    s->params.shared = 0;
> -    qemu_savevm_state_begin(s->to_dst_file, &s->params);
> -    ret = qemu_file_get_error(s->to_dst_file);
> -    if (ret < 0) {
> -        error_report("save vm state begin error\n");
> -        goto out;
> -    }
>  
>      qemu_mutex_lock_iothread();
>      /* Note: device state is saved into buffer */
> @@ -348,6 +339,21 @@ out:
>      return ret;
>  }
>  
> +static int colo_prepare_before_save(MigrationState *s)
> +{
> +    int ret;
> +    /* Disable block migration */
> +    s->params.blk = 0;
> +    s->params.shared = 0;
> +    qemu_savevm_state_begin(s->to_dst_file, &s->params);
> +    ret = qemu_file_get_error(s->to_dst_file);
> +    if (ret < 0) {
> +        error_report("save vm state begin error\n");

 '\n' again not needed.

> +        return ret;
> +    }
> +    return 0;
> +}
> +
>  static void colo_process_checkpoint(MigrationState *s)
>  {
>      QEMUSizedBuffer *buffer = NULL;
> @@ -363,6 +369,11 @@ static void colo_process_checkpoint(MigrationState *s)
>          goto out;
>      }
>  
> +    ret = colo_prepare_before_save(s);
> +    if (ret < 0) {
> +        goto out;
> +    }
> +
>      /*
>       * Wait for Secondary finish loading vm states and enter COLO
>       * restore.
> @@ -484,6 +495,18 @@ static int colo_wait_handle_cmd(QEMUFile *f, int *checkpoint_request)
>      }
>  }
>  
> +static int colo_prepare_before_load(QEMUFile *f)
> +{
> +    int ret;
> +
> +    ret = qemu_loadvm_state_begin(f);
> +    if (ret < 0) {
> +        error_report("load vm state begin error, ret=%d", ret);
> +        return ret;

You can simplify these returns; remove this line.

> +    }
> +    return 0;

and make this return ret; same in a few places.


Other than those minor issues;

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>


> +}
> +
>  void *colo_process_incoming_thread(void *opaque)
>  {
>      MigrationIncomingState *mis = opaque;
> @@ -522,6 +545,11 @@ void *colo_process_incoming_thread(void *opaque)
>          goto out;
>      }
>  
> +    ret = colo_prepare_before_load(mis->from_src_file);
> +    if (ret < 0) {
> +        goto out;
> +    }
> +
>      ret = colo_put_cmd(mis->to_src_file, COLO_COMMAND_CHECKPOINT_READY);
>      if (ret < 0) {
>          goto out;
> @@ -556,11 +584,6 @@ void *colo_process_incoming_thread(void *opaque)
>              goto out;
>          }
>  
> -        ret = qemu_loadvm_state_begin(mis->from_src_file);
> -        if (ret < 0) {
> -            error_report("load vm state begin error, ret=%d", ret);
> -            goto out;
> -        }
>          ret = qemu_load_ram_state(mis->from_src_file);
>          if (ret < 0) {
>              error_report("load ram state error");
> -- 
> 1.8.3.1
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK