[libvirt] [PATCH v2] qemu: enable user-defined cache bypassing while invoking
John Ferlan
jferlan at redhat.com
Thu Sep 22 17:45:26 UTC 2016
On 08/26/2016 02:56 AM, fuweiwei wrote:
> in the previous version, I mentioned the scenario of long-term pause while
> writing external memory checkpoints:
>
> v1: https://www.redhat.com/archives/libvir-list/2016-August/msg01194.html
It would seem the above link has a better explanation as to what's being
fixed. If this current patch were pushed, it would require the reader
to follow the link in order to get the explanation. Not only that but
the subsequent information would also be kept in the log.
So the "norm" has been when sending patches where there is no cover
letter, one adds "explanation" below the triple hashes (below [1])
When you create your v3, let's try to restore the v1 explanation and
anything else you add here that's pertinent...
>
> Daniel suggested not to hardcode the flag, but wire this upto the API. When
> the user invokes the snapshot they can request the
> VIR_DOMAIN_SAVE_BYPASS_CACHE flag explicitly. So in this version I
> introduce the --bypass-cache option in libvirt snapshot API. When invoking
> an external VM snapshots, we may use the command like this:
>
> virsh snapshot-create-as VM snap --memspec /path/to/memsnap --live
> --bypass-cache
>
> The VM snapshot can be done with inapparent VM suspend now. Without
inapparent???
> "--bypass-cache" flag, one may experience long-term VM suspend (so that the
> implication of "--live" option is not significant), provided that the VM
> has a large amount of dirty pages to save.
>
> Signed-off-by: fuweiwei <fuweiwei2 at huawei.com>
We really need a better or full name in order to push this patch.
There's plenty of examples on how to do that.
> ---
[1] here
When you use git send-email you can be dumped into an editor which will
allow you to add your text here. Anything added here won't be pushed
with the patch, but is meant to help the reviewers understand the
history. If however you create multiple patches and have a cover
letter, then than can be used. Hopefully this is clear.
> include/libvirt/libvirt-domain-snapshot.h | 3 +++
> src/qemu/qemu_driver.c | 20 ++++++++++++++++++--
> tools/virsh-snapshot.c | 12 ++++++++++++
> 3 files changed, 33 insertions(+), 2 deletions(-)
>
Caveat emptor - the snapshot code is not "in my wheelhouse"...
You need to modify tools/virsh.pod in order to describe the flag/option
along with when/how it should be used (for both snapshot-create and
create-as). See the existing --bypass-cache flags for an example.
You need to modify the API description in src/libvirt-domain-snapshot.c
to describe this new flag just like other @flags are described and of
course similar to virsh.pod's explanation.
FWIW: I'll make use of gitk a lot to find who made a previous/similar
change and see what was changed in that commit to make sure I don't miss
anything (I still miss stuff, but it lessens the chances). In particular
the previous flag was added in commit '5f75bd4bb'. Prior to that commit
id '922d498e1' added atomic. You'll also note that those commits
"broke" up the patches into "virsh/libvirt" (API) and "qemu" changes.
> diff --git a/include/libvirt/libvirt-domain-snapshot.h b/include/libvirt/libvirt-domain-snapshot.h
> index 0f73f24..aeff665 100644
> --- a/include/libvirt/libvirt-domain-snapshot.h
> +++ b/include/libvirt/libvirt-domain-snapshot.h
> @@ -70,6 +70,9 @@ typedef enum {
> VIR_DOMAIN_SNAPSHOT_CREATE_LIVE = (1 << 8), /* create the snapshot
> while the guest is
> running */
> + VIR_DOMAIN_SNAPSHOT_CREATE_BYPASS_CACHE = (1 << 9), i/* Bypass cache
You already know about the 'i'...
> + while writing external
> + checkpoint files. */
> } virDomainSnapshotCreateFlags;
>
> /* Take a snapshot of the current VM state */
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 2089359..d5f441f 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -14036,6 +14036,7 @@ qemuDomainSnapshotCreateActiveExternal(virConnectPtr conn,
> bool pmsuspended = false;
> virQEMUDriverConfigPtr cfg = NULL;
> int compressed = QEMU_SAVE_FORMAT_RAW;
> + unsigned int save_memory_flag = 0;
>
> /* If quiesce was requested, then issue a freeze command, and a
> * counterpart thaw command when it is actually sent to agent.
> @@ -14116,8 +14117,12 @@ qemuDomainSnapshotCreateActiveExternal(virConnectPtr conn,
> if (!(xml = qemuDomainDefFormatLive(driver, vm->def, true, true)))
> goto cleanup;
>
> + if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_BYPASS_CACHE)
> + save_memory_flag |= VIR_DOMAIN_SAVE_BYPASS_CACHE;
> +
> if ((ret = qemuDomainSaveMemory(driver, vm, snap->def->file,
> - xml, compressed, resume, 0,
> + xml, compressed, resume,
> + save_memory_flag,
> QEMU_ASYNC_JOB_SNAPSHOT)) < 0)
> goto cleanup;
>
> @@ -14224,7 +14229,8 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
> VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT |
> VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE |
> VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC |
> - VIR_DOMAIN_SNAPSHOT_CREATE_LIVE, NULL);
> + VIR_DOMAIN_SNAPSHOT_CREATE_LIVE |
> + VIR_DOMAIN_SNAPSHOT_CREATE_BYPASS_CACHE, NULL);
>
> VIR_REQUIRE_FLAG_RET(VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE,
> VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY,
> @@ -14297,6 +14303,16 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
> goto cleanup;
> }
>
> + /* the option of bypass cache is only supported for external checkpoints */
> + if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_BYPASS_CACHE &&
> + (def->memory != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL ||
> + redefine)) {
> + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> + _("only external memory snapshots support "
> + "cache bypass."));
> + goto cleanup;
> + }
> +
> /* allow snapshots only in certain states */
> switch ((virDomainState) vm->state.state) {
> /* valid states */
> diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c
> index f879e7a..0627baf 100644
> --- a/tools/virsh-snapshot.c
> +++ b/tools/virsh-snapshot.c
> @@ -160,6 +160,10 @@ static const vshCmdOptDef opts_snapshot_create[] = {
> .type = VSH_OT_BOOL,
> .help = N_("require atomic operation")
> },
> + {.name = "bypass-cache",
> + .type = VSH_OT_BOOL,
> + .help = N_("bypass system cache while writing external checkpoints")
> + },
> VIRSH_COMMON_OPT_LIVE(N_("take a live snapshot")),
> {.name = NULL}
> };
> @@ -191,6 +195,8 @@ cmdSnapshotCreate(vshControl *ctl, const vshCmd *cmd)
> flags |= VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC;
> if (vshCommandOptBool(cmd, "live"))
> flags |= VIR_DOMAIN_SNAPSHOT_CREATE_LIVE;
> + if (vshCommandOptBool(cmd, "bypass-cache"))
> + flags |= VIR_DOMAIN_SNAPSHOT_CREATE_BYPASS_CACHE;
>
> if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
> goto cleanup;
> @@ -358,6 +364,10 @@ static const vshCmdOptDef opts_snapshot_create_as[] = {
> .type = VSH_OT_BOOL,
> .help = N_("require atomic operation")
> },
> + {.name = "bypass-cache",
> + .type = VSH_OT_BOOL,
> + .help = N_("bypass system cache while writing external checkpoints")
> + },
> VIRSH_COMMON_OPT_LIVE(N_("take a live snapshot")),
> {.name = "memspec",
> .type = VSH_OT_STRING,
> @@ -404,6 +414,8 @@ cmdSnapshotCreateAs(vshControl *ctl, const vshCmd *cmd)
> flags |= VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC;
> if (vshCommandOptBool(cmd, "live"))
> flags |= VIR_DOMAIN_SNAPSHOT_CREATE_LIVE;
> + if (vshCommandOptBool(cmd, "bypass-cache"))
> + flags |= VIR_DOMAIN_SNAPSHOT_CREATE_BYPASS_CACHE;
>From your "help" explanation and the qemu check this can only be used
with external snapshots, right? Are there any of the other switches that
if used would preclude an external snapshot? Looks like "redefine"
might be one based on the qemu check above.
Check vsh.h for the VSH_EXCLUSIVE* or VSH_REQUIRE* macros and then of
course see how existing virsh*.c code uses them.
John
>
> if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
> return false;
>
More information about the libvir-list
mailing list