On Wed, Apr 17, 2019 at 09:09:21 -0500, Eric Blake wrote:
> Complete wiring up incremental backup, by adding in support for
> creating a checkpoint at the same time as a backup (make the
> transaction have a few more steps) as well as exposing the dirty
> bitmap for a prior backup over NBD (requires creating a temporary
> bitmap, merging all appropriate bitmaps in, then exposing that
> bitmap over NBD).
>
> Signed-off-by: Eric Blake <eblake@xxxxxxxxxx>
> ---
> src/qemu/qemu_driver.c | 198 +++++++++++++++++++++++++++++++++++------
> 1 file changed, 170 insertions(+), 28 deletions(-)
Again, I don't quite understand the split here. It makes the new code
actually harder to review.
> @@ -17695,19 +17713,44 @@ qemuDomainCheckpointDelete(virDomainCheckpointPtr checkpoint,
>
> static int
> qemuDomainBackupPrepare(virQEMUDriverPtr driver, virDomainObjPtr vm,
> - virDomainBackupDefPtr def)
> + virDomainBackupDefPtr def,
> + virDomainMomentObjPtr chk)
> {
> int ret = -1;
> size_t i;
> + virDomainCheckpointDefPtr chkdef;
>
> + chkdef = chk ? virDomainCheckpointObjGetDef(chk) : NULL;
virDomainCheckpointDefPtr chkdef = NULL
if (chk)
chkdef = virDomainCheckpointObjGetDef(chk);
This is way more readable.
> + if (chk && def->ndisks != chkdef->ndisks) {
Riddle for the compiler? Why don't you actually check 'chkdef' instead of
'chk'?
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("inconsistency between backup and checkpoint disks"));
> + goto cleanup;
> + }
> if (qemuBlockNodeNamesDetect(driver, vm, QEMU_ASYNC_JOB_NONE) < 0)
> goto cleanup;
> for (i = 0; i < def->ndisks; i++) {
> virDomainBackupDiskDef *disk = &def->disks[i];
> virStorageSourcePtr src = vm->def->disks[disk->idx]->src;
>
> - if (!disk->store)
> + /* For now, insist that atomic checkpoint affect same disks as
> + * those being backed up. */
> + if (!disk->store) {
> + if (chk &&
see above
> + chkdef->disks[i].type != VIR_DOMAIN_CHECKPOINT_TYPE_NONE) {
> + virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> + _("disk %s requested checkpoint without backup"),
> + disk->name);
> + goto cleanup;
> + }
> continue;
> + }
> + if (chk &&
> + chkdef->disks[i].type != VIR_DOMAIN_CHECKPOINT_TYPE_BITMAP) {
> + virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> + _("disk %s requested backup without checkpoint"),
> + disk->name);
> + goto cleanup;
> + }
> if (virAsprintf(&disk->store->nodeformat, "tmp-%s", disk->name) < 0)
> goto cleanup;
> if (!disk->store->format)
[...]
> @@ -17779,28 +17822,22 @@ qemuDomainBackupBegin(virDomainPtr domain, const char *diskXml,
> bool job_started = false;
> bool nbd_running = false;
> bool push;
> + const char *mode;
> size_t i;
> struct timeval tv;
> char *suffix = NULL;
> virCommandPtr cmd = NULL;
> const char *qemuImgPath;
> + virDomainCheckpointDefPtr chkdef = NULL;
> + virDomainMomentObjPtr chk = NULL;
> + virDomainMomentObjPtr other = NULL;
> + virDomainMomentObjPtr parent = NULL;
> + virDomainMomentObjPtr current;
> + virJSONValuePtr arr = NULL;
>
> virCheckFlags(VIR_DOMAIN_BACKUP_BEGIN_NO_METADATA, -1);
> /* TODO: VIR_DOMAIN_BACKUP_BEGIN_QUIESCE */
>
> - // FIXME: Support non-null checkpointXML for incremental - what
> - // code can be shared with CheckpointCreateXML, then add to transaction
> - // to create new checkpoint at same time as starting blockdev-backup
> - if (checkpointXml) {
> - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> - _("cannot create incremental backups yet"));
> - return -1;
> - }
> - // if (chk) VIR_STRDUP(suffix, chk->name);
> - gettimeofday(&tv, NULL);
> - if (virAsprintf(&suffix, "%lld", (long long)tv.tv_sec) < 0)
> - goto cleanup;
> -
Looks like leftovers form previous patches?
> if (!(vm = qemuDomObjFromDomain(domain)))
> goto cleanup;
>
> @@ -17827,6 +17864,17 @@ qemuDomainBackupBegin(virDomainPtr domain, const char *diskXml,
> if (!(def = virDomainBackupDefParseString(diskXml, driver->xmlopt, 0)))
> goto cleanup;
>
> + if (checkpointXml) {
> + if (!(chkdef = qemuDomainCheckpointDefParseString(driver, caps,
> + checkpointXml, 0)) ||
> + VIR_STRDUP(suffix, chkdef->common.name) < 0)
> + goto cleanup;
> + } else {
> + gettimeofday(&tv, NULL);
> + if (virAsprintf(&suffix, "%lld", (long long)tv.tv_sec) < 0)
> + goto cleanup;
> + }
> +
> push = def->type == VIR_DOMAIN_BACKUP_TYPE_PUSH;
> if (!push) {
> if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NBD_BITMAP)) {
> @@ -17864,15 +17912,25 @@ qemuDomainBackupBegin(virDomainPtr domain, const char *diskXml,
> goto cleanup;
> }
> }
> + current = virDomainCheckpointGetCurrent(vm->checkpoints);
> if (def->incremental) {
> if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BITMAP_MERGE)) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> _("qemu binary lacks persistent bitmaps support"));
> goto cleanup;
> }
> - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> - _("cannot create incremental backups yet"));
> - goto cleanup;
> + for (other = current; other;
> + other = other->def->parent ?
> + virDomainCheckpointFindByName(vm->checkpoints,
> + other->def->parent) : NULL)
> + if (STREQ(other->def->name, def->incremental))
> + break;
> + if (!other) {
> + virReportError(VIR_ERR_OPERATION_INVALID,
> + _("could not locate checkpoint '%s' for incremental backup"),
> + def->incremental);
> + goto cleanup;
> + }
> }
>
> if (!(qemuImgPath = qemuFindQemuImgBinary(driver)))
> @@ -17888,14 +17946,38 @@ qemuDomainBackupBegin(virDomainPtr domain, const char *diskXml,
> goto endjob;
> }
>
> + if (chkdef) {
> + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BITMAP_MERGE)) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("qemu binary lacks persistent bitmaps support"));
> + goto endjob;
> + }
> +
> + if (qemuDomainCheckpointPrepare(driver, caps, vm, chkdef) < 0)
> + goto endjob;
> + if (!(chk = virDomainCheckpointAssignDef(vm->checkpoints, chkdef)))
> + goto endjob;
> + chkdef = NULL;
> + if (current) {
> + parent = current;
> + if (VIR_STRDUP(chk->def->parent, parent->def->name) < 0)
> + goto endjob;
> + if (qemuDomainCheckpointWriteMetadata(vm, parent, driver->caps,
> + driver->xmlopt,
> + cfg->checkpointDir) < 0)
> + goto endjob;
> + }
> + }
> +
> if (virDomainBackupAlignDisks(def, vm->def, suffix) < 0 ||
> - qemuDomainBackupPrepare(driver, vm, def) < 0)
> + qemuDomainBackupPrepare(driver, vm, def, chk) < 0)
> goto endjob;
>
> /* actually start the checkpoint. 2x2 array of push/pull, full/incr,
> plus additional tweak if checkpoint requested */
> qemuDomainObjEnterMonitor(driver, vm);
> - /* - push/pull: blockdev-add per <disk> */
> + /* - push/pull: blockdev-add per <disk>
> + - incr: bitmap-add of tmp, bitmap-merge per <disk> */
> for (i = 0; i < def->ndisks; i++) {
> virDomainBackupDiskDef *disk = &def->disks[i];
> virJSONValuePtr file;
> @@ -17961,11 +18043,32 @@ qemuDomainBackupBegin(virDomainPtr domain, const char *diskXml,
> goto endmon;
> json = NULL;
> disk->state = VIR_DOMAIN_BACKUP_DISK_STATE_READY;
> +
> + if (def->incremental) {
> + if (!(arr = virJSONValueNewArray()))
> + goto endmon;
Declare this inside the for loop with VIR_AUTOPTR to avoid all the
Frees below
> + if (qemuMonitorAddBitmap(priv->mon, node, disk->name, false) < 0) {
> + virJSONValueFree(arr);
> + goto endmon;
> + }
> + disk->state = VIR_DOMAIN_BACKUP_DISK_STATE_BITMAP;
> + for (other = parent ? parent : current; other;
> + other = other->def->parent ?
> + virDomainCheckpointFindByName(vm->checkpoints,
Too many ternaries. The for loop is totaly unreadable.
> + other->def->parent) : NULL) {
> + if (virJSONValueArrayAppendString(arr, other->def->name) < 0) {
> + virJSONValueFree(arr);
> + goto endmon;
> + }
> + if (STREQ(other->def->name, def->incremental))
> + break;
> + }
> + if (qemuMonitorMergeBitmaps(priv->mon, node, disk->name, &arr) < 0)
This monitor API should better document that it binary-ands all the
bimaps in &arr to 'target'. I had to look into the qemu docs.
> + goto endmon;
> + }
> }
>
> - /* TODO:
> - - incr: bitmap-add of tmp, bitmap-merge per <disk>
> - - transaction, containing:
> + /* - transaction, containing:
> - push+full: blockdev-backup sync:full
> - push+incr: blockdev-backup sync:incremental bitmap:tmp
> - pull+full: blockdev-backup sync:none
> @@ -17974,24 +18077,61 @@ qemuDomainBackupBegin(virDomainPtr domain, const char *diskXml,
> */
> if (!(json = virJSONValueNewArray()))
> goto endmon;
> + if (push)
> + mode = def->incremental ? "incremental" : "full";
You have one full condition, thus expanding the ternary will not hurt.
> + else
> + mode = "none";
> for (i = 0; i < def->ndisks; i++) {
> virDomainBackupDiskDef *disk = &def->disks[i];
> - virStorageSourcePtr src = vm->def->disks[disk->idx]->src;
> + const char *node;
> + const char *push_bitmap = NULL;
>
> if (!disk->store)
> continue;
> + node = qemuBlockNodeLookup(vm, disk->name);
> + if (push && def->incremental)
> + push_bitmap = disk->name;
> if (qemuMonitorJSONTransactionAdd(json,
> "blockdev-backup",
> - "s:device", src->nodeformat,
> + "s:device", node,
> "s:target", disk->store->nodeformat,
> - "s:sync", push ? "full" : "none",
> + "s:sync", mode,
> + "S:bitmap", push_bitmap,
> "s:job-id", disk->name,
> NULL) < 0)
> goto endmon;
> + if (def->incremental && !push &&
> + qemuMonitorJSONTransactionAdd(json,
> + "block-dirty-bitmap-disable",
> + "s:node", node,
> + "s:name", disk->name,
All the conditions here are quite confusing to read. I think we need to
split the function at least into two so that there's only one dimension
of conditionals depending on the requested operation.
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
