Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; --- src/qemu/qemu_block.c | 177 +++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_block.h | 9 +++ src/qemu/qemu_driver.c | 162 +------------------------------------ 3 files changed, 187 insertions(+), 161 deletions(-) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index 8a6f601b29..825db3e088 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -3196,3 +3196,180 @@ qemuBlockExportAddNBD(virDomainObj *vm, return qemuMonitorBlockExportAdd(priv->mon, &nbdprops); } + + +int +qemuBlockCommit(virDomainObj *vm, + virDomainDiskDef *disk, + virStorageSource *baseSource, + virStorageSource *topSource, + virStorageSource *top_parent, + unsigned long bandwidth, + unsigned int flags) +{ + qemuDomainObjPrivate *priv = vm->privateData; + virQEMUDriver *driver = priv->driver; + int ret = -1; + bool clean_access = false; + g_autofree char *backingPath = NULL; + qemuBlockJobData *job = NULL; + g_autoptr(virStorageSource) mirror = NULL; + + if (virDomainObjCheckActive(vm) < 0) + return -1; + + /* Convert bandwidth MiB to bytes, if necessary */ + if (!(flags & VIR_DOMAIN_BLOCK_COMMIT_BANDWIDTH_BYTES)) { + if (bandwidth > LLONG_MAX >> 20) { + virReportError(VIR_ERR_OVERFLOW, + _("bandwidth must be less than %llu"), + LLONG_MAX >> 20); + return -1; + } + bandwidth <<= 20; + } + + if (!qemuDomainDiskBlockJobIsSupported(disk)) + return -1; + + if (virStorageSourceIsEmpty(disk->src)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("disk %s has no source file to be committed"), + disk->dst); + return -1; + } + + if (qemuDomainDiskBlockJobIsActive(disk)) + return -1; + + if (qemuDomainSupportsCheckpointsBlockjobs(vm) < 0) + return -1; + + if (topSource == disk->src) { + /* XXX Should we auto-pivot when COMMIT_ACTIVE is not specified? */ + if (!(flags & VIR_DOMAIN_BLOCK_COMMIT_ACTIVE)) { + virReportError(VIR_ERR_INVALID_ARG, + _("commit of '%s' active layer requires active flag"), + disk->dst); + return -1; + } + } else if (flags & VIR_DOMAIN_BLOCK_COMMIT_ACTIVE) { + virReportError(VIR_ERR_INVALID_ARG, + _("active commit requested but '%s' is not active"), + topSource->path); + return -1; + } + + if (!virStorageSourceHasBacking(topSource)) { + virReportError(VIR_ERR_INVALID_ARG, + _("top '%s' in chain for '%s' has no backing file"), + topSource->path, disk->src->path); + return -1; + } + + if ((flags & VIR_DOMAIN_BLOCK_COMMIT_SHALLOW) && + baseSource != topSource->backingStore) { + virReportError(VIR_ERR_INVALID_ARG, + _("base '%s' is not immediately below '%s' in chain " + "for '%s'"), + baseSource->path, topSource->path, disk->src->path); + return -1; + } + + /* For an active commit, clone enough of the base to act as the mirror */ + if (topSource == disk->src) { + if (!(mirror = virStorageSourceCopy(baseSource, false))) + return -1; + if (virStorageSourceInitChainElement(mirror, + disk->src, + true) < 0) + return -1; + } + + if (flags & VIR_DOMAIN_BLOCK_COMMIT_RELATIVE && + topSource != disk->src) { + if (top_parent && + qemuBlockUpdateRelativeBacking(vm, top_parent, disk->src) < 0) + return -1; + + if (virStorageSourceGetRelativeBackingPath(topSource, baseSource, + &backingPath) < 0) + return -1; + + if (!backingPath) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("can't keep relative backing relationship")); + return -1; + } + } + + /* For the commit to succeed, we must allow qemu to open both the + * 'base' image and the parent of 'top' as read/write; 'top' might + * not have a parent, or might already be read-write. XXX It + * would also be nice to revert 'base' to read-only, as well as + * revoke access to files removed from the chain, when the commit + * operation succeeds, but doing that requires tracking the + * operation in XML across libvirtd restarts. */ + clean_access = true; + if (qemuDomainStorageSourceAccessAllow(driver, vm, baseSource, + false, false, false) < 0) + goto cleanup; + + if (top_parent && top_parent != disk->src) { + /* While top_parent is topmost image, we don't need to remember its + * owner as it will be overwritten upon finishing the commit. Hence, + * pass chainTop = false. */ + if (qemuDomainStorageSourceAccessAllow(driver, vm, top_parent, + false, false, false) < 0) + goto cleanup; + } + + if (!(job = qemuBlockJobDiskNewCommit(vm, disk, top_parent, topSource, + baseSource, + flags & VIR_DOMAIN_BLOCK_COMMIT_DELETE, + flags))) + goto cleanup; + + disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_NONE; + + if (!backingPath && top_parent && + !(backingPath = qemuBlockGetBackingStoreString(baseSource, false))) + goto cleanup; + + qemuDomainObjEnterMonitor(vm); + + ret = qemuMonitorBlockCommit(priv->mon, + qemuDomainDiskGetTopNodename(disk), + job->name, + topSource->nodeformat, + baseSource->nodeformat, + backingPath, bandwidth); + + qemuDomainObjExitMonitor(vm); + + if (ret < 0) + goto cleanup; + + if (mirror) { + disk->mirror = g_steal_pointer(&mirror); + disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_ACTIVE_COMMIT; + } + qemuBlockJobStarted(job, vm); + + cleanup: + if (ret < 0 && clean_access) { + virErrorPtr orig_err; + virErrorPreserveLast(&orig_err); + /* Revert access to read-only, if possible. */ + qemuDomainStorageSourceAccessAllow(driver, vm, baseSource, + true, false, false); + if (top_parent && top_parent != disk->src) + qemuDomainStorageSourceAccessAllow(driver, vm, top_parent, + true, false, false); + + virErrorRestore(&orig_err); + } + qemuBlockJobStartupFinalize(vm, job); + + return ret; +} diff --git a/src/qemu/qemu_block.h b/src/qemu/qemu_block.h index 8a3a10344e..85b4805d89 100644 --- a/src/qemu/qemu_block.h +++ b/src/qemu/qemu_block.h @@ -276,3 +276,12 @@ qemuBlockExportAddNBD(virDomainObj *vm, const char *exportname, bool writable, const char *bitmap); + +int +qemuBlockCommit(virDomainObj *vm, + virDomainDiskDef *disk, + virStorageSource *baseSource, + virStorageSource *topSource, + virStorageSource *top_parent, + unsigned long bandwidth, + unsigned int flags); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d509582719..2f05da3d8c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15114,19 +15114,12 @@ qemuDomainBlockCommit(virDomainPtr dom, unsigned long bandwidth, unsigned int flags) { - virQEMUDriver *driver = dom->conn->privateData; - qemuDomainObjPrivate *priv; virDomainObj *vm = NULL; int ret = -1; virDomainDiskDef *disk = NULL; virStorageSource *topSource; virStorageSource *baseSource = NULL; virStorageSource *top_parent = NULL; - bool clean_access = false; - g_autofree char *backingPath = NULL; - unsigned long long speed = bandwidth; - qemuBlockJobData *job = NULL; - g_autoptr(virStorageSource) mirror = NULL; virCheckFlags(VIR_DOMAIN_BLOCK_COMMIT_SHALLOW | VIR_DOMAIN_BLOCK_COMMIT_ACTIVE | @@ -15136,7 +15129,6 @@ qemuDomainBlockCommit(virDomainPtr dom, if (!(vm = qemuDomainObjFromDomain(dom))) goto cleanup; - priv = vm->privateData; if (virDomainBlockCommitEnsureACL(dom->conn, vm->def) < 0) goto cleanup; @@ -15144,176 +15136,24 @@ qemuDomainBlockCommit(virDomainPtr dom, if (virDomainObjBeginJob(vm, VIR_JOB_MODIFY) < 0) goto cleanup; - if (virDomainObjCheckActive(vm) < 0) - goto endjob; - - /* Convert bandwidth MiB to bytes, if necessary */ - if (!(flags & VIR_DOMAIN_BLOCK_COMMIT_BANDWIDTH_BYTES)) { - if (speed > LLONG_MAX >> 20) { - virReportError(VIR_ERR_OVERFLOW, - _("bandwidth must be less than %llu"), - LLONG_MAX >> 20); - goto endjob; - } - speed <<= 20; - } - if (!(disk = qemuDomainDiskByName(vm->def, path))) goto endjob; - if (!qemuDomainDiskBlockJobIsSupported(disk)) - goto endjob; - - if (virStorageSourceIsEmpty(disk->src)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("disk %s has no source file to be committed"), - disk->dst); - goto endjob; - } - - if (qemuDomainDiskBlockJobIsActive(disk)) - goto endjob; - - if (qemuDomainSupportsCheckpointsBlockjobs(vm) < 0) - goto endjob; - if (!top || STREQ(top, disk->dst)) topSource = disk->src; else if (!(topSource = virStorageSourceChainLookup(disk->src, NULL, top, disk->dst, &top_parent))) goto endjob; - if (topSource == disk->src) { - /* XXX Should we auto-pivot when COMMIT_ACTIVE is not specified? */ - if (!(flags & VIR_DOMAIN_BLOCK_COMMIT_ACTIVE)) { - virReportError(VIR_ERR_INVALID_ARG, - _("commit of '%s' active layer requires active flag"), - disk->dst); - goto endjob; - } - } else if (flags & VIR_DOMAIN_BLOCK_COMMIT_ACTIVE) { - virReportError(VIR_ERR_INVALID_ARG, - _("active commit requested but '%s' is not active"), - topSource->path); - goto endjob; - } - - if (!virStorageSourceHasBacking(topSource)) { - virReportError(VIR_ERR_INVALID_ARG, - _("top '%s' in chain for '%s' has no backing file"), - topSource->path, path); - goto endjob; - } - if (!base && (flags & VIR_DOMAIN_BLOCK_COMMIT_SHALLOW)) baseSource = topSource->backingStore; else if (!(baseSource = virStorageSourceChainLookup(disk->src, topSource, base, disk->dst, NULL))) goto endjob; - if ((flags & VIR_DOMAIN_BLOCK_COMMIT_SHALLOW) && - baseSource != topSource->backingStore) { - virReportError(VIR_ERR_INVALID_ARG, - _("base '%s' is not immediately below '%s' in chain " - "for '%s'"), - base, topSource->path, path); - goto endjob; - } - - /* For an active commit, clone enough of the base to act as the mirror */ - if (topSource == disk->src) { - if (!(mirror = virStorageSourceCopy(baseSource, false))) - goto endjob; - if (virStorageSourceInitChainElement(mirror, - disk->src, - true) < 0) - goto endjob; - } - - if (flags & VIR_DOMAIN_BLOCK_COMMIT_RELATIVE && - topSource != disk->src) { - if (top_parent && - qemuBlockUpdateRelativeBacking(vm, top_parent, disk->src) < 0) - goto endjob; - - if (virStorageSourceGetRelativeBackingPath(topSource, baseSource, - &backingPath) < 0) - goto endjob; - - if (!backingPath) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("can't keep relative backing relationship")); - goto endjob; - } - } - - /* For the commit to succeed, we must allow qemu to open both the - * 'base' image and the parent of 'top' as read/write; 'top' might - * not have a parent, or might already be read-write. XXX It - * would also be nice to revert 'base' to read-only, as well as - * revoke access to files removed from the chain, when the commit - * operation succeeds, but doing that requires tracking the - * operation in XML across libvirtd restarts. */ - clean_access = true; - if (qemuDomainStorageSourceAccessAllow(driver, vm, baseSource, - false, false, false) < 0) - goto endjob; - - if (top_parent && top_parent != disk->src) { - /* While top_parent is topmost image, we don't need to remember its - * owner as it will be overwritten upon finishing the commit. Hence, - * pass chainTop = false. */ - if (qemuDomainStorageSourceAccessAllow(driver, vm, top_parent, - false, false, false) < 0) - goto endjob; - } - - if (!(job = qemuBlockJobDiskNewCommit(vm, disk, top_parent, topSource, - baseSource, - flags & VIR_DOMAIN_BLOCK_COMMIT_DELETE, - flags))) - goto endjob; - - disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_NONE; - - if (!backingPath && top_parent && - !(backingPath = qemuBlockGetBackingStoreString(baseSource, false))) - goto endjob; - - qemuDomainObjEnterMonitor(vm); - - ret = qemuMonitorBlockCommit(priv->mon, - qemuDomainDiskGetTopNodename(disk), - job->name, - topSource->nodeformat, - baseSource->nodeformat, - backingPath, speed); - - qemuDomainObjExitMonitor(vm); - - if (ret < 0) - goto endjob; - - if (mirror) { - disk->mirror = g_steal_pointer(&mirror); - disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_ACTIVE_COMMIT; - } - qemuBlockJobStarted(job, vm); + ret = qemuBlockCommit(vm, disk, baseSource, topSource, top_parent, bandwidth, flags); endjob: - if (ret < 0 && clean_access) { - virErrorPtr orig_err; - virErrorPreserveLast(&orig_err); - /* Revert access to read-only, if possible. */ - qemuDomainStorageSourceAccessAllow(driver, vm, baseSource, - true, false, false); - if (top_parent && top_parent != disk->src) - qemuDomainStorageSourceAccessAllow(driver, vm, top_parent, - true, false, false); - - virErrorRestore(&orig_err); - } - qemuBlockJobStartupFinalize(vm, job); virDomainObjEndJob(vm); cleanup: -- 2.39.0

Move the code for finishing a job in the ready state to qemu_block.c. Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_block.c | 121 ++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_block.h | 5 ++ src/qemu/qemu_driver.c | 123 +---------------------------------------- 3 files changed, 127 insertions(+), 122 deletions(-) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index 825db3e088..6d31269ddd 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -3373,3 +3373,124 @@ qemuBlockCommit(virDomainObj *vm, return ret; } + + +/* Called while holding the VM job lock, to implement a block job + * abort with pivot; this updates the VM definition as appropriate, on + * either success or failure. */ +int +qemuBlockPivot(virDomainObj *vm, + qemuBlockJobData *job, + virDomainDiskDef *disk) +{ + g_autoptr(qemuBlockStorageSourceChainData) chainattachdata = NULL; + int ret = -1; + qemuDomainObjPrivate *priv = vm->privateData; + g_autoptr(virJSONValue) bitmapactions = NULL; + g_autoptr(virJSONValue) reopenactions = NULL; + int rc = 0; + + if (job->state != QEMU_BLOCKJOB_STATE_READY) { + virReportError(VIR_ERR_BLOCK_COPY_ACTIVE, + _("block job '%s' not ready for pivot yet"), + job->name); + return -1; + } + + switch ((qemuBlockJobType) job->type) { + case QEMU_BLOCKJOB_TYPE_NONE: + case QEMU_BLOCKJOB_TYPE_LAST: + virReportError(VIR_ERR_INTERNAL_ERROR, + _("invalid job type '%d'"), job->type); + return -1; + + case QEMU_BLOCKJOB_TYPE_PULL: + case QEMU_BLOCKJOB_TYPE_COMMIT: + case QEMU_BLOCKJOB_TYPE_BACKUP: + case QEMU_BLOCKJOB_TYPE_INTERNAL: + case QEMU_BLOCKJOB_TYPE_CREATE: + case QEMU_BLOCKJOB_TYPE_BROKEN: + virReportError(VIR_ERR_OPERATION_INVALID, + _("job type '%s' does not support pivot"), + qemuBlockjobTypeToString(job->type)); + return -1; + + case QEMU_BLOCKJOB_TYPE_COPY: + if (!job->jobflagsmissing) { + bool shallow = job->jobflags & VIR_DOMAIN_BLOCK_COPY_SHALLOW; + bool reuse = job->jobflags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT; + + bitmapactions = virJSONValueNewArray(); + + if (qemuMonitorTransactionBitmapAdd(bitmapactions, + disk->mirror->nodeformat, + "libvirt-tmp-activewrite", + false, + false, + 0) < 0) + return -1; + + /* Open and install the backing chain of 'mirror' late if we can use + * blockdev-snapshot to do it. This is to appease oVirt that wants + * to copy data into the backing chain while the top image is being + * copied shallow */ + if (reuse && shallow && + virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV_SNAPSHOT_ALLOW_WRITE_ONLY) && + virStorageSourceHasBacking(disk->mirror)) { + + if (!(chainattachdata = qemuBuildStorageSourceChainAttachPrepareBlockdev(disk->mirror->backingStore))) + return -1; + + reopenactions = virJSONValueNewArray(); + + if (qemuMonitorTransactionSnapshotBlockdev(reopenactions, + disk->mirror->backingStore->nodeformat, + disk->mirror->nodeformat)) + return -1; + } + + } + break; + + case QEMU_BLOCKJOB_TYPE_ACTIVE_COMMIT: + bitmapactions = virJSONValueNewArray(); + + if (qemuMonitorTransactionBitmapAdd(bitmapactions, + job->data.commit.base->nodeformat, + "libvirt-tmp-activewrite", + false, + false, + 0) < 0) + return -1; + + break; + } + + qemuDomainObjEnterMonitor(vm); + + if (chainattachdata) { + if ((rc = qemuBlockStorageSourceChainAttach(priv->mon, chainattachdata)) == 0) { + /* install backing images on success, or unplug them on failure */ + if ((rc = qemuMonitorTransaction(priv->mon, &reopenactions)) != 0) + qemuBlockStorageSourceChainDetach(priv->mon, chainattachdata); + } + } + + if (bitmapactions && rc == 0) + ignore_value(qemuMonitorTransaction(priv->mon, &bitmapactions)); + + if (rc == 0) + ret = qemuMonitorJobComplete(priv->mon, job->name); + + qemuDomainObjExitMonitor(vm); + + /* The pivot failed. The block job in QEMU remains in the synchronised state */ + if (ret < 0) + return -1; + + if (disk && disk->mirror) + disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_PIVOT; + job->state = QEMU_BLOCKJOB_STATE_PIVOTING; + + return ret; +} diff --git a/src/qemu/qemu_block.h b/src/qemu/qemu_block.h index 85b4805d89..52deb15a3d 100644 --- a/src/qemu/qemu_block.h +++ b/src/qemu/qemu_block.h @@ -285,3 +285,8 @@ qemuBlockCommit(virDomainObj *vm, virStorageSource *top_parent, unsigned long bandwidth, unsigned int flags); + +int +qemuBlockPivot(virDomainObj *vm, + qemuBlockJobData *job, + virDomainDiskDef *disk); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2f05da3d8c..d8db7188f7 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -14001,127 +14001,6 @@ qemuDomainOpenChannel(virDomainPtr dom, } -/* Called while holding the VM job lock, to implement a block job - * abort with pivot; this updates the VM definition as appropriate, on - * either success or failure. */ -static int -qemuDomainBlockPivot(virDomainObj *vm, - qemuBlockJobData *job, - virDomainDiskDef *disk) -{ - g_autoptr(qemuBlockStorageSourceChainData) chainattachdata = NULL; - int ret = -1; - qemuDomainObjPrivate *priv = vm->privateData; - g_autoptr(virJSONValue) bitmapactions = NULL; - g_autoptr(virJSONValue) reopenactions = NULL; - int rc = 0; - - if (job->state != QEMU_BLOCKJOB_STATE_READY) { - virReportError(VIR_ERR_BLOCK_COPY_ACTIVE, - _("block job '%s' not ready for pivot yet"), - job->name); - return -1; - } - - switch ((qemuBlockJobType) job->type) { - case QEMU_BLOCKJOB_TYPE_NONE: - case QEMU_BLOCKJOB_TYPE_LAST: - virReportError(VIR_ERR_INTERNAL_ERROR, - _("invalid job type '%d'"), job->type); - return -1; - - case QEMU_BLOCKJOB_TYPE_PULL: - case QEMU_BLOCKJOB_TYPE_COMMIT: - case QEMU_BLOCKJOB_TYPE_BACKUP: - case QEMU_BLOCKJOB_TYPE_INTERNAL: - case QEMU_BLOCKJOB_TYPE_CREATE: - case QEMU_BLOCKJOB_TYPE_BROKEN: - virReportError(VIR_ERR_OPERATION_INVALID, - _("job type '%s' does not support pivot"), - qemuBlockjobTypeToString(job->type)); - return -1; - - case QEMU_BLOCKJOB_TYPE_COPY: - if (!job->jobflagsmissing) { - bool shallow = job->jobflags & VIR_DOMAIN_BLOCK_COPY_SHALLOW; - bool reuse = job->jobflags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT; - - bitmapactions = virJSONValueNewArray(); - - if (qemuMonitorTransactionBitmapAdd(bitmapactions, - disk->mirror->nodeformat, - "libvirt-tmp-activewrite", - false, - false, - 0) < 0) - return -1; - - /* Open and install the backing chain of 'mirror' late if we can use - * blockdev-snapshot to do it. This is to appease oVirt that wants - * to copy data into the backing chain while the top image is being - * copied shallow */ - if (reuse && shallow && - virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV_SNAPSHOT_ALLOW_WRITE_ONLY) && - virStorageSourceHasBacking(disk->mirror)) { - - if (!(chainattachdata = qemuBuildStorageSourceChainAttachPrepareBlockdev(disk->mirror->backingStore))) - return -1; - - reopenactions = virJSONValueNewArray(); - - if (qemuMonitorTransactionSnapshotBlockdev(reopenactions, - disk->mirror->backingStore->nodeformat, - disk->mirror->nodeformat)) - return -1; - } - - } - break; - - case QEMU_BLOCKJOB_TYPE_ACTIVE_COMMIT: - bitmapactions = virJSONValueNewArray(); - - if (qemuMonitorTransactionBitmapAdd(bitmapactions, - job->data.commit.base->nodeformat, - "libvirt-tmp-activewrite", - false, - false, - 0) < 0) - return -1; - - break; - } - - qemuDomainObjEnterMonitor(vm); - - if (chainattachdata) { - if ((rc = qemuBlockStorageSourceChainAttach(priv->mon, chainattachdata)) == 0) { - /* install backing images on success, or unplug them on failure */ - if ((rc = qemuMonitorTransaction(priv->mon, &reopenactions)) != 0) - qemuBlockStorageSourceChainDetach(priv->mon, chainattachdata); - } - } - - if (bitmapactions && rc == 0) - ignore_value(qemuMonitorTransaction(priv->mon, &bitmapactions)); - - if (rc == 0) - ret = qemuMonitorJobComplete(priv->mon, job->name); - - qemuDomainObjExitMonitor(vm); - - /* The pivot failed. The block job in QEMU remains in the synchronised state */ - if (ret < 0) - return -1; - - if (disk && disk->mirror) - disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_PIVOT; - job->state = QEMU_BLOCKJOB_STATE_PIVOTING; - - return ret; -} - - /* bandwidth in MiB/s per public API. Caller must lock vm beforehand, * and not access it afterwards. */ static int @@ -14284,7 +14163,7 @@ qemuDomainBlockJobAbort(virDomainPtr dom, qemuBlockJobSyncBegin(job); if (pivot) { - if ((ret = qemuDomainBlockPivot(vm, job, disk)) < 0) + if ((ret = qemuBlockPivot(vm, job, disk)) < 0) goto endjob; } else { qemuDomainObjEnterMonitor(vm); -- 2.39.0

This will allow to use it while having async domain job active which we will use when deleting external snapshots. Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_block.c | 4 +++- src/qemu/qemu_block.h | 1 + src/qemu/qemu_driver.c | 2 +- 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index d2a56ba19c..6a68362708 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -3402,6 +3402,7 @@ qemuBlockCommit(virDomainObj *vm, int qemuBlockPivot(virDomainObj *vm, qemuBlockJobData *job, + virDomainAsyncJob asyncJob, virDomainDiskDef *disk) { g_autoptr(qemuBlockStorageSourceChainData) chainattachdata = NULL; @@ -3487,7 +3488,8 @@ qemuBlockPivot(virDomainObj *vm, break; } - qemuDomainObjEnterMonitor(vm); + if (qemuDomainObjEnterMonitorAsync(vm, asyncJob) < 0) + return -1; if (chainattachdata) { if ((rc = qemuBlockStorageSourceChainAttach(priv->mon, chainattachdata)) == 0) { diff --git a/src/qemu/qemu_block.h b/src/qemu/qemu_block.h index a76d9be711..3f2082f102 100644 --- a/src/qemu/qemu_block.h +++ b/src/qemu/qemu_block.h @@ -290,4 +290,5 @@ qemuBlockCommit(virDomainObj *vm, int qemuBlockPivot(virDomainObj *vm, qemuBlockJobData *job, + virDomainAsyncJob asyncJob, virDomainDiskDef *disk); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 5ee6e2698d..acd67b1f5a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -14163,7 +14163,7 @@ qemuDomainBlockJobAbort(virDomainPtr dom, qemuBlockJobSyncBegin(job); if (pivot) { - if ((ret = qemuBlockPivot(vm, job, disk)) < 0) + if ((ret = qemuBlockPivot(vm, job, VIR_ASYNC_JOB_NONE, disk)) < 0) goto endjob; } else { qemuDomainObjEnterMonitor(vm); -- 2.39.0

Deleting external snapshots will require configuring autofinalize to synchronize the block jobs for disks withing single snapshot in order to be able safely abort of one of the jobs fails. Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_block.c | 3 ++- src/qemu/qemu_monitor.c | 9 +++++---- src/qemu/qemu_monitor.h | 3 ++- src/qemu/qemu_monitor_json.c | 4 ++-- src/qemu/qemu_monitor_json.h | 3 ++- tests/qemumonitorjsontest.c | 2 +- 6 files changed, 14 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index 6a68362708..99409a969b 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -3364,7 +3364,8 @@ qemuBlockCommit(virDomainObj *vm, job->name, topSource->nodeformat, baseSource->nodeformat, - backingPath, bandwidth); + backingPath, bandwidth, + VIR_TRISTATE_BOOL_YES); qemuDomainObjExitMonitor(vm); diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 0354832fa5..43525def0c 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2795,16 +2795,17 @@ qemuMonitorBlockCommit(qemuMonitor *mon, const char *topNode, const char *baseNode, const char *backingName, - unsigned long long bandwidth) + unsigned long long bandwidth, + virTristateBool autofinalize) { - VIR_DEBUG("device=%s, jobname=%s, topNode=%s, baseNode=%s, backingName=%s, bandwidth=%llu", + VIR_DEBUG("device=%s, jobname=%s, topNode=%s, baseNode=%s, backingName=%s, bandwidth=%llu, autofinalize=%d", device, NULLSTR(jobname), NULLSTR(topNode), - NULLSTR(baseNode), NULLSTR(backingName), bandwidth); + NULLSTR(baseNode), NULLSTR(backingName), bandwidth, autofinalize); QEMU_CHECK_MONITOR(mon); return qemuMonitorJSONBlockCommit(mon, device, jobname, topNode, baseNode, - backingName, bandwidth); + backingName, bandwidth, autofinalize); } diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index eebac918c1..d09439911f 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -973,7 +973,8 @@ int qemuMonitorBlockCommit(qemuMonitor *mon, const char *topNode, const char *baseNode, const char *backingName, - unsigned long long bandwidth) + unsigned long long bandwidth, + virTristateBool autofinalize) ATTRIBUTE_NONNULL(2); int qemuMonitorArbitraryCommand(qemuMonitor *mon, diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index baf01a7206..9aee84084e 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4028,11 +4028,11 @@ qemuMonitorJSONBlockCommit(qemuMonitor *mon, const char *topNode, const char *baseNode, const char *backingName, - unsigned long long speed) + unsigned long long speed, + virTristateBool autofinalize) { g_autoptr(virJSONValue) cmd = NULL; g_autoptr(virJSONValue) reply = NULL; - virTristateBool autofinalize = VIR_TRISTATE_BOOL_YES; virTristateBool autodismiss = VIR_TRISTATE_BOOL_NO; cmd = qemuMonitorJSONMakeCommand("block-commit", diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 073a0579b5..a03caf7740 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -315,7 +315,8 @@ qemuMonitorJSONBlockCommit(qemuMonitor *mon, const char *topNode, const char *baseNode, const char *backingName, - unsigned long long bandwidth) + unsigned long long bandwidth, + virTristateBool autofinalize) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); int diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index f224ccb364..1db1f2b949 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -1211,7 +1211,7 @@ GEN_TEST_FUNC(qemuMonitorJSONRemoveNetdev, "net0") GEN_TEST_FUNC(qemuMonitorJSONDelDevice, "ide0") GEN_TEST_FUNC(qemuMonitorJSONBlockdevMirror, "jobname", true, "vdb", "targetnode", 1024, 1234, 31234, true, true) GEN_TEST_FUNC(qemuMonitorJSONBlockStream, "vdb", "jobname", "backingnode", "backingfilename", 1024) -GEN_TEST_FUNC(qemuMonitorJSONBlockCommit, "vdb", "jobname", "topnode", "basenode", "backingfilename", 1024) +GEN_TEST_FUNC(qemuMonitorJSONBlockCommit, "vdb", "jobname", "topnode", "basenode", "backingfilename", 1024, VIR_TRISTATE_BOOL_YES) GEN_TEST_FUNC(qemuMonitorJSONScreendump, "devicename", 1, NULL, "/foo/bar") GEN_TEST_FUNC(qemuMonitorJSONOpenGraphics, "spice", "spicefd", false) GEN_TEST_FUNC(qemuMonitorJSONNBDServerAdd, "vda", "export", true, "bitmap") -- 2.39.0

Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_block.c | 32 ++++++++++++++++++++++++++++++++ src/qemu/qemu_block.h | 5 +++++ 2 files changed, 37 insertions(+) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index da3a1e8557..70811aa861 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -3525,3 +3525,35 @@ qemuBlockPivot(virDomainObj *vm, return ret; } + + +/** + * qemuBlockFinalize: + * @vm: domain object + * @job: qemu block job data object + * @asyncJob: qemu async job type + * + * When qemu job is started with autofinalize disabled it will wait in pending + * state for block job finalize to be called manually in order to finish the + * job. This is useful when we are running jobs on multiple disks to make + * a synchronization point before we finish. + * + * Return -1 on error, 0 on success. + */ +int +qemuBlockFinalize(virDomainObj *vm, + qemuBlockJobData *job, + virDomainAsyncJob asyncJob) +{ + int ret; + qemuDomainObjPrivate *priv = vm->privateData; + + if (qemuDomainObjEnterMonitorAsync(vm, asyncJob) < 0) + return -1; + + ret = qemuMonitorJobFinalize(priv->mon, job->name); + + qemuDomainObjExitMonitor(vm); + + return ret; +} diff --git a/src/qemu/qemu_block.h b/src/qemu/qemu_block.h index 7b6aec2a7d..c169432d9c 100644 --- a/src/qemu/qemu_block.h +++ b/src/qemu/qemu_block.h @@ -293,3 +293,8 @@ qemuBlockPivot(virDomainObj *vm, qemuBlockJobData *job, virDomainAsyncJob asyncJob, virDomainDiskDef *disk); + +int +qemuBlockFinalize(virDomainObj *vm, + qemuBlockJobData *job, + virDomainAsyncJob asyncJob); -- 2.39.0

On Thu, Jan 05, 2023 at 12:46:48 +0100, Pavel Hrdina wrote: [...] s/stourage/storage/ The comment specifies that passing a pointer for 'parent' is optional but here you hard-dereference it always. Add the same condition as at the beginning. Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt;

Extract code that deletes single snapshot to separate function. Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_snapshot.c | 47 ++++++++++++++++++++++++++-------------- 1 file changed, 31 insertions(+), 16 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index e9bb5abee2..47239e9e9c 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2281,6 +2281,36 @@ qemuSnapshotChildrenReparent(void *payload, } +static int +qemuSnapshotDeleteSingle(virDomainObj *vm, + virDomainMomentObj *snap, + bool metadata_only) +{ + qemuDomainObjPrivate *priv = vm->privateData; + virQEMUDriver *driver = priv->driver; + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); + + if (snap->nchildren) { + virQEMUMomentReparent rep; + + rep.dir = cfg->snapshotDir; + rep.parent = snap->parent; + rep.vm = vm; + rep.err = 0; + rep.xmlopt = driver->xmlopt; + rep.writeMetadata = qemuDomainSnapshotWriteMetadata; + virDomainMomentForEachChild(snap, + qemuSnapshotChildrenReparent, + &rep); + if (rep.err < 0) + return -1; + virDomainMomentMoveChildren(snap, snap->parent); + } + + return qemuDomainSnapshotDiscard(driver, vm, snap, true, metadata_only); +} + + int qemuSnapshotDelete(virDomainObj *vm, virDomainSnapshotPtr snapshot, @@ -2290,7 +2320,6 @@ qemuSnapshotDelete(virDomainObj *vm, int ret = -1; virDomainMomentObj *snap = NULL; virQEMUMomentRemove rem; - virQEMUMomentReparent rep; bool metadata_only = !!(flags & VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY); int external = 0; g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); @@ -2358,21 +2387,7 @@ qemuSnapshotDelete(virDomainObj *vm, ret = qemuDomainSnapshotDiscard(driver, vm, snap, true, metadata_only); } } else { - if (snap->nchildren) { - rep.dir = cfg->snapshotDir; - rep.parent = snap->parent; - rep.vm = vm; - rep.err = 0; - rep.xmlopt = driver->xmlopt; - rep.writeMetadata = qemuDomainSnapshotWriteMetadata; - virDomainMomentForEachChild(snap, - qemuSnapshotChildrenReparent, - &rep); - if (rep.err < 0) - goto endjob; - virDomainMomentMoveChildren(snap, snap->parent); - } - ret = qemuDomainSnapshotDiscard(driver, vm, snap, true, metadata_only); + ret = qemuSnapshotDeleteSingle(vm, snap, metadata_only); } endjob: -- 2.39.0

This simplifies the code a bit by reusing existing parts that deletes a single snapshot. The drawback of this change is that we will now call the re-parent bits to keep the metadata in sync for every child even though it will get deleted as well. Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_snapshot.c | 69 +++++++++++++++++++++------------------- 1 file changed, 36 insertions(+), 33 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index a3ff195685..ce25d17c99 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2311,6 +2311,31 @@ qemuSnapshotDeleteSingle(virDomainObj *vm, } +struct qemuSnapshotDeleteAllData { + virDomainObj *vm; + bool metadata_only; + int error; +}; + + +static int +qemuSnapshotDeleteAllHelper(void *payload, + const char *name G_GNUC_UNUSED, + void *opaque) +{ + int error; + virDomainMomentObj *snap = payload; + struct qemuSnapshotDeleteAllData *data = opaque; + + error = qemuSnapshotDeleteSingle(data->vm, snap, data->metadata_only); + + if (error != 0) + data->error = error; + + return 0; +} + + /** * qemuSnapshotDeleteChildren: * @vm: domain object @@ -2331,44 +2356,22 @@ qemuSnapshotDeleteChildren(virDomainObj *vm, bool metadata_only, bool children_only) { - virQEMUMomentRemove rem; - qemuDomainObjPrivate *priv = vm->privateData; - virQEMUDriver *driver = priv->driver; - g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); + struct qemuSnapshotDeleteAllData data = { + .vm = vm, + .metadata_only = metadata_only, + }; - rem.driver = driver; - rem.vm = vm; - rem.metadata_only = metadata_only; - rem.err = 0; - rem.current = virDomainSnapshotGetCurrent(vm->snapshots); - rem.found = false; - rem.momentDiscard = qemuDomainSnapshotDiscard; - virDomainMomentForEachDescendant(snap, qemuDomainMomentDiscardAll, - &rem); - if (rem.err < 0) - return -1; - if (rem.found) { - qemuSnapshotSetCurrent(vm, snap); + virDomainMomentForEachDescendant(snap, qemuSnapshotDeleteAllHelper, &data); - if (children_only) { - if (qemuDomainSnapshotWriteMetadata(vm, snap, - driver->xmlopt, - cfg->snapshotDir) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("failed to set snapshot '%s' as current"), - snap->def->name); - virDomainSnapshotSetCurrent(vm->snapshots, NULL); - return -1; - } - } - } + if (data.error < 0) + return -1; - if (children_only) { - virDomainMomentDropChildren(snap); - return 0; + if (!children_only && + qemuSnapshotDeleteSingle(vm, snap, metadata_only) < 0) { + return -1; } - return qemuDomainSnapshotDiscard(driver, vm, snap, true, metadata_only); + return 0; } -- 2.39.0

Previously the reparent happened before the actual snapshot deletion. This change moves the code closer to the rest of the code handling snapshot metadata when deletion happens. This makes the metadate deletion happen after the data files are deleted. Following patch will extract it into separate function Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_snapshot.c | 37 +++++++++++++++++++------------------ 1 file changed, 19 insertions(+), 18 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 307a3deeb1..471bd184d4 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2323,6 +2323,25 @@ qemuSnapshotDiscard(virQEMUDriver *driver, } } + if (update_parent) { + if (snap->nchildren) { + virQEMUMomentReparent rep; + + rep.dir = cfg->snapshotDir; + rep.parent = snap->parent; + rep.vm = vm; + rep.err = 0; + rep.xmlopt = driver->xmlopt; + rep.writeMetadata = qemuDomainSnapshotWriteMetadata; + virDomainMomentForEachChild(snap, + qemuSnapshotChildrenReparent, + &rep); + if (rep.err < 0) + return -1; + virDomainMomentMoveChildren(snap, snap->parent); + } + } + snapFile = g_strdup_printf("%s/%s/%s.xml", cfg->snapshotDir, vm->def->name, snap->def->name); @@ -2382,24 +2401,6 @@ qemuSnapshotDeleteSingle(virDomainObj *vm, { qemuDomainObjPrivate *priv = vm->privateData; virQEMUDriver *driver = priv->driver; - g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); - - if (snap->nchildren) { - virQEMUMomentReparent rep; - - rep.dir = cfg->snapshotDir; - rep.parent = snap->parent; - rep.vm = vm; - rep.err = 0; - rep.xmlopt = driver->xmlopt; - rep.writeMetadata = qemuDomainSnapshotWriteMetadata; - virDomainMomentForEachChild(snap, - qemuSnapshotChildrenReparent, - &rep); - if (rep.err < 0) - return -1; - virDomainMomentMoveChildren(snap, snap->parent); - } return qemuSnapshotDiscard(driver, vm, snap, true, metadata_only); } -- 2.39.0

Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_snapshot.c | 42 ++++++++++++++++++++++++++-------------- 1 file changed, 28 insertions(+), 14 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 3051e5c8df..cb4e0dcb78 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2482,6 +2482,33 @@ qemuSnapshotDeleteChildren(virDomainObj *vm, } +static int +qemuSnapshotDeleteValidate(virDomainMomentObj *snap, + unsigned int flags) +{ + int external = 0; + + if (!(flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY) && + virDomainSnapshotIsExternal(snap)) + external++; + + if (flags & (VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN | + VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY)) + virDomainMomentForEachDescendant(snap, + qemuSnapshotCountExternal, + &external); + + if (external) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("deletion of %d external disk snapshots not " + "supported yet"), external); + return -1; + } + + return 0; +} + + int qemuSnapshotDelete(virDomainObj *vm, virDomainSnapshotPtr snapshot, @@ -2490,7 +2517,6 @@ qemuSnapshotDelete(virDomainObj *vm, int ret = -1; virDomainMomentObj *snap = NULL; bool metadata_only = !!(flags & VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY); - int external = 0; virCheckFlags(VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN | VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY | @@ -2503,20 +2529,8 @@ qemuSnapshotDelete(virDomainObj *vm, goto endjob; if (!metadata_only) { - if (!(flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY) && - virDomainSnapshotIsExternal(snap)) - external++; - if (flags & (VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN | - VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY)) - virDomainMomentForEachDescendant(snap, - qemuSnapshotCountExternal, - &external); - if (external) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("deletion of %d external disk snapshots not " - "supported yet"), external); + if (qemuSnapshotDeleteValidate(snap, flags) < 0) goto endjob; - } } if (flags & (VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN | -- 2.39.0

Deleting internal snapshot when the currently active disk image is different than where the internal snapshot was taken doesn't work correctly. This applies to a running VM only as we are using QMP command and talking to the QEMU process that is using different disk. This works correctly when the VM is shut of as in this case we spawn qemu-img process to delete the snapshot. Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_snapshot.c | 26 ++++++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index d7da317e22..31e7087081 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2492,9 +2492,31 @@ qemuSnapshotCountExternalInternal(void *payload, static int -qemuSnapshotDeleteValidate(virDomainMomentObj *snap, +qemuSnapshotDeleteValidate(virDomainObj *vm, + virDomainMomentObj *snap, unsigned int flags) { + if (!virDomainSnapshotIsExternal(snap) && + virDomainObjIsActive(vm)) { + ssize_t i; + virDomainSnapshotDef *snapdef = virDomainSnapshotObjGetDef(snap); + + for (i = 0; i < snapdef->ndisks; i++) { + virDomainSnapshotDiskDef *snapDisk = &(snapdef->disks[i]); + virDomainDiskDef *vmdisk = NULL; + virDomainDiskDef *disk = NULL; + + vmdisk = qemuDomainDiskByName(vm->def, snapDisk->name); + disk = qemuDomainDiskByName(snapdef->parent.dom, snapDisk->name); + + if (!virStorageSourceIsSameLocation(vmdisk->src, disk->src)) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("disk image '%s' for internal snapshot '%s' is not the same as disk image currently used by VM"), + snapDisk->name, snap->def->name); + return -1; + } + } + } if (flags & (VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN | VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY)) { @@ -2548,7 +2570,7 @@ qemuSnapshotDelete(virDomainObj *vm, goto endjob; if (!metadata_only) { - if (qemuSnapshotDeleteValidate(snap, flags) < 0) + if (qemuSnapshotDeleteValidate(vm, snap, flags) < 0) goto endjob; } -- 2.39.0

In order to save some CPU cycles we will collect all the necessary data to delete external snapshot before we even start. They will be later used by code that deletes the snapshots and updates metadata when needed. With external snapshots we need data that libvirt gets from running QEMU process so if the VM is not running we need to start paused QEMU process for the snapshot deletion and kill at afterwards. Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_snapshot.c | 181 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 181 insertions(+) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index fe279a2374..fe050b7fc4 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2232,6 +2232,150 @@ qemuSnapshotRevert(virDomainObj *vm, } +typedef struct _qemuSnapshotDeleteExternalData { + virDomainSnapshotDiskDef *snapDisk; /* snapshot disk definition */ + virDomainDiskDef *domDisk; /* VM disk definition */ + virStorageSource *diskSrc; /* source of disk we are deleting */ + virDomainMomentObj *parentSnap; + virDomainDiskDef *parentDomDisk; /* disk definition from snapshot metadata */ + virStorageSource *parentDiskSrc; /* backing disk source of the @diskSrc */ + virStorageSource *prevDiskSrc; /* source of disk for which @diskSrc is + backing disk */ + qemuBlockJobData *job; +} qemuSnapshotDeleteExternalData; + + +static void +qemuSnapshotDeleteExternalDataFree(qemuSnapshotDeleteExternalData *data) +{ + if (!data) + return; + + virObjectUnref(data->job); + + g_free(data); +} + + +G_DEFINE_AUTOPTR_CLEANUP_FUNC(qemuSnapshotDeleteExternalData, qemuSnapshotDeleteExternalDataFree); + + +/** + * qemuSnapshotFindParentSnapForDisk: + * @snap: snapshot object that is about to be deleted + * @snapDisk: snapshot disk definition + * + * Find parent snapshot object that contains snapshot of the @snapDisk. + * It may not be the next snapshot in the snapshot tree as the disk in + * question may be skipped by using VIR_DOMAIN_SNAPSHOT_LOCATION_NO. + * + * Returns virDomainMomentObj* on success or NULL if there is no parent + * snapshot containing the @snapDisk. + * */ +static virDomainMomentObj* +qemuSnapshotFindParentSnapForDisk(virDomainMomentObj *snap, + virDomainSnapshotDiskDef *snapDisk) +{ + virDomainMomentObj *parentSnap = snap->parent; + + while (parentSnap) { + ssize_t i; + virDomainSnapshotDef *parentSnapdef = virDomainSnapshotObjGetDef(parentSnap); + + if (!parentSnapdef) + break; + + for (i = 0; i < parentSnapdef->ndisks; i++) { + virDomainSnapshotDiskDef *parentSnapDisk = &(parentSnapdef->disks[i]); + + if (parentSnapDisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_NO && + STREQ(snapDisk->name, parentSnapDisk->name)) { + return parentSnap; + } + } + + parentSnap = parentSnap->parent; + } + + return NULL; +} + + +static GSList* +qemuSnapshotDeleteExternalPrepare(virDomainObj *vm, + virDomainMomentObj *snap) +{ + ssize_t i; + virDomainSnapshotDef *snapdef = virDomainSnapshotObjGetDef(snap); + g_autoslist(qemuSnapshotDeleteExternalData) ret = NULL; + + for (i = 0; i < snapdef->ndisks; i++) { + g_autofree qemuSnapshotDeleteExternalData *data = NULL; + virDomainSnapshotDiskDef *snapDisk = &(snapdef->disks[i]); + + if (snapDisk->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NO) + continue; + + data = g_new0(qemuSnapshotDeleteExternalData, 1); + data->snapDisk = snapDisk; + + data->domDisk = qemuDomainDiskByName(vm->def, snapDisk->name); + if (!data->domDisk) + return NULL; + + data->diskSrc = virStorageSourceChainLookupBySource(data->domDisk->src, + data->snapDisk->src, + &data->prevDiskSrc); + if (!data->diskSrc) + return NULL; + + if (!virStorageSourceIsSameLocation(data->diskSrc, data->snapDisk->src)) { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("VM disk source and snapshot disk source are not the same")); + return NULL; + } + + data->parentDomDisk = virDomainDiskByTarget(snapdef->parent.dom, + data->snapDisk->name); + if (!data->parentDomDisk) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("failed to find disk '%s' in snapshot VM XML"), + snapDisk->name); + return NULL; + } + + if (virDomainObjIsActive(vm)) { + data->parentDiskSrc = data->diskSrc->backingStore; + if (!virStorageSourceIsBacking(data->parentDiskSrc)) { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("failed to find parent disk source in backing chain")); + return NULL; + } + + if (!virStorageSourceIsSameLocation(data->parentDiskSrc, data->parentDomDisk->src)) { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("snapshot VM disk source and parent disk source are not the same")); + return NULL; + } + } + + data->parentSnap = qemuSnapshotFindParentSnapForDisk(snap, data->snapDisk); + + if (data->parentSnap && !virDomainSnapshotIsExternal(data->parentSnap)) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("deleting external snapshot that has internal snapshot as parent not supported")); + return NULL; + } + + ret = g_slist_prepend(ret, g_steal_pointer(&data)); + } + + ret = g_slist_reverse(ret); + + return g_steal_pointer(&ret); +} + + typedef struct _virQEMUMomentReparent virQEMUMomentReparent; struct _virQEMUMomentReparent { const char *dir; @@ -2563,6 +2707,10 @@ qemuSnapshotDelete(virDomainObj *vm, int ret = -1; virDomainMomentObj *snap = NULL; bool metadata_only = !!(flags & VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY); + bool stop_qemu = false; + qemuDomainObjPrivate *priv = vm->privateData; + virQEMUDriver *driver = priv->driver; + g_autoslist(qemuSnapshotDeleteExternalData) externalData = NULL; virCheckFlags(VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN | VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY | @@ -2580,6 +2728,34 @@ qemuSnapshotDelete(virDomainObj *vm, if (!metadata_only) { if (qemuSnapshotDeleteValidate(vm, snap, flags) < 0) goto endjob; + + if (virDomainSnapshotIsExternal(snap) && + !(flags & (VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN | + VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY))) { + + externalData = qemuSnapshotDeleteExternalPrepare(vm, snap); + + if (!virDomainObjIsActive(vm)) { + g_autoslist(qemuSnapshotDeleteExternalData) delData = NULL; + + if (qemuProcessStart(NULL, driver, vm, NULL, VIR_ASYNC_JOB_SNAPSHOT, + NULL, -1, NULL, NULL, + VIR_NETDEV_VPORT_PROFILE_OP_CREATE, + VIR_QEMU_PROCESS_START_PAUSED) < 0) { + goto endjob; + } + + stop_qemu = true; + + /* Call the prepare again as some data require that the VM is + * running to get everything we need. */ + delData = g_steal_pointer(&externalData); + externalData = qemuSnapshotDeleteExternalPrepare(vm, snap); + } + + if (!externalData) + goto endjob; + } } if (flags & (VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN | @@ -2591,6 +2767,11 @@ qemuSnapshotDelete(virDomainObj *vm, } endjob: + if (stop_qemu) { + qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_SHUTDOWN, + VIR_ASYNC_JOB_SNAPSHOT, 0); + } + virDomainObjEndAsyncJob(vm); return ret; -- 2.39.0

When deleting external snapshots the operation may fail at any point which could lead to situation that some disks finished the block commit operation but for some disks it failed and the libvirt job ends. In order to make sure that the qcow2 images are in consistent state introduce new element "<snapshotDeleteInProgress/>" that will mark the disk in snapshot metadata as invalid until the snapshot delete is completed successfully. This will prevent deleting snapshot with the invalid disk and in future reverting to snapshot with the invalid disk. Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; --- src/conf/snapshot_conf.c | 8 ++++++ src/conf/snapshot_conf.h | 1 + src/qemu/qemu_snapshot.c | 60 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 69 insertions(+) diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 4b5b908d66..9bf3c78353 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -158,6 +158,11 @@ virDomainSnapshotDiskDefParseXML(xmlNodePtr node, return -1; } + if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE) { + def->snapshotDeleteInProgress = !!virXPathNode("./snapshotDeleteInProgress", + ctxt); + } + if ((cur = virXPathNode("./source", ctxt)) && virDomainStorageSourceParse(cur, ctxt, src, flags, xmlopt) < 0) return -1; @@ -744,6 +749,9 @@ virDomainSnapshotDiskDefFormat(virBuffer *buf, virBufferAsprintf(&attrBuf, " snapshot='%s'", virDomainSnapshotLocationTypeToString(disk->snapshot)); + if (disk->snapshotDeleteInProgress) + virBufferAddLit(&childBuf, "<snapshotDeleteInProgress/>\n"); + if (disk->src->path || disk->src->format != 0) { g_auto(virBuffer) driverAttrBuf = VIR_BUFFER_INITIALIZER; g_auto(virBuffer) driverChildBuf = VIR_BUFFER_INIT_CHILD(&childBuf); diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h index fec4a5a912..96c77ef42b 100644 --- a/src/conf/snapshot_conf.h +++ b/src/conf/snapshot_conf.h @@ -52,6 +52,7 @@ typedef struct _virDomainSnapshotDiskDef virDomainSnapshotDiskDef; struct _virDomainSnapshotDiskDef { char *name; /* name matching the <target dev='...' of the domain */ virDomainSnapshotLocation snapshot; + bool snapshotDeleteInProgress; /* details of wrapper external file. src is always non-NULL. * XXX optimize this to allow NULL for internal snapshots? */ diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 094d56b13d..0ed8a82aa9 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2316,6 +2316,13 @@ qemuSnapshotDeleteExternalPrepare(virDomainObj *vm, if (snapDisk->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NO) continue; + if (snapDisk->snapshotDeleteInProgress) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("snapshot disk '%s' was target of not completed snapshot delete"), + snapDisk->name); + return NULL; + } + data = g_new0(qemuSnapshotDeleteExternalData, 1); data->snapDisk = snapDisk; @@ -2628,6 +2635,53 @@ qemuSnapshotDeleteBlockJobFinishing(virDomainObj *vm, } +/** + * qemuSnapshotSetInvalid: + * @vm: vm object + * @snap: snapshot object that contains parent disk + * @disk: disk from the snapshot we are deleting + * @invalid: boolean to set/unset invalid state + * + * @snap should point to a ancestor snapshot from the snapshot tree that + * affected the @disk which doesn't have to be the direct parent. + * + * When setting snapshot with parent disk as invalid due to snapshot being + * deleted we should not mark the whole snapshot as invalid but only the + * affected disks because the snapshot can contain other disks that we are + * not modifying at the moment. + * + * Return 0 on success, -1 on error. + * */ +static int +qemuSnapshotSetInvalid(virDomainObj *vm, + virDomainMomentObj *snap, + virDomainSnapshotDiskDef *disk, + bool invalid) +{ + ssize_t i; + qemuDomainObjPrivate *priv = vm->privateData; + virQEMUDriver *driver = priv->driver; + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); + virDomainSnapshotDef *snapdef = NULL; + + if (!snap) + return 0; + + snapdef = virDomainSnapshotObjGetDef(snap); + if (!snapdef) + return 0; + + for (i = 0; i < snapdef->ndisks; i++) { + virDomainSnapshotDiskDef *snapDisk = &(snapdef->disks[i]); + + if (STREQ(snapDisk->name, disk->name)) + snapDisk->snapshotDeleteInProgress = invalid; + } + + return qemuDomainSnapshotWriteMetadata(vm, snap, driver->xmlopt, cfg->snapshotDir); +} + + static int qemuSnapshotDiscardExternal(virDomainObj *vm, GSList *externalData) @@ -2644,6 +2698,9 @@ qemuSnapshotDiscardExternal(virDomainObj *vm, autofinalize = VIR_TRISTATE_BOOL_YES; } + if (qemuSnapshotSetInvalid(vm, data->parentSnap, data->snapDisk, true) < 0) + goto error; + data->job = qemuBlockCommit(vm, data->domDisk, data->parentDiskSrc, @@ -2694,6 +2751,9 @@ qemuSnapshotDiscardExternal(virDomainObj *vm, } qemuBlockJobSyncEnd(vm, data->job, VIR_ASYNC_JOB_SNAPSHOT); + + if (qemuSnapshotSetInvalid(vm, data->parentSnap, data->snapDisk, false) < 0) + goto error; } return 0; -- 2.39.0

On Thu, Jan 05, 2023 at 12:47:04 +0100, Pavel Hrdina wrote: Note that this code path is taken with any job using VIR_ASYNC_JOB_SNAPSHOT thus also the snapshot creation job. For snapshot creation we check that only disks which will be part of the snapshot don't have an active block job, but we allow any other disks to keep their block job running. Since you use the same code path here, this recovery still can kill off unsuspecting jobs not related to snapshot deletion. The above code must not be called when the recovered job is snapshot creation.

Now that deletion of external snapshot is implemented document the current virDomainSnapshotDelete supported state. Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/libvirt-domain-snapshot.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/libvirt-domain-snapshot.c b/src/libvirt-domain-snapshot.c index 2917b8bd90..ab6a029915 100644 --- a/src/libvirt-domain-snapshot.c +++ b/src/libvirt-domain-snapshot.c @@ -1075,6 +1075,13 @@ virDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, * libvirt metadata to track snapshots, then this flag is silently * ignored. * + * Since libvirt 9.0.0 deletion of external snapshots is supported + * for QEMU driver. Using @flags VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN + * and VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY is not supported with + * external snapshots. In case that daemon process is terminated + * while the snapshot delete is in process the operation will be + * aborted when the daemon starts again. + * * Returns 0 if the selected snapshot(s) were successfully deleted, * -1 on error. * -- 2.39.0