On Tue, Jun 08, 2021 at 07:38:10PM +0300, Nir Soffer wrote: > On Tue, Jun 8, 2021 at 12:22 AM Eric Blake <ebl...@redhat.com> wrote: > > > > On Mon, Jun 07, 2021 at 11:22:04PM +0300, Nir Soffer wrote: > > > When zeroing a cluster in an image with backing file, qemu-img and > > > qemu-nbd reported the area as a hole. This does not affect the guest > > > since the area is read as zero, but breaks code trying to reconstruct > > > the image chain based on qemu-img map or qemu-nbd block status response. > > > > Trying to reconstruct the image chain based on qemu-nbd block status > > should not be attempted on just base:allocation data, but should also > > take into account qemu:allocation-depth. > > This is correct when looking at the entire chain, but when we reconstruct > image data, we copy each image in the layer *without* the backing chain. > > The example I provided was not detailed enough, what we actually do is: > > qemu-nbd .. 'json:{"driver": "qcow2", "backing": null, "file": > {"driver": "file", "filename": "top.qcow2"}}' > > So there is no backing chain and allocation depth is not relevant. > - Allocated areas should be reported with flags 0 > - Zero areas which are not holes should be reported as NBD_STATE_ZERO > - Zero areas which are holes (not allocated in this image) should be > reported as NBD_STATE_HOLE
Again, what you WANT is qemu:allocation-depth. $ ./qemu-nbd -r -t -f qcow2 -A 'json:{"driver":"qcow2", "backing":null, \ "file":{"driver":"file", "filename":"top.qcow2"}}' $ nbdinfo --map=qemu:allocation-depth nbd://localhost 0 65536 0 unallocated 65536 131072 1 local 196608 65536 0 unallocated $ nbdinfo --map nbd://localhost 0 65536 3 hole,zero 65536 65536 0 allocated 131072 131072 3 hole,zero You don't care whether the information reads as zero or not, but whether top.qcow2 is responsible for the data at that cluster. base:allocation does not answer that question. But qemu:allocation-depth answers it perfectly. > > > From the perspective of the > > core NBD protocol, there is no backing file, so trying to guess what > > the backing file contains without using qemu extensions is unlikely to > > be correct, as shown in your example. The fact that you could abuse > > it with qemu 5.2 but it broke in 6.0 > > I'm not abusing anything, I'm only using public APIs. qemu-nbd behavior > should not change without good reason, and we did not have any good > reason to change the behavior for qcow2 images. Ah, but we did. Exposing BDRV_BLOCK_ALLOCATED as server, but consuming it as BDRV_BLOCK_DATA as client, was inconsistent. It was a bug that we ever used BLOCK_ALLOCATED in the first place, when it has _always_ been that the NBD semantics were supposed to be modeled on our definition of BLOCK_DATA. That it took us a couple of years to notice our bug is unfortunate, but we DO have a good reason for the change - we were fixing an actual bug where we were reporting incorrect information compared to what the NBD spec was documenting. > > > is not necessarily the sign of a > > regression in 6.0, but rather could be evidence that you have been > > trying to use an undocumented implementation quirk rather than a > > stable interface. > > I'm pretty convinced that this is a regression in qemu-nbd 6.0 since I created > this regression :-) I understand that you were surprised by the ramifications of your patch causing more changes than what you expected, but I still argue that your patch was correct and that the decision to incorporate it was intentional because it was the right thing to do. Papering over the fallout for the sake of clients that should be using qemu:allocation-depth instead does not seem like it is worth the maintenance nightmare to me. > > Since we started using qemu-nbd in 2018, qemu-nbd has always reported > holes in qcow2 images, but not in raw files. We discussed this several times, > and you explained that we have allocation information from qcow2, but not > from raw format. > > My attempt to fix hole reporting in raw images has failed; reporting holes in > raw images is nice to have, but it broke the behavior of qemu-nbd with qcow2 > images, which is a critical issue for ovirt. Rather, ovirt had been relying on buggy behavior, and now that the bug has been fixed, we are scrambling to figure out how to make ovirt still play nicely. But my answer to that is to use qemu:allocation-depth. It was introduced in 5.2, so it predates the point where base:allocation behavior was fixed, and it provides the answer to the question you are really asking (which parts of my image came from the image directly, rather than a backing file), rather than merely an indirect answer (how can I abuse the determination of which parts of the image are allocated or sparse to imply that those same portions must come from a backing image). There is nothing semantically wrong with a sparse cluster in the top layer overriding a non-sparse cluster from the backing file. > > The code using this was tested and released 3-4 month ago. This was added > to support backup vendors using snapshot based backup, so they can move > to use the NBD based pipeline, which is safer than the old way, uploading > qcow2 images directly to storage. Downstream can play games with backporting or reverting patches as needed to make the entire ecosystem play nicely, but upstream, I don't think it is right to take a patch that re-introduces a bug in base:allocation merely because a client was unwilling to use the correct interface of qemu:allocation-depth to get the answer to the question they really meant to ask. > > If I revert: > > commit 0da9856851dcca09222a1467e16ddd05dc66e460 > Author: Nir Soffer <nir...@gmail.com> > Date: Fri Feb 19 18:07:52 2021 +0200 > > nbd: server: Report holes for raw images > > qemu-nbd reports zeroed areas in a useful way like it always did: > > $ ./qemu-nbd -r -t 'json:{"driver": "qcow2", "backing": null, "file": > {"driver": "file", "filename": "top.qcow2"}}' & > > $ nbdinfo --map nbd://localhost > 0 65536 3 hole,zero > 65536 65536 0 allocated > 131072 65536 2 zero > 196608 65536 3 hole,zero > > There is no need to use allocation depth info, the base:allocation works fine > for this use case, and the output makes sense. But you have to remember that the NBD specification for base:allocation is that flags are OPTIONAL at all times. You are at the mercy of the server for which flags it sets; it is ALWAYS valid for a server to reply with status 0 for any cluster; any server is free to return 0 in the future where it returned 1 today (perhaps because the server changed its mind on how expensive it was to compute when it is safe to set the bit to 1), or to return 1 in the future where it returned 0 today (perhaps because the server gained insight into how to quickly report something that happens to be true about that cluster). Basing decisions on what was returned as a 1 bit in NBD_CMD_BLOCK_STATUS is okay if those decisions are solely for an optimization purpose (because we KNOW this portion of this disk will read as zero, we don't have to read it), but wrong if they are for an accuracy purpose (assuming that only clusters where hole is clear is a 1:1 correspondence to which clusters override a backing image). And this is a case where qcow2 happens to quickly know that a sparse zero cluster exists, so reporting hole,zero IS more accurate than mere zero. The fact that we failed to omit hole in the past was not a violation of the NBD spec, but rather a bug in our code for looking at the wrong bit of information. But the mere presence of the hole bit is NOT sufficient knowledge to know that the NBD server is serving a qcow2 image that has a non-allocated zero cluster overriding the backing image, because we can't even guarantee the server is visiting a qcow2 file. > > ...but since NBD has no notion of a backing file, there is nothing > > that qemu can do to report depth information itself. If you want to > > reconstruct the backing chain, you should be able to further query > > qemu:allocation-depth, and piece the two queries together to get what > > you need: > > > > $ ./qemu-nbd -r -t -f qcow2 top.qcow2 -A > > What suggest is: > > 1. Get base:allocation - where the cluster is wrongly reported a hole The report is not wrong. That portion of the qcow2 file IS a hole, in that it is not pre-allocated and therefore has no on-disk storage reserved. It will still read as all-zeroes rather than deferring to the backing image, but calling it a hole is not wrong. > 2. Get also qemu:allocation-depath > 3. Merge both lists of extents, splitting and extending extents as needed > since there is no guarantee that we get same lists describing same extents > 4. Finally get correct base:allocation showing the zero range is not a hole > > This is a lot of unneeded and complicated work to cover a bug in qemu, > and it should be done in all clients. I don't find this an attractive > solution. Unfortunately, it is the only viable long-term solution. I would love to make libnbd handle the reconstruction of a unified view of multiple NBD metadata contexts in an easier manner, so that upper-layer applications could simply use libnbd to get at that work without having to repeat the coding of reconstructing things themselves. But I don't see any way around the fact that from a correctness perspective, this really IS the only way to get at the information you need (not only a knowledge of which portions of the disk are holes, but also which portions of the disk override a backing image). > > > > cluster is merged with forth cluster which is actually a hole. > > > > > > This is incorrect since if it was a hole, the third cluster would be > > > exposed to the guest. Programs using qemu-nbd output to reconstruct the > > > image chain on other storage would be confused and copy only the first 2 > > > cluster. The results of this copy will be an image exposing the third > > > cluster from the base image, corrupting the guest data. > > > > This is where I disagree - if the NBD protocol exposed the notion of a > > backing file, then reporting a local hole should indeed imply reading > > from the backing file. But since base NBD protocol does NOT support > > backing images of any sort, you cannot make any assumptions about what > > base:allocation says about holes, other than that the image presented > > over NBD was not fully allocated in some manner at that location. You > > instead need to fix your tooling to query qemu:allocation-depth if you > > are trying to reconstruct all state known by qemu. > > When we look at the entire chain, you are correct, but when we disable > the backing file and have no backing chain, NBD protocol hole bit is > the right way to describe an unallocated area in a qcow2 image. Your problem is that qcow2 images have two ways to represent a zero cluster. One is a pre-allocated cluster (there is host storage reserved; the host storage might or might not read as zero, but the qcow2 code will return zero, regardless of whether there is a backing file); the other is an unallocated cluster (there is no host storage reserved, but the qcow2 code still reads zero regardless of whether there is a backing file). One of those is allocated, the other is sparse. And NBD_STATE_HOLE is a great fit for telling those two types of clusters apart. In short, qemu has 3 bits of interesting information per cluster: whether it reads as zero, whether it has allocated space, and whether it overrides a backing file. The NBD protocol base:allocation only provides two of those bits of information (BDRV_BLOCK_DATA and BDRV_BLOCK_ZERO); to get at the third (BDRV_BLOCK_ALLOCATED), you HAVE to use a qemu-specific extension. That said, we CAN provide a _new_ qemu:block-status metacontext, which returns a bitmap of bit 0 (NBD_STATE_HOLE), bit 1 (NBD_STATE_ZERO), and bit 2 (BDRV_BLOCK_ALLOCATED) all from a single context, at which point you don't have to reconstruct the combination of base:allocation (two bits) and qemu:allocation-depth (an arbitrary integer) across possibly differing reporting lengths yourself. I'll post a counter-proposal patch along those lines, shortly. > > > > I found that it can be fixed using BDRV_BLOCK_OFFSET_VALID when > > > reporting the status of the extent. When we have a valid offset, we > > > report based on BDRV_BLOCK_DATA. Otherwise we report based on > > > BDRV_BLOCK_ALLOCATED. > > > > That sounds hairy. As an initial reaction, I'm not sure I like it. > > > > > > > > With this fix we get: > > > > > > $ build/qemu-img map --output json top.qcow2 > > > [{ "start": 0, "length": 65536, "depth": 1, "zero": false, "data": > > > true, "offset": 0}, > > > { "start": 65536, "length": 65536, "depth": 0, "zero": false, "data": > > > true, "offset": 327680}, > > > { "start": 131072, "length": 65536, "depth": 0, "zero": true, "data": > > > true}, > > > > Earlier, this line was "data":false, which made sense - there was no > > offset in either top.qcow2 nor in raw.base at which you could mmap to > > read the actual zeroes that were implied by the unallocated zero > > cluster. Your change here is reporting "data":true to state that the > > zeroes explicitly come from the "depth":0 layer, although it is a bit > > misleading because we did not actually allocate clusters in top.qcow2 > > for reading the zeroes. In reality, this really IS an instance of a > > qcow2 unallocated cluster, where "data":false fits better for the > > definitions in include/block/block.h. > > Well, this cluster is not a hole. How do you describe "not a hole" in > qemu-img map? Ah, but the cluster _is_ a hole. There is no storage space occupied in the qcow2 file corresponding to that slice of the file. So from the qcow2's perspective, that region of guest-visible address space comes from a hole, where the guest writing to it will require an allocation of additional host storage space. "not a hole" in qcow2 corresponds to any pre-allocated cluster, and shows up in qemu-img map as "data":true. > data=true seem that be the only way. > > But again we can leave qemu-img map as is since we have the depth argument. And since the depth parameter answered your question with direct qcow2, it should also answer your question over NBD with qemu:allocation-depth. In other words, you are using 3 bits of information to reconstruct your qcow2 file, so you need to ask for all 3 pieces of information over NBD. > > > > { "start": 196608, "length": 65536, "depth": 1, "zero": true, "data": > > > false, "offset": 196608}] > > > > > > $ build/qemu-nbd -r -t -f qcow2 top.qcow2 & > > > $ qemu-img map --output json nbd://localhost > > > [{ "start": 0, "length": 131072, "depth": 0, "zero": false, "data": > > > true, "offset": 0}, > > > { "start": 131072, "length": 65536, "depth": 0, "zero": true, "data": > > > true, "offset": 131072}, > > > { "start": 196608, "length": 65536, "depth": 0, "zero": true, "data": > > > false, "offset": 196608}] > > > > Meanwhile, this output is indeed arguably more precise, although it > > differs from the qcow2 output in that you provide an offset for > > cluster 2. > > I guess qemu-img have less information when using qemu-nbd, the image > is presented as > a raw image. > ... > > > - flags = (ret & BDRV_BLOCK_DATA ? 0 : NBD_STATE_HOLE) | > > > - (ret & BDRV_BLOCK_ZERO ? NBD_STATE_ZERO : 0); > > > + flags = (ret & BDRV_BLOCK_ZERO ? NBD_STATE_ZERO : 0); > > > + > > > + if (ret & BDRV_BLOCK_OFFSET_VALID) > > > + flags |= (ret & BDRV_BLOCK_DATA ? 0 : NBD_STATE_HOLE); > > > + else > > > + flags |= (ret & BDRV_BLOCK_ALLOCATED ? 0 : NBD_STATE_HOLE); > > > > This will fall apart on compressed or encrypted images, where data is > > allocated but offset_valid is false. > > So this is not the right way to trigger the use of BDRV_BLOCK_DATA. The intent, all along, was that NBD's STATE_HOLE was identical in semantics to !BDRV_BLOCK_DATA (the inverted value was chosen because having a default of all 0s as the safe point was easier to document in the NBD spec). Vladimir and I had a bug when first implementing that intent, but your patch fixed that bug. Unfortunately, as you have pointed out, that fix has now exposed clients that were abusing the bug to infer information about an NBD server that was not actually viable to be inferred (whether a portion of an image is a hole is NOT a reliable indicator of whether that portion of the image overrides a backing file, but it wasn't until we started accurately reporting HOLEs in more contexts that it actually tripped you up). > > > > if (nbd_extent_array_add(ea, num, flags) < 0) { > > > return 0; > > > diff --git a/qemu-img.c b/qemu-img.c > > > index a5993682aa..6808e12d87 100644 > > > --- a/qemu-img.c > > > +++ b/qemu-img.c > > > @@ -3039,7 +3039,9 @@ static int get_block_status(BlockDriverState *bs, > > > int64_t offset, > > > *e = (MapEntry) { > > > .start = offset, > > > .length = bytes, > > > - .data = !!(ret & BDRV_BLOCK_DATA), > > > + .data = !!(has_offset > > > + ? ret & BDRV_BLOCK_DATA > > > + : ret & BDRV_BLOCK_ALLOCATED), > > > > I'm really not sure about this. You are not only changing what > > qemu-nbd advertises as a server, but also what qemu-img interprets as > > a client. Are you sure this will still work when you mix-and-match > > old server + new client, or new server + old client? > > I did not test these combinations, but let's simplify by removing this change > for now. > > > > .zero = !!(ret & BDRV_BLOCK_ZERO), > > > .offset = map, > > > .has_offset = has_offset, > > > -- > > > 2.26.3 > > > > > Since the original change introduced a regression breaking existing users, we > don't have good enough tests in this area, and is not clear how this > can be fixed > properly, I suggest to revert the original change for 6.0. Then we have time > to > work on a proper fix upstream. 6.0 is already released upstream, so you are stuck with that new behavior. And I think it is worse to ping-pong the behavior back to the way it was in 5.2 for 6.1, than to just keep the 6.0 behavior (which fixed a bug) unchanged going forward into 6.1 upstream. Downstream, we can backport and/or revert whatever patches we want to make the downstream ecosystem play nicely. But that is an argument for downstream. > > What do you think? I'll go ahead and work up my counter-proposal patch to add yet another qemu:... context to make it easier for you to get combined status in one go. I don't know whether it will be attractive enough to be worth incorporating upstream, but if it is, that is certainly another option that downstream can use, because it is easier to backport a new qemu: context than it is to play games with "which behavior will base:allocation have" - you are guaranteed that if qemu knows how to advertise the new context, it provides the information you want. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org