Re: Avoid copying unallocated clusters during full backup

[John Snow]

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;
>