6:53 p.m.
A couple of updates to the managed save code in qemu. Details
are in the individual patches.
6:53 p.m.
New subject: [libvirt] [PATCH 1/3] Fix up qemu domain save/managed save locking.
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(a)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
8:17 p.m.
New subject: [libvirt] [PATCH 1/3] Fix up qemu domain save/managed save locking.
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(a)redhat.com>
---
src/qemu/qemu_driver.c | 79 ++++++++++++++++++-----------------------------
1 files changed, 30 insertions(+), 49 deletions(-)
ACK.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
12:25 a.m.
New subject: [libvirt] [PATCH 1/3] Fix up qemu domain save/managed save locking.
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(a)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
6:53 p.m.
New subject: [libvirt] [PATCH 2/3] Managed save compression flags.
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(a)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
7:06 p.m.
New subject: [libvirt] [PATCH 2/3] Managed save compression flags.
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 :|
8:21 p.m.
New subject: [libvirt] [PATCH 2/3] Managed save compression flags.
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
8:39 p.m.
New subject: [libvirt] [PATCH 2/3] Managed save compression flags.
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 :|
11:58 p.m.
New subject: [libvirt] [PATCH 2/3] Managed save compression flags.
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
12:32 a.m.
New subject: [libvirt] [PATCH 2/3] Managed save compression flags.
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
12:32 p.m.
New subject: [libvirt] [PATCH 2/3] Managed save compression flags.
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 :|
5:28 p.m.
New subject: [libvirt] [PATCH 2/3] Managed save compression flags.
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
6:53 p.m.
New subject: [libvirt] [PATCH 3/3] Plumb managedsave compression through to virsh.
Signed-off-by: Chris Lalancette <clalance(a)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
8:24 p.m.
New subject: [libvirt] [PATCH 3/3] Plumb managedsave compression through to virsh.
On 08/13/2010 08:53 AM, Chris Lalancette wrote:
Signed-off-by: Chris Lalancette <clalance(a)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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
5361
days inactive
5366
days old
13 comments
3 participants
participants (3)
-
Chris Lalancette -
Daniel P. Berrange -
Eric Blake