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

[Fabiano Rosas <farosas@suse.de>]

Cédric Le Goater <clg@redhat.com> writes:

> On 3/12/24 14:34, Cédric Le Goater wrote:
>> On 3/12/24 13:32, Cédric Le Goater wrote:
>>> On 3/11/24 20:03, Fabiano Rosas wrote:
>>>> Cédric Le Goater <clg@redhat.com> writes:
>>>>
>>>>> On 3/8/24 15:36, Fabiano Rosas wrote:
>>>>>> Cédric Le Goater <clg@redhat.com> writes:
>>>>>>
>>>>>>> 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;
>>>>>>> +     }
>>>>>>> +
>>>>>>>        qemu_savevm_wait_unplug(s, MIGRATION_STATUS_SETUP,
>>>>>>>                                   MIGRATION_STATUS_ACTIVE);
>>>>>>
>>>>>> This^ should be before the new block it seems:
>>>>>>
>>>>>> GOOD:
>>>>>> migrate_set_state new state setup
>>>>>> migrate_set_state new state wait-unplug
>>>>>> migrate_fd_cancel
>>>>>> migrate_set_state new state cancelling
>>>>>> migrate_fd_cleanup
>>>>>> migrate_set_state new state cancelled
>>>>>> migrate_fd_cancel
>>>>>> ok 1 /x86_64/failover-virtio-net/migrate/abort/wait-unplug
>>>>>>
>>>>>> BAD:
>>>>>> migrate_set_state new state setup
>>>>>> migrate_fd_cancel
>>>>>> migrate_set_state new state cancelling
>>>>>> migrate_fd_cleanup
>>>>>> migrate_set_state new state cancelled
>>>>>> 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")
>>>>>>
>>>>>> Otherwise migration_iteration_finish() will schedule the cleanup BH and
>>>>>> that will run concurrently with migrate_fd_cancel() issued by the test
>>>>>> and bad things happens.
>>>>>
>>>>> This hack makes things work :
>>>>>
>>>>> @@ -3452,6 +3452,9 @@ static void *migration_thread(void *opaq
>>>>>            qemu_savevm_send_colo_enable(s->to_dst_file);
>>>>>        }
>>>>> +    qemu_savevm_wait_unplug(s, MIGRATION_STATUS_SETUP,
>>>>> +                            MIGRATION_STATUS_SETUP);
>>>>> +
>>>>
>>>> Why move it all the way up here? Has moving the wait_unplug before the
>>>> 'if (ret)' block not worked for you?
>>>
>>> We could be sleeping while holding the BQL. It looked wrong.
>> 
>> Sorry wrong answer. Yes I can try moving it before the 'if (ret)' block.
>> I can reproduce easily with an x86 guest running on PPC64.
>
> That works just the same.
>
> Peter, Fabiano,
>
> What would you prefer  ?
>
> 1. move qemu_savevm_wait_unplug() before qemu_savevm_state_setup(),
>     means one new patch.

Is there a point to this except "because we can"? Honest question, I
might have missed the motivation.

Also a couple of points:

- The current version of this proposal seems it will lose the transition
from SETUP->ACTIVE no? As in, after qemu_savevm_state_setup, there's
nothing changing the state to ACTIVE anymore.

- You also need to change the bg migration path.

>
> 2. leave qemu_savevm_wait_unplug() after qemu_savevm_state_setup()
>     and handle state_setup() errors after waiting. means an update
>     of this patch.

I vote for this. This failover feature is a pretty complex one, let's
not risk changing the behavior for no good reason. Just look at the
amount of head-banging going on in these threads:

https://patchwork.ozlabs.org/project/qemu-devel/cover/20181025140631.634922-1-sameeh@daynix.com/
https://www.mail-archive.com/qemu-devel@nongnu.org/msg609296.html

>
>
> Thanks,
>
> C.
>
>
>