[libvirt] [PATCH v3 0/4] qemu: block: implement optional removal of committed snapshot images
v3 aims to incorporate Peter's feedback to v2, mainly: - patch 1 was split into two - the delete flag propagation stays in patch 1, the actual enabling the feature was moved to what's now patch 4 - patch 2 was modified to handle NFS and to use g_strerror() instead of strerror_r() - the former patch 4 was squashed into patch 3. Pavel Mores (4): qemu: block: propagate the delete flag to where it can actually be used qemu: block: use the delete flag to delete snapshot images if requested qemu: block: store the delete flag in libvirtd's status XML qemu: block: enable the snapshot image deletion feature src/qemu/qemu_blockjob.c | 43 ++++++++++++++++++- src/qemu/qemu_blockjob.h | 4 +- src/qemu/qemu_domain.c | 6 ++- src/qemu/qemu_domain.h | 6 +++ src/qemu/qemu_driver.c | 5 ++- .../blockjob-blockdev-in.xml | 1 + 6 files changed, 60 insertions(+), 5 deletions(-) -- 2.21.0
Propagate the delete flag from qemuDomainBlockCommit() (which was just ignoring it until now) to qemuBlockJobDiskNewCommit() where it can be stored in the qemuBlockJobCommitData structure which holds information necessary to finish the job asynchronously. In the actual qemuBlockJobDiskNewCommit() in this commit, we temporarily pass a literal 'false' to preserve the current behaviour until the whole implementation of the feature is in place. Signed-off-by: Pavel Mores <pmores@redhat.com> --- src/qemu/qemu_blockjob.c | 4 +++- src/qemu/qemu_blockjob.h | 4 +++- src/qemu/qemu_driver.c | 2 +- 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index 5c294f8024..7d94a6ce38 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -250,7 +250,8 @@ qemuBlockJobDiskNewCommit(virDomainObjPtr vm, virDomainDiskDefPtr disk, virStorageSourcePtr topparent, virStorageSourcePtr top, - virStorageSourcePtr base) + virStorageSourcePtr base, + bool delete_imgs) { qemuDomainObjPrivatePtr priv = vm->privateData; g_autoptr(qemuBlockJobData) job = NULL; @@ -273,6 +274,7 @@ qemuBlockJobDiskNewCommit(virDomainObjPtr vm, job->data.commit.topparent = topparent; job->data.commit.top = top; job->data.commit.base = base; + job->data.commit.deleteCommittedImages = delete_imgs; if (qemuBlockJobRegister(job, vm, disk, true) < 0) return NULL; diff --git a/src/qemu/qemu_blockjob.h b/src/qemu/qemu_blockjob.h index d8da918f2f..d77f1dcdb3 100644 --- a/src/qemu/qemu_blockjob.h +++ b/src/qemu/qemu_blockjob.h @@ -85,6 +85,7 @@ struct _qemuBlockJobCommitData { virStorageSourcePtr topparent; virStorageSourcePtr top; virStorageSourcePtr base; + bool deleteCommittedImages; }; @@ -165,7 +166,8 @@ qemuBlockJobDiskNewCommit(virDomainObjPtr vm, virDomainDiskDefPtr disk, virStorageSourcePtr topparent, virStorageSourcePtr top, - virStorageSourcePtr base); + virStorageSourcePtr base, + bool delete_imgs); qemuBlockJobDataPtr qemuBlockJobNewCreate(virDomainObjPtr vm, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index dc14ec86a3..a3ff4b2177 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -18638,7 +18638,7 @@ qemuDomainBlockCommit(virDomainPtr dom, goto endjob; if (!(job = qemuBlockJobDiskNewCommit(vm, disk, top_parent, topSource, - baseSource))) + baseSource, false))) goto endjob; disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_NONE; -- 2.21.0
On Thu, Nov 21, 2019 at 11:00:44 +0100, Pavel Mores wrote:
Propagate the delete flag from qemuDomainBlockCommit() (which was just ignoring it until now) to qemuBlockJobDiskNewCommit() where it can be stored in the qemuBlockJobCommitData structure which holds information necessary to finish the job asynchronously.
In the actual qemuBlockJobDiskNewCommit() in this commit, we temporarily pass a literal 'false' to preserve the current behaviour until the whole implementation of the feature is in place.
Signed-off-by: Pavel Mores <pmores@redhat.com> --- src/qemu/qemu_blockjob.c | 4 +++- src/qemu/qemu_blockjob.h | 4 +++- src/qemu/qemu_driver.c | 2 +- 3 files changed, 7 insertions(+), 3 deletions(-)
ACK
When blockcommit finishes successfully, one of the qemuBlockJobProcessEventCompletedCommit() and qemuBlockJobProcessEventCompletedActiveCommit() event handlers is called. This is where the delete flag (stored in qemuBlockJobCommitData since the previous commit) can actually be used to delete the committed snapshot images if requested. We use virFileRemove() instead of a simple unlink() to cover the case where the image to be removed is on an NFS volume. To acquire the uid/gid arguments for the virFileRemove() call, we call qemuDomainGetImageIds() which was previously static in its file so we first have to export it. Signed-off-by: Pavel Mores <pmores@redhat.com> --- src/qemu/qemu_blockjob.c | 39 +++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_domain.h | 6 ++++++ 3 files changed, 46 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index 7d94a6ce38..1bf23dac3c 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -965,6 +965,37 @@ qemuBlockJobProcessEventCompletedPull(virQEMUDriverPtr driver, } +/** + * Helper for qemuBlockJobProcessEventCompletedCommit() and + * qemuBlockJobProcessEventCompletedActiveCommit(). Relies on adjustments + * these functions perform on the 'backingStore' chain to function correctly. + * + * TODO look into removing backing store for non-local snapshots too + */ +static void +qemuBlockJobUnlinkCommittedStorage(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainDiskDefPtr disk, + virStorageSourcePtr top) +{ + virStorageSourcePtr p = top; + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + uid_t uid; + gid_t gid; + + for (; p != NULL; p = p->backingStore) { + if (virStorageSourceIsLocalStorage(p)) { + + qemuDomainGetImageIds(cfg, vm, p, disk->src, &uid, &gid); + + if (virFileRemove(p->path, uid, gid) < 0) { + VIR_WARN("Unable to remove snapshot image file %s (%s)", + p->path, g_strerror(errno)); + } + } + } +} + /** * qemuBlockJobProcessEventCompletedCommit: * @driver: qemu driver object @@ -1031,6 +1062,10 @@ qemuBlockJobProcessEventCompletedCommit(virQEMUDriverPtr driver, job->data.commit.topparent->backingStore = job->data.commit.base; qemuBlockJobEventProcessConcludedRemoveChain(driver, vm, asyncJob, job->data.commit.top); + + if (job->data.commit.deleteCommittedImages) + qemuBlockJobUnlinkCommittedStorage(driver, vm, job->disk, job->data.commit.top); + virObjectUnref(job->data.commit.top); job->data.commit.top = NULL; @@ -1121,6 +1156,10 @@ qemuBlockJobProcessEventCompletedActiveCommit(virQEMUDriverPtr driver, job->disk->src->readonly = job->data.commit.top->readonly; qemuBlockJobEventProcessConcludedRemoveChain(driver, vm, asyncJob, job->data.commit.top); + + if (job->data.commit.deleteCommittedImages) + qemuBlockJobUnlinkCommittedStorage(driver, vm, job->disk, job->data.commit.top); + virObjectUnref(job->data.commit.top); job->data.commit.top = NULL; /* the mirror element does not serve functional purpose for the commit job */ diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 262b74d1ab..07bf8f5a54 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -10133,7 +10133,7 @@ qemuDomainCleanupRun(virQEMUDriverPtr driver, priv->ncleanupCallbacks_max = 0; } -static void +void qemuDomainGetImageIds(virQEMUDriverConfigPtr cfg, virDomainObjPtr vm, virStorageSourcePtr src, diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 98a9540275..f66610c7d3 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -834,6 +834,12 @@ bool qemuDomainDiskChangeSupported(virDomainDiskDefPtr disk, const char *qemuDomainDiskNodeFormatLookup(virDomainObjPtr vm, const char *disk); +void qemuDomainGetImageIds(virQEMUDriverConfigPtr cfg, + virDomainObjPtr vm, + virStorageSourcePtr src, + virStorageSourcePtr parentSrc, + uid_t *uid, gid_t *gid); + int qemuDomainStorageFileInit(virQEMUDriverPtr driver, virDomainObjPtr vm, virStorageSourcePtr src, -- 2.21.0
On Thu, Nov 21, 2019 at 11:00:45 +0100, Pavel Mores wrote:
When blockcommit finishes successfully, one of the qemuBlockJobProcessEventCompletedCommit() and qemuBlockJobProcessEventCompletedActiveCommit() event handlers is called. This is where the delete flag (stored in qemuBlockJobCommitData since the previous commit) can actually be used to delete the committed snapshot images if requested.
We use virFileRemove() instead of a simple unlink() to cover the case where the image to be removed is on an NFS volume. To acquire the uid/gid arguments for the virFileRemove() call, we call qemuDomainGetImageIds() which was previously static in its file so we first have to export it.
Signed-off-by: Pavel Mores <pmores@redhat.com> --- src/qemu/qemu_blockjob.c | 39 +++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_domain.h | 6 ++++++ 3 files changed, 46 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index 7d94a6ce38..1bf23dac3c 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -965,6 +965,37 @@ qemuBlockJobProcessEventCompletedPull(virQEMUDriverPtr driver, }
+/** + * Helper for qemuBlockJobProcessEventCompletedCommit() and + * qemuBlockJobProcessEventCompletedActiveCommit(). Relies on adjustments + * these functions perform on the 'backingStore' chain to function correctly.
Preferably use the format of the comments we use elsewhere too.
+ * + * TODO look into removing backing store for non-local snapshots too + */ +static void +qemuBlockJobUnlinkCommittedStorage(virQEMUDriverPtr driver,
You could name it more generic as this same operation may be added for other block jobs in the future. E.g. qemuBlockJobDeleteImages.
+ virDomainObjPtr vm, + virDomainDiskDefPtr disk, + virStorageSourcePtr top) +{ + virStorageSourcePtr p = top; + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + uid_t uid; + gid_t gid; + + for (; p != NULL; p = p->backingStore) { + if (virStorageSourceIsLocalStorage(p)) {
This also returns true for 'VIR_STORAGE_TYPE_FILE' but we certainly don't want to unlink those.
+ + qemuDomainGetImageIds(cfg, vm, p, disk->src, &uid, &gid); + + if (virFileRemove(p->path, uid, gid) < 0) { + VIR_WARN("Unable to remove snapshot image file %s (%s)",
Add apostrophes around the first %s -> '%s'
+ p->path, g_strerror(errno)); + } + } + } +} + /** * qemuBlockJobProcessEventCompletedCommit: * @driver: qemu driver object @@ -1031,6 +1062,10 @@ qemuBlockJobProcessEventCompletedCommit(virQEMUDriverPtr driver, job->data.commit.topparent->backingStore = job->data.commit.base;
qemuBlockJobEventProcessConcludedRemoveChain(driver, vm, asyncJob, job->data.commit.top); + + if (job->data.commit.deleteCommittedImages) + qemuBlockJobUnlinkCommittedStorage(driver, vm, job->disk, job->data.commit.top); + virObjectUnref(job->data.commit.top); job->data.commit.top = NULL;
@@ -1121,6 +1156,10 @@ qemuBlockJobProcessEventCompletedActiveCommit(virQEMUDriverPtr driver, job->disk->src->readonly = job->data.commit.top->readonly;
qemuBlockJobEventProcessConcludedRemoveChain(driver, vm, asyncJob, job->data.commit.top); + + if (job->data.commit.deleteCommittedImages) + qemuBlockJobUnlinkCommittedStorage(driver, vm, job->disk, job->data.commit.top); + virObjectUnref(job->data.commit.top); job->data.commit.top = NULL; /* the mirror element does not serve functional purpose for the commit job */ diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 262b74d1ab..07bf8f5a54 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -10133,7 +10133,7 @@ qemuDomainCleanupRun(virQEMUDriverPtr driver, priv->ncleanupCallbacks_max = 0; }
-static void +void qemuDomainGetImageIds(virQEMUDriverConfigPtr cfg, virDomainObjPtr vm, virStorageSourcePtr src,
Preferably export this in a separate commit.
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 98a9540275..f66610c7d3 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -834,6 +834,12 @@ bool qemuDomainDiskChangeSupported(virDomainDiskDefPtr disk, const char *qemuDomainDiskNodeFormatLookup(virDomainObjPtr vm, const char *disk);
+void qemuDomainGetImageIds(virQEMUDriverConfigPtr cfg, + virDomainObjPtr vm, + virStorageSourcePtr src, + virStorageSourcePtr parentSrc, + uid_t *uid, gid_t *gid); + int qemuDomainStorageFileInit(virQEMUDriverPtr driver, virDomainObjPtr vm, virStorageSourcePtr src, -- 2.21.0
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Nov 21, 2019 at 11:00:45 +0100, Pavel Mores wrote:
When blockcommit finishes successfully, one of the qemuBlockJobProcessEventCompletedCommit() and qemuBlockJobProcessEventCompletedActiveCommit() event handlers is called. This is where the delete flag (stored in qemuBlockJobCommitData since the previous commit) can actually be used to delete the committed snapshot images if requested.
We use virFileRemove() instead of a simple unlink() to cover the case where the image to be removed is on an NFS volume. To acquire the uid/gid arguments for the virFileRemove() call, we call qemuDomainGetImageIds() which was previously static in its file so we first have to export it.
Signed-off-by: Pavel Mores <pmores@redhat.com> --- src/qemu/qemu_blockjob.c | 39 +++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_domain.h | 6 ++++++ 3 files changed, 46 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index 7d94a6ce38..1bf23dac3c 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -965,6 +965,37 @@ qemuBlockJobProcessEventCompletedPull(virQEMUDriverPtr driver, }
+/** + * Helper for qemuBlockJobProcessEventCompletedCommit() and + * qemuBlockJobProcessEventCompletedActiveCommit(). Relies on adjustments + * these functions perform on the 'backingStore' chain to function correctly. + * + * TODO look into removing backing store for non-local snapshots too + */ +static void +qemuBlockJobUnlinkCommittedStorage(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainDiskDefPtr disk, + virStorageSourcePtr top) +{ + virStorageSourcePtr p = top; + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
The reference acquired here is leaked from this function. You can use g_autoptr for it.
Since blockcommit is asynchronous, libvirtd can be restarted while the operation runs. To ensure the information necessary to finish up the job is not lost, serialisation to and deserialisation from the status XML is added. To unittest this, the new element was only added to the active commit test, the non-active commit test doesn't have the new element so as to test its absence. Signed-off-by: Pavel Mores <pmores@redhat.com> --- src/qemu/qemu_domain.c | 4 ++++ tests/qemustatusxml2xmldata/blockjob-blockdev-in.xml | 1 + 2 files changed, 5 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 07bf8f5a54..f4cb0f9ea0 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2512,6 +2512,8 @@ qemuDomainObjPrivateXMLFormatBlockjobIterator(void *payload, virBufferAsprintf(&childBuf, "<top node='%s'/>\n", job->data.commit.top->nodeformat); if (job->data.commit.topparent) virBufferAsprintf(&childBuf, "<topparent node='%s'/>\n", job->data.commit.topparent->nodeformat); + if (job->data.commit.deleteCommittedImages) + virBufferAddLit(&childBuf, "<deleteCommittedImages/>\n"); break; case QEMU_BLOCKJOB_TYPE_CREATE: @@ -3069,6 +3071,8 @@ qemuDomainObjPrivateXMLParseBlockjobDataSpecific(qemuBlockJobDataPtr job, "string(./base/@node)", &job->data.commit.base, ctxt); + if (virXPathNode("./deleteCommittedImages", ctxt)) + job->data.commit.deleteCommittedImages = true; if (!job->data.commit.top || !job->data.commit.base) goto broken; diff --git a/tests/qemustatusxml2xmldata/blockjob-blockdev-in.xml b/tests/qemustatusxml2xmldata/blockjob-blockdev-in.xml index 4f6930001e..5fd2ed1d78 100644 --- a/tests/qemustatusxml2xmldata/blockjob-blockdev-in.xml +++ b/tests/qemustatusxml2xmldata/blockjob-blockdev-in.xml @@ -242,6 +242,7 @@ <disk dst='vde'/> <base node='libvirt-19-format'/> <top node='libvirt-17-format'/> + <deleteCommittedImages/> </blockjob> <blockjob name='create-libvirt-1337-storage' type='create' state='running'> <create mode='storage'/> -- 2.21.0
On Thu, Nov 21, 2019 at 11:00:46 +0100, Pavel Mores wrote:
Since blockcommit is asynchronous, libvirtd can be restarted while the operation runs. To ensure the information necessary to finish up the job is not lost, serialisation to and deserialisation from the status XML is added.
To unittest this, the new element was only added to the active commit test, the non-active commit test doesn't have the new element so as to test its absence.
Signed-off-by: Pavel Mores <pmores@redhat.com> --- src/qemu/qemu_domain.c | 4 ++++ tests/qemustatusxml2xmldata/blockjob-blockdev-in.xml | 1 + 2 files changed, 5 insertions(+)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
With all plumbing in place, we can now enable the new functionality. Signed-off-by: Pavel Mores <pmores@redhat.com> --- src/qemu/qemu_driver.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a3ff4b2177..75458f5c8a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -18496,10 +18496,10 @@ qemuDomainBlockCommit(virDomainPtr dom, bool persistjob = false; bool blockdev = false; - /* XXX Add support for COMMIT_DELETE */ 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))) @@ -18638,7 +18638,8 @@ qemuDomainBlockCommit(virDomainPtr dom, goto endjob; if (!(job = qemuBlockJobDiskNewCommit(vm, disk, top_parent, topSource, - baseSource, false))) + baseSource, + flags & VIR_DOMAIN_BLOCK_COMMIT_DELETE))) goto endjob; disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_NONE; -- 2.21.0
On Thu, Nov 21, 2019 at 11:00:47 +0100, Pavel Mores wrote:
With all plumbing in place, we can now enable the new functionality.
Signed-off-by: Pavel Mores <pmores@redhat.com> --- src/qemu/qemu_driver.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a3ff4b2177..75458f5c8a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -18496,10 +18496,10 @@ qemuDomainBlockCommit(virDomainPtr dom, bool persistjob = false; bool blockdev = false;
- /* XXX Add support for COMMIT_DELETE */ 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))) @@ -18638,7 +18638,8 @@ qemuDomainBlockCommit(virDomainPtr dom, goto endjob;
You must check that VIR_QEMU_CAPS_BLOCKDEV is enabled for this VM otherwise the flag won't work. If blockdev isn't enabled you still must produce an error message.
participants (2)
-
Pavel Mores -
Peter Krempa