On 11/25/24 7:07 PM, Peter Xu wrote:
> On Mon, Nov 25, 2024 at 11:46:11AM -0300, Fabiano Rosas wrote:
>> Currently a VM that has been target of a migration using
>> late-block-activate will crash at the end of a new migration (with it
>> as source) when releasing ownership of the disks due to the VM having
>> never taken ownership of the disks in the first place.
>>
>> The issue is that late-block-activate expects a qmp_continue command
>> to be issued at some point on the destination VM after the migration
>> finishes. If the user decides to never continue the VM, but instead
>> issue a new migration, then bdrv_activate_all() will never be called
>> and the assert will be reached:
>>
>> bdrv_inactivate_recurse: Assertion `!(bs->open_flags &
>> BDRV_O_INACTIVE)' failed.
>>
>> Fix the issue by checking at the start of migration if the VM is
>> paused and call bdrv_activate_all() before migrating. Even if the
>> late-block-activate capability is not in play or if the VM has been
>> paused manually, there is no harm calling that function again.
>>
>> Signed-off-by: Fabiano Rosas <faro...@suse.de>
>> ---
>> migration/migration.c | 19 +++++++++++++++++++
>> 1 file changed, 19 insertions(+)
>>
>> diff --git a/migration/migration.c b/migration/migration.c
>> index aedf7f0751..26af30137b 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -2029,6 +2029,25 @@ static bool migrate_prepare(MigrationState *s, bool
>> resume, Error **errp)
>> return false;
>> }
>>
>> + /*
>> + * The VM might have been target of a previous migration. If it
>> + * was in the paused state then nothing will have required the
>> + * block layer to be activated. Do it now to ensure this QEMU
>> + * instance owns the disk locks.
>> + */
>> + if (!resume && runstate_check(RUN_STATE_PAUSED)) {
>
> I hope this will cover all the cases that QEMU could overlook.. or I'm not
> sure whether we could invoke bdrv_activate_all() unconditionally, as it
> looks like safe to be used if all disks are active already.
>
> I wished we don't need to bother with disk activation status at all,
> because IIUC migration could work all fine even if all disks are inactivate
> when preparing migration.. hence such change always looks like a workaround
> of a separate issue.
>
>> + Error *local_err = NULL;
>> +
>> + g_assert(bql_locked());
>> +
>> + bdrv_activate_all(&local_err);
>> + if (local_err) {
>> + error_propagate(errp, local_err);
>> + return false;
>> + }
>> + s->block_inactive = false;
>
> This var so far was only used within one migration iteration, and the var
> was only set in migration_completion_precopy() so far. Now we're resetting
> it upfront of a migration. I'm not 100% sure if it's needed, or should be
> put somewhere else.
>
This variable is unconditionally set in migration_completion_precopy()
and used as a flag for whether or not we should be deactivating disks in
qemu_savevm_state_complete_precopy():
> /*
>
> * Inactivate disks except in COLO, and track that we have done so in
> order
> * to remember to reactivate them if migration fails or is cancelled.
>
> */
>
> s->block_inactive = !migrate_colo();
>
> migration_rate_set(RATE_LIMIT_DISABLED);
>
>
> ret = qemu_savevm_state_complete_precopy(s->to_dst_file, false,
>
> s->block_inactive);
Ideally we'd like to take our paused state into account right here and
skip inactivation. However at this point during the 2nd migration (in
paused state) our current_run_state == RUN_STATE_FINISH_MIGRATE. So if
we truly wanted to skip unnecessary bdrv_activate_all(), we'd have to
remember our paused state somewhere earlier on the call stack and pass
an additional flag for that. Personally I don't think this is any
cleaner than just blatantly calling bdrv_activate_all().
> In general, I saw the mention of other places that may also try to
> invalidate disks that used to be invalidated. If that's the case, I wish
> we don't need to touch migration code at all, but instead if block layer
> can cope with "invalidate on top of invalidated disks" it'll be perfect.
>
This sounds similar to my initial very naive fix, which we decided not
to take:
https://lists.nongnu.org/archive/html/qemu-devel/2024-09/msg04254.html
>> + }
>> +
>> return true;
>> }
>>
>> --
>> 2.35.3
>>
>
