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

Re: [libvirt] [PATCH v4 5/5] virsh: add compression options for migration



On Fri, Mar 04, 2016 at 14:20:58 +0300, Nikolay Shirokovskiy wrote:
> From: ShaoHe Feng <shaohe feng intel com>
> 
> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy virtuozzo com>
> ---
>  tools/virsh-domain.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  tools/virsh.pod      | 25 ++++++++++++----
>  2 files changed, 103 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index 979f115..6dd8eca 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -9666,6 +9666,31 @@ static const vshCmdOptDef opts_migrate[] = {
>       .type = VSH_OT_STRING,
>       .help = N_("comma separated list of disks to be migrated")
>      },
> +    {.name = "compression-method",

Looks a bit long to me, personally I'd prefer "comp-" prefix, feel free
to ignore my preference though :-)

Anyway, this is a rare exception when it makes sense not to put the new
stuff at the end of the list. All the new options should go just after
--compressed options so that they are all together.

> +     .type = VSH_OT_STRING,
> +     .help = N_("comma separated list of compression method to be used")

s/method/methods/

> +    },
> +    {.name = "compression-mt-level",

I think the options should contain the method name rather than some
abbreviated form of it.

> +     .type = VSH_OT_INT,
> +     .help = N_("compress level for multithread compression. "

The above description is IMHO enough, the rest can go into virsh man
page.

> +                "Values are in range 0-9, 9 means maximum compression. "
> +                "'multithread' compression method must be selected with this option.")
> +    },
> +    {.name = "compression-mt-threads",
> +     .type = VSH_OT_INT,
> +     .help = N_("number of compession threads from multithread compression. "

Same here.

> +                "'multithread' compression method must be selected with this option.")
> +    },
> +    {.name = "compression-mt-dthreads",
> +     .type = VSH_OT_INT,
> +     .help = N_("number of decompession threads from multithread compression. "

Here.

> +                "'multithread' compression method must be selected with this option.")
> +    },
> +    {.name = "compression-xbzrle-cache",
> +     .type = VSH_OT_INT,
> +     .help = N_("page cache size for xbzrle compression. "

And here.

> +                "'xbzrle' compression method must be selected with this option.")
> +    },
>      {.name = NULL}
>  };
>  
> @@ -9684,6 +9709,9 @@ doMigrate(void *opaque)
>      virTypedParameterPtr params = NULL;
>      int nparams = 0;
>      int maxparams = 0;
> +    int intOpt = 0;
> +    unsigned long long ullOpt = 0;
> +    int rv;
>      virConnectPtr dconn = data->dconn;
>  
>      sigemptyset(&sigmask);
> @@ -9744,6 +9772,61 @@ doMigrate(void *opaque)
>          VIR_FREE(val);
>      }
>  
> +    if (vshCommandOptStringReq(ctl, cmd, "compression-method", &opt) < 0)
> +        goto out;
> +    if (opt) {
> +        char **val = NULL;
> +
> +        val = virStringSplit(opt, ",", 0);

It would be more compact and still readable and clear as

           char **val = virStringSplit(opt, ",", 0);

> +
> +        if (virTypedParamsAddStringList(&params,
> +                                        &nparams,
> +                                        &maxparams,
> +                                        VIR_MIGRATE_PARAM_COMPRESSION,
> +                                        (const char **)val) < 0) {
> +            VIR_FREE(val);
> +            goto save_error;
> +        }
> +
> +        VIR_FREE(val);
> +    }
> +
> +    if ((rv = vshCommandOptInt(ctl, cmd, "compression-mt-level", &intOpt)) < 0) {
> +        goto save_error;

goto out;

> +    } else if (rv > 0) {
> +        if (virTypedParamsAddInt(&params, &nparams, &maxparams,
> +                                 VIR_MIGRATE_PARAM_COMPRESSION_MT_LEVEL,
> +                                 intOpt) < 0)
> +            goto save_error;
> +    }
> +
> +    if ((rv = vshCommandOptInt(ctl, cmd, "compression-mt-threads", &intOpt)) < 0) {
> +        goto save_error;

goto out;

> +    } else if (rv > 0) {
> +        if (virTypedParamsAddInt(&params, &nparams, &maxparams,
> +                                 VIR_MIGRATE_PARAM_COMPRESSION_MT_THREADS,
> +                                 intOpt) < 0)
> +            goto save_error;
> +    }
> +
> +    if ((rv = vshCommandOptInt(ctl, cmd, "compression-mt-dthreads", &intOpt)) < 0) {
> +        goto save_error;

goto out;

> +    } else if (rv > 0) {
> +        if (virTypedParamsAddInt(&params, &nparams, &maxparams,
> +                                 VIR_MIGRATE_PARAM_COMPRESSION_MT_DTHREADS,
> +                                 intOpt) < 0)
> +            goto save_error;
> +    }
> +
> +    if ((rv = vshCommandOptULongLong(ctl, cmd, "compression-xbzrle-cache", &ullOpt)) < 0) {
> +        goto save_error;

goto out;

> +    } else if (rv > 0) {
> +        if (virTypedParamsAddULLong(&params, &nparams, &maxparams,
> +                                    VIR_MIGRATE_PARAM_COMPRESSION_XBZRLE_CACHE,
> +                                    ullOpt) < 0)
> +            goto save_error;
> +    }
> +
>      if (vshCommandOptStringReq(ctl, cmd, "xml", &opt) < 0)
>          goto out;
>      if (opt) {
> diff --git a/tools/virsh.pod b/tools/virsh.pod
> index 4662658..854c7ae 100644
> --- a/tools/virsh.pod
> +++ b/tools/virsh.pod
> @@ -1532,7 +1532,9 @@ to the I<uri> namespace is displayed instead of being modified.
>  [I<--compressed>] [I<--abort-on-error>] [I<--auto-converge>]
>  I<domain> I<desturi> [I<migrateuri>] [I<graphicsuri>] [I<listen-address>]
>  [I<dname>] [I<--timeout> B<seconds>] [I<--xml> B<file>]
> -[I<--migrate-disks> B<disk-list>]
> +[I<--migrate-disks> B<disk-list>] [I<--compression-method> B<method-list>]
> +[I<--compression-mt-level>] [I<--compression-mt-threads>] [I<--compression-mt-dthreads>]
> +[I<--compression-xbzrle-cache>]
>  
>  Migrate domain to another host.  Add I<--live> for live migration; <--p2p>
>  for peer-2-peer migration; I<--direct> for direct migration; or I<--tunnelled>
> @@ -1555,10 +1557,14 @@ enforces that no incompatible configuration changes will be made to the domain
>  while the migration is underway; this flag is implicitly enabled when supported
>  by the hypervisor, but can be explicitly used to reject the migration if the
>  hypervisor lacks change protection support.  I<--verbose> displays the progress
> -of migration.  I<--compressed> activates compression of memory pages that have
> -to be transferred repeatedly during live migration. I<--abort-on-error> cancels
> -the migration if a soft error (for example I/O error) happens during the
> -migration. I<--auto-converge> forces convergence during live migration.
> +of migration.  I<--compressed> activates default compression method
> +during migration.

I'd say something like: --compressed activates compression, the
compression method is chosen with --compression-method. When no method
is specified, a hypervisor default method will be used.

> I<--abort-on-error> cancels the migration if a soft error
> +(for example I/O error) happens during the migration. I<--auto-converge>
> +forces convergence during live migration. I<--compression-methods>
> +configures compression methods to use during migration in the comma separated
> +B<method-list> argument. Compression methods are driver specific.
> +Compression methods can be tuned further using various [--<compression-*>]
> +options.

--compressed and --compression-* options should be documented next to
each other and ideally in a separate paragraph.

>  
>  B<Note>: Individual hypervisors usually do not support all possible types of
>  migration. For example, QEMU does not support direct migration.
> @@ -1594,6 +1600,15 @@ managed migration.
>  B<Note>: The I<desturi> parameter for normal migration and peer2peer migration
>  has different semantics:
>  
> +QEMU supports "multithread" and "xbzrle" methods which can be used in any
> +combination. I<--compression-mt-level> sets compression level for multithread
> +method. Values are in range from 0 to 9, where 1 is maximum speed and 9 is maximum
> +compression. I<--compression-mt-threads> and I<--compression-mt-dthreads> set
> +the number of compress threads on source and the number of decompress threads
> +on target respectively. I<--compression-xbzrle-cache> sets size of page cache in
> +bytes for xbzrle method. I<--compressed> turns on "xbzrle" compression method with
> +current compression parameters.

This should go into the same place where the options are documented.

Jirka


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