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@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.
I see. I agree copying the bitmaps whenever possible is a good idea.
Reviewed-by: Jiri Denemark <jdenemar@xxxxxxxxxx>
