[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2] migration: Stop migration immediately in RDMA error paths
From: |
Peter Xu |
Subject: |
Re: [PATCH v2] migration: Stop migration immediately in RDMA error paths |
Date: |
Tue, 24 Oct 2023 12:31:55 -0400 |
On Tue, Oct 24, 2023 at 06:16:27PM +0200, Juan Quintela wrote:
> 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.
IIUC that'll be the same as this patch, but sure thing I'll prepare a v3.
Thanks,
--
Peter Xu