On 4/17/20 2:33 PM, Leo Luan wrote:
> When doing a full backup from a single layer qcow2 disk file to a new
> qcow2 file, the backup_run function does not unset unallocated parts in
> the copy bit map. The subsequent backup_loop call goes through these
> unallocated clusters unnecessarily. In the case when the target and
> source reside in different file systems, an EXDEV error would cause
> zeroes to be actually copied into the target and that causes a target
> file size explosion to the full virtual disk size.
>
I think the idea, generally, is to leave the detection of unallocated
portions to the format (qcow2) and the protocol (posix file) respectively.
As far as I know, it is incorrect to assume that unallocated data
can/will/should always be read as zeroes; so it may not be the case that
it is "safe" to skip this data, because the target may or may not need
explicit zeroing.
> This patch aims to unset the unallocated parts in the copy bitmap when
> it is safe to do so, thereby avoid dealing with unallocated clusters in
> the backup loop to prevent significant performance or storage efficiency
> impacts when running full backup jobs.
>
> Any insights or corrections?
>
> diff --git a/block/backup.c b/block/backup.c
> index cf62b1a38c..609d551b1e 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -139,6 +139,29 @@ static void backup_clean(Job *job)
> bdrv_backup_top_drop(s->backup_top);
> }
>
> +static bool backup_ok_to_skip_unallocated(BackupBlockJob *s)
> +{
> + /* Checks whether this backup job can avoid copying or dealing with
> + unallocated clusters in the backup loop and their associated
> + performance and storage effciency impacts. Check for the condition
> + when it's safe to skip copying unallocated clusters that allows the
> + corresponding bits in the copy bitmap to be unset. The assumption
> + here is that it is ok to do so when we are doing a full backup,
> + the target file is a qcow2, and the source is single layer.
> + Do we need to add additional checks (so that it does not break
> + something) or add addtional conditions to optimize additional use
> + cases?
> + */
> +
> + 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.
I think what you're really after is something like
bdrv_unallocated_blocks_are_zero().
> + return true;
> + else
> + return false;
> +}
> +
> void backup_do_checkpoint(BlockJob *job, Error **errp)
> {
> BackupBlockJob *backup_job = container_of(job, BackupBlockJob, common);
> @@ -248,7 +271,7 @@ static int coroutine_fn backup_run(Job *job, Error
> **errp)
>
> backup_init_copy_bitmap(s);
>
> - if (s->sync_mode == MIRROR_SYNC_MODE_TOP) {
> + if (s->sync_mode == MIRROR_SYNC_MODE_TOP ||
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 :)
--js
> backup_ok_to_skip_unallocated(s)) {
> int64_t offset = 0;
> int64_t count;
>
