Re: [PATCH v2] migration: Stop migration immediately in RDMA error paths

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> In multiple places, RDMA errors are handled in a strange way, where it only
> sets qemu_file_set_error() but not stop the migration immediately.
>
> It's not obvious what will happen later if there is already an error.  Make
> all such failures stop migration immediately.
>
> Cc: Zhijian Li (Fujitsu) <lizhijian@fujitsu.com>
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: Juan Quintela <quintela@redhat.com>
> Cc: Fabiano Rosas <farosas@suse.de>
> Reported-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>
> v2:
> - One more line squashed into to fix the build error...  Please ignore v1,
>   sorry for the noise.
>
> This patch is based on Thomas's patch:
>
> [PATCH v2] migration/ram: Fix compilation with -Wshadow=local
> 20231024092220.55305-1-thuth@redhat.com">https://lore.kernel.org/r/20231024092220.55305-1-thuth@redhat.com
>
> Above patch should have been queued by both Markus and Juan.
> ---
>  migration/ram.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/migration/ram.c b/migration/ram.c
> index 212add4481..1473bb593a 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -3034,11 +3034,13 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
>      ret = rdma_registration_start(f, RAM_CONTROL_SETUP);
>      if (ret < 0) {
>          qemu_file_set_error(f, ret);
> +        return ret;

I agree

>      }
>  
>      ret = rdma_registration_stop(f, RAM_CONTROL_SETUP);
>      if (ret < 0) {
>          qemu_file_set_error(f, ret);
> +        return ret;

I agree

>      }
>  
>      migration_ops = g_malloc0(sizeof(MigrationOps));
> @@ -3104,6 +3106,7 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
>          ret = rdma_registration_start(f, RAM_CONTROL_ROUND);
>          if (ret < 0) {
>              qemu_file_set_error(f, ret);
> +            goto out;

Seems sensible

>          }
>  
>          t0 = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
> @@ -3208,8 +3211,6 @@ static int ram_save_complete(QEMUFile *f, void *opaque)
>      rs->last_stage = !migration_in_colo_state();
>  
>      WITH_RCU_READ_LOCK_GUARD() {
> -        int rdma_reg_ret;
> -
>          if (!migration_in_postcopy()) {
>              migration_bitmap_sync_precopy(rs, true);
>          }
> @@ -3217,6 +3218,7 @@ static int ram_save_complete(QEMUFile *f, void *opaque)
>          ret = rdma_registration_start(f, RAM_CONTROL_FINISH);
>          if (ret < 0) {
>              qemu_file_set_error(f, ret);
> +            break;

Please
              return ret;


We can do exactly the same with pages < 0.

>          }
>  
>          /* try transferring iterative blocks of memory */
> @@ -3240,9 +3242,10 @@ static int ram_save_complete(QEMUFile *f, void *opaque)
>  
>          ram_flush_compressed_data(rs);
>  
> -        rdma_reg_ret = rdma_registration_stop(f, RAM_CONTROL_FINISH);
> -        if (rdma_reg_ret < 0) {
> -            qemu_file_set_error(f, rdma_reg_ret);
> +        ret = rdma_registration_stop(f, RAM_CONTROL_FINISH);
> +        if (ret < 0) {
> +            qemu_file_set_error(f, ret);
> +            break;
>          }
>      }

And if we return here, we can just drop the:

    if (ret < 0) {
        return ret;
    }


At the ext of the loop.

Later, Juan.