On Fri, Apr 17, 2020 at 1:11 PM John Snow <js...@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
