On 4/17/20 3:11 PM, John Snow wrote:
>> +
>> + if (s->sync_mode == MIRROR_SYNC_MODE_FULL &&
>> + s->bcs->target->bs->drv != NULL &&
>> + strncmp(s->bcs->target->bs->drv->format_name, "qcow2", 5) == 0 &&
>> + s->bcs->source->bs->backing_file[0] == '\0')
>
> This isn't going to suffice upstream; the backup job can't be performing
> format introspection to determine behavior on the fly.
Agreed. The idea is right (we NEED to make backup operations smarter
based on knowledge about both source and destination block status), but
the implementation is not (a check for strcncmp("qcow2") is not ideal).
I see/agree that using strncmp("qcow2") is not general enough for the upstream. Would changing it to bdrv_unallocated_blocks_are_zero() suffice?
>
> I think what you're really after is something like
> bdrv_unallocated_blocks_are_zero().
The fact that qemu-img already has a lot of optimizations makes me
wonder what we can salvage from there into reusable code that both
qemu-img and block backup can share, so that we're not reimplementing
block status handling in multiple places.
A general fix reusing some existing code would be great. When will it appear in the upstream? We are hoping to avoid needing to use a private branch if possible.
> So the basic premise is that if you are copying a qcow2 file and the
> unallocated portions as defined by the qcow2 metadata are zero, it's
> safe to skip those, so you can treat it like SYNC_MODE_TOP.
>
> I think you *also* have to know if the *source* needs those regions
> explicitly zeroed, and it's not always safe to just skip them at the
> manifest level.
>
> I thought there was code that handled this to some extent already, but I
> don't know. I think Vladimir has worked on it recently and can probably
> let you know where I am mistaken :)
Yes, I'm hoping Vladimir (or his other buddies at Virtuozzo) can chime
in. Meanwhile, I've working on v2 of some patches that will improve
qemu's ability to tell if a destination qcow2 file already reads as all
zeroes, and we already have bdrv_block_status() for telling which
portions of a source image already read as all zeroes (whether or not it
is due to not being allocated, the goal here is that we should NOT have
to copy anything that reads as zero on the source over to the
destination if the destination already starts life as reading all zero).
Can the eventual/optimal solution allow unallocated clusters to be skipped entirely in the backup loop and make the detection of allocated zeroes an option, not forcing the backup thread to loop through a potentially huge empty virtual disk?
And if nothing else, qemu 5.0 just added 'qemu-img convert
--target-is-zero' as a last-ditch means of telling qemu to assume the
destination reads as all zeroes, even if it cannot quickly prove it; we
probably want to add a similar knob into the QMP commands for initiating
block backup, for the same reasons.
This seems a good way of assuring the status of the target file.
Thanks!
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org