Re: [Qemu-devel] [PATCH v7 23/42] MIGRATION_STATUS_POSTCOPY_ACTIVE: Add new migration state

[Dr. David Alan Gilbert]

* Juan Quintela (address@hidden) wrote:
> "Dr. David Alan Gilbert (git)" <address@hidden> wrote:
> > From: "Dr. David Alan Gilbert" <address@hidden>
> >
> > 'MIGRATION_STATUS_POSTCOPY_ACTIVE' is entered after migrate_start_postcopy
> >
> > 'migration_postcopy_phase' is provided for other sections to know if
> > they're in postcopy.
> >
> > Signed-off-by: Dr. David Alan Gilbert <address@hidden>
> > Reviewed-by: David Gibson <address@hidden>
> > Reviewed-by: Eric Blake <address@hidden>
> 
> Reviewed-by: Juan Quintela <address@hidden>
> 
> 
> But (there is always a but....)
> 
> 
> > @@ -358,6 +359,39 @@ MigrationInfo *qmp_query_migrate(Error **errp)
> >  
> >          get_xbzrle_cache_stats(info);
> >          break;
> > +    case MIGRATION_STATUS_POSTCOPY_ACTIVE:
> > +        /* Mostly the same as active; TODO add some postcopy stats */
> > +        info->has_status = true;
> > +        info->has_total_time = true;
> > +        info->total_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME)
> > +            - s->total_time;
> > +        info->has_expected_downtime = true;
> > +        info->expected_downtime = s->expected_downtime;
> > +        info->has_setup_time = true;
> > +        info->setup_time = s->setup_time;
> > +
> > +        info->has_ram = true;
> > +        info->ram = g_malloc0(sizeof(*info->ram));
> > +        info->ram->transferred = ram_bytes_transferred();
> > +        info->ram->remaining = ram_bytes_remaining();
> > +        info->ram->total = ram_bytes_total();
> > +        info->ram->duplicate = dup_mig_pages_transferred();
> > +        info->ram->skipped = skipped_mig_pages_transferred();
> > +        info->ram->normal = norm_mig_pages_transferred();
> > +        info->ram->normal_bytes = norm_mig_bytes_transferred();
> > +        info->ram->dirty_pages_rate = s->dirty_pages_rate;
> > +        info->ram->mbps = s->mbps;
> > +
> > +        if (blk_mig_active()) {
> > +            info->has_disk = true;
> > +            info->disk = g_malloc0(sizeof(*info->disk));
> > +            info->disk->transferred = blk_mig_bytes_transferred();
> > +            info->disk->remaining = blk_mig_bytes_remaining();
> > +            info->disk->total = blk_mig_bytes_total();
> > +        }
> 
> Can we have block migration active with postcopy?  I would assume that
> this would get disk corruption, no?  Or if you preffer the other
> question, what protects us from disk corruption?

I think you can, I've not tried it; however I also think it should
be safe.

 migration/block.c's block_save_pending always puts a value in the
non_postcopiable_pending return value (and 0 in the postcopiable_pending);
the migrate thread checks the non_postcopiable_pending size to
decide when it can switch to postcopy, and performs a call to the complete
method on each device before it does.  Thus the block migration should
be finished before we start doing the actual postcopy stage, and thus
before the destination CPU starts running.

A possibly harder question is what happens if block.c did implement
postcopy and you had both block postcopy and ram postcopy active at
the same time; again I think it should work but I'm not sure if one
would starve the other.

> Once here, I guess we can get the migrate_already_active() bit without
> problem?

I'm not sure of the question here; but the idea of migration_already_active()
is just to avoid all of the open-coded checks for each possible state;
now we've added anothe state they were getting messy.

Dave

> 
> Later, Juan.
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK