[libvirt] [PATCH v4 0/4] qemu: block: implement optional removal of committed snapshot images
This version incorporates Peter's feedback to v3. 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 | 50 ++++++++++++++++++- src/qemu/qemu_blockjob.h | 4 +- src/qemu/qemu_domain.c | 4 ++ src/qemu/qemu_driver.c | 12 ++++- .../blockjob-blockdev-in.xml | 1 + 5 files changed, 67 insertions(+), 4 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 81aa46c2fb..718e311213 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -282,7 +282,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; @@ -305,6 +306,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 52b03aaf9e..42b973fe96 100644 --- a/src/qemu/qemu_blockjob.h +++ b/src/qemu/qemu_blockjob.h @@ -87,6 +87,7 @@ struct _qemuBlockJobCommitData { virStorageSourcePtr topparent; virStorageSourcePtr top; virStorageSourcePtr base; + bool deleteCommittedImages; }; @@ -180,7 +181,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 9197dffadd..9c07b6b393 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -18686,7 +18686,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 Tue, Dec 10, 2019 at 17:25:38 +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(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
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. Signed-off-by: Pavel Mores <pmores@redhat.com> --- src/qemu/qemu_blockjob.c | 46 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index 718e311213..498e2a716f 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -1004,6 +1004,44 @@ qemuBlockJobProcessEventCompletedPull(virQEMUDriverPtr driver, } +/** + * qemuBlockJobDeleteImages: + * @driver: qemu driver object + * @vm: domain object + * @disk: disk object that the chain to be deleted is associated with + * @top: top snapshot of the chain to be deleted + * + * Helper for removing snapshot images. Intended for callers like + * qemuBlockJobProcessEventCompletedCommit() and + * qemuBlockJobProcessEventCompletedActiveCommit() as it 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 +qemuBlockJobDeleteImages(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainDiskDefPtr disk, + virStorageSourcePtr top) +{ + virStorageSourcePtr p = top; + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); + uid_t uid; + gid_t gid; + + for (; p != NULL; p = p->backingStore) { + if (virStorageSourceGetActualType(p) == VIR_STORAGE_TYPE_FILE) { + + 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 @@ -1070,6 +1108,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) + qemuBlockJobDeleteImages(driver, vm, job->disk, job->data.commit.top); + virObjectUnref(job->data.commit.top); job->data.commit.top = NULL; @@ -1160,6 +1202,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) + qemuBlockJobDeleteImages(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 */ -- 2.21.0
On Tue, Dec 10, 2019 at 17:25:39 +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.
Signed-off-by: Pavel Mores <pmores@redhat.com> --- src/qemu/qemu_blockjob.c | 46 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
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 767790bfc0..27926c7670 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2586,6 +2586,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: @@ -3185,6 +3187,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 67ab099bd9..b5d62fd4ab 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 Tue, Dec 10, 2019 at 17:25:40 +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 | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9c07b6b393..0cbc746c22 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -18544,10 +18544,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))) @@ -18568,6 +18568,13 @@ qemuDomainBlockCommit(virDomainPtr dom, blockdev = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV); + if (!blockdev && (flags & VIR_DOMAIN_BLOCK_COMMIT_DELETE)) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("deleting committed images is only supported for VMs " + "with blockdev enabled")); + goto endjob; + } + /* Convert bandwidth MiB to bytes, if necessary */ if (!(flags & VIR_DOMAIN_BLOCK_COMMIT_BANDWIDTH_BYTES)) { if (speed > LLONG_MAX >> 20) { @@ -18686,7 +18693,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 Tue, Dec 10, 2019 at 17:25:41 +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 | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9c07b6b393..0cbc746c22 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -18544,10 +18544,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))) @@ -18568,6 +18568,13 @@ qemuDomainBlockCommit(virDomainPtr dom,
blockdev = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV);
+ if (!blockdev && (flags & VIR_DOMAIN_BLOCK_COMMIT_DELETE)) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("deleting committed images is only supported for VMs " + "with blockdev enabled")); + goto endjob; + }
make[1]: Leaving directory '/home/pipo/build/libvirt/gcc/src' /home/pipo/libvirt/src/qemu/qemu_driver.c:18572: virReportError(VIR_ERR_OPERATION_UNSUPPORTED, /home/pipo/libvirt/src/qemu/qemu_driver.c-18573- _("deleting committed images is only supported for VMs " /home/pipo/libvirt/src/qemu/qemu_driver.c-18574- "with blockdev enabled")); build-aux/syntax-check.mk: found diagnostic without % make: *** [/home/pipo/libvirt/build-aux/syntax-check.mk:756: sc_prohibit_diagnostic_without_format] Error 1 Please make sure you run 'make syntax-check' before submitting patches. I'll fix this before pushing with with: if (!blockdev && (flags & VIR_DOMAIN_BLOCK_COMMIT_DELETE)) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("deleting committed images is not supported by this VM")); goto endjob; } Reviewed-by: Peter Krempa <pkrempa@redhat.com>
On Wed, Dec 11, 2019 at 08:34:38AM +0100, Peter Krempa wrote:
On Tue, Dec 10, 2019 at 17:25:41 +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 | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9c07b6b393..0cbc746c22 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -18544,10 +18544,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))) @@ -18568,6 +18568,13 @@ qemuDomainBlockCommit(virDomainPtr dom,
blockdev = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV);
+ if (!blockdev && (flags & VIR_DOMAIN_BLOCK_COMMIT_DELETE)) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("deleting committed images is only supported for VMs " + "with blockdev enabled")); + goto endjob; + }
make[1]: Leaving directory '/home/pipo/build/libvirt/gcc/src' /home/pipo/libvirt/src/qemu/qemu_driver.c:18572: virReportError(VIR_ERR_OPERATION_UNSUPPORTED, /home/pipo/libvirt/src/qemu/qemu_driver.c-18573- _("deleting committed images is only supported for VMs " /home/pipo/libvirt/src/qemu/qemu_driver.c-18574- "with blockdev enabled")); build-aux/syntax-check.mk: found diagnostic without % make: *** [/home/pipo/libvirt/build-aux/syntax-check.mk:756: sc_prohibit_diagnostic_without_format] Error 1
Please make sure you run 'make syntax-check' before submitting patches.
I'll fix this before pushing with with:
if (!blockdev && (flags & VIR_DOMAIN_BLOCK_COMMIT_DELETE)) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("deleting committed images is not supported by this VM")); goto endjob; }
Okay, my apologies, and thanks for the speedy review! pvl
participants (2)
-
Pavel Mores -
Peter Krempa