On Fri, Apr 17, 2020 at 1:11 PM John Snow <jsnow@redhat.com> wrote:

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

Thanks for pointing this out.  Would it be safe to skip unallocated
clusters if both source and target's bdrv_unallocated_blocks_are_zero()
returns true?

> 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().
>

Thanks for this pointer.


>
> > +       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.
>

In the MIRROR_SYNC_MODE_TOP case, the check for unallocated clusters does
not go all the way to the base level.  So it would be incorrect to treat
the MIRROR_SYNC_MODE_FULL the same as MIRROR_SYNC_MODE_TOP unless the
source does not have a backing and the base itself.  That's why I added a
check for the backing_file field of the source.  I guess the code can be
potentially extended with a new flag to do the block status check all the
way into the base level for the case of the FULL mode?

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

Do you mean some operation changing the target into non-sparse?

>
> 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 :)
>

Thanks for the reply!


> --js
>
> > backup_ok_to_skip_unallocated(s)) {
> >          int64_t offset = 0;
> >          int64_t count;
> >
>
> John Snow