Re: [PATCH] qemu-img: Use "depth":-1 to make backing probes obvious

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

11.06.2021 11:08, Vladimir Sementsov-Ogievskiy wrote:
> 11.06.2021 00:39, Eric Blake wrote:
>> The recently-added NBD context qemu:allocation-depth makes an obvious
>> case for why it is important to distinguish between locally-present
>> data (even with that data is sparse) [shown as depth 1 over NBD], and
>> data that could not be found anywhere in the backing chain [shown as
>> depth 0].  But qemu-img map --output=json predates that addition, and
>> has the unfortunate behavior that all portions of the backing chain
>> that resolve without finding a hit in any backing layer report the
>> same depth as the final backing layer.  This makes it harder to
>> reconstruct a qcow2 backing chain using just 'qemu-img map' output
>> when using "backing":null to artificially limit a backing chain,
>> because it is impossible to distinguish between a
>> QCOW2_CLUSTER_UNALLOCATED (which defers to a [missing] backing file)
>> and a QCOW2_CLUSTER_ZERO_PLAIN cluster (which would override any
>> backing file), since both types of clusters otherwise show as
>> "data":false,"zero":true" (but note that we can distinguish a
>> QCOW2_CLUSTER_ZERO_ALLOCATED, which would also have an "offset":
>> listing).
>>
>> The task of reconstructing a qcow2 chain was made harder in commit
>> 0da9856851 (nbd: server: Report holes for raw images), because prior
>> to that point, it was possible to abuse NBD's block status command to
>> see which portions of a qcow2 file resulted in BDRV_BLOCK_ALLOCATED
>> (showing up as NBD_STATE_ZERO in isolation) vs. missing from the chain
>> (showing up as NBD_STATE_ZERO|NBD_STATE_HOLE); but now qemu reports
>> more accurate sparseness information over NBD.
>>
>> An obvious solution is to make 'qemu-img map --output=json' visually
>> distinguish between clusters that have a local allocation from those
>> that are found nowhere in the chain, by adding "depth":-1 as the new
>> witness of data that could not be tied to a specific backing image.
>> Several iotests are impacted, but glancing through the changes shows
>> that it is an improvement in that it shows more accurate details.
>>
>> Note that the documentation is specifically worded to allow qemu-img
>> to report "depth":-1 both in the scenario where the last file in the
>> backing chain still defers the cluster (corresponding to
>> BDRV_BLOCK_ALLOCATED not being set anywhere in the chain), and in the
>> scenario where qemu is unable to determine which backing chain element
>> (if any) provides the data.  The latter case does not exist now, but
>> I'm considering an upcoming patch to add a BDRV_BLOCK_BACKING that
>> would let a specific driver (such as NBD) inform the block layer that
>> it is known that a cluster comes from a backing layer, but where there
>> is insufficient data to determine which layer.
>>
>> As a quick demonstration:
>>
>>      # Create a qcow2 image with a raw backing file:
>>      $ qemu-img create base.raw $((4*64*1024))
>>      $ qemu-img create -f qcow2 -b base.raw -F raw top.qcow2
>>
>>      # Write to first 3 clusters of base:
>>      $ qemu-io -f raw -c "w -P 65 0 64k" -c "w -P 66 64k 64k" \
>>        -c "w -P 67 128k 64k" base.raw
>>
>>      # Write to second and third clusters of top, hiding base:
>>      $ qemu-io -f qcow2 -c "w -P 69 64k 64k" -c "w -z 128k 64k" top.qcow2
>>
>>      # Examine the full backing chain
>>      $ qemu-img map --output=json -f qcow2 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": false},
>>      { "start": 196608, "length": 65536, "depth": 1, "zero": true, "data": false, "offset": 196608}]
>>
>>      # Repeat, but with the backing chain clamped. Pre-patch:
>>      $ qemu-img map --output=json -f qcow2 json:'{"driver":"qcow2", \
>>        "backing":null, "file":{"driver":"file", "filename":"top.qcow2"}}'
>>      [{ "start": 0, "length": 65536, "depth": 0, "zero": true, "data": false},
>>      { "start": 65536, "length": 65536, "depth": 0, "zero": false, "data": true, "offset": 327680},
>>      { "start": 131072, "length": 131072, "depth": 0, "zero": true, "data": false}]
>>
>>      # Repeat, but post-patch:
>>      $ qemu-img map --output=json -f qcow2 json:'{"driver":"qcow2", \
>>        "backing":null, "file":{"driver":"file", "filename":"top.qcow2"}}'
>>      [{ "start": 0, "length": 65536, "depth": -1, "zero": true, "data": false},
>>      { "start": 65536, "length": 65536, "depth": 0, "zero": false, "data": true, "offset": 327680},
>>      { "start": 131072, "length": 65536, "depth": 0, "zero": true, "data": false},
>>      { "start": 196608, "length": 65536, "depth": -1, "zero": true, "data": false}]
>>
>> Note that pre-patch, it was impossible to determine which portions of
>> the qcow2 file override the backing file because the "depth":0 regions
>> were combined, so even though qemu internally can tell the difference
>> between sclusters 2 and 3, the command line user could not.  But
>> post-patch, the "depth":-1 markings match the "depth":1 markings when
>> the backing chain is intact, and it becomes obvious which clusters are
>> important.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> I'm not sure do we need a kind of new option or deprecation period for such incompatible change..

I see Kevin worries about same thing..

So, probably better is just add a new optional boolean field, that shows that data is "nowhere in the chain". Such change is a lot more compatible.

> 
> Personally I'm OK with it and like the idea:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>


-- 
Best regards,
Vladimir