Re: [Qemu-devel] [PATCH v2 1/1] Add QMP bits for blockdev-snapshot-sync.

[Jes Sorensen]

On 04/28/11 17:10, Anthony Liguori wrote:
> On 04/28/2011 09:57 AM, Jes Sorensen wrote:
>> On 04/28/11 16:46, Anthony Liguori wrote:
>> Sorry this is inherently broken. The management tool should not be
>> keeping state in this process. I agree an async interface would be nice,
>> but the above process is plain wrong.
>>
>> The async snapshot process needs to be doing the exact same as in the
>> current implementation, the main difference is that it would be running
>> asynchronously and that QMP would be able to query the state of it.
> 
> No, the command does too many things and as such, makes it impossible
> for a management tool to gracefully recover.

It is exactly the same for the management tool:
- Creation of the new image either succeeds or fails
- Switchover either succeeds or fails

If you have a crash in the process, you still can only know by checking
the consistency of the new image after rebooting.

>> There is no issue here, you have the exact same problem if you get a
>> crash during d) in your example. It is the same with the existing
>> command, the crash is only an issue if it happens right in the middle of
>> the switch over. Until then, only the original image remains valid.
> 
> But the new image is always valid once it's been created as pending
> writes are lost no matter what in a hard power failure.  That suggests
> the following flow:
> 
> 1) Create new image with a backing file
> 2) Completion ACK
> 
> At this stage, the management tool should update it's internal state to
> point to the new image.

This can still be done with the existing code - it's not the ideal
setup, but it is possible due to evil things such as selinux labeling.

> 3) Begin switch over to new image
> 4) Switch over image.
> 5) Receive notification when it's complete

Sorry but this isn't an accurate description of the process. Once you
have a new image ready, you need to halt writes, flush buffers, and then
you can do the switch, which has to be atomic. The only thing that can
really be async in all of this is the buffer flushing. There isn't any
long switch-over process.

> If (4) is happening asynchronously, things get kind of complicated.  You
> can either wait for things to quiesce on their own or you can queue
> pending requests.  It's unclear to me what the right strategy is or if
> there's a mixed strategy that's needed.  I think it makes sense to treat
> 3/4 as a command with (5) being an event notification.

The actual guest application freeze (what some strange people use the
quiicannotspell word for) is done prior to the switchover, you cannot do
it in parallel with it.

> But combining 1-5 in a single interface creates a command that while
> convenient on the command line, is not usable for a robust management tool.

As I explained you can already use an externally created image with the
current interface, the only issue that may be worth doing async is the
buffer flushing. However once you do that, guest writes are going to
stall anyway, and eventually the guest applications will all stall.

The single command is both better from a consistency perspective, and it
will do the right job. Things are not going to get any easier from the
management tool's perspective than they are with the current interface.

Cheers,
Jes