[libvirt PATCH v2 00/10] qemu: block: basic implementation of deletion of external snapshots
Deleting external snapshots has been unimplemented so far. This series aims to enable limited functionality in that direction, handling just the common cases for now. The intention is to subsequently build upon this to eventually cover the more complex/exotic cases as well. It works by blockcommitting the snapshot to be deleted into its parent. Handles both deleting children as well and deleting children only (by committing the children into the snapshot referred to by snapshot-delete argument). If the necessary block commit operations turns out to be active (the snapshot referred to by snapshot-delete argument is current, or deleting children is requested) pivot is performed as well. This implemetation is limited to the straightforward case which assumes no branching snapshot chains below the snapshot to be deleted, and the current snapshot has to be the leaf of the (linear) chain starting at the snapshot to be deleted. These requirements are checked and enforced at the top of qemuDomainSnapshotDeleteExternal(). They might even be too restrictive for the case where deletion of children is not requested as in that case, requiring that the parent of the snapshot to be deleted not be a branching point in snapshot tree should be enough. The above limitations do not appear too severe under the current state of matters where a major source of branching snapshot structures, snapshot-revert, is not even implemented for external snapshots yet. The only other known cause of branching in external snapshots thus seems to be if a user creates branching by manually creating snapshot images and metadata and applies them using snapshot-create --redefine. Also, the snapshot images involved basically have to be files. This does not seem overly limiting either, at least for the time being, because it's a limitation shared with the underlying blockcommit code. Just as the existing code, the new code ultimately relies on virStorageFileChainLookup() as its image comparison and look-up engine and is affected by its limitaions. At any rate, this work should be understood as just a first step to a full support of deleting external snapshots. The first 5 commits of this series are just refactors used by the last 5 commits which actually implement snapshot deletion. However, the qemuDomainBlockCommit() refactor (the initial 2 commits of this series) was affected by a rather hairy merge on rebasing to the current master and although I tried to do my best to get the merge right I still feel a careful review is in order for these two commits. Pavel Mores (10): qemu: block: factor implementation out of qemuDomainBlockCommit() qemu: block: refactor blockcommit so that it's callable with storage sources qemu: block: factor implementation out of qemuDomainBlockJobAbort() conf: rename virDomainMomentFindLeaf() to virDomainMomentObjListFindLeaf() conf: add virDomainMomentFindLeaf() which operates on virDomainMomentObjPtr qemu: block: add function to collect blockcommit params and verify them qemu: block: add function to launch all prepared blockcommits qemu: block: add function to wait for blockcommits and collect results qemu: block: add external snapshot-delete top-level algorithm qemu: block: add actual invocation of external snapshot-delete src/conf/virdomaincheckpointobjlist.c | 2 +- src/conf/virdomainmomentobjlist.c | 27 +- src/conf/virdomainmomentobjlist.h | 3 +- src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 552 ++++++++++++++++++++++---- 5 files changed, 485 insertions(+), 100 deletions(-) -- 2.24.1
We'll need blockcommit to be callable from within the QEMU driver where we have no virDomain instance available, just virDomainObj. Signed-off-by: Pavel Mores <pmores@redhat.com> --- src/qemu/qemu_driver.c | 78 ++++++++++++++++++++++++++++-------------- 1 file changed, 52 insertions(+), 26 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1c7c87128d..b642b24fa2 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -154,6 +154,15 @@ static int qemuOpenFileAs(uid_t fallback_uid, gid_t fallback_gid, static virQEMUDriverPtr qemu_driver; +static int +qemuDomainBlockCommitImpl(virDomainObjPtr vm, + virQEMUDriverPtr driver, + const char *path, + const char *base, + const char *top, + unsigned long bandwidth, + unsigned int flags); + /* Looks up the domain object from snapshot and unlocks the * driver. The returned domain object is locked and ref'd and the * caller must call virDomainObjEndAPI() on it. */ @@ -18431,18 +18440,16 @@ qemuDomainBlockPull(virDomainPtr dom, const char *path, unsigned long bandwidth, return qemuDomainBlockPullCommon(vm, path, NULL, bandwidth, flags); } - static int -qemuDomainBlockCommit(virDomainPtr dom, - const char *path, - const char *base, - const char *top, - unsigned long bandwidth, - unsigned int flags) +qemuDomainBlockCommitImpl(virDomainObjPtr vm, + virQEMUDriverPtr driver, + const char *path, + const char *base, + const char *top, + unsigned long bandwidth, + unsigned int flags) { - virQEMUDriverPtr driver = dom->conn->privateData; - qemuDomainObjPrivatePtr priv; - virDomainObjPtr vm = NULL; + qemuDomainObjPrivatePtr priv = vm->privateData; const char *device = NULL; const char *jobname = NULL; int ret = -1; @@ -18466,22 +18473,6 @@ qemuDomainBlockCommit(virDomainPtr dom, g_autoptr(virJSONValue) bitmapDisableActions = NULL; VIR_AUTOSTRINGLIST bitmapDisableList = NULL; - virCheckFlags(VIR_DOMAIN_BLOCK_COMMIT_SHALLOW | - VIR_DOMAIN_BLOCK_COMMIT_ACTIVE | - VIR_DOMAIN_BLOCK_COMMIT_RELATIVE | - VIR_DOMAIN_BLOCK_COMMIT_DELETE | - VIR_DOMAIN_BLOCK_COMMIT_BANDWIDTH_BYTES, -1); - - if (!(vm = qemuDomainObjFromDomain(dom))) - goto cleanup; - priv = vm->privateData; - - if (virDomainBlockCommitEnsureACL(dom->conn, vm->def) < 0) - goto cleanup; - - if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) - goto cleanup; - if (virDomainObjCheckActive(vm) < 0) goto endjob; @@ -18730,6 +18721,40 @@ qemuDomainBlockCommit(virDomainPtr dom, virErrorRestore(&orig_err); } qemuBlockJobStartupFinalize(vm, job); + + return ret; +} + + +static int +qemuDomainBlockCommit(virDomainPtr dom, + const char *path, + const char *base, + const char *top, + unsigned long bandwidth, + unsigned int flags) +{ + virQEMUDriverPtr driver = dom->conn->privateData; + virDomainObjPtr vm = NULL; + int ret = -1; + + virCheckFlags(VIR_DOMAIN_BLOCK_COMMIT_SHALLOW | + VIR_DOMAIN_BLOCK_COMMIT_ACTIVE | + VIR_DOMAIN_BLOCK_COMMIT_RELATIVE | + VIR_DOMAIN_BLOCK_COMMIT_DELETE | + VIR_DOMAIN_BLOCK_COMMIT_BANDWIDTH_BYTES, -1); + + if (!(vm = qemuDomainObjFromDomain(dom))) + goto cleanup; + + if (virDomainBlockCommitEnsureACL(dom->conn, vm->def) < 0) + goto cleanup; + + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) + goto cleanup; + + ret = qemuDomainBlockCommitImpl(vm, driver, path, base, top, bandwidth, flags); + qemuDomainObjEndJob(driver, vm); cleanup: @@ -18737,6 +18762,7 @@ qemuDomainBlockCommit(virDomainPtr dom, return ret; } + static int qemuDomainOpenGraphics(virDomainPtr dom, unsigned int idx, -- 2.24.1
So far, only paths could be used to specify blockcommit's base and top images. However, this is not general enough as paths have limitations (most notably they can only work for file-based storage sources). This commit preserves the path-based interface but factors out the core of blockcommit implementation into a separate function that takes its base and top as virStorageSources. The path-based interface basically just converts the paths into virStorageSource just as it was done before and calls the virStorageSource-based implementation core. Signed-off-by: Pavel Mores <pmores@redhat.com> --- src/qemu/qemu_driver.c | 108 +++++++++++++++++++++++++---------------- 1 file changed, 65 insertions(+), 43 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b642b24fa2..09300c1e90 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -18441,25 +18441,27 @@ qemuDomainBlockPull(virDomainPtr dom, const char *path, unsigned long bandwidth, } static int -qemuDomainBlockCommitImpl(virDomainObjPtr vm, - virQEMUDriverPtr driver, - const char *path, - const char *base, - const char *top, - unsigned long bandwidth, - unsigned int flags) +qemuDomainBlockCommitCommon(virDomainObjPtr vm, + virQEMUDriverPtr driver, + virDomainDiskDefPtr disk, + virStorageSourcePtr baseSource, + virStorageSourcePtr topSource, + virStorageSourcePtr topParentSource, + unsigned long bandwidth, + unsigned int flags) { qemuDomainObjPrivatePtr priv = vm->privateData; const char *device = NULL; const char *jobname = NULL; int ret = -1; - virDomainDiskDefPtr disk = NULL; - virStorageSourcePtr topSource; - unsigned int topIndex = 0; - virStorageSourcePtr baseSource = NULL; - unsigned int baseIndex = 0; - virStorageSourcePtr top_parent = NULL; bool clean_access = false; + /* TODO the following 2 are just for error reporting. Originally these + * could have been either paths or indexed identifiers (like 'vda[1]'). + * Now the error reporting will always use paths instead of what the + * user originally specified. Find out if this is fine, find a solution + * if it isn't. */ + const char *path = disk->src->path; + const char *base = baseSource->path; g_autofree char *topPath = NULL; g_autofree char *basePath = NULL; g_autofree char *backingPath = NULL; @@ -18473,9 +18475,6 @@ qemuDomainBlockCommitImpl(virDomainObjPtr vm, g_autoptr(virJSONValue) bitmapDisableActions = NULL; VIR_AUTOSTRINGLIST bitmapDisableList = NULL; - if (virDomainObjCheckActive(vm) < 0) - goto endjob; - blockdev = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV); /* Convert bandwidth MiB to bytes, if necessary */ @@ -18489,9 +18488,6 @@ qemuDomainBlockCommitImpl(virDomainObjPtr vm, speed <<= 20; } - if (!(disk = qemuDomainDiskByName(vm->def, path))) - goto endjob; - if (virStorageSourceIsEmpty(disk->src)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("disk %s has no source file to be committed"), @@ -18511,14 +18507,6 @@ qemuDomainBlockCommitImpl(virDomainObjPtr vm, goto endjob; } - if (!top || STREQ(top, disk->dst)) - topSource = disk->src; - else if (virStorageFileParseChainIndex(disk->dst, top, &topIndex) < 0 || - !(topSource = virStorageFileChainLookup(disk->src, NULL, - top, topIndex, - &top_parent))) - goto endjob; - if (topSource == disk->src) { if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_ACTIVE_COMMIT)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", @@ -18546,13 +18534,6 @@ qemuDomainBlockCommitImpl(virDomainObjPtr vm, goto endjob; } - if (!base && (flags & VIR_DOMAIN_BLOCK_COMMIT_SHALLOW)) - baseSource = topSource->backingStore; - else if (virStorageFileParseChainIndex(disk->dst, base, &baseIndex) < 0 || - !(baseSource = virStorageFileChainLookup(disk->src, topSource, - base, baseIndex, NULL))) - goto endjob; - if ((flags & VIR_DOMAIN_BLOCK_COMMIT_SHALLOW) && baseSource != topSource->backingStore) { virReportError(VIR_ERR_INVALID_ARG, @@ -18580,8 +18561,8 @@ qemuDomainBlockCommitImpl(virDomainObjPtr vm, goto endjob; } - if (blockdev && top_parent && - qemuBlockUpdateRelativeBacking(vm, top_parent, disk->src) < 0) + if (blockdev && topParentSource && + qemuBlockUpdateRelativeBacking(vm, topParentSource, disk->src) < 0) goto endjob; if (virStorageFileGetRelativeBackingPath(topSource, baseSource, @@ -18607,11 +18588,11 @@ qemuDomainBlockCommitImpl(virDomainObjPtr vm, 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 + if (topParentSource && topParentSource != disk->src) { + /* While topParentSource 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, + if (qemuDomainStorageSourceAccessAllow(driver, vm, topParentSource, false, false, false) < 0) goto endjob; } @@ -18637,7 +18618,7 @@ qemuDomainBlockCommitImpl(virDomainObjPtr vm, } } - if (!(job = qemuBlockJobDiskNewCommit(vm, disk, top_parent, topSource, + if (!(job = qemuBlockJobDiskNewCommit(vm, disk, topParentSource, topSource, baseSource, &bitmapDisableList, flags & VIR_DOMAIN_BLOCK_COMMIT_DELETE, flags))) @@ -18657,7 +18638,7 @@ qemuDomainBlockCommitImpl(virDomainObjPtr vm, nodetop = topSource->nodeformat; nodebase = baseSource->nodeformat; device = qemuDomainDiskGetTopNodename(disk); - if (!backingPath && top_parent && + if (!backingPath && topParentSource && !(backingPath = qemuBlockGetBackingStoreString(baseSource, false))) goto endjob; @@ -18714,8 +18695,8 @@ qemuDomainBlockCommitImpl(virDomainObjPtr vm, /* 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, + if (topParentSource && topParentSource != disk->src) + qemuDomainStorageSourceAccessAllow(driver, vm, topParentSource, true, false, false); virErrorRestore(&orig_err); @@ -18725,6 +18706,47 @@ qemuDomainBlockCommitImpl(virDomainObjPtr vm, return ret; } +static int +qemuDomainBlockCommitImpl(virDomainObjPtr vm, + virQEMUDriverPtr driver, + const char *path, + const char *base, + const char *top, + unsigned long bandwidth, + unsigned int flags) +{ + virDomainDiskDefPtr disk = NULL; + virStorageSourcePtr topSource; + unsigned int topIndex = 0; + virStorageSourcePtr baseSource = NULL; + unsigned int baseIndex = 0; + virStorageSourcePtr top_parent = NULL; + + if (virDomainObjCheckActive(vm) < 0) + return -1; + + if (!(disk = qemuDomainDiskByName(vm->def, path))) + return -1; + + if (!top || STREQ(top, disk->dst)) + topSource = disk->src; + else if (virStorageFileParseChainIndex(disk->dst, top, &topIndex) < 0 || + !(topSource = virStorageFileChainLookup(disk->src, NULL, + top, topIndex, + &top_parent))) + return -1; + + if (!base && (flags & VIR_DOMAIN_BLOCK_COMMIT_SHALLOW)) + baseSource = topSource->backingStore; + else if (virStorageFileParseChainIndex(disk->dst, base, &baseIndex) < 0 || + !(baseSource = virStorageFileChainLookup(disk->src, topSource, + base, baseIndex, NULL))) + return -1; + + return qemuDomainBlockCommitCommon(vm, driver, disk, baseSource, topSource, + top_parent, bandwidth, flags); +} + static int qemuDomainBlockCommit(virDomainPtr dom, -- 2.24.1
Just as with the qemuDomainBlockCommit() refactor in the commit before the previous one, the motivation is primarily to make blockjob abortion callable from within the QEMU driver where only virDomainObj instance is available but no virDomain. Signed-off-by: Pavel Mores <pmores@redhat.com> --- src/qemu/qemu_driver.c | 62 ++++++++++++++++++++++++++++-------------- 1 file changed, 41 insertions(+), 21 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 09300c1e90..6ffd53503b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -163,6 +163,12 @@ qemuDomainBlockCommitImpl(virDomainObjPtr vm, unsigned long bandwidth, unsigned int flags); +static int +qemuDomainBlockJobAbortImpl(virQEMUDriverPtr driver, + virDomainObjPtr vm, + const char *path, + unsigned int flags); + /* Looks up the domain object from snapshot and unlocks the * driver. The returned domain object is locked and ref'd and the * caller must call virDomainObjEndAPI() on it. */ @@ -17541,45 +17547,31 @@ qemuDomainBlockPullCommon(virDomainObjPtr vm, return ret; } - static int -qemuDomainBlockJobAbort(virDomainPtr dom, - const char *path, - unsigned int flags) +qemuDomainBlockJobAbortImpl(virQEMUDriverPtr driver, + virDomainObjPtr vm, + const char *path, + unsigned int flags) { - virQEMUDriverPtr driver = dom->conn->privateData; virDomainDiskDefPtr disk = NULL; g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); bool pivot = !!(flags & VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT); bool async = !!(flags & VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC); g_autoptr(qemuBlockJobData) job = NULL; - virDomainObjPtr vm; qemuDomainObjPrivatePtr priv = NULL; bool blockdev = false; int ret = -1; - virCheckFlags(VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC | - VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT, -1); - - if (!(vm = qemuDomainObjFromDomain(dom))) - return -1; - - if (virDomainBlockJobAbortEnsureACL(dom->conn, vm->def) < 0) - goto cleanup; - - if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) - goto cleanup; - if (virDomainObjCheckActive(vm) < 0) - goto endjob; + return -1; if (!(disk = qemuDomainDiskByName(vm->def, path))) - goto endjob; + return -1; if (!(job = qemuBlockJobDiskGetJob(disk))) { virReportError(VIR_ERR_INVALID_ARG, _("disk %s does not have an active block job"), disk->dst); - goto endjob; + return -1; } priv = vm->privateData; @@ -17650,6 +17642,34 @@ qemuDomainBlockJobAbort(virDomainPtr dom, endjob: if (job && !async) qemuBlockJobSyncEnd(vm, job, QEMU_ASYNC_JOB_NONE); + + return ret; +} + + +static int +qemuDomainBlockJobAbort(virDomainPtr dom, + const char *path, + unsigned int flags) +{ + virQEMUDriverPtr driver = dom->conn->privateData; + virDomainObjPtr vm; + int ret = -1; + + virCheckFlags(VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC | + VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT, -1); + + if (!(vm = qemuDomainObjFromDomain(dom))) + return -1; + + if (virDomainBlockJobAbortEnsureACL(dom->conn, vm->def) < 0) + goto cleanup; + + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) + goto cleanup; + + ret = qemuDomainBlockJobAbortImpl(driver, vm, path, flags); + qemuDomainObjEndJob(driver, vm); cleanup: -- 2.24.1
We'll need the original name for a function that finds leaf for a single virDomainMomentObj. The new name properly advertises that the function actually works on a virDomainMomentObjList, as some (but not all) of existing functions operating on lists do. Signed-off-by: Pavel Mores <pmores@redhat.com> --- src/conf/virdomaincheckpointobjlist.c | 2 +- src/conf/virdomainmomentobjlist.c | 2 +- src/conf/virdomainmomentobjlist.h | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/conf/virdomaincheckpointobjlist.c b/src/conf/virdomaincheckpointobjlist.c index a4942ea706..2df1c426a2 100644 --- a/src/conf/virdomaincheckpointobjlist.c +++ b/src/conf/virdomaincheckpointobjlist.c @@ -186,7 +186,7 @@ virDomainCheckpointUpdateRelations(virDomainCheckpointObjListPtr checkpoints, int ret = virDomainMomentUpdateRelations(checkpoints->base); if (ret == 0) - *leaf = virDomainMomentFindLeaf(checkpoints->base); + *leaf = virDomainMomentObjListFindLeaf(checkpoints->base); return ret; } diff --git a/src/conf/virdomainmomentobjlist.c b/src/conf/virdomainmomentobjlist.c index 18dbd434fb..41e887ba18 100644 --- a/src/conf/virdomainmomentobjlist.c +++ b/src/conf/virdomainmomentobjlist.c @@ -588,7 +588,7 @@ virDomainMomentCheckCycles(virDomainMomentObjListPtr list, /* If there is exactly one leaf node, return that node. */ virDomainMomentObjPtr -virDomainMomentFindLeaf(virDomainMomentObjListPtr list) +virDomainMomentObjListFindLeaf(virDomainMomentObjListPtr list) { virDomainMomentObjPtr moment = &list->metaroot; diff --git a/src/conf/virdomainmomentobjlist.h b/src/conf/virdomainmomentobjlist.h index 75198909ba..cab51edbc4 100644 --- a/src/conf/virdomainmomentobjlist.h +++ b/src/conf/virdomainmomentobjlist.h @@ -121,4 +121,4 @@ int virDomainMomentUpdateRelations(virDomainMomentObjListPtr moments); int virDomainMomentCheckCycles(virDomainMomentObjListPtr list, virDomainMomentDefPtr def, const char *domname); -virDomainMomentObjPtr virDomainMomentFindLeaf(virDomainMomentObjListPtr list); +virDomainMomentObjPtr virDomainMomentObjListFindLeaf(virDomainMomentObjListPtr list); -- 2.24.1
On Wed, May 06, 2020 at 13:42:20 +0200, Pavel Mores wrote:
We'll need the original name for a function that finds leaf for a single virDomainMomentObj. The new name properly advertises that the function actually works on a virDomainMomentObjList, as some (but not all) of existing functions operating on lists do.
Signed-off-by: Pavel Mores <pmores@redhat.com> ---
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
The new function operates on virDomainMomentObjPtr which is indicated by its name. To avoid code duplication, virDomainMomentObjListFindLeaf() was reimplemented in terms of the new function. Signed-off-by: Pavel Mores <pmores@redhat.com> --- src/conf/virdomainmomentobjlist.c | 25 +++++++++++++++---------- src/conf/virdomainmomentobjlist.h | 1 + 2 files changed, 16 insertions(+), 10 deletions(-) diff --git a/src/conf/virdomainmomentobjlist.c b/src/conf/virdomainmomentobjlist.c index 41e887ba18..c57d9cc787 100644 --- a/src/conf/virdomainmomentobjlist.c +++ b/src/conf/virdomainmomentobjlist.c @@ -207,6 +207,20 @@ virDomainMomentMoveChildren(virDomainMomentObjPtr from, } +/* If there is exactly one leaf node, return that node. */ +virDomainMomentObjPtr +virDomainMomentFindLeaf(virDomainMomentObjPtr moment) +{ + if (moment->nchildren != 1) + return NULL; + while (moment->nchildren == 1) + moment = moment->first_child; + if (moment->nchildren == 0) + return moment; + return NULL; +} + + static virDomainMomentObjPtr virDomainMomentObjNew(void) { @@ -586,17 +600,8 @@ virDomainMomentCheckCycles(virDomainMomentObjListPtr list, return 0; } -/* If there is exactly one leaf node, return that node. */ virDomainMomentObjPtr virDomainMomentObjListFindLeaf(virDomainMomentObjListPtr list) { - virDomainMomentObjPtr moment = &list->metaroot; - - if (moment->nchildren != 1) - return NULL; - while (moment->nchildren == 1) - moment = moment->first_child; - if (moment->nchildren == 0) - return moment; - return NULL; + return virDomainMomentFindLeaf(&list->metaroot); } diff --git a/src/conf/virdomainmomentobjlist.h b/src/conf/virdomainmomentobjlist.h index cab51edbc4..9c2d01ff8f 100644 --- a/src/conf/virdomainmomentobjlist.h +++ b/src/conf/virdomainmomentobjlist.h @@ -62,6 +62,7 @@ void virDomainMomentMoveChildren(virDomainMomentObjPtr from, virDomainMomentObjPtr to); void virDomainMomentLinkParent(virDomainMomentObjListPtr moments, virDomainMomentObjPtr moment); +virDomainMomentObjPtr virDomainMomentFindLeaf(virDomainMomentObjPtr moment); virDomainMomentObjListPtr virDomainMomentObjListNew(void); void virDomainMomentObjListFree(virDomainMomentObjListPtr moments); -- 2.24.1
On Wed, May 06, 2020 at 13:42:21 +0200, Pavel Mores wrote:
The new function operates on virDomainMomentObjPtr which is indicated by its name. To avoid code duplication, virDomainMomentObjListFindLeaf() was reimplemented in terms of the new function.
Signed-off-by: Pavel Mores <pmores@redhat.com> --- src/conf/virdomainmomentobjlist.c | 25 +++++++++++++++---------- src/conf/virdomainmomentobjlist.h | 1 + 2 files changed, 16 insertions(+), 10 deletions(-)
The addition to libvirt_private.syms belongs to this patch. After that: Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Snapshot deletion is implemented as a three-phase process - first we collect specifications of blockcommits used to perform the deletion and run sanity checks to verify that the whole operation is reasonably safe, then we launch the blockcommits and finally we wait for them to finish and check the results. This commit implements the first phase. All information necessary to launch and finish a blockcommit job per disk included in the snapshot is collected in a blockcommit descriptor structure and checked to verify the job makes sense and has a reasonable chance to succeed. Broadly speaking, the checks aim to ensure that the disks in the current running VM are the same as they were when the snapshot was created. Signed-off-by: Pavel Mores <pmores@redhat.com> --- src/qemu/qemu_driver.c | 154 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 154 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 6ffd53503b..dc1176bd9c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -16752,6 +16752,160 @@ qemuDomainMomentReparentChildren(void *payload, } +/* Blockcommit operation descriptor. Holds data required to launch and + * conclude an individual blockcommit job. Generic parameters + * (virDomainObjPtr, virQEMUDriverPtr) are not stored as they are assumed to be + * available at the point of blockcommit invocation. */ + +typedef struct { + virDomainDiskDefPtr disk; + virStorageSourcePtr baseSource; + virStorageSourcePtr topSource; + virStorageSourcePtr topParentSource; + int blockCommitFlags; + bool isActive; /* used to find out if pivot is needed to finish the job */ +} virBlockCommitDesc; + +/* Transforms a snapshot into an array of descriptors of blockcommit jobs + * required to delete the snapshot, one descriptor per affected disk. Also + * runs sanity checks for each affected disk and each individual job to make + * sure the deletion is safe to perform. Returns NULL if safety cannot be + * guaranteed. + * + * (A snapshot is considered safe to delete if, loosely speaking, we can be + * reasonably sure that all disks it touches are still those that were there + * when the snapshot was created. For instance, a different disk might be + * attached to the VM under the same target name, or a disk included in the + * snapshot might be reattached to the VM under a different target name. If + * any such thing happens between the times of snapshot creation and deletion, + * that shapshot would not be considered safe to delete.) */ + +static virBlockCommitDesc * +qemuDomainSnapshotDeleteExternalGetJobDescriptors(virDomainObjPtr vm, + virDomainMomentObjPtr snap, + unsigned int flags) +{ + size_t i; + bool isActive; + virDomainSnapshotDefPtr snapdef = virDomainSnapshotObjGetDef(snap); + virDomainMomentObjPtr parent = snap->parent; + virDomainSnapshotDefPtr parentdef = virDomainSnapshotObjGetDef(parent); + g_autofree virBlockCommitDesc *blockCommitDescs = g_new(virBlockCommitDesc, snapdef->ndisks); + int blockCommitFlags = VIR_DOMAIN_BLOCK_COMMIT_DELETE; + + if (flags & (VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN | + VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY)) { + isActive = true; + } else { + isActive = snap == virDomainSnapshotGetCurrent(vm->snapshots); + } + + if (isActive) + blockCommitFlags |= VIR_DOMAIN_BLOCK_COMMIT_ACTIVE; + + for (i = 0; i < snapdef->ndisks; i++) { + virDomainSnapshotDiskDefPtr snapDisk = &(snapdef->disks[i]); + const char *diskName = snapDisk->name; + virStorageSourcePtr baseSource = NULL; + virStorageSourcePtr topSource = NULL; + virStorageSourcePtr topParentSource = NULL; + virDomainDiskDefPtr domDiskNow; + virDomainDiskDefPtr domDiskThen = virDomainDiskByTarget(snapdef->parent.dom, diskName); + virStorageSourcePtr snapSource = NULL; + virStorageSourcePtr snapParentSource = NULL; + unsigned int snapIndex; + + if (!(domDiskNow = qemuDomainDiskByName(vm->def, diskName))) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("can't delete %s, disk '%s' not found in running VM"), + snapdef->parent.name, diskName); + return NULL; + } + if (snapDisk->src->type != VIR_STORAGE_TYPE_FILE || + domDiskNow->src->type != VIR_STORAGE_TYPE_FILE) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("can't delete %s, only storage type 'file' is supported"), + snapdef->parent.name); + return NULL; + } + + /* TODO Apr 24, 2020: this is not great long-term as it still looks up + * essentially just by path (or target[index]), far from a full + * comparison operator for virStorageSources */ + if (virStorageFileParseChainIndex(domDiskNow->dst, snapDisk->src->path, &snapIndex) < 0 || + !(snapSource = virStorageFileChainLookup(domDiskNow->src, NULL, + snapDisk->src->path, snapIndex, + &snapParentSource))) + return NULL; + + if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY) { + baseSource = snapSource; + } else { + if (parentdef == NULL) { + baseSource = virStorageFileChainLookup(domDiskNow->src, NULL, NULL, 0, NULL); + if (!baseSource) + return NULL; + } else { + baseSource = snapSource->backingStore; + } + } + + if (flags & (VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN | VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY)) { + topSource = domDiskNow->src; + } else { + topSource = snapSource; + topParentSource = snapParentSource; + } + + if (topSource == NULL) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("can't delete %s, couldn't find top (TODO improve this message)"), + snapdef->parent.name); + return NULL; + } + + if (baseSource == NULL) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("can't delete %s, couldn't find base (TODO improve this message)"), + snapdef->parent.name); + return NULL; + } + + /* Check if the disk called 'diskName' is the same as it was at the + * point 'snap' was created. This is more of a heuristic test as + * virStorageSources don't seem to be well equipped for establishing + * identity. The idea is based on the assumption that snapshots can + * only be removed mid-chain, not added. If that holds, the currently + * running chain has to be the same as it was when 'snap' was created + * *minus* the snapshots that were deleted in the meantime, if any. In + * other words, the chain back at the time of 'snap's creation has to + * be an ordered superset of the currently running disk's chain. */ + virStorageSourcePtr chainNow = snapSource->backingStore; + virStorageSourcePtr chainThen = NULL; + for (; virStorageSourceIsBacking(chainNow); chainNow = chainNow->backingStore) { + chainThen = virStorageFileChainLookup(domDiskThen->src, + chainThen, chainNow->path, 0, NULL); + if (!chainThen) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("can't delete %s, disk '%s' doesn't seem the same as when it was created (%s is in %s's backing chain now but it wasn't when the snapshot was created)"), + snapdef->parent.name, diskName, chainNow->path, + snapdef->parent.name); + return NULL; + } + } + + blockCommitDescs[i].disk = domDiskNow; + blockCommitDescs[i].baseSource = baseSource; + blockCommitDescs[i].topSource = topSource; + blockCommitDescs[i].topParentSource = topParentSource; + blockCommitDescs[i].blockCommitFlags = blockCommitFlags; + blockCommitDescs[i].isActive = isActive; + } + + return g_steal_pointer(&blockCommitDescs); +} + + static int qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot, unsigned int flags) -- 2.24.1
On Wed, May 06, 2020 at 13:42:22 +0200, Pavel Mores wrote:
Snapshot deletion is implemented as a three-phase process - first we collect specifications of blockcommits used to perform the deletion and run sanity checks to verify that the whole operation is reasonably safe, then we launch the blockcommits and finally we wait for them to finish and check the results.
This commit implements the first phase. All information necessary to launch and finish a blockcommit job per disk included in the snapshot is collected in a blockcommit descriptor structure and checked to verify the job makes sense and has a reasonable chance to succeed. Broadly speaking, the checks aim to ensure that the disks in the current running VM are the same as they were when the snapshot was created.
Signed-off-by: Pavel Mores <pmores@redhat.com> --- src/qemu/qemu_driver.c | 154 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 154 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 6ffd53503b..dc1176bd9c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -16752,6 +16752,160 @@ qemuDomainMomentReparentChildren(void *payload, }
+/* Blockcommit operation descriptor. Holds data required to launch and + * conclude an individual blockcommit job. Generic parameters + * (virDomainObjPtr, virQEMUDriverPtr) are not stored as they are assumed to be + * available at the point of blockcommit invocation. */ + +typedef struct { + virDomainDiskDefPtr disk; + virStorageSourcePtr baseSource; + virStorageSourcePtr topSource; + virStorageSourcePtr topParentSource; + int blockCommitFlags; + bool isActive; /* used to find out if pivot is needed to finish the job */ +} virBlockCommitDesc; + +/* Transforms a snapshot into an array of descriptors of blockcommit jobs + * required to delete the snapshot, one descriptor per affected disk. Also + * runs sanity checks for each affected disk and each individual job to make + * sure the deletion is safe to perform. Returns NULL if safety cannot be + * guaranteed. + * + * (A snapshot is considered safe to delete if, loosely speaking, we can be + * reasonably sure that all disks it touches are still those that were there + * when the snapshot was created. For instance, a different disk might be + * attached to the VM under the same target name, or a disk included in the + * snapshot might be reattached to the VM under a different target name. If + * any such thing happens between the times of snapshot creation and deletion, + * that shapshot would not be considered safe to delete.) */ + +static virBlockCommitDesc * +qemuDomainSnapshotDeleteExternalGetJobDescriptors(virDomainObjPtr vm, + virDomainMomentObjPtr snap, + unsigned int flags) +{ + size_t i; + bool isActive; + virDomainSnapshotDefPtr snapdef = virDomainSnapshotObjGetDef(snap); + virDomainMomentObjPtr parent = snap->parent; + virDomainSnapshotDefPtr parentdef = virDomainSnapshotObjGetDef(parent); + g_autofree virBlockCommitDesc *blockCommitDescs = g_new(virBlockCommitDesc, snapdef->ndisks); + int blockCommitFlags = VIR_DOMAIN_BLOCK_COMMIT_DELETE; + + if (flags & (VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN | + VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY)) { + isActive = true; + } else { + isActive = snap == virDomainSnapshotGetCurrent(vm->snapshots); + } + + if (isActive) + blockCommitFlags |= VIR_DOMAIN_BLOCK_COMMIT_ACTIVE; + + for (i = 0; i < snapdef->ndisks; i++) { + virDomainSnapshotDiskDefPtr snapDisk = &(snapdef->disks[i]); + const char *diskName = snapDisk->name; + virStorageSourcePtr baseSource = NULL; + virStorageSourcePtr topSource = NULL; + virStorageSourcePtr topParentSource = NULL; + virDomainDiskDefPtr domDiskNow; + virDomainDiskDefPtr domDiskThen = virDomainDiskByTarget(snapdef->parent.dom, diskName); + virStorageSourcePtr snapSource = NULL; + virStorageSourcePtr snapParentSource = NULL; + unsigned int snapIndex; + + if (!(domDiskNow = qemuDomainDiskByName(vm->def, diskName))) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("can't delete %s, disk '%s' not found in running VM"), + snapdef->parent.name, diskName); + return NULL; + } + if (snapDisk->src->type != VIR_STORAGE_TYPE_FILE ||
I can understand this so that you can look up based on the path ..
+ domDiskNow->src->type != VIR_STORAGE_TYPE_FILE) {
But the code doesn't seem to care about the path of the current top image. And I'd expect that it might be a problem if there are other non-file disks in the chain too.
+ virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("can't delete %s, only storage type 'file' is supported"), + snapdef->parent.name); + return NULL; + } + + /* TODO Apr 24, 2020: this is not great long-term as it still looks up + * essentially just by path (or target[index]), far from a full + * comparison operator for virStorageSources */ + if (virStorageFileParseChainIndex(domDiskNow->dst, snapDisk->src->path, &snapIndex) < 0 ||
Additionally disk->src->path never contains target[index] syntax so this call is pointless.
+ !(snapSource = virStorageFileChainLookup(domDiskNow->src, NULL, + snapDisk->src->path, snapIndex, + &snapParentSource))) + return NULL; + + if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY) { + baseSource = snapSource; + } else { + if (parentdef == NULL) { + baseSource = virStorageFileChainLookup(domDiskNow->src, NULL, NULL, 0, NULL); + if (!baseSource) + return NULL; + } else { + baseSource = snapSource->backingStore; + } + } + + if (flags & (VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN | VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY)) { + topSource = domDiskNow->src; + } else { + topSource = snapSource; + topParentSource = snapParentSource; + }
I'm actually missing a check that anything between topSource and baseSource are virStorageSources which are tracked by snapshot metadata which is going to be deleted. Basically we must make sure that we are not merging layers which are not described by snapshot metadata.
+ + if (topSource == NULL) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("can't delete %s, couldn't find top (TODO improve this message)"), + snapdef->parent.name); + return NULL; + } + + if (baseSource == NULL) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("can't delete %s, couldn't find base (TODO improve this message)"), + snapdef->parent.name); + return NULL; + } + + /* Check if the disk called 'diskName' is the same as it was at the + * point 'snap' was created. This is more of a heuristic test as + * virStorageSources don't seem to be well equipped for establishing + * identity. The idea is based on the assumption that snapshots can + * only be removed mid-chain, not added. If that holds, the currently + * running chain has to be the same as it was when 'snap' was created + * *minus* the snapshots that were deleted in the meantime, if any. In + * other words, the chain back at the time of 'snap's creation has to + * be an ordered superset of the currently running disk's chain. */ + virStorageSourcePtr chainNow = snapSource->backingStore; + virStorageSourcePtr chainThen = NULL; + for (; virStorageSourceIsBacking(chainNow); chainNow = chainNow->backingStore) { + chainThen = virStorageFileChainLookup(domDiskThen->src, + chainThen, chainNow->path, 0, NULL); + if (!chainThen) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("can't delete %s, disk '%s' doesn't seem the same as when it was created (%s is in %s's backing chain now but it wasn't when the snapshot was created)"), + snapdef->parent.name, diskName, chainNow->path, + snapdef->parent.name); + return NULL; + } + }
IMO the above is not as important as making sure that the chain between the images which are going to be deleted is the same as we track in metadata. The rest of the chain is not important if we zoom in to this one particular deletion.
+ + blockCommitDescs[i].disk = domDiskNow; + blockCommitDescs[i].baseSource = baseSource; + blockCommitDescs[i].topSource = topSource; + blockCommitDescs[i].topParentSource = topParentSource; + blockCommitDescs[i].blockCommitFlags = blockCommitFlags; + blockCommitDescs[i].isActive = isActive; + } + + return g_steal_pointer(&blockCommitDescs); +} + + static int qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot, unsigned int flags) -- 2.24.1
This is the second phase of snapshot deletion. We have all information necessary to delete the snapshot by running blockcommits and we haven't detected any problems that would make the deletion unsafe. Now we just launch the blockcommits in parallel. Signed-off-by: Pavel Mores <pmores@redhat.com> --- src/qemu/qemu_driver.c | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index dc1176bd9c..35b7fb69d5 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -154,6 +154,16 @@ static int qemuOpenFileAs(uid_t fallback_uid, gid_t fallback_gid, static virQEMUDriverPtr qemu_driver; +static int +qemuDomainBlockCommitCommon(virDomainObjPtr vm, + virQEMUDriverPtr driver, + virDomainDiskDefPtr disk, + virStorageSourcePtr baseSource, + virStorageSourcePtr topSource, + virStorageSourcePtr topParentSource, + unsigned long bandwidth, + unsigned int flags); + static int qemuDomainBlockCommitImpl(virDomainObjPtr vm, virQEMUDriverPtr driver, @@ -16906,6 +16916,31 @@ qemuDomainSnapshotDeleteExternalGetJobDescriptors(virDomainObjPtr vm, } +static int +qemuDomainSnapshotDeleteExternalLaunchJobs(virDomainObjPtr vm, + virQEMUDriverPtr driver, + const virBlockCommitDesc *blockCommitDescs, + int numDescs) +{ + size_t i; + + for (i = 0; i < numDescs; i++) { + virDomainDiskDefPtr disk = blockCommitDescs[i].disk; + virStorageSourcePtr baseSource = blockCommitDescs[i].baseSource; + virStorageSourcePtr topSource = blockCommitDescs[i].topSource; + virStorageSourcePtr topParentSource = blockCommitDescs[i].topParentSource; + int blockCommitFlags = blockCommitDescs[i].blockCommitFlags; + + if (qemuDomainBlockCommitCommon(vm, driver, disk, baseSource, + topSource, topParentSource, + 0, blockCommitFlags) < 0) + return -1; + } + + return 0; +} + + static int qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot, unsigned int flags) -- 2.24.1
This is the third phase of snapshot deletion. Blockcommits to delete the snapshot have been launched and now we can wait for them to finish, check results and report errors if any. Signed-off-by: Pavel Mores <pmores@redhat.com> --- src/qemu/qemu_driver.c | 59 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 35b7fb69d5..a2629e9002 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -16941,6 +16941,65 @@ qemuDomainSnapshotDeleteExternalLaunchJobs(virDomainObjPtr vm, } +static int +qemuDomainSnapshotDeleteExternalWaitForJobs(virDomainObjPtr vm, + virQEMUDriverPtr driver, + const virBlockCommitDesc *blockCommitDescs, + int numDescs) +{ + size_t i; + + for (i = 0; i < numDescs; i++) { + virDomainDiskDefPtr disk = blockCommitDescs[i].disk; + bool isActive = blockCommitDescs[i].isActive; + + /* wait for the blockcommit job to finish (in particular, reach + * one of the finished QEMU_BLOCKJOB_STATE_* states)... */ + g_autoptr(qemuBlockJobData) job = NULL; + + if (!(job = qemuBlockJobDiskGetJob(disk))) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("disk %s does not have an active block job"), disk->dst); + return -1; + } + + qemuBlockJobSyncBegin(job); + qemuBlockJobUpdate(vm, job, QEMU_ASYNC_JOB_NONE); + while (job->state != QEMU_BLOCKJOB_STATE_READY && + job->state != QEMU_BLOCKJOB_STATE_FAILED && + job->state != QEMU_BLOCKJOB_STATE_CANCELLED && + job->state != QEMU_BLOCKJOB_STATE_COMPLETED) { + + if (virDomainObjWait(vm) < 0) + return -1; + qemuBlockJobUpdate(vm, job, QEMU_ASYNC_JOB_NONE); + } + qemuBlockJobSyncEnd(vm, job, QEMU_ASYNC_JOB_NONE); + + if ((isActive && job->state != QEMU_BLOCKJOB_STATE_READY) || + (!isActive && job->state != QEMU_BLOCKJOB_STATE_COMPLETED)) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("blockcomit job failed for disk %s"), disk->dst); + /* TODO Apr 30, 2020: how to handle this? Bailing out doesn't + * seem an obvious option in this case as all blockjobs are now + * created and running - if any of them are to fail they will, + * regardless of whether we break here. It might make more + * sense to continue and at least report all errors. */ + /*return -1;*/ + } + + /* ... and pivot if necessary */ + if (isActive) { + if (qemuDomainBlockJobAbortImpl(driver, vm, disk->dst, + VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT) < 0) + return -1; + } + } + + return 0; +} + + static int qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot, unsigned int flags) -- 2.24.1
On Wed, May 06, 2020 at 13:42:24 +0200, Pavel Mores wrote:
This is the third phase of snapshot deletion. Blockcommits to delete the snapshot have been launched and now we can wait for them to finish, check results and report errors if any.
Signed-off-by: Pavel Mores <pmores@redhat.com> --- src/qemu/qemu_driver.c | 59 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 35b7fb69d5..a2629e9002 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -16941,6 +16941,65 @@ qemuDomainSnapshotDeleteExternalLaunchJobs(virDomainObjPtr vm, }
+static int +qemuDomainSnapshotDeleteExternalWaitForJobs(virDomainObjPtr vm, + virQEMUDriverPtr driver, + const virBlockCommitDesc *blockCommitDescs, + int numDescs) +{ + size_t i; + + for (i = 0; i < numDescs; i++) { + virDomainDiskDefPtr disk = blockCommitDescs[i].disk; + bool isActive = blockCommitDescs[i].isActive; + + /* wait for the blockcommit job to finish (in particular, reach + * one of the finished QEMU_BLOCKJOB_STATE_* states)... */ + g_autoptr(qemuBlockJobData) job = NULL; + + if (!(job = qemuBlockJobDiskGetJob(disk))) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("disk %s does not have an active block job"), disk->dst); + return -1; + } + + qemuBlockJobSyncBegin(job); + qemuBlockJobUpdate(vm, job, QEMU_ASYNC_JOB_NONE); + while (job->state != QEMU_BLOCKJOB_STATE_READY && + job->state != QEMU_BLOCKJOB_STATE_FAILED && + job->state != QEMU_BLOCKJOB_STATE_CANCELLED && + job->state != QEMU_BLOCKJOB_STATE_COMPLETED) { + + if (virDomainObjWait(vm) < 0) + return -1; + qemuBlockJobUpdate(vm, job, QEMU_ASYNC_JOB_NONE); + } + qemuBlockJobSyncEnd(vm, job, QEMU_ASYNC_JOB_NONE);
I'm pretty sure that this will not work for more than one disk. If the job for the disk with i=2 finishes sooner than the one for i=1 it will be auto-completed because you set qemuBlockJobSyncBegin(job); after you started the commit. This creates a race condition between this handler and the job itself. All the jobs MUST be marked as 'sync' prior to actually starting them in qemu if you need to wait/handle them here.
+ + if ((isActive && job->state != QEMU_BLOCKJOB_STATE_READY) || + (!isActive && job->state != QEMU_BLOCKJOB_STATE_COMPLETED)) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("blockcomit job failed for disk %s"), disk->dst); + /* TODO Apr 30, 2020: how to handle this? Bailing out doesn't + * seem an obvious option in this case as all blockjobs are now + * created and running - if any of them are to fail they will, + * regardless of whether we break here. It might make more + * sense to continue and at least report all errors. */
Aborting the jobs would be wrong IMO. I think that a better option is to wait for all other jobs to complete and report an error. The surrounding code needs to be able to try re-deleting such a snapshot which will then ignore the disks which were already commited as there wouldn't be an recovery path otherwise. The snapshot metadata must stay intact though.
+ /*return -1;*/ + } + + /* ... and pivot if necessary */ + if (isActive) { + if (qemuDomainBlockJobAbortImpl(driver, vm, disk->dst, + VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT) < 0) + return -1; + } + } + + return 0; +} + + static int qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot, unsigned int flags) -- 2.24.1
This uses the helpers introduced in previous three commits and assembles the top-level snapshot deletion algorithm in their terms. Note that the third phase - waiting for blockcommits to finish - is conceptually optional and a flag/parameter might be exposed to the user in the future to skip the third phase. This would make the whole operation asynchronous and let the user deal with concluding the blockcommits manually, using the generic blockjob tools. Signed-off-by: Pavel Mores <pmores@redhat.com> --- src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 64 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 935ef7303b..f68eb500ec 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1049,6 +1049,7 @@ virDomainListCheckpoints; # conf/virdomainmomentobjlist.h virDomainMomentDropChildren; virDomainMomentDropParent; +virDomainMomentFindLeaf; virDomainMomentForEachChild; virDomainMomentForEachDescendant; virDomainMomentMoveChildren; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a2629e9002..57e81e3720 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17000,6 +17000,70 @@ qemuDomainSnapshotDeleteExternalWaitForJobs(virDomainObjPtr vm, } +static int +qemuDomainSnapshotDeleteExternal(virDomainObjPtr vm, + virQEMUDriverPtr driver, + virDomainMomentObjPtr snap, + unsigned int flags) +{ + /* TODO Apr 29, 2020: ultimately, use 'flags' to set this. Until that + * is supported, just run always synchronously. */ + bool async = false; + virDomainSnapshotDefPtr snapdef = virDomainSnapshotObjGetDef(snap); + virDomainMomentObjPtr leaf = snap->nchildren ? virDomainMomentFindLeaf(snap) : snap; + virDomainMomentObjPtr parent = snap->parent; + g_autofree virBlockCommitDesc *blockCommitDescs = NULL; + int numBlockCommits = snapdef->ndisks; + + /* This function only works if the chain below 'snap' is linear. If + * there's no unique leaf it means the chain of 'snap's children + * branches at some point. Also, if there *is* a leaf but it's not + * the current snapshot, bail out as well. */ + if (leaf == NULL) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("can't delete '%s', snapshot chain branches"), + snapdef->parent.name); + return -1; + } + if (leaf != virDomainSnapshotGetCurrent(vm->snapshots)) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("can't delete '%s', leaf snapshot is not current"), + snapdef->parent.name); + return -1; + } + if (parent->nchildren > 1) { + /* TODO 'snap's parent has multiple children, meaning it's a + * branching point in snapshot tree. This means we can't + * delete 'snap' by commiting into its parent as doing so would + * corrupt the other branches rooted in the parent. We might + * still be able to delete 'snap' though by pulling into its + * child/children. */ + + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("can't delete %s, its parent has multiple children"), + snapdef->parent.name); + return -1; + } + + if (!virDomainObjIsActive(vm)) + return -1; + + blockCommitDescs = qemuDomainSnapshotDeleteExternalGetJobDescriptors(vm, snap, flags); + if (blockCommitDescs == NULL) + return -1; + + if (qemuDomainSnapshotDeleteExternalLaunchJobs(vm, driver, blockCommitDescs, numBlockCommits) < 0) + return -1; + + if (!async) { + if (qemuDomainSnapshotDeleteExternalWaitForJobs(vm, driver, blockCommitDescs, numBlockCommits) < 0) + return -1; + } + + return 0; +} + + static int qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot, unsigned int flags) -- 2.24.1
On Wed, May 06, 2020 at 13:42:25 +0200, Pavel Mores wrote:
This uses the helpers introduced in previous three commits and assembles the top-level snapshot deletion algorithm in their terms.
Note that the third phase - waiting for blockcommits to finish - is conceptually optional and a flag/parameter might be exposed to the user in the future to skip the third phase. This would make the whole operation asynchronous and let the user deal with concluding the blockcommits manually, using the generic blockjob tools.
Signed-off-by: Pavel Mores <pmores@redhat.com> --- src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 64 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 935ef7303b..f68eb500ec 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1049,6 +1049,7 @@ virDomainListCheckpoints; # conf/virdomainmomentobjlist.h virDomainMomentDropChildren; virDomainMomentDropParent; +virDomainMomentFindLeaf;
This definitely belongs to a different commit.
virDomainMomentForEachChild; virDomainMomentForEachDescendant; virDomainMomentMoveChildren; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a2629e9002..57e81e3720 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17000,6 +17000,70 @@ qemuDomainSnapshotDeleteExternalWaitForJobs(virDomainObjPtr vm, }
+static int
This is a static function which is unused in this patch. Compilation will fail.
+qemuDomainSnapshotDeleteExternal(virDomainObjPtr vm, + virQEMUDriverPtr driver, + virDomainMomentObjPtr snap, + unsigned int flags) +{ + /* TODO Apr 29, 2020: ultimately, use 'flags' to set this. Until that + * is supported, just run always synchronously. */ + bool async = false; + virDomainSnapshotDefPtr snapdef = virDomainSnapshotObjGetDef(snap); + virDomainMomentObjPtr leaf = snap->nchildren ? virDomainMomentFindLeaf(snap) : snap; + virDomainMomentObjPtr parent = snap->parent; + g_autofree virBlockCommitDesc *blockCommitDescs = NULL; + int numBlockCommits = snapdef->ndisks; + + /* This function only works if the chain below 'snap' is linear. If + * there's no unique leaf it means the chain of 'snap's children + * branches at some point. Also, if there *is* a leaf but it's not + * the current snapshot, bail out as well. */ + if (leaf == NULL) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("can't delete '%s', snapshot chain branches"), + snapdef->parent.name); + return -1; + } + if (leaf != virDomainSnapshotGetCurrent(vm->snapshots)) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("can't delete '%s', leaf snapshot is not current"), + snapdef->parent.name); + return -1; + } + if (parent->nchildren > 1) { + /* TODO 'snap's parent has multiple children, meaning it's a + * branching point in snapshot tree. This means we can't + * delete 'snap' by commiting into its parent as doing so would + * corrupt the other branches rooted in the parent. We might + * still be able to delete 'snap' though by pulling into its + * child/children. */ + + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("can't delete %s, its parent has multiple children"), + snapdef->parent.name); + return -1; + } + + if (!virDomainObjIsActive(vm)) + return -1; + + blockCommitDescs = qemuDomainSnapshotDeleteExternalGetJobDescriptors(vm, snap, flags); + if (blockCommitDescs == NULL) + return -1; + + if (qemuDomainSnapshotDeleteExternalLaunchJobs(vm, driver, blockCommitDescs, numBlockCommits) < 0) + return -1; + + if (!async) { + if (qemuDomainSnapshotDeleteExternalWaitForJobs(vm, driver, blockCommitDescs, numBlockCommits) < 0) + return -1;
This isn't really what I've meant by making it asynchronous. The asyncrhonous job is started by qemuDomainObjBeginAsyncJob and allows other operations to be executed while a long-running API is being executed. In this case we'll need a new async job type for this scenario and this also requires all the monitor calls done here to be converted to take the async job type. The above then allows for the snapshot deletion API to block until the snapshot deletion is done, while other operations such as statistic gathering still work.
+ } + + return 0; +} + + static int qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot, unsigned int flags) -- 2.24.1
Plug in the external snapshot deletion framework built in previous commits. Signed-off-by: Pavel Mores <pmores@redhat.com> --- src/qemu/qemu_driver.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 57e81e3720..34c0e27eb9 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17106,10 +17106,8 @@ qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot, qemuDomainSnapshotCountExternal, &external); if (external) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("deletion of %d external disk snapshots not " - "supported yet"), external); - goto endjob; + if (qemuDomainSnapshotDeleteExternal(vm, driver, snap, flags) < 0) + goto endjob; } } -- 2.24.1
participants (2)
-
Pavel Mores -
Peter Krempa