Re: [PATCH RFC] memory: Don't allow to resize RAM while migrating

[Peter Xu]

On Fri, Feb 14, 2020 at 07:26:59PM +0100, David Hildenbrand wrote:
> >>>> +    if (!postcopy_is_running()) {
> >>>> +        Error *err = NULL;
> >>>> +
> >>>> +        /*
> >>>> +         * Precopy code cannot deal with the size of ram blocks 
> >>>> changing at
> >>>> +         * random points in time. We're still running on the source, 
> >>>> abort
> >>>> +         * the migration and continue running here. Make sure to wait 
> >>>> until
> >>>> +         * migration was canceled.
> >>>> +         */
> >>>> +        error_setg(&err, "RAM resized during precopy.");
> >>>> +        migrate_set_error(migrate_get_current(), err);
> >>>> +        error_free(err);
> >>>> +        migration_cancel();
> >>>> +    } else {
> >>>> +        /*
> >>>> +         * Postcopy code cannot deal with the size of ram blocks 
> >>>> changing at
> >>>> +         * random points in time. We're running on the target. Fail 
> >>>> hard.
> >>>> +         *
> >>>> +         * TODO: How to handle this in a better way?
> >>>> +         */
> >>>> +        error_report("RAM resized during postcopy.");
> >>>> +        exit(-1);
> >>>
> >>> Now I'm rethinking the postcopy case....
> >>>
> >>> ram_dirty_bitmap_reload() should only happen during the postcopy
> >>> recovery, and when that happens the VM should be stopped on both
> >>> sides.  Which means, ram resizing should not trigger there...
> >>
> >> But that guest got the chance to run for a bit and eventually reboot
> >> AFAIK. Also, there are other data races possible when used_length
> >> suddenly changes, this is just the most obvious one where things will;
> >> get screwed up.
> > 
> > Right, the major one could be in ram_load_postcopy() where we call
> > host_from_ram_block_offset().  However if FW extension is the major
> > use case then it seems to still work (still better than crashing,
> > isn't it? :).
> 
> "Let's close our eyes and hope it will never happen" ? :) No, I don't
> like that. This screams for a better solution long term, and until then
> a proper fencing IMHO. We're making here wild guesses about data races
> and why they might not be that bad in certain cases (did I mention
> load/store tearing? used_length is not an atomic value ...).

Yeah fencing is good, but crashing a VM while it wasn't going to crash
is another thing, imho.  You can dump an error message if you really
like, but instead of exit() I really prefer we either still let the
old way to at least work or really fix it.

When I say "really fix it", I mean we can even start to think about
the shrinking case and how to support that for postcopy.  For example,
in the above case host_from_ram_block_offset() will return NULL then,
and the fix could be that we drop that extra page because we don't
need that any more, instead of bailing out.

Thanks,

-- 
Peter Xu