On Tue, May 12, 2020 at 10:39 PM Eric Blake <ebl...@redhat.com> wrote: > > On 5/12/20 6:10 AM, Max Reitz wrote: > > > >> This does not break old code since previously we always reported only > >> guest visible content > >> here, but it changes the semantics, and now you cannot allocate > >> "required" size, you need > >> to allocate "required" size with "bitmaps" size. > > > > Only if you copy the bitmaps, though, which requires using the --bitmaps > > switch for convert. > > First, a usage question: would you rather that 'qemu-img convert > --bitmaps' silently succeeds even when the image being converted has no > bitmaps, or would you rather that it fails loudly when there are no > bitmaps to be copied over?
I think the meaning of --bitmaps should be "copy also bitmaps". This request makes sense only for qcow2 images, since other images do not have bitmaps, so failing loudly when the source format does not support bitmaps seems right. Same for the target format does not support bitmaps, this is invalid request and it should fail loudly. If the source and target are qcow2, and there are no bitmaps in source, I don't see any reason to fail. We don't want to check if an image has bitmaps before we copy the image, it does not help us. > As implemented in this patch series, patch 8 > currently silently succeeds. Sounds good for qcow2 format. > But in order to make patch 7 and 8 > consistent with one another, I need to know which behavior is easier to > use: failing convert if the source lacks bitmaps (and thus measure would > suppress the bitmaps:0 output), or treating lack of bitmaps as nothing > additional to copy and thereby succeeding (and thus measure should > output bitmaps:0 to show that no additional space is needed because > nothing else will be copied, successfully). I don't think showing "bitmaps: 0" in measure is related to how --bitmaps behave in convert. If we will have --bitmaps in measure, we don't need to show "bitmaps" at all since "required" will include it. If we want to show bitmaps in measure, I think using the same logic is fine: - if format does not support bitmaps - fail - if format suppots bitmaps, show what we have - zero is valid result when image does not have any bitmap. > >> If we add a new > >> extension all users will have to > >> change the calculation again. > > > > It was my impression that existing users won’t have to do that, because > > they don’t use --bitmaps yet. > > > > In contrast, if we included the bitmap size in @required or > > @fully-allocated, then previous users that didn’t copy bitmaps to the > > destination (which they couldn’t) would allocate too much space. > > > > ...revisiting this after reading more of your mail: With a --bitmaps > > switch, existing users wouldn’t suffer from such compatibility problems. > > However, then users (that now want to copy bitmaps) will have to pass > > the new --bitmaps flag in the command line, and I don’t see how that’s > > less complicated than just adding @bitmaps to @required. > > More concretely, suppose down the road that we add the ability to copy > internal snapshots (not that you want to mix internal and external > snapshots, but that such information already exists and therefore can be > used as an example). Which is easier: > > $ qemu-img measure -f qcow2 image.qcow2 > required size: 8716288 > fully allocated size: 8716288 > bitmaps size: 1024 > internal snapshot size: 2048 > > where you now have to add three numbers prior to creating dest.qcow2 and > calling: > > $ qemu-img convert image.qcow2 -f dest.qcow2 --bitmaps --snapshots > > or using: > > $ qemu-img measure --bitmaps --snapshots -f qcow2 image.qcow2 > required size: 8719360 > fully allocated size: 8719360 > > where you then call: > > $ qemu-img convert image.qcow2 -f dest.qcow2 --bitmaps --snapshots > > with a single size that matches the same arguments you pass to qemu-img > convert? Yes, the second form is a good example of using --bitmaps consistently. > What about failure cases? What happens when qemu-img doesn't > understand --snapshots but does understand --bitmaps? Do you have to > try a series of up to three calls to find how much information is supported: > > $ qemu-img measure -f qcow2 image.qcow2 --bitmaps --snapshots > error: unknown argument > $ qemu-img measure -f qcow2 image.qcow2 --bitmaps > error: unknown argument > $ qemu-img measure -f qcow2 image.qcow2 > data given, now you know that neither --bitmaps nor --snapshots will work Assuming that I cannot require a version that support both features, which is usually the case when we have to support different platforms with different versions of qemu-img, I will check the capabilities using qemu-img --help and cache the result. For vdsm case we control the host so qemu-img should not be upgraded behind vdsm back. Then I will use the supported features. If both are missing, this convert will drop the the bitmaps and the snaphosts, and the next incremetnal backup will fail or fallback to full backup. > or is it nicer to issue just one measure without options, getting > separate output lines, and seeing which output lines exist to know which > convert options are supported, at the minor expense of having to add > values yourself? It is a little nicer since we don't need to check the capabilities, but we need to check them anyway for qemu-img convert, so this does not help much. But it introduces other issues: - we need to calculate the required size using required + bitmap or required + bitmaps + snapshots - what if measuring a snapshot is expensive - let's say take 20 seconds. This is still fast enough if the entire copy takes several minutes, so having a way to measure is useful. But users that do not care about the snapshot have to pay for this one every call. So we would end with a --snapshot flag to avoid this, and inconsistent API. > And then back to my question: should 'measure --bitmaps' fail if there > are no bitmaps to be measured, or silently succeed and not change the > output size? For raw file yes (invalid request), for qcow2 file no, it should just add 0 since this is the actual size required for bitmaps in this image. > >> With the current way, to measure an image we need to do: > >> > >> qemu-img info --output json ... > >> check if image contains bitmaps > >> qemu-img measure --output json ... > >> check if bitmaps are reported. > > Why do you have to check 'qemu-img info' first? If measure reports > bitmaps, then you know bitmaps can be copied; This works only if qemu-img measure will report "bitmaps": 0 when there are no bitmaps. Otherwise I don't know if this version does not report bitmaps because it does not understand them, or because there are no bitmaps. Using qemu-img info I can tell the difference if measure does not report 0. > if it doesn't, then you > can check info as a fallback path to compute size yourself - but > computing the size yourself doesn't help unless you also have fallbacks > to copy the bitmaps via QMP commands, because that qemu-img will also > lack 'qemu-img convert --bitmaps' or 'qemu-img bitmaps' to do it via > qemu-img. When we started to work on this it was not clear that we will have a way to measure bitmaps. If we are going to have support both in convert and measure, we can check capability only in convert or only in measure. > >> If image has bitmaps and bitmaps are not reported, we know that we have an > >> old > >> version of qemu-img that does not know how to measure bitmaps. > > > > Well, but you’ll also see that convert --bitmaps will fail. But I can > > see that you probably want to know about that before you create the > > target image. > > > >> If bitmaps are reported in both commands we will use the value when > >> creating > >> block devices. > > > > And otherwise? Do you error out, or do you just omit --bitmaps from > > convert? > > > >> If we always report bitmaps even when they are zero, we don't need to > >> run "qemu-img info". > >> > >> But my preferred interface is: > >> > >> qemu-img measure --bitmaps ... > >> > >> And report bitmaps only if the user asked to get the value. In this > >> case the required and > >> fully-allocated values will include bitmaps. > > > > Well, it would be consistent with the convert interface. If you specify > > it for one, you specify it for the other. > > > > OTOH, this would mean having to pass around the @bitmaps bool in the > > block layer, which is a bit more difficult than just adding a new field > > in BlockMeasureInfo. It would also mean to add a new bool every time we > > add a new extension (which you hinted at above that it might happen). > > Or, that could be a CLI-only feature: the QMP interface _always_ reports > bitmaps separately, but if 'qemu-img measure --bitmaps' is used, the CLI > then adds that value in on your behalf after the QMP command but before > printing to the end user. > > > > > (We could also let img_measure() in qemu-img add @bitmaps to @required > > if --bitmaps was given, so we wouldn’t have to pass the bool around; but > > I think letting qemu-img fix up a QAPI object filled by the block driver > > sounds wrong. (Because that means the block driver didn’t fill it > > correctly.)) > > If we only touch it up in the CLI, then we would have two forms of CLI > output: > > $ qemu-img measure -f qcow2 image.qcow2 > required size: 8716288 > fully allocated size: 8716288 > bitmaps size: 1024 > $ qemu-img measure -f qcow2 image.qcow2 --bitmaps > required size: 8717312 > fully allocated size: 8717312 I hope we will not have 2 forms. qemu-img is complicated enough ;-) > > And I don’t see how the interface proposed here by Eric (or rather, what > > I think we had agreed on for the next version) poses any problems for > > users. If you want to copy bitmaps, you just use @required + @bitmaps. > > (If @bitmaps isn’t present, you can’t copy bitmaps, so that should be > > an error.) If you don’t want to copy bitmaps, you just use @required. > > > > (And if you want to copy bitmaps if present, you use @required + > > @bitmaps, and treat @bitmaps as 0 if not present.) > > > >> To learn if qemu-img measure understand bitmaps we can check --help > >> output, like we did > >> in the past until we can require the version on all platforms. > >> > >> An advantage is not having to change old tests. > > I personally don’t really consider that a significant advantage... (On > > the contrary, seeing the field in all old tests means the code path is > > exercised more often, even though it’s supposed to always just report 0.) > > > > So all in all the main benefit I see in your proposal, i.e. having > > @bitmaps be included in @required with --bitmaps given, is that it would > > give a symmetrical interface between measure and convert: For simple > > cases, you can just replace the “convert” in your command line by > > “measure”, retrieve @required/@fully-allocated, create the target image > > based on that, and then re-run the same command line, but with “convert” > > this time. > > > > But I’m not sure whether that’s really an advantage in practice or more > > of a gimmick. With Eric’s proposal, if you want to convert with > > bitmaps, just add @bitmaps to the target size. If you don’t, don’t. If > > you’d prefer to but don’t really care, add “@bitmaps ?: 0”. > > > > The benefit of Eric’s proposal (not including @bitmaps in @required or > > @fully-allocated) is that it doesn’t require passing an additional > > parameter to the block driver. It also makes the definition of > > BlockMeasureInfo simpler. With your proposal, it would need to be > > parameterized. (As in, @required sometimes includes the bitmaps, > > sometimes it doesn’t, depending on the parameter used to retrieve > > BlockMeasureInfo.) I’m not sure whether that even makes sense in the > > QAPI definition. > > I'm leaning towards making v4 try a CLI-only 'measure --bitmaps', to see > if I can speed the discussion along with concrete patches for comparison. Thanks, that would be useful. Nir
