Re: [PATCH v2] fix Remus failover regression

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

On 28/07/14 10:29, Hongyang Yang wrote:
> Hi Andrew,
>
> On 07/28/2014 05:24 PM, Andrew Cooper wrote:
>> On 28/07/14 05:03, Yang Hongyang wrote:
>>> commit: c2ba706c
>>> tools/libxc: goto correct label on error paths by Andrew Cooper
>>> broke Remus in Xen 4.4 or earlier versions that has this commit
>>> backported.
>>
>> My appologies for breaking Remus. (it just goes to show how fragile this
>> code is).
>>
>>>
>>> With Remus, this jump essentially discards the current incomplete
>>> checkpoint received by the backup and restore backup from the
>>> last complete checkpoint.
>>> This is required for Remus to work and this does not break live
>>> migration.
>>> It has been around since Xen 4.0.
>>
>> However, it is a genuine bugfix for regular migration, so simply
>> reverting it as this patch does is not appropriate.
>>
>> For regular migration, you absolutely have to goto out; on a failure
>> otherwise the finish code will run and declare the migration a success
>> despite only having half a domain restored.
>
> I think regular migration shouldn't run into this path (see what I
> commented
> in v1), but I agree that add a check will be better.

Hmm - I see what you mean.  I can't spot how a regular migration would
end up at that point.

When I debugged the issue, I was encountering the pagebuf error message
on a regular migrate, although I was debugging a single isolated failure
from logs alone.  With a bit of hindsight now, this probably means that
ctx->last_checkpoint was wrong.

We regularly test migration from before the point that
ctx->last_checkpoint was introduced and broke the migration
backwards-compatibility, but the purpose of checkpointed_stream was to
re-fix this without regressing backwards compatibility.

I have to admit that I somewhat confused as to what actually went on,
but it is also clear that my changes were based on incorrect reasoning
and further rereasoning at this point suggests the changes were wrong.

Therefore, this patch with comments is probably best.

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

IanJ: This fix needs backporting to 4.4  (4.3 and older are fine)

~Andrew