[libvirt] API Proposal: virDomainBlockPull() (Block device streaming) V4

[Adam Litke]

I need to clarify this a bit...

On 05/24/2011 08:06 AM, Adam Litke wrote:
> 
> 
> On 05/24/2011 07:16 AM, Daniel P. Berrange wrote:
>> On Tue, May 24, 2011 at 01:00:04PM +0100, Stefan Hajnoczi wrote:
>>> On Mon, May 23, 2011 at 5:56 PM, Adam Litke <agl at us.ibm.com> wrote:
>>>> /**
>>>>  * virDomainBlockPull:
>>>>  * @dom: pointer to domain object
>>>>  * @path: Fully-qualified filename of disk
>>>>  * @cursor: pointer to a virDomainBlockPullCursor, or NULL
>>>>  * @flags: One of virDomainBlockPullFlags, or zero
>>>>  *
>>>>  * Populate a disk image with data from its backing image.  Once all data from
>>>>  * its backing image has been pulled, the disk no longer depends on a backing
>>>>  * image.
>>>>  *
>>>>  * If VIR_DOMAIN_BLOCK_PULL_CONTINUOUS is specified, the entire device will be
>>>>  * streamed in the background.  Otherwise, the value stored in @cursor will be
>>>>  * used to stream an increment.
>>>>  *
>>>>  * Returns -1 in case of failure, 0 when successful.  On success and when @flags
>>>>  * does not contain VIR_DOMAIN_BLOCK_PULL_CONTINUOUS, the iterator @cursor will
>>>>  * be updated to the proper value for use in a subsequent call.
>>>>  */
>>>> int                 virDomainBlockPull(virDomainPtr dom,
>>>>                                       const char *path,
>>>>                                       virDomainBlockPullCursor *cursor,
>>>>                                       unsigned int flags);
>>>
>>> If this function is used without VIR_DOMAIN_BLOCK_PULL_CONTINUOUS then
>>> the "end" value is unknown.  Therefore it is not possible to calculate
>>> streaming progress.  Perhaps instead of cursor we need a
>>> virDomainBlockStreamInfoPtr info argument?
>>
>> It almost feels like we should just not overload semantics using
>> flags and have a separate APIs:
>>
>> Incremental, just iterate on:
>>
>>  int virDomainBlockPull(virDomainPtr dom,
>>                         const char *path,
>>                         virDomainBlockPullInfoPtr *pos,
> 
> We don't even need 'pos' anymore.  See below.

We still will want 'pos', but it changes from an in/out parameter to
out-only.  Qemu tracks the position internally so this is just for
progress reporting.

> 
>>                         unsigned int flags);
>>
>> Continuous, invoke once:
>>
>>  int virDomainBlockPullAll(virDomainPtr dom,
>>                            const char *path,
>>                            unsigned int flags);
>>
>>  ...and wait for the async event notification for completion, or
>> optionally poll on virDomainGetBlockPullInfo, or use BlockPullAbort()
> 
> Yeah, despite adding four functions to the API this seems like a natural
> way to segment it out.
> 
>>>> NOTE: Qemu will emit an asynchronous event (VIR_DOMAIN_BLOCK_PULL_COMPLETED)
>>>> after any operation that eliminates the dependency on the backing file.  This
>>>> could be a virDomainBlockPull() without VIR_DOMAIN_BLOCK_PULL_CONTINUOUS and
>>>> will allow libvirt to manage backing file security regardless of the pull mode
>>>> used.
>>>
>>> Currently QEMU does not change the backing file when incremental
>>> streaming is used (instead of continuous).  This is because it is hard
>>> to know whether or not the entire file has finished streaming - it's
>>> an operation that requires traversing all block allocation metadata to
>>> check that the backing file is no longer necessary.
>>
>> Having different semantics for what happens at the end of streaming
>> for incremental vs continuous is a really bad idea. I assume this
>> limitation will be fixed in QEMU before streaming is finally merged ?
> 
> After talking to Stefan I think we have resolved this issue.  We have
> decided to drop the 'pos' argument to virDomainBlockPull() completely.
> If pos is a sequential, opaque cursor then there is only ever one valid
> value that can be passed to qemu at any given time.  If qemu has to
> store the valid value to check against, then we might as well just store
> the position internally to qemu and save the API user the trouble of
> managing the iterator.  An added benefit is that we now know it is safe
> to know that the entire disk has been pulled after the last cursor
> position has finished (regardless of which mode was used).

As stated above, 'pos' remains in the API but becomes an out parameter.

> I will follow-up with V5 of the API...



-- 
Adam Litke
IBM Linux Technology Center