[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] support compressed crashdump of guests



On Thu, 21 Oct 2010 10:35:10 +0200
Daniel Veillard <veillard redhat com> wrote:

> On Thu, Oct 21, 2010 at 12:02:11PM +0900, KAMEZAWA Hiroyuki wrote:
> > 
> > Now, virsh dump doesn't support compresses dump.
> > This patch adds GZIP and LZOP option to virsh dump and support
> > it at qemu coredump. (AFAIK, LZOP is available on RHEL6.)
> > 
> > When I did 4G guest dump,
> > (Raw)	3844669750 
> > (Gzip)	1029846577
> > (LZOP)	1416263880 (faster than gzip in general)
> > 
> > This will be a help for a host where crash-dump is used
> > and several guests works on it.
> > 
> > help message is modified as this.
> >   NAME
> >     dump - dump the core of a domain to a file for analysis
> > 
> >   SYNOPSIS
> >     dump [--live] [--crash] [--gzip] [--lzop] <domain> <file>
> > 
> >   DESCRIPTION
> >     Core dump a domain.
> > 
> >   OPTIONS
> >     --live           perform a live core dump if supported
> >     --crash          crash the domain after core dump
> >     --gzip           gzip dump(only one compression allowed
> >     --lzop           lzop dump(only one compression allowed
> >     [--domain] <string>  domain name, id or uuid
> >     [--file] <string>  where to dump the core
> > 
> > Tested on Fedora-13+x86-64.
> > 
> > Note: for better compression, we may have to skip pages filled by
> > zero or freed pages. But it seems it's qemu's works.
> 

Thank you for review.


>   The patch looks relatively simple and clean, I still have one comemnt
> below though. It also seems to me that any use of the dump need a follow
> up decompression before it can be fed to debug tools (gdb ...) as I
> don't think they handle compressed dumps natively.
> 
Yes. But at support-job, coredump must be transfered to support-team enviroment
in many case. In that case, dump image tend to be compressed.

>   The other comment on principle about this patch is that for saving
> compression we use a qemu.conf option, maybe we should instead use
> a new option there for core compression, or simply reuse the same
> option for both (core being just a special kind of save). You will
> also note that save_image_format takes more options than lzop or gzip.
> If doing so one doesn't need to change virsh too.
> 
Ah, okay, this one ?
http://www.mail-archive.com/libvir-list redhat com/msg15564.html

Doesn't this affect behaviors other than dump ?

>   It's a trade off. If using the configuration option you need to plan
> in advance the fact you will be dumping compressed core, if you expose
> it at the API level itself, you can activate it anytime, and that may be
> more useful for example when interacting with a customer facing an
> issue needing debug. So both ways are acceptable to me.
> 
Sure.
 

> > Signed-off-by: KAMEZAWA Hiroyuki <kamezawa hiroyu jp fujitsu com>
> > ---
> >  include/libvirt/libvirt.h.in |    2 ++
> >  src/qemu/qemu_driver.c       |   23 +++++++++++++++++++----
> >  tools/virsh.c                |   10 +++++++++-
> >  3 files changed, 30 insertions(+), 5 deletions(-)
> > 
> > Index: libvirt-0.8.4/src/qemu/qemu_driver.c
> > ===================================================================
> > --- libvirt-0.8.4.orig/src/qemu/qemu_driver.c
> > +++ libvirt-0.8.4/src/qemu/qemu_driver.c
> > @@ -5710,7 +5710,7 @@ cleanup:
> >  
> >  static int qemudDomainCoreDump(virDomainPtr dom,
> >                                 const char *path,
> > -                               int flags ATTRIBUTE_UNUSED) {
> > +                               int flags) {
> >      struct qemud_driver *driver = dom->conn->privateData;
> >      virDomainObjPtr vm;
> >      int resume = 0, paused = 0;
> > @@ -5720,6 +5720,14 @@ static int qemudDomainCoreDump(virDomain
> >          "cat",
> >          NULL,
> >      };
> > +    const char *zargs[] = {
> > +	"gzip",
> > +	NULL,
> > +    };
> > +    const char *lzargs[] = {
> > +	"lzop",
> > +	NULL,
> > +    };
> >      qemuDomainObjPrivatePtr priv;
> >  
> >      qemuDriverLock(driver);
> > @@ -5787,9 +5795,16 @@ static int qemudDomainCoreDump(virDomain
> >      }
> >  
> >      qemuDomainObjEnterMonitorWithDriver(driver, vm);
> > -    ret = qemuMonitorMigrateToFile(priv->mon,
> > -                                   QEMU_MONITOR_MIGRATE_BACKGROUND,
> > -                                   args, path, 0);
> > +    if (flags & VIR_DUMP_GZIP)
> > +	ret = qemuMonitorMigrateToFile(priv->mon,
> > +		QEMU_MONITOR_MIGRATE_BACKGROUND, zargs, path, 0);
> > +    else if (flags & VIR_DUMP_LZOP)
> > +	ret = qemuMonitorMigrateToFile(priv->mon,
> > +		QEMU_MONITOR_MIGRATE_BACKGROUND, lzargs, path, 0);
> > +    else
> > +	ret = qemuMonitorMigrateToFile(priv->mon,
> > +		QEMU_MONITOR_MIGRATE_BACKGROUND, args, path, 0);
> > +
> 
>   I'm wondering what happens if the compression command is not present,
> if the current error message we get back from qemu is clear enough then
> fine, but otherwise we may have to check for the presence of the
> compressor program.
> 

Ok, I will add check code. But hmm, calling system() for checking command
exists is ok ? Or some good function do we have ?


> >      qemuDomainObjExitMonitorWithDriver(driver, vm);
> >      if (ret < 0)
> >          goto endjob;
> > Index: libvirt-0.8.4/tools/virsh.c
> > ===================================================================
> > --- libvirt-0.8.4.orig/tools/virsh.c
> > +++ libvirt-0.8.4/tools/virsh.c
> > @@ -1751,6 +1751,8 @@ static const vshCmdInfo info_dump[] = {
> >  static const vshCmdOptDef opts_dump[] = {
> >      {"live", VSH_OT_BOOL, 0, N_("perform a live core dump if supported")},
> >      {"crash", VSH_OT_BOOL, 0, N_("crash the domain after core dump")},
> > +    {"gzip", VSH_OT_BOOL, 0, N_("gzip dump(only one compression allowed")},
> > +    {"lzop", VSH_OT_BOOL, 0, N_("lzop dump(only one compression allowed")},
> 
>   what about bzip2 and xz, the first one is probably way too slow, but
>   we accept it for saves.
> 
will add bzip2 and xz. (but maybe very slow.)


> >      {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")},
> >      {"file", VSH_OT_DATA, VSH_OFLAG_REQ, N_("where to dump the core")},
> >      {NULL, 0, 0, NULL}
> > @@ -1778,7 +1780,13 @@ cmdDump(vshControl *ctl, const vshCmd *c
> >          flags |= VIR_DUMP_LIVE;
> >      if (vshCommandOptBool (cmd, "crash"))
> >          flags |= VIR_DUMP_CRASH;
> > -
> > +    if (vshCommandOptBool (cmd, "gzip"))
> > +	flags |= VIR_DUMP_GZIP;
> > +    if (vshCommandOptBool (cmd, "lzop"))
> > +        flags |= VIR_DUMP_LZOP;
> > +    if ((flags & (VIR_DUMP_GZIP | VIR_DUMP_LZOP))
> > +	 == (VIR_DUMP_GZIP | VIR_DUMP_LZOP))
> > +	return FALSE;
> 
>   I think an error message need to be provided stating that both options
> are mutually exclusive.
> 
I'll add one.

Thank you.
-Kame


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]