On Fri, Jun 11, 2021 at 5:59 PM Eric Blake <ebl...@redhat.com> wrote: > > On Fri, Jun 11, 2021 at 05:35:12PM +0300, Vladimir Sementsov-Ogievskiy wrote: > > > An obvious solution is to make 'qemu-img map --output=json' > > > distinguish between clusters that have a local allocation from those > > > that are found nowhere in the chain. We already have a one-off > > > mismatch between qemu-img map and NBD qemu:allocation-depth (the > > > former chose 0, and the latter 1 for the local layer), so exposing the > > > latter's choice of 0 for unallocated in the entire chain would mean > > > using using "depth":-1 in the former, but a negative depth may confuse > > > existing tools. But there is an easy out: for any chain of length N, > > > we can simply represent an unallocated cluster as "depth":N+1. This > > > does have a slight risk of confusing any tool that might try to > > > dereference NULL when finding the backing image for the last file in > > > the backing chain, but that risk sseems worth the more precise output. > > > The iotests have several examples where this distinction demonstrates > > > the additional accuracy. > > > > > > Signed-off-by: Eric Blake <ebl...@redhat.com> > > > --- > > > > > > Replaces v1: 20210610213906.1313440-1-ebl...@redhat.com > > > (qemu-img: Use "depth":-1 to make backing probes obvious) > > > > > > Use N+1 instead of -1 for unallocated [Kevin] > > > > > > > Bit in contrast with -1, or with separate boolean flag, you lose the > > possibility to distinguish case when we have 3 layers and the cluster is > > absent in all of them, and the case when we have 4 layers and the cluster > > is absent in top 3 but in 4 it is qcow2 UNALLOCATED_ZERO cluster. > > Using just 'qemu-img map --output-json', you only see depth numbers. > You also have to use 'qemu-img info --backing-chain' to see what file > those depth numbers correspond to, at which point it becomes obvious > whether "depth":4 meant unallocated (because the chain was length 3) > or allocated at depth 4 (because the chain was length 4 or longer). > But that's no worse than pre-patch, where you had to use qemu-img info > --backing-chain to learn which file a particular "depth" maps to. > > > > > So, if someone use this API to reconstruct the chain, then for original 3 > > empty layers he will create 3 empty layers and 4rd additional ZERO layer. > > And such reconstructed chain would not be equal to original chain (as if we > > take these two chains and add additional backing file as a new bottom > > layer, effect would be different).. I'm not sure is it a problem in the > > task you are solving :\ > > It should be fairly easy to optimize the case of a backing chain where > EVERY listed cluster at the final depth was "data":false,"zero":true > to omit that file after all. > > And in oVirt's case, Nir pointed out that we have one more tool at our > disposal in recreating a backing chain: if you use > json:{"driver":"qcow2", "backing":null, ...} as your image file, you > don't have to worry about arbitrary files in the backing chain, only > about recreating the top-most layer of a chain. And in that case, it > becomes very obvious that "depth":0 is something you must recreate, > and "depth":1 would be a non-existent backing file because you just > passed "backing":null.
Note that oVirt does not use qemu-img map, we use qemu-nbd to get image extents, since it is used only in context we already connect to qemu-nbd server or run qemu-nbd. Management tools already know the image format (they should avoid doing format probing anyway), and using a json uri allows single command to get the needed info when you inspect a single layer. But this change introduces a risk that some program using qemu-img map will interrupt the result in the wrong way, assuming that there is N+1 layer. I think adding a new flag for absent extents is better. It cannot break any user and it is easier to understand and use. Nir