[libvirt] [PATCH 0/4] qemu: block: implement optional removal of committed snapshot images
Adds handling of the VIR_DOMAIN_BLOCK_COMMIT_DELETE flag to the qemu driver which previously just ignored it. This should fix a problem with the virsh blockcommit command where --delete did nothing. https://bugzilla.redhat.com/show_bug.cgi?id=1008350 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: add unit test of storing the delete flag in status XML src/qemu/qemu_blockjob.c | 35 ++++++++++++++++++- src/qemu/qemu_blockjob.h | 4 ++- src/qemu/qemu_domain.c | 4 +++ src/qemu/qemu_driver.c | 5 +-- .../blockjob-blockdev-in.xml | 1 + 5 files changed, 45 insertions(+), 4 deletions(-) -- 2.21.0
Since the blockcommit operation is asynchronous, this has conceptually two parts. First, we have to propagate the flag from qemuDomainBlockCommit() (which was just ignoring it until now) to qemuBlockJobDiskNewCommit(). Then it can be stored in the qemuBlockJobCommitData structure which holds information necessary to finish the job asynchronously. 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 | 5 +++-- 3 files changed, 9 insertions(+), 4 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..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))) + baseSource, + flags & VIR_DOMAIN_BLOCK_COMMIT_DELETE))) goto endjob; disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_NONE; -- 2.21.0
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. Signed-off-by: Pavel Mores <pmores@redhat.com> --- src/qemu/qemu_blockjob.c | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index 7d94a6ce38..dae1fa44c8 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -965,6 +965,31 @@ 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(virStorageSourcePtr top) +{ + virStorageSourcePtr p = top; + const size_t errmsg_len = 128; + char errmsg[errmsg_len]; + + for (; p != NULL; p = p->backingStore) { + if (virStorageSourceIsLocalStorage(p)) + if (unlink(p->path) < 0) { + char *dummy = strerror_r(errno, errmsg, errmsg_len); + (void)dummy; + VIR_WARN("Unable to remove snapshot image file %s (%s)", + p->path, errmsg); + } + } +} + /** * qemuBlockJobProcessEventCompletedCommit: * @driver: qemu driver object @@ -1031,6 +1056,9 @@ 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(job->data.commit.top); virObjectUnref(job->data.commit.top); job->data.commit.top = NULL; @@ -1121,6 +1149,9 @@ 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(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 */ -- 2.21.0
On Wed, Nov 20, 2019 at 10:55:25AM +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.
Signed-off-by: Pavel Mores <pmores@redhat.com> --- src/qemu/qemu_blockjob.c | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+)
diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index 7d94a6ce38..dae1fa44c8 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -965,6 +965,31 @@ 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(virStorageSourcePtr top) +{ + virStorageSourcePtr p = top; + const size_t errmsg_len = 128; + char errmsg[errmsg_len]; + + for (; p != NULL; p = p->backingStore) { + if (virStorageSourceIsLocalStorage(p)) + if (unlink(p->path) < 0) { + char *dummy = strerror_r(errno, errmsg, errmsg_len); + (void)dummy;
Sorry, just realised that a non-standard version of strerror_r() is used here where the return value is actually useful. I'll fix this is in v2.
+ VIR_WARN("Unable to remove snapshot image file %s (%s)", + p->path, errmsg); + } + } +} + /** * qemuBlockJobProcessEventCompletedCommit: * @driver: qemu driver object @@ -1031,6 +1056,9 @@ 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(job->data.commit.top); virObjectUnref(job->data.commit.top); job->data.commit.top = NULL;
@@ -1121,6 +1149,9 @@ 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(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 */ -- 2.21.0
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. Signed-off-by: Pavel Mores <pmores@redhat.com> --- src/qemu/qemu_domain.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 262b74d1ab..dba287973c 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; -- 2.21.0
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> --- tests/qemustatusxml2xmldata/blockjob-blockdev-in.xml | 1 + 1 file changed, 1 insertion(+) 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
participants (1)
-
Pavel Mores