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