[libvirt] [PATCH 0/3] Managed save updates
A couple of updates to the managed save code in qemu. Details are in the individual patches.
The current version of the qemu managed save implementation is subject to a race where the domain shuts down between the time that we start the command and the time that we actually try to do the save. Close this race by making qemuDomainSaveFlags() expect both the driver and the passed-in vm object to be locked before executing. Signed-off-by: Chris Lalancette <clalance@redhat.com> --- src/qemu/qemu_driver.c | 79 ++++++++++++++++++----------------------------- 1 files changed, 30 insertions(+), 49 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 31cf1fe..b72aace 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5213,12 +5213,11 @@ endjob: return ret; } - -static int qemudDomainSaveFlag(virDomainPtr dom, const char *path, +/* this internal function expects the driver lock to already be held on entry */ +static int qemudDomainSaveFlag(struct qemud_driver *driver, virDomainPtr dom, + virDomainObjPtr vm, const char *path, int compressed) { - struct qemud_driver *driver = dom->conn->privateData; - virDomainObjPtr vm = NULL; char *xml = NULL; struct qemud_save_header header; struct fileOpHookData hdata; @@ -5236,19 +5235,8 @@ static int qemudDomainSaveFlag(virDomainPtr dom, const char *path, memcpy(header.magic, QEMUD_SAVE_MAGIC, sizeof(header.magic)); header.version = QEMUD_SAVE_VERSION; - qemuDriverLock(driver); - header.compressed = compressed; - vm = virDomainFindByUUID(&driver->domains, dom->uuid); - - if (!vm) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(dom->uuid, uuidstr); - qemuReportError(VIR_ERR_NO_DOMAIN, - _("no domain with matching uuid '%s'"), uuidstr); - goto cleanup; - } priv = vm->privateData; if (qemuDomainObjBeginJobWithDriver(driver, vm) < 0) @@ -5533,12 +5521,9 @@ cleanup: VIR_FREE(xml); if (ret != 0 && is_reg) unlink(path); - if (vm) - virDomainObjUnlock(vm); if (event) qemuDomainEventQueue(driver, event); virCgroupFree(&cgroup); - qemuDriverUnlock(driver); return ret; } @@ -5546,8 +5531,11 @@ static int qemudDomainSave(virDomainPtr dom, const char *path) { struct qemud_driver *driver = dom->conn->privateData; int compressed; + int ret = -1; + virDomainObjPtr vm = NULL; + + qemuDriverLock(driver); - /* Hm, is this safe against qemudReload? */ if (driver->saveImageFormat == NULL) compressed = QEMUD_SAVE_FORMAT_RAW; else { @@ -5560,7 +5548,23 @@ static int qemudDomainSave(virDomainPtr dom, const char *path) } } - return qemudDomainSaveFlag(dom, path, compressed); + vm = virDomainFindByUUID(&driver->domains, dom->uuid); + if (!vm) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(dom->uuid, uuidstr); + qemuReportError(VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s'"), uuidstr); + goto cleanup; + } + + ret = qemudDomainSaveFlag(driver, dom, vm, path, compressed); + +cleanup: + if (vm) + virDomainObjUnlock(vm); + qemuDriverUnlock(driver); + + return ret; } static char * @@ -5593,48 +5597,25 @@ qemuDomainManagedSave(virDomainPtr dom, unsigned int flags) virUUIDFormat(dom->uuid, uuidstr); qemuReportError(VIR_ERR_NO_DOMAIN, _("no domain with matching uuid '%s'"), uuidstr); - goto error; - } - - if (!virDomainObjIsActive(vm)) { - qemuReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("domain is not running")); - goto error; + goto cleanup; } name = qemuDomainManagedSavePath(driver, vm); if (name == NULL) - goto error; + goto cleanup; VIR_DEBUG("Saving state to %s", name); - /* FIXME: we should take the flags parameter, and use bits out - * of there to control whether we are compressing or not - */ compressed = QEMUD_SAVE_FORMAT_RAW; - - /* we have to drop our locks here because qemudDomainSaveFlag is - * going to pick them back up. Unfortunately it opens a race window - * between us dropping and qemudDomainSaveFlag picking it back up, but - * if we want to allow other operations to be able to happen while - * qemuDomainSaveFlag is running (possibly for a long time), I'm not - * sure if there is a better solution - */ - virDomainObjUnlock(vm); - qemuDriverUnlock(driver); - - ret = qemudDomainSaveFlag(dom, name, compressed); + ret = qemudDomainSaveFlag(driver, dom, vm, name, compressed); cleanup: - VIR_FREE(name); - - return ret; - -error: if (vm) virDomainObjUnlock(vm); qemuDriverUnlock(driver); - goto cleanup; + VIR_FREE(name); + + return ret; } static int -- 1.7.2.1
On 08/13/2010 08:53 AM, Chris Lalancette wrote:
The current version of the qemu managed save implementation is subject to a race where the domain shuts down between the time that we start the command and the time that we actually try to do the save. Close this race by making qemuDomainSaveFlags() expect both the driver and the passed-in vm object to be locked before executing.
Signed-off-by: Chris Lalancette <clalance@redhat.com> --- src/qemu/qemu_driver.c | 79 ++++++++++++++++++----------------------------- 1 files changed, 30 insertions(+), 49 deletions(-)
ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 08/16/10 - 10:17:09AM, Eric Blake wrote:
On 08/13/2010 08:53 AM, Chris Lalancette wrote:
The current version of the qemu managed save implementation is subject to a race where the domain shuts down between the time that we start the command and the time that we actually try to do the save. Close this race by making qemuDomainSaveFlags() expect both the driver and the passed-in vm object to be locked before executing.
Signed-off-by: Chris Lalancette <clalance@redhat.com> --- src/qemu/qemu_driver.c | 79 ++++++++++++++++++----------------------------- 1 files changed, 30 insertions(+), 49 deletions(-)
ACK.
Thanks, I've pushed this one. I'll wait on the other two until we finish disucssing. -- Chris Lalancette
Add in the ability to specify compression flags from the managed save API. We map these to the supported QEMU compression flags internally. Signed-off-by: Chris Lalancette <clalance@redhat.com> --- include/libvirt/libvirt.h.in | 7 +++++++ src/qemu/qemu_driver.c | 25 +++++++++++++++++++++++-- 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 2ff484d..e4e7c84 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -648,6 +648,13 @@ int virDomainRestore (virConnectPtr conn, /* * Managed domain save */ +typedef enum { + VIR_DOMAIN_SAVE_COMPRESS_RAW = 0, + VIR_DOMAIN_SAVE_COMPRESS_GZIP = 1 << 0, + VIR_DOMAIN_SAVE_COMPRESS_BZIP2 = 1 << 1, + VIR_DOMAIN_SAVE_COMPRESS_XZ = 1 << 2, + VIR_DOMAIN_SAVE_COMPRESS_LZOP = 1 << 3, +} virDomainSaveCompression; int virDomainManagedSave (virDomainPtr dom, unsigned int flags); int virDomainHasManagedSaveImage(virDomainPtr dom, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b72aace..637cf28 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -79,6 +79,7 @@ #include "domain_nwfilter.h" #include "hooks.h" #include "storage_file.h" +#include "count-one-bits.h" #define VIR_FROM_THIS VIR_FROM_QEMU @@ -5588,7 +5589,28 @@ qemuDomainManagedSave(virDomainPtr dom, unsigned int flags) int ret = -1; int compressed; - virCheckFlags(0, -1); + if (count_one_bits(flags) > 1) { + qemuReportError(VIR_ERR_OPERATION_INVALID, + _("Too many compression flags specified 0x%x"), flags); + return -1; + } + + /* this is in lieu of virCheckFlags */ + if (flags == 0) + compressed = QEMUD_SAVE_FORMAT_RAW; + else if (flags & VIR_DOMAIN_SAVE_COMPRESS_GZIP) + compressed = QEMUD_SAVE_FORMAT_GZIP; + else if (flags & VIR_DOMAIN_SAVE_COMPRESS_BZIP2) + compressed = QEMUD_SAVE_FORMAT_BZIP2; + else if (flags & VIR_DOMAIN_SAVE_COMPRESS_XZ) + compressed = QEMUD_SAVE_FORMAT_XZ; + else if (flags & VIR_DOMAIN_SAVE_COMPRESS_LZOP) + compressed = QEMUD_SAVE_FORMAT_LZOP; + else { + qemuReportError(VIR_ERR_OPERATION_INVALID, + _("Invalid compression flags 0x%x"), flags); + return -1; + } qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); @@ -5606,7 +5628,6 @@ qemuDomainManagedSave(virDomainPtr dom, unsigned int flags) VIR_DEBUG("Saving state to %s", name); - compressed = QEMUD_SAVE_FORMAT_RAW; ret = qemudDomainSaveFlag(driver, dom, vm, name, compressed); cleanup: -- 1.7.2.1
On Fri, Aug 13, 2010 at 10:53:48AM -0400, Chris Lalancette wrote:
Add in the ability to specify compression flags from the managed save API. We map these to the supported QEMU compression flags internally.
I'm not really convinced we need todo this. I think the host level setting in /etc/libvirt/qemu.conf is sufficient already. Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On 08/13/2010 09:06 AM, Daniel P. Berrange wrote:
On Fri, Aug 13, 2010 at 10:53:48AM -0400, Chris Lalancette wrote:
Add in the ability to specify compression flags from the managed save API. We map these to the supported QEMU compression flags internally.
I'm not really convinced we need todo this. I think the host level setting in /etc/libvirt/qemu.conf is sufficient already.
But that's a global setting for all qemu domains. I can see where it might make sense to compression for some domains but not others, at which point exposing the ability to suggest a non-default compression makes sense. On the other hand, if we agree that a qemu.conf setting is adequate, then this patch still needs a respin, in order to honor the conf-file default rather than hard-coding it to raw. So either way, we need more discussion and probably a respin. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Mon, Aug 16, 2010 at 10:21:31AM -0600, Eric Blake wrote:
On 08/13/2010 09:06 AM, Daniel P. Berrange wrote:
On Fri, Aug 13, 2010 at 10:53:48AM -0400, Chris Lalancette wrote:
Add in the ability to specify compression flags from the managed save API. We map these to the supported QEMU compression flags internally.
I'm not really convinced we need todo this. I think the host level setting in /etc/libvirt/qemu.conf is sufficient already.
But that's a global setting for all qemu domains. I can see where it might make sense to compression for some domains but not others, at which point exposing the ability to suggest a non-default compression makes sense.
I know that's a global setting. IMHO setting compression per VM is a feature in search of a problem.
On the other hand, if we agree that a qemu.conf setting is adequate, then this patch still needs a respin, in order to honor the conf-file default rather than hard-coding it to raw.
Yes, that's a serious bug in the current code. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On 08/13/10 - 04:06:24PM, Daniel P. Berrange wrote:
On Fri, Aug 13, 2010 at 10:53:48AM -0400, Chris Lalancette wrote:
Add in the ability to specify compression flags from the managed save API. We map these to the supported QEMU compression flags internally.
I'm not really convinced we need todo this. I think the host level setting in /etc/libvirt/qemu.conf is sufficient already.
Don't get me wrong, I don't think this is a killer feature. But when I initially implemented compression, I definitely would have done it through the API if we had had a "flags" parameter available for virDomainSave(). Now that we have a ManagedSave that does have a flags parameter, I figured we do it the right way. I think it's a little cleaner, and more intuitive, to do it through the API, and it makes the feature available to hypervisors other than qemu. -- Chris Lalancette
On 08/17/2010 01:58 PM, Chris Lalancette wrote:
On 08/13/10 - 04:06:24PM, Daniel P. Berrange wrote:
On Fri, Aug 13, 2010 at 10:53:48AM -0400, Chris Lalancette wrote:
Add in the ability to specify compression flags from the managed save API. We map these to the supported QEMU compression flags internally.
I'm not really convinced we need todo this. I think the host level setting in /etc/libvirt/qemu.conf is sufficient already.
Don't get me wrong, I don't think this is a killer feature. But when I initially implemented compression, I definitely would have done it through the API if we had had a "flags" parameter available for virDomainSave(). Now that we have a ManagedSave that does have a flags parameter, I figured we do it the right way. I think it's a little cleaner, and more intuitive, to do it through the API, and it makes the feature available to hypervisors other than qemu.
Are there any other hypervisors that (currently or plan to) support compression? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Tue, Aug 17, 2010 at 03:58:22PM -0400, Chris Lalancette wrote:
On 08/13/10 - 04:06:24PM, Daniel P. Berrange wrote:
On Fri, Aug 13, 2010 at 10:53:48AM -0400, Chris Lalancette wrote:
Add in the ability to specify compression flags from the managed save API. We map these to the supported QEMU compression flags internally.
I'm not really convinced we need todo this. I think the host level setting in /etc/libvirt/qemu.conf is sufficient already.
Don't get me wrong, I don't think this is a killer feature. But when I initially implemented compression, I definitely would have done it through the API if we had had a "flags" parameter available for virDomainSave(). Now that we have a ManagedSave that does have a flags parameter, I figured we do it the right way. I think it's a little cleaner, and more intuitive, to do it through the API, and it makes the feature available to hypervisors other than qemu.
I still can't imagine anyone needing the ability to specify a different compression method per-guest VM. Perhaps a VIR_SAVE_COMPRESS to toggle compression on / off, with actual type of compression determined in the host config, but anything more seems rather overkill. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On 08/18/2010 02:32 AM, Daniel P. Berrange wrote:
Don't get me wrong, I don't think this is a killer feature. But when I initially implemented compression, I definitely would have done it through the API if we had had a "flags" parameter available for virDomainSave(). Now that we have a ManagedSave that does have a flags parameter, I figured we do it the right way. I think it's a little cleaner, and more intuitive, to do it through the API, and it makes the feature available to hypervisors other than qemu.
I still can't imagine anyone needing the ability to specify a different compression method per-guest VM. Perhaps a VIR_SAVE_COMPRESS to toggle compression on / off, with actual type of compression determined in the host config, but anything more seems rather overkill.
But where is the host config for hypervisors other than qemu? Having only a compress/no-compress toggle makes sense as a reasonable compromise to me, if we know how to let the user specify the preferred compression type for all hypervisors. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Signed-off-by: Chris Lalancette <clalance@redhat.com> --- tools/virsh.c | 22 +++++++++++++++++++++- 1 files changed, 21 insertions(+), 1 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 352d44a..5e97293 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -1404,6 +1404,7 @@ static const vshCmdInfo info_managedsave[] = { }; static const vshCmdOptDef opts_managedsave[] = { + {"compression", VSH_OT_STRING, 0, N_("compress save image")}, {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, {NULL, 0, 0, NULL} }; @@ -1414,14 +1415,33 @@ cmdManagedSave(vshControl *ctl, const vshCmd *cmd) virDomainPtr dom; char *name; int ret = TRUE; + char *compress = NULL; + unsigned int flags = 0; if (!vshConnectionUsability(ctl, ctl->conn)) return FALSE; + compress = vshCommandOptString(cmd, "compression", NULL); + if (compress) { + if (STREQ(compress, "gzip")) + flags |= VIR_DOMAIN_SAVE_COMPRESS_GZIP; + else if (STREQ(compress, "bzip2")) + flags |= VIR_DOMAIN_SAVE_COMPRESS_BZIP2; + else if (STREQ(compress, "xz")) + flags |= VIR_DOMAIN_SAVE_COMPRESS_XZ; + else if (STREQ(compress, "lzop")) + flags |= VIR_DOMAIN_SAVE_COMPRESS_LZOP; + else { + vshError(ctl, "%s", + _("Invalid compression type; it must be one of 'gzip', 'bzip2', 'xz', or 'lzop'")); + return FALSE; + } + } + if (!(dom = vshCommandOptDomain(ctl, cmd, &name))) return FALSE; - if (virDomainManagedSave(dom, 0) == 0) { + if (virDomainManagedSave(dom, flags) == 0) { vshPrint(ctl, _("Domain %s state saved by libvirt\n"), name); } else { vshError(ctl, _("Failed to save domain %s state"), name); -- 1.7.2.1
On 08/13/2010 08:53 AM, Chris Lalancette wrote:
Signed-off-by: Chris Lalancette <clalance@redhat.com> --- tools/virsh.c | 22 +++++++++++++++++++++- 1 files changed, 21 insertions(+), 1 deletions(-)
Depending on how the discussion on 2/3 goes:
+ compress = vshCommandOptString(cmd, "compression", NULL); + if (compress) { + if (STREQ(compress, "gzip")) + flags |= VIR_DOMAIN_SAVE_COMPRESS_GZIP; + else if (STREQ(compress, "bzip2")) + flags |= VIR_DOMAIN_SAVE_COMPRESS_BZIP2; + else if (STREQ(compress, "xz")) + flags |= VIR_DOMAIN_SAVE_COMPRESS_XZ; + else if (STREQ(compress, "lzop")) + flags |= VIR_DOMAIN_SAVE_COMPRESS_LZOP; + else { + vshError(ctl, "%s", + _("Invalid compression type; it must be one of 'gzip', 'bzip2', 'xz', or 'lzop'")); + return FALSE;
If we add a qemu.conf default compression setting, then virsh also needs to support a '--compress none' to override the default. Hmm, that also means that VIR_DOMAIN_SAVE_COMPRESS_RAW needs to be distinct from 0 (that is, passing 0 means that you are requesting the default, while requesting raw means that you are explicitly requesting no compression even if the default would use compression). -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (3)
-
Chris Lalancette -
Daniel P. Berrange -
Eric Blake