[libvirt] [PATCH RFC 0/2] support compression with dumping guest memory

[Martin Kletzander]

On Thu, Feb 13, 2014 at 11:08:50AM +0800, Qiao Nuohan wrote:
> On 01/29/2014 10:04 AM, qiaonuohan wrote:
> > --memory-only option is introduced without compression supported. Therefore,
> > this is a freature regression of virsh dump. This patchset is used to add
> > compression support in libvirt side and please refer the following address to
> > see the qemu side, the lastest version of qemu side v7(ready for comment now).
> >
> > http://lists.gnu.org/archive/html/qemu-devel/2014-01/msg03669.html
> >
> > qiaonuohan (2):
> >    support compression when --memory-only option is specified
> >    support configuring the format of dumping memory in qemu.conf
> >
> >   include/libvirt/libvirt.h.in       | 18 +++++++++----
> >   src/libvirt.c                      | 15 +++++++++++
> >   src/qemu/libvirtd_qemu.aug         |  1 +
> >   src/qemu/qemu.conf                 |  6 +++++
> >   src/qemu/qemu_conf.c               |  2 ++
> >   src/qemu/qemu_conf.h               |  1 +
> >   src/qemu/qemu_driver.c             | 52 +++++++++++++++++++++++++++++++++++---
> >   src/qemu/qemu_monitor.c            |  6 ++---
> >   src/qemu/qemu_monitor.h            |  3 ++-
> >   src/qemu/qemu_monitor_json.c       |  4 ++-
> >   src/qemu/qemu_monitor_json.h       |  3 ++-
> >   src/qemu/test_libvirtd_qemu.aug.in |  1 +
> >   tests/qemumonitorjsontest.c        |  2 +-
> >   tools/virsh-domain.c               | 29 +++++++++++++++++++++
> >   14 files changed, 127 insertions(+), 16 deletions(-)
> >
>
> Hello
>
> Do you have some comments on my patches?
>

Hi.

Having an option to set the compression type is good to have, but I'd
go with a different approach.  I don't know if by the "regression" in
1/2 you mean that if someone specifies a compression it is not used
with '--memory-only' then yes, but that's the only one I see.  And you
are mixing two different compressions together, one what qemu will use
and one that libvirt uses.

Can we first address just the fact that we ignore the dump image
compression (which I believe should be used for memory-only dumps too)
and make it consistent and then (possibly) deal with selecting the
compression and the fact who will do the compression?

You are adding too many flags which seem to overlap or intersect with
each other.  How about adding a flag "nocompress" that would disable
any compression set in qemu.conf in current API as a second patch and
then work your way to the result?

One question is whether specifying the format or compression is useful
on every call.  Also, are these "qemu-compressions" available only on
the dump-guest-memory call?  I guess yes as it can't be very well used
for migrations, there's only xbzrle IIRC.  If we just add an option to
select the default format qemu should use for memory dumps it should
be enough.

It's probably not expressed, so let me re-capitulate what I'd suggest:

 1) Do a patch that properly propagates the settings from qemu.conf
    into the dump call even when memory-only is selected.  This makes
    it possible to compress memory-only dumps even with old qemu.

 2) (optional) Add a NOCOMPRESS flag that disables any compression set
    in qemu.conf for current API call that works for both normal and
    memory-only dump.

 3) Add a configuration option (basically what you did) to select the
    default format qemu will use.

Have a nice day,
Martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140213/3dea677d/attachment-0001.sig>