Re: [Qemu-devel] live block copy/stream/snapshot discussion

[Stefan Hajnoczi]

On Tue, Jul 12, 2011 at 5:10 PM, Kevin Wolf <address@hidden> wrote:
> Am 12.07.2011 17:45, schrieb Stefan Hajnoczi:
>>>>> The command synopses are as follows:
>>>>>
>>>>> block_stream
>>>>> ------------
>>>>>
>>>>> Copy data from a backing file into a block device.
>>>>>
>>>>> If the optional 'all' argument is true, this operation is performed in the
>>>>> background until the entire backing file has been copied.  The status of
>>>>> ongoing block_stream operations can be checked with query-block-stream.
>>>
>>> Not sure if it's a good idea to use a bool argument to turn a command
>>> into its opposite. I think having a separate command for stopping would
>>> be cleaner. Something for the QMP folks to decide, though.
>>
>> git branch new_branch
>> git branch -D new_branch
>>
>> Makes sense to me :)
>
> I don't think you should compare a command line option to a programming
> interface. Having a git_create_branch(const char *name, bool delete)
> would really look strange. Anyway, probably a matter of taste.
>
> A hint that separate commands would make sense is that the stop command
> won't need the other arguments that the start command gets ('all' and
> 'base').

I can see your point.  Splitting the command might make the code more
straightforward and eliminate the need for checking invalid argument
combinations.

>>>>> Arguments:
>>>>>
>>>>> - all:    copy entire device (json-bool, optional)
>>>>> - stop:   stop copying to device (json-bool, optional)
>>>>> - device: device name (json-string)
>>>>
>>>> It must be possible to specify backing file that will be
>>>> active after streaming finishes (data from that file will not
>>>> be streamed into active file, of course).
>>>
>>> Yes, I think the common base image belongs here.
>>
>> Right.  We need to specify it by filename:
>>
>>   - base: filename of base file (json-string, optional)
>>
>>   Sectors are not copied from the base file and its backing file
>>   chain.  The following describes this feature:
>>     Before: base <- sn1 <- sn2 <- sn3 <- vm.img
>>     After:  base <- vm.img
>
> Does this imply that a rebase -u happens always after completion?

Yes.  The current implementation removes the backing file when
streaming completes.  I think this is the right thing to do since all
sectors are now allocated - there is no way to use the backing file
anymore.

If we don't change the backing file on streaming completion, then the
user has to issue an extra command.  There's nothing to gain by doing
that so I think rebase -u should happen on completion.

>>> With all = false, where does the streaming begin?
>>
>> Streaming begins at the start of the image.
>>
>>> Do you have something like the "current streaming offset" in the state of 
>>> each BlockDriverState?
>>
>> Yes, there is a StreamState for each block device that has an
>> in-progress operation.  The progress is saved between block_stream
>> (without -a) invocations so the caller does not need to specify the
>> streaming offset as an argument.
>>
>> Thanks for pointing out these weaknesses in the documentation.  It
>> should really be explained fully.
>
> I think we also need to describe error cases. For example, what happens
> if you try to start streaming while it's already in progress?

Yes, will do.

>>>>> Return:
>>>>>
>>>>> - device: device name (json-string)
>>>>> - len:    size of the device, in bytes (json-int)
>>>>> - offset: ending offset of the completed I/O, in bytes (json-int)
>>>
>>> So you only get the reply when the request has completed? With the
>>> current monitor, this means that QMP is blocked while we stream, doesn't
>>> it? How are you supposed to send the stop command then?
>>
>> Incomplete documentation again, sorry.  The block_stream command
>> behaves as follows:
>>
>> 1. block_stream all returns immediately and the BLOCK_STREAM_COMPLETED
>> event is raised when streaming completes either successfully or with
>> an error.
>>
>> 2. block_stream stop returns when the in-progress streaming operation
>> has been safely stopped.
>>
>> 3. block_stream returns when one iteration of streaming has completed.
>>
>>> Two of three examples below have an empty return value instead, so they
>>> are not compliant to this specification.
>>
>> I will update the documentation, the non-all invocations do not return 
>> anything.
>
> Okay, then I don't understand what the 'offset' return value means. The
> text says "offset of the completed I/O". If all=true immediately
> returns, shouldn't it always be 0?

The 'offset' value gives you an indication of progress when using the
iteration interface.  You don't need to separately call
query-block-stream, instead you can use the return value from the
iteration interface to get progress information.

However, let's drop iteration.

>>> I find it rather disturbing that a command like 'change' has made it
>>> into QMP... Anyway, I don't think this is really what we need.
>>>
>>> We have two switches to do. The first one happens before starting the
>>> copy: Creating the copy, with the source as its backing file, and
>>> switching to that. The monitor command to achieve this is snapshot_blkdev.
>>
>> I don't think that creating image files in QEMU is going to work when
>> running KVM with libvirt (SELinux).  The QEMU process does not have
>> the ability to create new image files.  It needs at least a file
>> descriptor to an empty file or maybe a file that has been created
>> using qemu-img like I showed above.
>
> Independent problem. We're really creating an external snapshot here, so
> we should use the function for external snapshots. libvirt can
> pre-create an empty image file, so that qemu will write the image format
> data into it, but we have discussed this before.

Cool.  If snapshot_blkdev will be able to work in an sVirt environment
then great.

Stefan