Re: [Qemu-devel] [PATCH v2 3/8] fdc: Introduce fdctrl->phase

[Kevin Wolf <kwolf@redhat.com>]

Am 29.05.2015 um 10:33 hat Dr. David Alan Gilbert geschrieben:
> * Markus Armbruster (armbru@redhat.com) wrote:
> > "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
> > 
> > > * John Snow (jsnow@redhat.com) wrote:
> > >> 
> > >> 
> > >> On 05/21/2015 09:19 AM, Kevin Wolf wrote:
> > >> > The floppy controller spec describes three different controller phases,
> > >> > which are currently not explicitly modelled in our emulation. Instead,
> > >> > each phase is represented by a combination of flags in registers.
> > >> > 
> > >> > This patch makes explicit in which phase the controller currently is.
> > >> > 
> > >> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> 
> <snip>
> 
> > >> > +static bool fdc_phase_needed(void *opaque)
> > >> > +{
> > >> > +    FDCtrl *fdctrl = opaque;
> > >> > +
> > >> > +    return reconstruct_phase(fdctrl) != fdctrl->phase;
> > >> > +}
> > >
> > > OK, that is one way of doing it which should work, as long
> > > as most of the time that matches.
> > >
> > > My preference for subsections is normally to make them just
> > > conditional on machine type, that way backwards migration just
> > > works.  However, if the result of the backwards migration would
> > > be data corruption (which if I understand what you saying it could
> > > in this case) then it's arguably better to fail migration in those
> > > cases.
> > 
> > This isn't a matter of preference.
> > 
> > Subsections were designed to solves a problem we only have because we're
> > not perfect: device model bugs, namely forgetting to migrate a bit of
> > state, or failing to model it in the first place.
> > 
> > Subsections tied to machine types solve an entirely different problem:
> > the old version of the device is just fine, including migration, but we
> > now want a new and improved variant, which needs some more state
> > migrated.  Putting that piece of state into a subsection tied to the new
> > machine type avoids code duplication.
> > 
> > But what do you do for device model bugs serious enough to need fixing
> > even for existing machine types, when the fix needs additional state
> > migrated?  Subsections tied to machine types are *not* a solution there.
> > That's why this isn't about preference!  It's about having a bug to fix
> > or not.
> 
> My problem is that people keep adding subsections to fix minor device
> bugs because they think breaking migration is irrelevant.  If the bug
> being fixed causes data corruption then OK I agree it is probably better
> to break migration, otherwise  you need to make a decision about whether
> fixing the device bug during migration is better or worse than having
> the migration fail.
> Some people say 'Migration failure is ok - people just try it again';
> sorry, no, that's useless in environments where the VM has 100s of GB
> of RAM and takes hours to migrate only for it then to fail, and it
> was urgent that it was migrated off that source host.

If this is a problem in practice, it sounds a lot like we need to
improve our handling of that situation in general. Why do we abort
the whole migration when serialising the state fails? Can't we abort
just the completion and switch back to the live state, and then retry a
bit later?

Most, if not all, of the subsections you mentioned that fix minor bugs
fix some states while the device is actively used and shorty after we
should be back in a state where the subsection isn't needed.

This is exactly the "fails 1 in 100 migrations" cases you talked about,
and they would be practically fixed if you made a few more attempts to
complete the migration before you let it fail (or perhaps even retry
indefinitely, as long as the user doesn't cancel migration).

Kevin