Re: [PATCH v2 6/6] migration/block: Rewrite disk activation

[Peter Xu]

On Mon, Dec 16, 2024 at 12:58:58PM -0300, Fabiano Rosas wrote:
> > @@ -3286,6 +3237,11 @@ static void 
> > migration_iteration_finish(MigrationState *s)
> >      case MIGRATION_STATUS_FAILED:
> >      case MIGRATION_STATUS_CANCELLED:
> >      case MIGRATION_STATUS_CANCELLING:
> 
> Pre-existing, but can we even reach here with CANCELLED? If we can start
> the VM with both CANCELLED and CANCELLING, that means the
> MIG_EVENT_PRECOPY_FAILED notifier is not being consistently called. So I
> think CANCELLED here must be unreachable...

Yeah I can't see how it's reachable, because the only place that we can set
it to CANCELLED is:

migrate_fd_cleanup():
    if (s->state == MIGRATION_STATUS_CANCELLING) {
        migrate_set_state(&s->state, MIGRATION_STATUS_CANCELLING,
                          MIGRATION_STATUS_CANCELLED);
    }

In this specific context, it (as a bottom half) can only be scheduled after
this path.

Looks like the event MIG_EVENT_PRECOPY_FAILED will work regardless, though?
As that's also done after above:

    type = migration_has_failed(s) ? MIG_EVENT_PRECOPY_FAILED :
                                     MIG_EVENT_PRECOPY_DONE;
    migration_call_notifiers(s, type, NULL);

So looks like no matter it was CANCELLING or CANCELLED, it'll always be
CANCELLED when reaching migration_has_failed().

[...]

> > @@ -103,13 +104,8 @@ void qmp_cont(Error **errp)
> >           * Continuing after completed migration. Images have been
> >           * inactivated to allow the destination to take control. Need to
> >           * get control back now.
> > -         *
> > -         * If there are no inactive block nodes (e.g. because the VM was
> > -         * just paused rather than completing a migration),
> > -         * bdrv_inactivate_all() simply doesn't do anything.
> >           */
> > -        bdrv_activate_all(&local_err);
> > -        if (local_err) {
> > +        if (!migration_block_activate(&local_err)) {
> >              error_propagate(errp, local_err);
> 
> Could use errp directly here.

True..

-- 
Peter Xu