qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [Qemu-block] [PATCH v4 1/2] live-block-ops.txt: Rename,


From: Eric Blake
Subject: Re: [Qemu-devel] [Qemu-block] [PATCH v4 1/2] live-block-ops.txt: Rename, rewrite, and improve it
Date: Wed, 28 Jun 2017 15:33:49 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.0

On 06/28/2017 03:15 PM, Alberto Garcia wrote:
> On Wed 28 Jun 2017 04:58:00 PM CEST, Kashyap Chamarthy wrote:
>> This patch documents (including their QMP invocations) all the four
>> major kinds of live block operations:
>>
>>   - `block-stream`
>>   - `block-commit`
>>   - `drive-mirror` (& `blockdev-mirror`)
>>   - `drive-backup` (& `blockdev-backup`)
> 
> This is excellent work, thanks for doing this!
> 
> I haven't had the time to review the complete document yet, but here are
> my comments from the first half:
> 
>> +Disk image backing chain notation
>> +---------------------------------
>   [...]
>> +.. important::
>> +    The base disk image can be raw format; however, all the overlay
>> +    files must be of QCOW2 format.
> 
> This is not quite like that: overlay files must be in a format that
> supports backing files. QCOW2 is the most common one, but there are
> others (qed). Grep for 'supports_backing' in the source code.

At the same time, other image formats are not as frequently tested, or
may be read-only.  Maybe a compromise of "The overlay files can
generally be any format that supports a backing file, although qcow2 is
the preferred format and the one used in this document".

> 
>> +(1) ``block-stream``: Live copy of data from backing files into overlay
>> +    files (with the optional goal of removing the backing file from the
>> +    chain).
> 
> optional? The backing file is removed from the chain as soon as the
> operation finishes, although the image file is not removed from the
> disk. Maybe you meant that?

Hmm, you're right. In this case, qemu ALWAYS rewrites the backing chain
to get rid of the (now-streamed) backing image.

> 
>> +(2) ``block-commit``: Live merge of data from overlay files into backing
>> +    files (with the optional goal of removing the overlay file from the
>> +    chain).  Since QEMU 2.0, this includes "active ``block-commit``"
>> +    (i.e.  merge the current active layer into the base image).
> 
> Same question about the 'optional' here.

Here, optional is a bit more correct. With non-active (intermediate)
commit, qemu ALWAYS rewrites the backing chain to be shorter; but with
live commit, you can chose whether to pivot to the now-shorter chain
(job-complete) or whether to keep the active image intact (starting to
collect a new delta from the point-in-time of the just-completed commit,
by job-cancel).

> 
>> +writing to it.  (The rule of thumb is: live QEMU will always be pointing
>> +to the right-most image in a disk image chain.)
> 
> I think it's 'rightmost', without the hyphen.

Sadly, I think this is one case where both spellings work to a native
reader, and where I don't know of a specific style-guide preference.  I
probably would have written with the hyphen.

> 
>> +(3) Intermediate streaming (available since QEMU 2.8): Starting afresh
>> +    with the original example disk image chain, with a total of four
>> +    images, it is possible to copy contents from image [B] into image
>> +    [C].  Once the copy is finished, image [B] can now be (optionally)
>> +    discarded; and the backing file pointer of image [C] will be
>> +    adjusted to point to [A].
> 
> The 'optional' usage again. [B] will be removed from the chain and can
> be (optionally) removed from the disk, but that you have to do yourself,
> QEMU won't do that.

Indeed, we may need to be specifically clear of the cases where qemu
shortens the chain, but where disk images that are no longer used by the
chain (whether they are still viable [as in stream], or invalidated [as
in commit crossing more than one element of the chain]) are still left
on the disk for the user to discard separately from qemu.

> 
>> +The ``block-commit`` command lets you to live merge data from overlay
>> +images into backing file(s).
> 
> I don't think "lets you to live merge data" is correct English.

Probably better as "lets you merge live data from overlay..."

> 
>> +The disk image chain can be shortened in one of the following ways:
>> +
>> +.. _`block-commit_Case-1`:
>> +
>> +(1) Commit content from only image [B] into image [A].  The resulting
>> +    chain is the following, where image [C] is adjusted to point at [A]
>> +    as its new backing file::
>> +
>> +        [A] <-- [C] <-- [D]
>> +
>> +(2) Commit content from images [B] and [C] into image [A].  The
>> +    resulting chain, where image [D] is adjusted to point to image [A]
>> +    as its new backing file::
>> +
>> +        [A] <-- [D]
> 
> Aren't these two just different variants of the same case?

Almost. But in case 1, image [B] is still viable (from a guest point of
view, the contents of [B] have not changed); in case 2, image [B] is
most likely corrupted (any changes propagated from [C] into [A] that
were not already overridden in [B] now read differently, making image
[B] no longer match anything the guest ever saw at any point in past time).

> 
>> +
>> +.. _`block-commit_Case-3`:
>> +
>> +(3) Commit content from images [B], [C], and the active layer [D] into
>> +    image [A].  The resulting chain (in this case, a consolidated single
>> +    image)::
>> +
>> +        [A]
>> +
>> +(4) Commit content from image only image [C] into image [B].  The
>> +    resulting chain::
>> +
>> +    [A] <-- [B] <-- [D]
>> +
>> +(5) Commit content from image [C] and the active layer [D] into image
>> +    [B].  The resulting chain::
>> +
>> +    [A] <-- [B]
> 
> Same here.

4 and 5 indeed are variants of 1 and 2.

> 
> I mean, it's fine to have several different examples, but I think
> there's really two main cases here (as you correctly explain later).
> 
>> +    (QEMU) block-commit device=node-D base=a.qcow2 top=d.qcow2 job-id=job0
>> +    {
>> +        "execute": "block-commit",
>> +        "arguments": {
>> +            "device": "node-D",
>> +            "job-id": "job0",
>> +            "top": "d.qcow2",
>> +            "base": "a.qcow2"
>> +        }
>> +    }
> 
> This is correct, but I don't know if it's worth mentioning that if you
> omit the 'top' parameter it defaults to the active layer (node-D in this
> example).

I think it's better to be explicit in the examples (always provide all
parameters, even if what you provide would also be the default when
omitted), and then maybe the prose can mention which parameters have
defaults.

> 
> Same with 'base'.
> 
> Else this part looks good! I'll check the rest of the document tomorrow
> and give you my feedback.
> 
> Berto
> 
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]