Re: [PATCH v4 10/25] migration: Add Error** argument to qemu_savevm_state_setup()

[Peter Xu]

On Mon, Mar 11, 2024 at 07:12:11PM +0100, Cédric Le Goater wrote:
> On 3/8/24 15:17, Peter Xu wrote:
> > On Fri, Mar 08, 2024 at 02:55:30PM +0100, Cédric Le Goater wrote:
> > > On 3/8/24 14:39, Cédric Le Goater wrote:
> > > > On 3/8/24 14:14, Cédric Le Goater wrote:
> > > > > On 3/8/24 13:56, Peter Xu wrote:
> > > > > > On Wed, Mar 06, 2024 at 02:34:25PM +0100, Cédric Le Goater wrote:
> > > > > > > This prepares ground for the changes coming next which add an 
> > > > > > > Error**
> > > > > > > argument to the .save_setup() handler. Callers of 
> > > > > > > qemu_savevm_state_setup()
> > > > > > > now handle the error and fail earlier setting the migration state 
> > > > > > > from
> > > > > > > MIGRATION_STATUS_SETUP to MIGRATION_STATUS_FAILED.
> > > > > > > 
> > > > > > > In qemu_savevm_state(), move the cleanup to preserve the error
> > > > > > > reported by .save_setup() handlers.
> > > > > > > 
> > > > > > > Since the previous behavior was to ignore errors at this step of
> > > > > > > migration, this change should be examined closely to check that
> > > > > > > cleanups are still correctly done.
> > > > > > > 
> > > > > > > Signed-off-by: Cédric Le Goater <clg@redhat.com>
> > > > > > > ---
> > > > > > > 
> > > > > > >    Changes in v4:
> > > > > > >    - Merged cleanup change in qemu_savevm_state()
> > > > > > >    Changes in v3:
> > > > > > >    - Set migration state to MIGRATION_STATUS_FAILED
> > > > > > >    - Fixed error handling to be done under lock in 
> > > > > > > bg_migration_thread()
> > > > > > >    - Made sure an error is always set in case of failure in
> > > > > > >      qemu_savevm_state_setup()
> > > > > > >    migration/savevm.h    |  2 +-
> > > > > > >    migration/migration.c | 27 ++++++++++++++++++++++++---
> > > > > > >    migration/savevm.c    | 26 +++++++++++++++-----------
> > > > > > >    3 files changed, 40 insertions(+), 15 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/migration/savevm.h b/migration/savevm.h
> > > > > > > index 
> > > > > > > 74669733dd63a080b765866c703234a5c4939223..9ec96a995c93a42aad621595f0ed58596c532328
> > > > > > >  100644
> > > > > > > --- a/migration/savevm.h
> > > > > > > +++ b/migration/savevm.h
> > > > > > > @@ -32,7 +32,7 @@
> > > > > > >    bool qemu_savevm_state_blocked(Error **errp);
> > > > > > >    void qemu_savevm_non_migratable_list(strList **reasons);
> > > > > > >    int qemu_savevm_state_prepare(Error **errp);
> > > > > > > -void qemu_savevm_state_setup(QEMUFile *f);
> > > > > > > +int qemu_savevm_state_setup(QEMUFile *f, Error **errp);
> > > > > > >    bool qemu_savevm_state_guest_unplug_pending(void);
> > > > > > >    int qemu_savevm_state_resume_prepare(MigrationState *s);
> > > > > > >    void qemu_savevm_state_header(QEMUFile *f);
> > > > > > > diff --git a/migration/migration.c b/migration/migration.c
> > > > > > > index 
> > > > > > > a49fcd53ee19df1ce0182bc99d7e064968f0317b..6d1544224e96f5edfe56939a9c8395d88ef29581
> > > > > > >  100644
> > > > > > > --- a/migration/migration.c
> > > > > > > +++ b/migration/migration.c
> > > > > > > @@ -3408,6 +3408,8 @@ static void *migration_thread(void *opaque)
> > > > > > >        int64_t setup_start = qemu_clock_get_ms(QEMU_CLOCK_HOST);
> > > > > > >        MigThrError thr_error;
> > > > > > >        bool urgent = false;
> > > > > > > +    Error *local_err = NULL;
> > > > > > > +    int ret;
> > > > > > >        thread = migration_threads_add("live_migration", 
> > > > > > > qemu_get_thread_id());
> > > > > > > @@ -3451,9 +3453,17 @@ static void *migration_thread(void *opaque)
> > > > > > >        }
> > > > > > >        bql_lock();
> > > > > > > -    qemu_savevm_state_setup(s->to_dst_file);
> > > > > > > +    ret = qemu_savevm_state_setup(s->to_dst_file, &local_err);
> > > > > > >        bql_unlock();
> > > > > > > +    if (ret) {
> > > > > > > +        migrate_set_error(s, local_err);
> > > > > > > +        error_free(local_err);
> > > > > > > +        migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
> > > > > > > +                          MIGRATION_STATUS_FAILED);
> > > > > > > +        goto out;
> > > > > > > +     }
> > > > > > 
> > > > > > There's a small indent issue, I can fix it.
> > > > > 
> > > > > checkpatch did report anything.
> > > > > 
> > > > > > 
> > > > > > The bigger problem is I _think_ this will trigger a ci failure in 
> > > > > > the
> > > > > > virtio-net-failover test:
> > > > > > 
> > > > > > ▶ 121/464 
> > > > > > ERROR:../tests/qtest/virtio-net-failover.c:1203:test_migrate_abort_wait_unplug:
> > > > > >  assertion failed (status == "cancelling"): ("cancelled" == 
> > > > > > "cancelling") ERROR
> > > > > > 121/464 qemu:qtest+qtest-x86_64 / qtest-x86_64/virtio-net-failover  
> > > > > >   ERROR            4.77s   killed by signal 6 SIGABRT
> > > > > > > > > PYTHON=/builds/peterx/qemu/build/pyvenv/bin/python3.8 
> > > > > > > > > G_TEST_DBUS_DAEMON=/builds/peterx/qemu/tests/dbus-vmstate-daemon.sh
> > > > > > > > >  MALLOC_PERTURB_=161 QTEST_QEMU_IMG=./qemu-img 
> > > > > > > > > QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon
> > > > > > > > >  QTEST_QEMU_BINARY=./qemu-system-x86_64 
> > > > > > > > > /builds/peterx/qemu/build/tests/qtest/virtio-net-failover 
> > > > > > > > > --tap -k
> > > > > > ――――――――――――――――――――――――――――――――――――― ✀  
> > > > > > ―――――――――――――――――――――――――――――――――――――
> > > > > > stderr:
> > > > > > qemu-system-x86_64: ram_save_setup failed: Input/output error
> > > > > > **
> > > > > > ERROR:../tests/qtest/virtio-net-failover.c:1203:test_migrate_abort_wait_unplug:
> > > > > >  assertion failed (status == "cancelling"): ("cancelled" == 
> > > > > > "cancelling")
> > > > > > (test program exited with status code -6)
> > > > > > ――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――
> > > > > > 
> > > > > > I am not familiar enough with the failover code, and may not have 
> > > > > > time
> > > > > > today to follow this up, copy Laurent.  Cedric, if you have time, 
> > > > > > please
> > > > > > have a look.
> > > > > 
> > > > > 
> > > > > Sure. Weird because I usually run make check on x86_64, s390x, ppc64 
> > > > > and
> > > > > aarch64. Let me check again.
> > > > 
> > > > I see one timeout error on s390x but not always. See below. It occurs 
> > > > with
> > > > or without this patchset. the other x86_64, ppc64 arches run fine (a 
> > > > part
> > > > from one io  test failing from time to time)
> > > 
> > > Ah ! I got this once on aarch64 :
> > > 
> > >   161/486 
> > > ERROR:../tests/qtest/virtio-net-failover.c:1222:test_migrate_abort_wait_unplug:
> > >  'device' should not be NULL ERROR
> > > 161/486 qemu:qtest+qtest-x86_64 / qtest-x86_64/virtio-net-failover        
> > >           ERROR            5.98s   killed by signal 6 SIGABRT
> > > > > > G_TEST_DBUS_DAEMON=/home/legoater/work/qemu/qemu.git/tests/dbus-vmstate-daemon.sh
> > > > > >  MALLOC_PERTURB_=119 QTEST_QEMU_BINARY=./qemu-system-x86_64 
> > > > > > QTEST_QEMU_IMG=./qemu-img 
> > > > > > PYTHON=/home/legoater/work/qemu/qemu.git/build/pyvenv/bin/python3 
> > > > > > QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon
> > > > > >  
> > > > > > /home/legoater/work/qemu/qemu.git/build/tests/qtest/virtio-net-failover
> > > > > >  --tap -k
> > > ―――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――― 
> > > ✀  
> > > ―――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――
> > > stderr:
> > > qemu-system-x86_64: ram_save_setup failed: Input/output error
> > > **
> > > ERROR:../tests/qtest/virtio-net-failover.c:1222:test_migrate_abort_wait_unplug:
> > >  'device' should not be NULL
> > > 
> > > (test program exited with status code -6)
> > > ―――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――
> > 
> > Hmm, this one seems different..
> > 
> > > 
> > > I couldn't reproduce yet :/
> > 
> > I never reproduced it locally on x86, and my failure is always at checking
> > "cancelling" v.s. "cancelled" rather than the NULL check.  It's much easier
> > to trigger on CI in check-system-centos (I don't know why centos..):
> > 
> > https://gitlab.com/peterx/qemu/-/jobs/6351020546
> > 
> > I think at least for the error I hit, the problem is the failover test will
> > cancel the migration, but if it cancels too fast and during setup now it
> > can already fail it (while it won't fail before when we ignore
> > qemu_savevm_state_setup() errors), and I think it'll skip:
> > 
> >      qemu_savevm_wait_unplug(s, MIGRATION_STATUS_SETUP,
> >                                 MIGRATION_STATUS_ACTIVE);
> > 
> > It seems the test wants the "cancelling" to hold until later:
> > 
> >      /* while the card is not ejected, we must be in "cancelling" state */
> >      ret = migrate_status(qts);
> > 
> >      status = qdict_get_str(ret, "status");
> >      g_assert_cmpstr(status, ==, "cancelling");
> >      qobject_unref(ret);
> > 
> >      /* OS unplugs the cards, QEMU can move from wait-unplug state */
> >      qtest_outl(qts, ACPI_PCIHP_ADDR_ICH9 + PCI_EJ_BASE, 1);
> > 
> > Again, since I'll need to read the failover code, not much I can tell.
> > Laurent might have a clue.
> 
> I guess we need to fix the test to handle failures and this looks
> like a complex task.

It may not be as complicated, but that'll need some reviews outside
migration people, and someone will need to post a formal patch.  That'll
definitely take some time.

We can keep working on this issue during the 9.0-rc if you want.  Our
current plan for migration (at least what I have in mind in this
release... Fabiano will take over 9.1 release pulls, so he has the freedom
to change this) is we will allow patches to be queued even during RCs,
while I'll prepare two trees, one for -next and one for -stable, then -next
candidates will only be included in the first 9.1 pull, -stable for RC
pulls if necessary.

Thanks,

-- 
Peter Xu