On Wed, Jun 28, 2017 at 03:33:49PM -0500, Eric Blake wrote: > 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!
Thanks! > > I haven't had the time to review the complete document yet, but here are > > my comments from the first half: [I'm responding out of order -- as I first replied to your other email, which gave feedback about the sceond 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". Yes, I'll use Eric's wording [thanks] here, that makes the intent clearer. > >> +(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. Correct. I will clarify the wording here. > >> +(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). Right. I'll workout a way to mention this, too. [Me will not wonder whether it will confuse the reader about mentioning it so early -- as the said reader will be using these primitives to make higher level tools, and they can easily figure out the semantics.] > >> +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. Heh, indeed. Peter Maydell has said Oxford English Dictionary has mentions for both variants. But Alberto is correct that the non-hyphen variant, "rightmost", is much more widely used. > >> +(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. Yes, I'll keep this in mind for v5. > >> +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..." Yes. Will fix. [...] > >> +(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). Indeed. Good explanation. (I now wonder if I should workout a way to mention some comments from the guest POV) > >> +.. _`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). True; I just spelled out all possible cases for one primitive. for the rest, I didn't -- as mentioned in a note. > > > >> + (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. Yeah, precisely -- I didn't wanted to skip mentioning something just because it's the default, for clarity's sake. > > > > Same with 'base'. > > > > Else this part looks good! I'll check the rest of the document tomorrow > > and give you my feedback. Thanks, Alberto and Eric. I'll spin a v5 from this, and feedback from the other email. [...] -- /kashyap