[PATCH 18/19] qemu: migration: Migrate block dirty bitmaps corresponding to checkpoints

[Jiri Denemark]

On Thu, Feb 18, 2021 at 16:50:43 +0100, Peter Krempa wrote:
> On Thu, Feb 18, 2021 at 15:54:37 +0100, Jiri Denemark wrote:
> > On Thu, Feb 11, 2021 at 16:37:57 +0100, Peter Krempa wrote:
> > > Preserve block dirty bitmaps after migration with
> > > QEMU_MONITOR_MIGRATE_NON_SHARED_(DISK|INC).
> > > 
> > > This patch implements functions which offer the bitmaps to the
> > > destination, check for eligibility on destination and then configure
> > > source for the migration.
> > > 
> > > Signed-off-by: Peter Krempa <pkrempa at redhat.com>
> > > ---
> > >  src/qemu/qemu_migration.c | 333 +++++++++++++++++++++++++++++++++++++-
> > >  1 file changed, 331 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> > > index 36424f8493..16bfad0390 100644
> > > --- a/src/qemu/qemu_migration.c
> > > +++ b/src/qemu/qemu_migration.c
> > ...
> > > @@ -2528,6 +2619,92 @@ qemuMigrationDstPrepare(virDomainObjPtr vm,
> > >                                       migrateFrom, fd, NULL);
> > >  }
> > > 
> > > +
> > > +/**
> > > + * qemuMigrationDstPrepareAnyBlockDirtyBitmaps:
> > > + * @vm: domain object
> > > + * @mig: migration cookie
> > > + * @migParams: migration parameters
> > > + * @flags: migration flags
> > > + *
> > > + * Checks whether block dirty bitmaps offered by the migration source are
> > > + * to be migrated (e.g. they don't exist, the destination is compatible etc)
> > > + * and sets up destination qemu for migrating the bitmaps as well as updates the
> > > + * list of eligible bitmaps in the migration cookie to be sent back to the
> > > + * source.
> > > + */
> > > +static int
> > > +qemuMigrationDstPrepareAnyBlockDirtyBitmaps(virDomainObjPtr vm,
> > > +                                            qemuMigrationCookiePtr mig,
> > > +                                            qemuMigrationParamsPtr migParams,
> > > +                                            unsigned int flags)
> > > +{
> > > +    qemuDomainObjPrivatePtr priv = vm->privateData;
> > > +    g_autoptr(virJSONValue) mapping = NULL;
> > > +    g_autoptr(GHashTable) blockNamedNodeData = NULL;
> > > +    GSList *nextdisk;
> > > +
> > > +    if (!mig->nbd ||
> > > +        !mig->blockDirtyBitmaps ||
> > > +        !(flags & (VIR_MIGRATE_NON_SHARED_DISK | VIR_MIGRATE_NON_SHARED_INC)) ||
> > > +        !virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATION_PARAM_BLOCK_BITMAP_MAPPING))
> > > +        return 0;
> > 
> > Shouldn't we report an error in case the source sent bitmaps, but local
> > QEMU does not support QEMU_CAPS_MIGRATION_PARAM_BLOCK_BITMAP_MAPPING?
> 
> See below.
> 
> > 
> > > +
> > > +    if (qemuMigrationCookieBlockDirtyBitmapsMatchDisks(vm->def, mig->blockDirtyBitmaps) < 0)
> > > +        return -1;
> > > +
> > > +    if (!(blockNamedNodeData = qemuBlockGetNamedNodeData(vm, QEMU_ASYNC_JOB_MIGRATION_IN)))
> > > +        return -1;
> > > +
> > > +    for (nextdisk = mig->blockDirtyBitmaps; nextdisk; nextdisk = nextdisk->next) {
> > > +        qemuMigrationBlockDirtyBitmapsDiskPtr disk = nextdisk->data;
> > > +        qemuBlockNamedNodeDataPtr nodedata;
> > > +        GSList *nextbitmap;
> > > +
> > > +        if (!(nodedata = virHashLookup(blockNamedNodeData, disk->nodename))) {
> > > +            virReportError(VIR_ERR_INTERNAL_ERROR,
> > > +                           _("failed to find data for block node '%s'"),
> > > +                           disk->nodename);
> > > +            return -1;
> > > +        }
> > > +
> > > +        /* don't migrate bitmaps into non-qcow2v3+ images */
> > 
> > How about "Bitmaps can only be migrated to qcow2 v3+"?
> > 
> > > +        if (disk->disk->src->format != VIR_STORAGE_FILE_QCOW2 ||
> > > +            nodedata->qcow2v2) {
> > > +            disk->skip = true;
> > 
> > Is skipping the disk the right thing to do? Should we report an error
> > and abort migration instead? Just checking, maybe we can't do so for
> > backward compatibility...
> > 
> > > +            continue;
> > > +        }
> > > +
> > > +        for (nextbitmap = disk->bitmaps; nextbitmap; nextbitmap = nextbitmap->next) {
> > > +            qemuMigrationBlockDirtyBitmapsDiskBitmapPtr bitmap = nextbitmap->data;
> > > +            size_t k;
> > > +
> > > +            /* don't migrate into existing bitmaps */
> > > +            for (k = 0; k < nodedata->nbitmaps; k++) {
> > > +                if (STREQ(bitmap->bitmapname, nodedata->bitmaps[k]->name)) {
> > > +                    bitmap->skip = true;
> > 
> > And similar questions for bitmaps here.
> 
> That would require that we have the users explicitly enable this feature
> rather than doing it implicitly. The disk format can be changed during
> the migration.
> 
> If we want to do it explicitly only I'll need to add a migration flag
> and all the infra for it.

I see. I agree copying the bitmaps whenever possible is a good idea.

Reviewed-by: Jiri Denemark <jdenemar at redhat.com>