Re: [Qemu-devel] [PATCH v8 29/54] 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_in_postcopy' 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>
> > Reviewed-by: Amit Shah <address@hidden>
> 
> Just wondering
> 
> 
> > diff --git a/migration/migration.c b/migration/migration.c
> > index 5ee2c11..2ae5909 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -439,6 +439,7 @@ static bool migration_is_active(MigrationState *ms)
> >  {
> >      switch (ms->state) {
> >      case MIGRATION_STATUS_ACTIVE:
> > +    case MIGRATION_STATUS_POSTCOPY_ACTIVE:
> >      case MIGRATION_STATUS_SETUP:
> >          return true;
> >  
> > @@ -509,6 +510,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();
> > +        }
> 
> Are we sure that disk migration works with postcopy?  I would expect no ...

Well, the theory goes that it should; although probably not a good
idea.
What should happen is that since the block migration will reply
with a non-postcopiable pending size > 0, it will stay in precopy
mode until the block migration is done; once that happens
(and the block code returns a pending_size < bandwidth*downtime)
it should flip into postcopy.

It's a bad idea because:
  a) While still in precopy mode the RAM migration will still be
trying to transfer in precopy mode as well, which is wasting bandwidth
fighting with the block IO (we could probably quiesce that once
postcopy is turned on).
  b) I'm a bit worried about how much the block migration might try
and use in RAM when it does it's save_complete into the postcopy
tempoary buffer.

It should be possible to implement a postcopy for block as well
that would run concurrently with the psotcopying of RAM; you'd have
to make sure you scheduled requests that came back to the source so
that slow disc reads didn't block RAM requests.

(Having said that, my test I just did of it didn't work; I'll
try and have a look at that).

Dave

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