Re: [Patch v2] tools/migrate: Fix regression when migrating from older version of Xen

[Andrew Cooper <andrew.cooper3@citrix.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3341 bytes --]

On 18/07/13 19:47, Shriram Rajagopalan wrote:
> On Thu, Jul 18, 2013 at 12:27 PM, Andrew Cooper
> <andrew.cooper3@citrix.com <mailto:andrew.cooper3@citrix.com>> wrote:
>
>     On 18/07/13 17:20, Shriram Rajagopalan wrote:
>>     On Tue, Jul 16, 2013 at 6:52 AM, Andrew Cooper
>>     <andrew.cooper3@citrix.com <mailto:andrew.cooper3@citrix.com>>
>>     wrote:
>>     and you setting ctx->last_checkpoint = 0 basically means that you are
>>     banking on the far end (with older version of tools) to close the
>>     socket, causing
>>     "get pagebuf" to fail and subsequently land on finish.
>>     IIRC, this was the behavior before XC_SAVE_ID_LAST_CHECKPOINT was
>>     introduced
>>     by Ian Campbell, to get rid of this benign error message.
>
>
>     That might have been 'fine' for PV guests, but it cause active
>     breakage for HVM domains where the qemu save record immediately
>     follows in the migration stream.
>
>
> Just to clarify.. the code flows like this, iirc.
>
> loadpages:
>  while (1)
>      if !completed
>         get pagebufs
>         if 0 pages sent, break
>      endif
>      apply batch (pagebufs)
>  endwhile
>
>  if !completed
>    get tailbuf [[this is where the QEMU record would be obtained]]
>    completed = 1
>  endif
>
>  if last_checkpoint
>    goto finish
>  endif
>
>  get pagebuf or goto finish on error ---> this is where old code used
> to exit
>  get tailbuf
> goto loadpages
> finish:
>    apply tailbuf [tailbuf obtained inside the 'if !completed' block]
>    do the rest of the restore

(Logically joining the two divergent threads, as this is the answer to both)

This has nothing to do with the buffering mode, and that is not where
the Qemu record would be obtained from.

As the code currently stands, if a XC_SAVE_ID_LAST_CHECKPOINT chunk is
not seen in the stream, we complete the loadpages: section, including
the magic pages (TSS, console info etc).

We then read the buffer_tail() on line 171, set ctx->completed on line
1725, but fail the ctx->last_checkpoint check on line 1758.

What we should do is pass the last_checkpoint test, and goto finish
which then calls dump_qemu().  What actually happens is a call to
pagebuf_get() on line 1766 which raises an error because of finding a
Qemu save record rather than more pages.

So this is very much a bug directly introduced by 00a4b65f85, and can
only be fixed with knowledge from the higher levels of the toolstack.

As for the wording of the parameter, I still prefer the original
"last_checkpoint_unaware" over "checkpointed_stream" as it is more accurate.

Any migration stream started from a version of the tools after c/s
00a4b65f85 will work, whether it is checkpointed or not (as the
XC_SAVE_ID_LAST_CHECKPOINT chunk will be sent in the correct place). 
Any migration started from a version of the tools before c/s 00a4b65f85
will fail because it has no idea that the receiving end is expecting it
to insert XC_SAVE_ID_LAST_CHECKPOINT chunk.  The fault here is with the
receiving end expecting to find a XC_SAVE_ID_LAST_CHECKPOINT chunk.

The only fix is for newer toolstack to be aware that the migration
stream is from an older toolstack, and to set last_checkpoint_unaware=1,
which will set ctx->last_checkpoint to 1, allowing the receiving side of
the migration to correctly read the migration stream.

~Andrew

[-- Attachment #1.2: Type: text/html, Size: 6883 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel