[libvirt] [PATCH] snapshot: improve qemu handling of reused snapshot targets
Peter Krempa
pkrempa at redhat.com
Tue Mar 27 11:35:37 UTC 2012
Dňa 20.3.2012 22:29, Eric Blake wrote / napísal(a):
> The oVirt developers have stated that the real reasons they want
> to have qemu reuse existing volumes when creating a snapshot are:
> 1. the management framework is set up so that creation has to be
> done from a central node for proper resource tracking, and having
> libvirt and/or qemu create things violates the framework, and
> 2. qemu defaults to creating snapshots with an absolute path to
> the backing file, but oVirt wants to manage a backing chain that
> uses just relative names, to allow for easier migration of a chain
> across storage locations.
>
> When 0.9.10 added VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT (commit
> 4e9953a4), it only addressed point 1, but libvirt was still using
> O_TRUNC which violates point 2. Meanwhile, the new qemu
> 'transaction' monitor command includes a new optional mode argument
> that will force qemu to reuse the metadata of the file it just
> opened (with the burden on the caller to have valid metadata there
> in the first place). So, this tweaks the meaning of the flag to
> cover both points as intended for use by oVirt. It is not strictly
> backward-compatible to 0.9.10 behavior, but it can be argued that
> the O_TRUNC of 0.9.10 was a bug.
>
> Note that this flag is all-or-nothing, and only selects between
> 'existing' and the default 'absolute-paths'. A more flexible
> approach that would allow per-disk selections, as well as adding
> support for the 'no-backing-file' mode, would be possible by
> extending the<domainsnapshot> xml to have a per-disk mode, but
> until we have a management application expressing a need for that
> additional complexity, it is not worth doing.
>
> * src/libvirt.c (virDomainSnapshotCreateXML): Tweak documentation.
> * src/qemu/qemu_monitor.h (qemuMonitorDiskSnapshot): Add
> parameters.
> * src/qemu/qemu_monitor_json.h (qemuMonitorJSONDiskSnapshot):
> Likewise.
> * src/qemu/qemu_monitor.c (qemuMonitorDiskSnapshot): Pass them
> through.
> * src/qemu/qemu_monitor_json.c (qemuMonitorJSONDiskSnapshot): Use
> new monitor command arguments.
> * src/qemu/qemu_driver.c (qemuDomainSnapshotCreateDiskActive)
> (qemuDomainSnapshotCreateSingleDiskActive): Adjust callers.
> (qemuDomainSnapshotDiskPrepare): Allow qed, modify rules on reuse.
> ---
> src/libvirt.c | 11 +++++++----
> src/qemu/qemu_driver.c | 33 ++++++++++++++++++++++-----------
> src/qemu/qemu_monitor.c | 14 ++++++++------
> src/qemu/qemu_monitor.h | 4 +++-
> src/qemu/qemu_monitor_json.c | 6 +++++-
> src/qemu/qemu_monitor_json.h | 4 +++-
> 6 files changed, 48 insertions(+), 24 deletions(-)
>
> diff --git a/src/libvirt.c b/src/libvirt.c
> index 534c8ac..7df3667 100644
> --- a/src/libvirt.c
> +++ b/src/libvirt.c
> @@ -17113,10 +17113,13 @@ virDomainSnapshotGetConnect(virDomainSnapshotPtr snapshot)
> * VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY to be passed as well.
> *
> * By default, if the snapshot involves external files, and any of the
> - * destination files already exist as a regular file, the snapshot is
> - * rejected to avoid losing contents of those files. However, if
> - * @flags includes VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT, then existing
> - * destination files are instead truncated and reused.
> + * destination files already exist as a non-empty regular file, the
> + * snapshot is rejected to avoid losing contents of those files.
> + * However, if @flags includes VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT,
> + * then the destination files must already exist and contain content
> + * identical to the source files (this allows a management app to
> + * pre-create files with relative backing file names, rather than the
> + * default of creating with absolute backing file names).
> *
> * Be aware that although libvirt prefers to report errors up front with
> * no other effect, some hypervisors have certain types of failures where
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 2bc8d5d..bc21df0 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -9725,6 +9725,12 @@ qemuDomainSnapshotDiskPrepare(virDomainObjPtr vm, virDomainSnapshotDefPtr def,
> int external = 0;
> qemuDomainObjPrivatePtr priv = vm->privateData;
>
> + if (allow_reuse&& !qemuCapsGet(priv->qemuCaps, QEMU_CAPS_TRANSACTION)) {
> + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("reuse is not supported with this QEMU binary"));
> + goto cleanup;
> + }
> +
> for (i = 0; i< def->ndisks; i++) {
> virDomainSnapshotDiskDefPtr disk =&def->disks[i];
>
> @@ -9755,8 +9761,8 @@ qemuDomainSnapshotDiskPrepare(virDomainObjPtr vm, virDomainSnapshotDefPtr def,
> virReportOOMError();
> goto cleanup;
> }
> - } else if (STRNEQ(disk->driverType, "qcow2")) {
> - /* XXX We should also support QED */
> + } else if (STRNEQ(disk->driverType, "qcow2")&&
> + STRNEQ(disk->driverType, "qed")) {
> qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> _("external snapshot format for disk %s "
> "is unsupported: %s"),
> @@ -9770,7 +9776,7 @@ qemuDomainSnapshotDiskPrepare(virDomainObjPtr vm, virDomainSnapshotDefPtr def,
> disk->name, disk->file);
> goto cleanup;
> }
> - } else if (!(S_ISBLK(st.st_mode) || allow_reuse)) {
> + } else if (!(S_ISBLK(st.st_mode) || !st.st_size || allow_reuse)) {
> qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> _("external snapshot file for disk %s already "
> "exists and is not a block device: %s"),
> @@ -9823,7 +9829,8 @@ qemuDomainSnapshotCreateSingleDiskActive(struct qemud_driver *driver,
> virDomainSnapshotDiskDefPtr snap,
> virDomainDiskDefPtr disk,
> virDomainDiskDefPtr persistDisk,
> - virJSONValuePtr actions)
> + virJSONValuePtr actions,
> + bool reuse)
> {
> qemuDomainObjPrivatePtr priv = vm->privateData;
> char *device = NULL;
> @@ -9845,19 +9852,20 @@ qemuDomainSnapshotCreateSingleDiskActive(struct qemud_driver *driver,
>
> if (virAsprintf(&device, "drive-%s", disk->info.alias)< 0 ||
> !(source = strdup(snap->file)) ||
> - (STRNEQ_NULLABLE(disk->driverType, "qcow2")&&
> - !(driverType = strdup("qcow2"))) ||
> + (STRNEQ_NULLABLE(disk->driverType, snap->driverType)&&
> + !(driverType = strdup(snap->driverType))) ||
If the source and snapshod drivers are equal, nothing gets assigned to
driverType.
> (persistDisk&&
> (!(persistSource = strdup(source)) ||
> - (STRNEQ_NULLABLE(persistDisk->driverType, "qcow2")&&
> - !(persistDriverType = strdup("qcow2")))))) {
> + (STRNEQ_NULLABLE(persistDisk->driverType, snap->driverType)&&
> + !(persistDriverType = strdup(snap->driverType)))))) {
> virReportOOMError();
> goto cleanup;
> }
>
> /* create the stub file and set selinux labels; manipulate disk in
> * place, in a way that can be reverted on failure. */
> - fd = qemuOpenFile(driver, source, O_WRONLY | O_TRUNC | O_CREAT,
> + fd = qemuOpenFile(driver, source,
> + O_WRONLY | (reuse ? 0 : O_TRUNC | O_CREAT),
> &need_unlink, NULL);
> if (fd< 0)
> goto cleanup;
> @@ -9883,7 +9891,8 @@ qemuDomainSnapshotCreateSingleDiskActive(struct qemud_driver *driver,
> origdriver = NULL;
>
> /* create the actual snapshot */
> - ret = qemuMonitorDiskSnapshot(priv->mon, actions, device, source);
> + ret = qemuMonitorDiskSnapshot(priv->mon, actions, device, source,
> + driverType, reuse);
> virDomainAuditDisk(vm, disk->src, source, "snapshot", ret>= 0);
> if (ret< 0)
> goto cleanup;
> @@ -10004,6 +10013,7 @@ qemuDomainSnapshotCreateDiskActive(virConnectPtr conn,
> bool persist = false;
> int thaw = 0; /* 1 if freeze succeeded, -1 if freeze failed */
> bool atomic = (flags& VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC) != 0;
> + bool reuse = (flags& VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT) != 0;
>
> if (qemuDomainObjBeginJobWithDriver(driver, vm, QEMU_JOB_MODIFY)< 0)
> return -1;
> @@ -10079,7 +10089,8 @@ qemuDomainSnapshotCreateDiskActive(virConnectPtr conn,
> ret = qemuDomainSnapshotCreateSingleDiskActive(driver, vm,
> &snap->def->disks[i],
> vm->def->disks[i],
> - persistDisk, actions);
> + persistDisk, actions,
> + reuse);
> if (ret< 0)
> break;
> }
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index ba07e84..0e8bcce 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -3060,7 +3060,8 @@ cleanup:
>
> int
> qemuMonitorJSONDiskSnapshot(qemuMonitorPtr mon, virJSONValuePtr actions,
> - const char *device, const char *file)
> + const char *device, const char *file,
> + const char *format, bool reuse)
> {
> int ret;
> virJSONValuePtr cmd;
> @@ -3070,6 +3071,9 @@ qemuMonitorJSONDiskSnapshot(qemuMonitorPtr mon, virJSONValuePtr actions,
> "blockdev-snapshot-sync",
> "s:device", device,
> "s:snapshot-file", file,
> + "s:format", format,
and you pass NULL as format the json framework creates an invalid json
string like:
{"execute":"transaction","arguments":
{"actions": [
{"type":"blockdev-snapshot-sync","data":
{"device":"drive-ide0-0-0","snapshot-file":"/var/lib/libvirt/images/test.1332842808","format":null}
<< - here the "format":null
}
]}
,"id":"libvirt-6"}
which makes qemu sad :(
I'll post a patch soon.
Peter
More information about the libvir-list
mailing list