Re: [PATCH v2 6/6] iotests: Add test 291 to for qemu-img bitmap coverage

[Eric Blake <eblake@redhat.com>]

On 5/4/20 8:05 AM, Max Reitz wrote:
> On 21.04.20 23:20, Eric Blake wrote:
>> Add a new test covering the 'qemu-img bitmap' subcommand, as well as
>> 'qemu-img convert --bitmaps', both added in recent patches.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>

>> +echo
>> +echo "=== Bitmap preservation not possible to non-qcow2 ==="
>> +echo
>> +
>> +mv "$TEST_IMG" "$TEST_IMG.orig"
> 
> “mv” doesn’t work images with external data files.
> 
> (ORIG_IMG=$TEST_IMG; TEST_IMG="$TEST_IMG".orig should work)

Good idea.

> 
>> +$QEMU_IMG convert --bitmaps -O raw "$TEST_IMG.orig" "$TEST_IMG"
>> +
>> +echo
>> +echo "=== Convert with bitmap preservation ==="
>> +echo
>> +
>> +# Only bitmaps from the active layer are copied
> 
> That’s kind of obvious when you think about (whenever an image is
> attached to a VM, only the active layer’s bitmaps are visible, not those
> from the backing chain), but maybe this should be noted in the
> documentation?

As part of integrating bitmaps with external snapshots, libvirt actually 
depends on being able to see bitmaps from the backing chain - but as 
bitmaps are always referenced as a 'node name, bitmap name' tuple, this 
is indeed doable.

> 
>> +$QEMU_IMG convert --bitmaps -O qcow2 "$TEST_IMG.orig" "$TEST_IMG"
>> +$QEMU_IMG info "$TEST_IMG" | _filter_img_info --format-specific
>> +# But we can also merge in bitmaps from other layers
>> +$QEMU_IMG bitmap --add --disabled -f $IMGFMT "$TEST_IMG" b0
>> +$QEMU_IMG bitmap --add -f $IMGFMT "$TEST_IMG" tmp
>> +$QEMU_IMG bitmap --merge b0 -b "$TEST_IMG.base" -F $IMGFMT "$TEST_IMG" tmp
>> +$QEMU_IMG bitmap --merge tmp "$TEST_IMG" b0
>> +$QEMU_IMG bitmap --remove -f $IMGFMT "$TEST_IMG" tmp
> 
> Why do we need tmp here?  Can’t we just merge base’s b0 directly into
> $TEST_IMG’s b0?

Yes, we could.  But then I wouldn't cover as many bitmap subcommands. 
Adding a comment about why the example is contrived (for maximal 
coverage) is a good idea.


>> +=== Check bitmap contents ===
>> +
>> +[{ "start": 0, "length": 3145728, "depth": 0, "zero": false, "data": true, "offset": OFFSET},
>> +{ "start": 3145728, "length": 1048576, "depth": 0, "zero": false, "data": false},
>> +{ "start": 4194304, "length": 6291456, "depth": 0, "zero": false, "data": true, "offset": OFFSET}]
>> +[{ "start": 0, "length": 1048576, "depth": 0, "zero": false, "data": true, "offset": OFFSET},
>> +{ "start": 1048576, "length": 1048576, "depth": 0, "zero": false, "data": false},
>> +{ "start": 2097152, "length": 8388608, "depth": 0, "zero": false, "data": true, "offset": OFFSET}]
>> +[{ "start": 0, "length": 2097152, "depth": 0, "zero": false, "data": true, "offset": OFFSET},
>> +{ "start": 2097152, "length": 1048576, "depth": 0, "zero": false, "data": false},
>> +{ "start": 3145728, "length": 7340032, "depth": 0, "zero": false, "data": true, "offset": OFFSET}]
> 
> Am I looking at this wrong or does the bitmap data seem to be inverted?
>   Everywhere where I’d expect the bitmaps to be cleared, this map reports
> data=true, whereas where I’d expect them to be set, it reports data=false.
> 
> I suppose that’s intentional, but can you explain this behavior to me?

This is an artifact of how x-dirty-bitmap works (it has the x- prefix 
because it is a hack, but we don't have anything better for reading out 
bitmap contents).  The NBD spec returns block status as a 32-bit value 
for a 'metadata context'; normally, we use context 'base:allocation' 
context where bit 0 is set for holes or clear for allocated, and bit 1 
is set for reads-as-zero or clear for unknown contents (favoring all-0 
as the most-common case).  But with x-dirty-bitmap, we are instead 
abusing NBD to query the 'qemu:dirty-bitmap:FOO' context, where bit 0 is 
set for anywhere the bitmap has a 1, yet feed that information into the 
pre-existing qemu code for handling block status.  So qemu-img map is 
reporting "data":true for what it thinks is the normal 0-for-allocated, 
and "data":false for 1-for-sparse, and we just have to translate that 
back into an understanding of what the bitmap reported.  Yes, a comment 
would be helpful.

I would _really_ love to enhance 'qemu-img map' to output image-specific 
metadata _in addition_ to the existing "zero" and "data" fields (by 
having qemu-img read two NBD contexts at once: both base:allocation and 
qemu:dirty-bitmap:FOO), at which point we can drop the x- prefix and 
avoid the abuse of qemu's internals by overwriting the block_status 
code.  But that's a bigger project.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org