[libvirt] [PATCH v3 0/5] Active commit
I still don't have qemu capability detection working reliably, but want to post this series right now so that it can be built into a scratch build containing Peter's and my changes. (Or put another way, I was testing what conflict resolutions are required - patch 2/5 (virsh) and 5/5 (qemu_driver) has some conflicts with Peter's addition of relative backing name; and I think the resolutions were fairly simple). These patches are tested on top of: https://www.redhat.com/archives/libvir-list/2014-June/msg00492.html I may still end up posting a v4 and/or pushing my series before Peter's, once I get capability detection working the way I want. Eric Blake (5): virsh: improve blockcopy UI virsh: expose new active commit controls blockcommit: update error messages related to block jobs blockcommit: track job type in xml blockcommit: turn on active commit docs/formatdomain.html.in | 20 +++--- docs/schemas/domaincommon.rng | 6 ++ src/conf/domain_conf.c | 26 ++++++- src/conf/domain_conf.h | 1 + src/qemu/qemu_driver.c | 56 ++++++++++++--- src/qemu/qemu_hotplug.c | 2 +- src/qemu/qemu_process.c | 18 +++-- .../qemuxml2argv-disk-active-commit.xml | 37 ++++++++++ .../qemuxml2argvdata/qemuxml2argv-disk-mirror.xml | 4 +- .../qemuxml2xmlout-disk-mirror-old.xml | 4 +- tests/qemuxml2xmltest.c | 1 + tools/virsh-domain.c | 83 +++++++++++++++++----- tools/virsh.pod | 53 +++++++++----- 13 files changed, 242 insertions(+), 69 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-active-commit.xml -- 1.9.3
Peter's review of my addition of active block commit pointed out some issues that I had copied from block copy. It makes sense to allow the shorter command-line of 'blockcopy $dom $disk --pivot' without having to explicitly specify --wait. And my use of embedded ?: ternaries bordered on unreadable. * tools/virsh-domain.c (cmdBlockCopy): Make --pivot and --finish imply --wait. Drop excess ?: operators. * tools/virsh.pod (blockcopy): Update documentation. Signed-off-by: Eric Blake <eblake@redhat.com> --- tools/virsh-domain.c | 25 ++++++++++++++----------- tools/virsh.pod | 17 +++++++++-------- 2 files changed, 23 insertions(+), 19 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index e9162db..294b594 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -1786,15 +1786,15 @@ static const vshCmdOptDef opts_block_copy[] = { }, {.name = "timeout", .type = VSH_OT_INT, - .help = N_("with --wait, abort if copy exceeds timeout (in seconds)") + .help = N_("implies --wait, abort if copy exceeds timeout (in seconds)") }, {.name = "pivot", .type = VSH_OT_BOOL, - .help = N_("with --wait, pivot when mirroring starts") + .help = N_("implies --wait, pivot when mirroring starts") }, {.name = "finish", .type = VSH_OT_BOOL, - .help = N_("with --wait, quit when mirroring starts") + .help = N_("implies --wait, quit when mirroring starts") }, {.name = "async", .type = VSH_OT_BOOL, @@ -1808,10 +1808,10 @@ cmdBlockCopy(vshControl *ctl, const vshCmd *cmd) { virDomainPtr dom = NULL; bool ret = false; - bool blocking = vshCommandOptBool(cmd, "wait"); bool verbose = vshCommandOptBool(cmd, "verbose"); bool pivot = vshCommandOptBool(cmd, "pivot"); bool finish = vshCommandOptBool(cmd, "finish"); + bool blocking = vshCommandOptBool(cmd, "wait"); int timeout = 0; struct sigaction sig_action; struct sigaction old_sig_action; @@ -1822,6 +1822,7 @@ cmdBlockCopy(vshControl *ctl, const vshCmd *cmd) bool quit = false; int abort_flags = 0; + blocking |= vshCommandOptBool(cmd, "timeout") || pivot || finish; if (blocking) { if (pivot && finish) { vshError(ctl, "%s", _("cannot mix --pivot and --finish")); @@ -1844,8 +1845,7 @@ cmdBlockCopy(vshControl *ctl, const vshCmd *cmd) sigaction(SIGINT, &sig_action, &old_sig_action); GETTIMEOFDAY(&start); - } else if (verbose || vshCommandOptBool(cmd, "timeout") || - vshCommandOptBool(cmd, "async") || pivot || finish) { + } else if (verbose || vshCommandOptBool(cmd, "async")) { vshError(ctl, "%s", _("missing --wait option")); return false; } @@ -1911,11 +1911,14 @@ cmdBlockCopy(vshControl *ctl, const vshCmd *cmd) vshError(ctl, _("failed to finish job for disk %s"), path); goto cleanup; } - vshPrint(ctl, "\n%s", - quit ? _("Copy aborted") : - pivot ? _("Successfully pivoted") : - finish ? _("Successfully copied") : - _("Now in mirroring phase")); + if (quit) + vshPrint(ctl, "\n%s", _("Copy aborted")); + else if (pivot) + vshPrint(ctl, "\n%s", _("Successfully pivoted")); + else if (finish) + vshPrint(ctl, "\n%s", _("Successfully copied")); + else + vshPrint(ctl, "\n%s", _("Now in mirroring phase")); ret = true; cleanup: diff --git a/tools/virsh.pod b/tools/virsh.pod index 84a60a7..77013f5 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -804,8 +804,8 @@ I<bandwidth> specifies copying bandwidth limit in MiB/s, although for qemu, it may be non-zero only for an online domain. =item B<blockcopy> I<domain> I<path> I<dest> [I<bandwidth>] [I<--shallow>] -[I<--reuse-external>] [I<--raw>] [I<--wait> [I<--verbose>] -[{I<--pivot> | I<--finish>}] [I<--timeout> B<seconds>] [I<--async>]] +[I<--reuse-external>] [I<--raw>] [I<--wait> [I<--async>] [I<--verbose>]] +[{I<--pivot> | I<--finish>}] [I<--timeout> B<seconds>] Copy a disk backing image chain to I<dest>. By default, this command flattens the entire chain; but if I<--shallow> is specified, the copy @@ -834,12 +834,13 @@ faithful copy of that point in time. However, if I<--wait> is specified, then this command will block until the mirroring phase begins, or cancel the operation if the optional I<timeout> in seconds elapses or SIGINT is sent (usually with C<Ctrl-C>). Using I<--verbose> along with I<--wait> -will produce periodic status updates. Using I<--pivot> or I<--finish> -along with I<--wait> will additionally end the job cleanly rather than -leaving things in the mirroring phase. If job cancellation is triggered, -I<--async> will return control to the user as fast as possible, otherwise -the command may continue to block a little while longer until the job -is done cleaning up. +will produce periodic status updates. Using I<--pivot> (similar to +B<blockjob> I<--pivot>) or I<--finish> (similar to B<blockjob> I<--abort>) +implies I<--wait>, and will additionally end the job cleanly rather than +leaving things in the mirroring phase. If job cancellation is triggered +by timeout or by I<--finish>, I<--async> will return control to the user +as fast as possible, otherwise the command may continue to block a little +while longer until the job has actually cancelled. I<path> specifies fully-qualified path of the disk. I<bandwidth> specifies copying bandwidth limit in MiB/s. -- 1.9.3
On 06/11/14 18:27, Eric Blake wrote:
Peter's review of my addition of active block commit pointed out some issues that I had copied from block copy. It makes sense to allow the shorter command-line of 'blockcopy $dom $disk --pivot' without having to explicitly specify --wait. And my use of embedded ?: ternaries bordered on unreadable.
* tools/virsh-domain.c (cmdBlockCopy): Make --pivot and --finish imply --wait. Drop excess ?: operators. * tools/virsh.pod (blockcopy): Update documentation.
Signed-off-by: Eric Blake <eblake@redhat.com> --- tools/virsh-domain.c | 25 ++++++++++++++----------- tools/virsh.pod | 17 +++++++++-------- 2 files changed, 23 insertions(+), 19 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index e9162db..294b594 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c
@@ -1808,10 +1808,10 @@ cmdBlockCopy(vshControl *ctl, const vshCmd *cmd) { virDomainPtr dom = NULL; bool ret = false; - bool blocking = vshCommandOptBool(cmd, "wait"); bool verbose = vshCommandOptBool(cmd, "verbose"); bool pivot = vshCommandOptBool(cmd, "pivot"); bool finish = vshCommandOptBool(cmd, "finish"); + bool blocking = vshCommandOptBool(cmd, "wait");
Spurious line move?
int timeout = 0; struct sigaction sig_action; struct sigaction old_sig_action;
ACK, Peter
On 06/12/2014 02:45 AM, Peter Krempa wrote:
On 06/11/14 18:27, Eric Blake wrote:
Peter's review of my addition of active block commit pointed out some issues that I had copied from block copy. It makes sense to allow the shorter command-line of 'blockcopy $dom $disk --pivot' without having to explicitly specify --wait. And my use of embedded ?: ternaries bordered on unreadable.
@@ -1808,10 +1808,10 @@ cmdBlockCopy(vshControl *ctl, const vshCmd *cmd) { virDomainPtr dom = NULL; bool ret = false; - bool blocking = vshCommandOptBool(cmd, "wait"); bool verbose = vshCommandOptBool(cmd, "verbose"); bool pivot = vshCommandOptBool(cmd, "pivot"); bool finish = vshCommandOptBool(cmd, "finish"); + bool blocking = vshCommandOptBool(cmd, "wait");
Spurious line move?
Yeah, fallout from an earlier attempt to do: bool blocking = vshCommandOptBool(...) || pivot || finish; before deciding to add timeout to the list of options that auto-implies blocking. I'll drop this hunk.
int timeout = 0; struct sigaction sig_action; struct sigaction old_sig_action;
ACK,
Pushing shortly, as fixed. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Add knobs to virsh to manage a 2-phase active commit of the top layer, similar to knobs already present on blockcopy. While this code will fail until later patches actually implement the new knobs in the qemu driver, doing it now proves that the API is usable and also makes it easier for testing the qemu changes as they are made. * tools/virsh-domain.c (cmdBlockCommit): Add --active, --pivot, and --finish options, modeled after blockcopy. (blockJobImpl): Support --active flag. * tools/virsh.pod (blockcommit): Document new flags. (blockjob): Mention 2-phase commit interaction. Signed-off-by: Eric Blake <eblake@redhat.com> --- tools/virsh-domain.c | 58 +++++++++++++++++++++++++++++++++++++++++++++------- tools/virsh.pod | 36 ++++++++++++++++++++++---------- 2 files changed, 76 insertions(+), 18 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 294b594..3db0bae 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -1498,6 +1498,10 @@ blockJobImpl(vshControl *ctl, const vshCmd *cmd, flags |= VIR_DOMAIN_BLOCK_COMMIT_DELETE; if (vshCommandOptBool(cmd, "keep-relative")) flags |= VIR_DOMAIN_BLOCK_COMMIT_RELATIVE; + if (vshCommandOptBool(cmd, "active") || + vshCommandOptBool(cmd, "pivot") || + vshCommandOptBool(cmd, "keep-overlay")) + flags |= VIR_DOMAIN_BLOCK_COMMIT_ACTIVE; ret = virDomainBlockCommit(dom, path, base, top, bandwidth, flags); break; case VSH_CMD_BLOCK_JOB_COPY: @@ -1598,13 +1602,18 @@ static const vshCmdOptDef opts_block_commit[] = { .type = VSH_OT_DATA, .help = N_("path of top file to commit from (default top of chain)") }, + {.name = "active", + .type = VSH_OT_BOOL, + .help = N_("trigger two-stage active commit of top file") + }, {.name = "delete", .type = VSH_OT_BOOL, .help = N_("delete files that were successfully committed") }, {.name = "wait", .type = VSH_OT_BOOL, - .help = N_("wait for job to complete") + .help = N_("wait for job to complete " + "(with --active, wait for job to sync)") }, {.name = "verbose", .type = VSH_OT_BOOL, @@ -1612,7 +1621,15 @@ static const vshCmdOptDef opts_block_commit[] = { }, {.name = "timeout", .type = VSH_OT_INT, - .help = N_("with --wait, abort if copy exceeds timeout (in seconds)") + .help = N_("implies --wait, abort if copy exceeds timeout (in seconds)") + }, + {.name = "pivot", + .type = VSH_OT_BOOL, + .help = N_("implies --active --wait, pivot when commit is synced") + }, + {.name = "keep-overlay", + .type = VSH_OT_BOOL, + .help = N_("implies --active --wait, quit when commit is synced") }, {.name = "async", .type = VSH_OT_BOOL, @@ -1621,7 +1638,7 @@ static const vshCmdOptDef opts_block_commit[] = { {.name = "keep-relative", .type = VSH_OT_BOOL, .help = N_("keep the backing chain relative if it was relatively " - "referenced if it was before") + "referenced before") }, {.name = NULL} }; @@ -1631,8 +1648,11 @@ cmdBlockCommit(vshControl *ctl, const vshCmd *cmd) { virDomainPtr dom = NULL; bool ret = false; - bool blocking = vshCommandOptBool(cmd, "wait"); bool verbose = vshCommandOptBool(cmd, "verbose"); + bool pivot = vshCommandOptBool(cmd, "pivot"); + bool finish = vshCommandOptBool(cmd, "keep-overlay"); + bool active = vshCommandOptBool(cmd, "active") || pivot || finish; + bool blocking = vshCommandOptBool(cmd, "wait"); int timeout = 0; struct sigaction sig_action; struct sigaction old_sig_action; @@ -1643,7 +1663,12 @@ cmdBlockCommit(vshControl *ctl, const vshCmd *cmd) bool quit = false; int abort_flags = 0; + blocking |= vshCommandOptBool(cmd, "timeout") || pivot || finish; if (blocking) { + if (pivot && finish) { + vshError(ctl, "%s", _("cannot mix --pivot and --keep-overlay")); + return false; + } if (vshCommandOptTimeoutToMs(ctl, cmd, &timeout) < 0) return false; if (vshCommandOptStringReq(ctl, cmd, "path", &path) < 0) @@ -1661,8 +1686,7 @@ cmdBlockCommit(vshControl *ctl, const vshCmd *cmd) sigaction(SIGINT, &sig_action, &old_sig_action); GETTIMEOFDAY(&start); - } else if (verbose || vshCommandOptBool(cmd, "timeout") || - vshCommandOptBool(cmd, "async")) { + } else if (verbose || vshCommandOptBool(cmd, "async")) { vshError(ctl, "%s", _("missing --wait option")); return false; } @@ -1694,6 +1718,8 @@ cmdBlockCommit(vshControl *ctl, const vshCmd *cmd) if (verbose) vshPrintJobProgress(_("Block Commit"), info.end - info.cur, info.end); + if (active && info.cur == info.end) + break; GETTIMEOFDAY(&curr); if (intCaught || (timeout && @@ -1720,7 +1746,25 @@ cmdBlockCommit(vshControl *ctl, const vshCmd *cmd) /* printf [100 %] */ vshPrintJobProgress(_("Block Commit"), 0, 1); } - vshPrint(ctl, "\n%s", quit ? _("Commit aborted") : _("Commit complete")); + if (!quit && pivot) { + abort_flags |= VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT; + if (virDomainBlockJobAbort(dom, path, abort_flags) < 0) { + vshError(ctl, _("failed to pivot job for disk %s"), path); + goto cleanup; + } + } else if (finish && !quit && + virDomainBlockJobAbort(dom, path, abort_flags) < 0) { + vshError(ctl, _("failed to finish job for disk %s"), path); + goto cleanup; + } + if (quit) + vshPrint(ctl, "\n%s", _("Commit aborted")); + else if (pivot) + vshPrint(ctl, "\n%s", _("Successfully pivoted")); + else if (!finish) + vshPrint(ctl, "\n%s", _("Now in synchronized phase")); + else + vshPrint(ctl, "\n%s", _("Commit complete")); ret = true; cleanup: diff --git a/tools/virsh.pod b/tools/virsh.pod index 77013f5..21c29ec 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -769,8 +769,9 @@ address of virtual interface (such as I<detach-interface> or I<domif-setlink>) will accept the MAC address printed by this command. =item B<blockcommit> I<domain> I<path> [I<bandwidth>] -{[I<base>] | [I<--shallow>]} [I<top>] [I<--delete>] [I<--keep-relative>] -[I<--wait> [I<--verbose>] [I<--timeout> B<seconds>] [I<--async>]] +[I<base>] [I<--shallow>] [I<top>] [I<--delete>] [I<--keep-relative>] +[I<--wait> [I<--async>] [I<--verbose>]] [I<--timeout> B<seconds>] +[I<--active>] [{I<--pivot> | I<--keep-overlay>}] Reduce the length of a backing image chain, by committing changes at the top of the chain (snapshot or delta files) into backing images. By @@ -780,21 +781,34 @@ operation is constrained to committing just that portion of the chain; I<--shallow> can be used instead of I<base> to specify the immediate backing file of the resulting top image to be committed. The files being committed are rendered invalid, possibly as soon as the operation -starts; using the I<--delete> flag will remove these files at the successful -completion of the commit operation. Using the I<--keep-relative> flag -will try to keep the backing chain names relative (if they were -relative before). +starts; using the I<--delete> flag will attempt to remove these invalidated +files at the successful completion of the commit operation. Using the +I<--keep-relative> flag will try to keep the backing chain names relative +(if they were relative before). + +When I<top> is omitted or specified as the active image, it is also +possible to specify I<--active> to trigger a two-phase active commit. In +the first phase, I<top> is copied into I<base> and the job can only be +canceled, with top still containing data not yet in base. In the second +phase, I<top> and I<base> remain identical until a call to B<blockjob> +with the I<--abort> flag (keeping top as the active image that tracks +changes from that point in time) or the I<--pivot> flag (making base +the new active image and invalidating top). By default, this command returns as soon as possible, and data for the entire disk is committed in the background; the progress of the operation can be checked with B<blockjob>. However, if I<--wait> is -specified, then this command will block until the operation completes, -or cancel the operation if the optional I<timeout> in seconds elapses +specified, then this command will block until the operation completes +(or for I<--active>, enters the second phase), or until the operation +is canceled because the optional I<timeout> in seconds elapses or SIGINT is sent (usually with C<Ctrl-C>). Using I<--verbose> along with I<--wait> will produce periodic status updates. If job cancellation is triggered, I<--async> will return control to the user as fast as possible, otherwise the command may continue to block a little while -longer until the job is done cleaning up. +longer until the job is done cleaning up. Using I<--pivot> is shorthand +for combining I<--active> I<--wait> with an automatic B<blockjob> +I<--pivot>; and using I<--keep-overlay> is shorthand for combining +I<--active> I<--wait> with an automatic B<blockjob> I<--abort>. I<path> specifies fully-qualified path of the disk; it corresponds to a unique target name (<target dev='name'/>) or source file (<source @@ -925,8 +939,8 @@ also B<domblklist> for listing these names). If I<--abort> is specified, the active job on the specified disk will be aborted. If I<--async> is also specified, this command will return immediately, rather than waiting for the cancellation to complete. If -I<--pivot> is specified, this requests that an active copy job -be pivoted over to the new copy. +I<--pivot> is specified, this requests that an active copy or active +commit job be pivoted over to the new image. If I<--info> is specified, the active job information on the specified disk will be printed. I<bandwidth> can be used to set bandwidth limit for the active job. -- 1.9.3
On 06/11/14 18:27, Eric Blake wrote:
Add knobs to virsh to manage a 2-phase active commit of the top layer, similar to knobs already present on blockcopy. While this code will fail until later patches actually implement the new knobs in the qemu driver, doing it now proves that the API is usable and also makes it easier for testing the qemu changes as they are made.
* tools/virsh-domain.c (cmdBlockCommit): Add --active, --pivot, and --finish options, modeled after blockcopy.
s/--finish/--keep-overlay/
(blockJobImpl): Support --active flag. * tools/virsh.pod (blockcommit): Document new flags. (blockjob): Mention 2-phase commit interaction.
Signed-off-by: Eric Blake <eblake@redhat.com> --- tools/virsh-domain.c | 58 +++++++++++++++++++++++++++++++++++++++++++++------- tools/virsh.pod | 36 ++++++++++++++++++++++---------- 2 files changed, 76 insertions(+), 18 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 294b594..3db0bae 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c
@@ -1621,7 +1638,7 @@ static const vshCmdOptDef opts_block_commit[] = { {.name = "keep-relative", .type = VSH_OT_BOOL, .help = N_("keep the backing chain relative if it was relatively " - "referenced if it was before") + "referenced before") },
This should go into my patch. As we discussed, this stuff has better chances going in earlier as the relative naming patches as it already is supported by qemu, so you should rebase this to master instead to my series.
{.name = NULL} }; @@ -1631,8 +1648,11 @@ cmdBlockCommit(vshControl *ctl, const vshCmd *cmd) { virDomainPtr dom = NULL; bool ret = false; - bool blocking = vshCommandOptBool(cmd, "wait");
Spurious move again?
bool verbose = vshCommandOptBool(cmd, "verbose"); + bool pivot = vshCommandOptBool(cmd, "pivot"); + bool finish = vshCommandOptBool(cmd, "keep-overlay"); + bool active = vshCommandOptBool(cmd, "active") || pivot || finish; + bool blocking = vshCommandOptBool(cmd, "wait"); int timeout = 0; struct sigaction sig_action; struct sigaction old_sig_action; @@ -1643,7 +1663,12 @@ cmdBlockCommit(vshControl *ctl, const vshCmd *cmd) bool quit = false; int abort_flags = 0;
+ blocking |= vshCommandOptBool(cmd, "timeout") || pivot || finish; if (blocking) { + if (pivot && finish) { + vshError(ctl, "%s", _("cannot mix --pivot and --keep-overlay")); + return false; + }
You can use VSH_EXCLUSIVE_OPTIONS("pivot", "keep-overlay") here. Also you can bove it out of the if (blocking) block as they can't be ever used together. (Yes it will add a function call to get the value, but on the other hand it will unify this piece with a majority of others).
if (vshCommandOptTimeoutToMs(ctl, cmd, &timeout) < 0) return false; if (vshCommandOptStringReq(ctl, cmd, "path", &path) < 0)
@@ -1694,6 +1718,8 @@ cmdBlockCommit(vshControl *ctl, const vshCmd *cmd) if (verbose) vshPrintJobProgress(_("Block Commit"), info.end - info.cur, info.end);
Empty line here will separate those two independent blocks visually.
+ if (active && info.cur == info.end) + break;
GETTIMEOFDAY(&curr); if (intCaught || (timeout && @@ -1720,7 +1746,25 @@ cmdBlockCommit(vshControl *ctl, const vshCmd *cmd) /* printf [100 %] */ vshPrintJobProgress(_("Block Commit"), 0, 1); }
In this hunk too. Please separate the blocks.
- vshPrint(ctl, "\n%s", quit ? _("Commit aborted") : _("Commit complete")); + if (!quit && pivot) { + abort_flags |= VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT; + if (virDomainBlockJobAbort(dom, path, abort_flags) < 0) { + vshError(ctl, _("failed to pivot job for disk %s"), path); + goto cleanup; + } + } else if (finish && !quit && + virDomainBlockJobAbort(dom, path, abort_flags) < 0) { + vshError(ctl, _("failed to finish job for disk %s"), path); + goto cleanup; + } + if (quit) + vshPrint(ctl, "\n%s", _("Commit aborted")); + else if (pivot) + vshPrint(ctl, "\n%s", _("Successfully pivoted")); + else if (!finish) + vshPrint(ctl, "\n%s", _("Now in synchronized phase")); + else + vshPrint(ctl, "\n%s", _("Commit complete"));
ret = true; cleanup: diff --git a/tools/virsh.pod b/tools/virsh.pod index 77013f5..21c29ec 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -769,8 +769,9 @@ address of virtual interface (such as I<detach-interface> or I<domif-setlink>) will accept the MAC address printed by this command.
=item B<blockcommit> I<domain> I<path> [I<bandwidth>] -{[I<base>] | [I<--shallow>]} [I<top>] [I<--delete>] [I<--keep-relative>] -[I<--wait> [I<--verbose>] [I<--timeout> B<seconds>] [I<--async>]] +[I<base>] [I<--shallow>] [I<top>] [I<--delete>] [I<--keep-relative>] +[I<--wait> [I<--async>] [I<--verbose>]] [I<--timeout> B<seconds>] +[I<--active>] [{I<--pivot> | I<--keep-overlay>}]
Again, rebase this to master please if you want to go first.
Reduce the length of a backing image chain, by committing changes at the top of the chain (snapshot or delta files) into backing images. By
Otherwise looks good. If you want to get this into upstream prior to my series then please rebase and repost this patch. If you are about to wait until the relative-backing stuff goes in, then ACK with the nits fixed. Peter
A future patch will add two-phase block commit jobs; as the mechanism for managing them is similar to managing a block copy job, existing errors should be made generic enough to occur for either job type. * src/conf/domain_conf.c (virDomainHasDiskMirror): Update comment. * src/qemu/qemu_driver.c (qemuDomainDefineXML) (qemuDomainSnapshotCreateXML, qemuDomainRevertToSnapshot) (qemuDomainBlockJobImpl, qemuDomainBlockCopy): Update error message. * src/qemu/qemu_hotplug.c (qemuDomainDetachDiskDevice): Likewise. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/conf/domain_conf.c | 2 +- src/qemu/qemu_driver.c | 10 +++++----- src/qemu/qemu_hotplug.c | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ff2d447..be81dbe 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -10379,7 +10379,7 @@ virDomainDiskRemoveByName(virDomainDefPtr def, const char *name) } /* Return true if VM has at least one disk involved in a current block - * copy job (that is, with a <mirror> element in the disk xml). */ + * copy/commit job (that is, with a <mirror> element in the disk xml). */ bool virDomainHasDiskMirror(virDomainObjPtr vm) { diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3f42af9..3399b2a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6260,7 +6260,7 @@ static virDomainPtr qemuDomainDefineXML(virConnectPtr conn, const char *xml) def = NULL; if (virDomainHasDiskMirror(vm)) { virReportError(VIR_ERR_BLOCK_COPY_ACTIVE, "%s", - _("domain has active block copy job")); + _("domain has active block job")); virDomainObjAssignDef(vm, NULL, false, NULL); goto cleanup; } @@ -13464,7 +13464,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, } if (virDomainHasDiskMirror(vm)) { virReportError(VIR_ERR_BLOCK_COPY_ACTIVE, "%s", - _("domain has active block copy job")); + _("domain has active block job")); goto cleanup; } @@ -14074,7 +14074,7 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, if (virDomainHasDiskMirror(vm)) { virReportError(VIR_ERR_BLOCK_COPY_ACTIVE, "%s", - _("domain has active block copy job")); + _("domain has active block job")); goto cleanup; } @@ -15083,7 +15083,7 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, if (mode == BLOCK_JOB_PULL && disk->mirror) { virReportError(VIR_ERR_BLOCK_COPY_ACTIVE, - _("disk '%s' already in active block copy job"), + _("disk '%s' already in active block job"), disk->dst); goto endjob; } @@ -15318,7 +15318,7 @@ qemuDomainBlockCopy(virDomainObjPtr vm, disk = vm->def->disks[idx]; if (disk->mirror) { virReportError(VIR_ERR_BLOCK_COPY_ACTIVE, - _("disk '%s' already in active block copy job"), + _("disk '%s' already in active block job"), disk->dst); goto endjob; } diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index cd43dd5..7289055 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2986,7 +2986,7 @@ qemuDomainDetachDiskDevice(virQEMUDriverPtr driver, if (detach->mirror) { virReportError(VIR_ERR_BLOCK_COPY_ACTIVE, - _("disk '%s' is in an active block copy job"), + _("disk '%s' is in an active block job"), detach->dst); goto cleanup; } -- 1.9.3
On 06/11/14 18:27, Eric Blake wrote:
A future patch will add two-phase block commit jobs; as the mechanism for managing them is similar to managing a block copy job, existing errors should be made generic enough to occur for either job type.
* src/conf/domain_conf.c (virDomainHasDiskMirror): Update comment. * src/qemu/qemu_driver.c (qemuDomainDefineXML) (qemuDomainSnapshotCreateXML, qemuDomainRevertToSnapshot) (qemuDomainBlockJobImpl, qemuDomainBlockCopy): Update error message. * src/qemu/qemu_hotplug.c (qemuDomainDetachDiskDevice): Likewise.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/conf/domain_conf.c | 2 +- src/qemu/qemu_driver.c | 10 +++++----- src/qemu/qemu_hotplug.c | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-)
ACK still stands. Peter
On 06/12/2014 03:21 AM, Peter Krempa wrote:
On 06/11/14 18:27, Eric Blake wrote:
A future patch will add two-phase block commit jobs; as the mechanism for managing them is similar to managing a block copy job, existing errors should be made generic enough to occur for either job type.
* src/conf/domain_conf.c (virDomainHasDiskMirror): Update comment. * src/qemu/qemu_driver.c (qemuDomainDefineXML) (qemuDomainSnapshotCreateXML, qemuDomainRevertToSnapshot) (qemuDomainBlockJobImpl, qemuDomainBlockCopy): Update error message. * src/qemu/qemu_hotplug.c (qemuDomainDetachDiskDevice): Likewise.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/conf/domain_conf.c | 2 +- src/qemu/qemu_driver.c | 10 +++++----- src/qemu/qemu_hotplug.c | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-)
ACK still stands.
I pushed this one. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
A future patch is going to wire up qemu active block commit jobs; but as they have similar events and are canceled/pivoted in the same way as block copy jobs, it is easiest to track all bookkeeping for the commit job by reusing the <mirror> element. This patch adds domain XML to track which job was responsible for creating a mirroring situation, and adds a job='copy' attribute to all existing uses of <mirror>. Along the way, it also massages the qemu monitor backend to read the new field in order to generate the correct type of libvirt job (even though it requires a future patch to actually cause a qemu event that can be reported as an active commit). * docs/schemas/domaincommon.rng: Enhance schema. * docs/formatdomain.html.in: Document it. * src/conf/domain_conf.h (_virDomainDiskDef): Add a field. * src/conf/domain_conf.c (virDomainBlockJobType): String conversion. (virDomainDiskDefParseXML): Parse job type. (virDomainDiskDefFormat): Output job type. * src/qemu/qemu_process.c (qemuProcessHandleBlockJob): Distinguish active from regular commit. * src/qemu/qemu_driver.c (qemuDomainBlockCopy): Set job type. (qemuDomainBlockPivot, qemuDomainBlockJobImpl): Clean up job type on completion. * tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-mirror-old.xml: Update tests. * tests/qemuxml2argvdata/qemuxml2argv-disk-mirror.xml: Likewise. * tests/qemuxml2argvdata/qemuxml2argv-disk-active-commit.xml: New file. * tests/qemuxml2xmltest.c (mymain): Drive new test. Signed-off-by: Eric Blake <eblake@redhat.com> --- docs/formatdomain.html.in | 20 ++++++------ docs/schemas/domaincommon.rng | 6 ++++ src/conf/domain_conf.c | 24 ++++++++++++++ src/conf/domain_conf.h | 1 + src/qemu/qemu_driver.c | 3 ++ src/qemu/qemu_process.c | 18 +++++++---- .../qemuxml2argv-disk-active-commit.xml | 37 ++++++++++++++++++++++ .../qemuxml2argvdata/qemuxml2argv-disk-mirror.xml | 4 +-- .../qemuxml2xmlout-disk-mirror-old.xml | 4 +-- tests/qemuxml2xmltest.c | 1 + 10 files changed, 98 insertions(+), 20 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-active-commit.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 1b6ced8..512132d 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1878,16 +1878,18 @@ </dd> <dt><code>mirror</code></dt> <dd> - This element is present if the hypervisor has started a block - copy operation (via the <code>virDomainBlockRebase</code> - API), where the mirror location in the <code>source</code> - sub-element will eventually have the same contents as the - source, and with the file format in the + This element is present if the hypervisor has started a + long-running block job operation, where the mirror location in + the <code>source</code> sub-element will eventually have the + same contents as the source, and with the file format in the sub-element <code>format</code> (which might differ from the - format of the source). The details of the <code>source</code> - sub-element are determined by the <code>type</code> attribute - of the mirror, similar to what is done for the - overall <code>disk</code> device element. If + format of the source). The <code>job</code> attribute + mentions which API started the operation ("copy" for + the <code>virDomainBlockRebase</code> API, or "commit" for + the <code>virDomainBlockCommit</code> API). The details of + the <code>source</code> sub-element are determined by + the <code>type</code> attribute of the mirror, similar to what + is done for the overall <code>disk</code> device element. If attribute <code>ready</code> is present, then it is known the disk is ready to pivot; otherwise, the disk is probably still copying. For now, this element only valid in output; it is diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 6cc922c..3badb6d 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -4187,6 +4187,12 @@ </optional> </group> <group> + <attribute name='job'> + <choice> + <value>copy</value> + <value>active-commit</value> + </choice> + </attribute> <interleave> <ref name="diskSource"/> <optional> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index be81dbe..fbe9fa4 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -760,6 +760,12 @@ VIR_ENUM_IMPL(virDomainDiskDiscard, VIR_DOMAIN_DISK_DISCARD_LAST, "unmap", "ignore") +/* Internal mapping: subset of block job types that can be present in + * <mirror> XML (remaining types are not two-phase). */ +VIR_ENUM_DECL(virDomainBlockJob) +VIR_ENUM_IMPL(virDomainBlockJob, VIR_DOMAIN_BLOCK_JOB_TYPE_LAST, + "", "", "copy", "", "active-commit") + #define VIR_DOMAIN_XML_WRITE_FLAGS VIR_DOMAIN_XML_SECURE #define VIR_DOMAIN_XML_READ_FLAGS VIR_DOMAIN_XML_INACTIVE @@ -5377,10 +5383,26 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, xmlStrEqual(cur->name, BAD_CAST "mirror") && !(flags & VIR_DOMAIN_XML_INACTIVE)) { char *ready; + char *blockJob; if (VIR_ALLOC(def->mirror) < 0) goto error; + blockJob = virXMLPropString(cur, "job"); + if (blockJob) { + def->mirrorJob = virDomainBlockJobTypeFromString(blockJob); + if (def->mirrorJob <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown mirror job type '%s'"), + blockJob); + VIR_FREE(blockJob); + goto error; + } + VIR_FREE(blockJob); + } else { + def->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_COPY; + } + mirrorType = virXMLPropString(cur, "type"); if (mirrorType) { def->mirror->type = virStorageTypeFromString(mirrorType); @@ -15193,6 +15215,8 @@ virDomainDiskDefFormat(virBufferPtr buf, if (def->mirror && !(flags & VIR_DOMAIN_XML_INACTIVE)) { virBufferAsprintf(buf, "<mirror type='%s'", virStorageTypeToString(def->mirror->type)); + virBufferEscapeString(buf, " job='%s'", + virDomainBlockJobTypeToString(def->mirrorJob)); if (def->mirroring) virBufferAddLit(buf, " ready='yes'"); virBufferAddLit(buf, ">\n"); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index a6ac95a..27e6118 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -608,6 +608,7 @@ struct _virDomainDiskDef { virStorageSourcePtr mirror; bool mirroring; + int mirrorJob; /* virDomainBlockJobType */ struct { unsigned int cylinders; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3399b2a..728aa85 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -14994,6 +14994,7 @@ qemuDomainBlockPivot(virConnectPtr conn, virStorageSourceFree(disk->mirror); disk->mirror = NULL; disk->mirroring = false; + disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_UNKNOWN; cleanup: if (resume && virDomainObjIsActive(vm) && @@ -15155,6 +15156,7 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, virStorageSourceFree(disk->mirror); disk->mirror = NULL; disk->mirroring = false; + disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_UNKNOWN; } /* With synchronous block cancel, we must synthesize an event, and @@ -15429,6 +15431,7 @@ qemuDomainBlockCopy(virDomainObjPtr vm, need_unlink = false; disk->mirror = mirror; mirror = NULL; + disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_COPY; endjob: if (need_unlink && unlink(dest)) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index e4845ba..ed2991a 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -1020,26 +1020,30 @@ qemuProcessHandleBlockJob(qemuMonitorPtr mon ATTRIBUTE_UNUSED, disk = qemuProcessFindDomainDiskByAlias(vm, diskAlias); if (disk) { + if (type == VIR_DOMAIN_BLOCK_JOB_TYPE_COMMIT && + disk->mirrorJob == VIR_DOMAIN_BLOCK_JOB_TYPE_ACTIVE_COMMIT) + type = disk->mirrorJob; path = virDomainDiskGetSource(disk); event = virDomainEventBlockJobNewFromObj(vm, path, type, status); /* XXX If we completed a block pull or commit, then recompute * the cached backing chain to match. Better would be storing - * the chain ourselves rather than reprobing, but this - * requires modifying domain_conf and our XML to fully track - * the chain across libvirtd restarts. For that matter, if - * qemu gains support for committing the active layer, we have - * to update disk->src. */ + * the chain ourselves rather than reprobing, but we haven't + * quite completed that conversion to use our XML tracking. */ if ((type == VIR_DOMAIN_BLOCK_JOB_TYPE_PULL || - type == VIR_DOMAIN_BLOCK_JOB_TYPE_COMMIT) && + type == VIR_DOMAIN_BLOCK_JOB_TYPE_COMMIT || + type == VIR_DOMAIN_BLOCK_JOB_TYPE_ACTIVE_COMMIT) && status == VIR_DOMAIN_BLOCK_JOB_COMPLETED) qemuDomainDetermineDiskChain(driver, vm, disk, true); - if (disk->mirror && type == VIR_DOMAIN_BLOCK_JOB_TYPE_COPY) { + if (disk->mirror && + (type == VIR_DOMAIN_BLOCK_JOB_TYPE_COPY || + type == VIR_DOMAIN_BLOCK_JOB_TYPE_ACTIVE_COMMIT)) { if (status == VIR_DOMAIN_BLOCK_JOB_READY) { disk->mirroring = true; } else if (status == VIR_DOMAIN_BLOCK_JOB_FAILED) { virStorageSourceFree(disk->mirror); disk->mirror = NULL; disk->mirroring = false; + disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_UNKNOWN; } } } diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-active-commit.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-active-commit.xml new file mode 100644 index 0000000..6ba5724 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-active-commit.xml @@ -0,0 +1,37 @@ +<domain type='qemu' id='1'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='file' device='disk'> + <driver name='qemu' type='qcow2'/> + <source file='/tmp/HostVG/QEMUGuest1-snap'/> + <backingStore type='block' index='1'> + <format type='raw'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <backingStore/> + </backingStore> + <mirror type='block' job='active-commit'> + <format type='raw'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + </mirror> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-mirror.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-mirror.xml index a937d0a..6c28687 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-mirror.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-mirror.xml @@ -17,7 +17,7 @@ <disk type='block' device='disk'> <source dev='/dev/HostVG/QEMUGuest1'/> <backingStore/> - <mirror type='file' ready='yes'> + <mirror type='file' job='copy' ready='yes'> <source file='/dev/HostVG/QEMUGuest1Copy'/> </mirror> <target dev='hda' bus='ide'/> @@ -33,7 +33,7 @@ <disk type='file' device='disk'> <source file='/tmp/data.img'/> <backingStore/> - <mirror type='file'> + <mirror type='file' job='copy'> <format type='qcow2'/> <source file='/tmp/copy.img'/> </mirror> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-mirror-old.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-mirror-old.xml index a937d0a..6c28687 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-mirror-old.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-mirror-old.xml @@ -17,7 +17,7 @@ <disk type='block' device='disk'> <source dev='/dev/HostVG/QEMUGuest1'/> <backingStore/> - <mirror type='file' ready='yes'> + <mirror type='file' job='copy' ready='yes'> <source file='/dev/HostVG/QEMUGuest1Copy'/> </mirror> <target dev='hda' bus='ide'/> @@ -33,7 +33,7 @@ <disk type='file' device='disk'> <source file='/tmp/data.img'/> <backingStore/> - <mirror type='file'> + <mirror type='file' job='copy'> <format type='qcow2'/> <source file='/tmp/copy.img'/> </mirror> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 200d50f..98b2c15 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -227,6 +227,7 @@ mymain(void) DO_TEST_DIFFERENT("disk-mirror-old"); DO_TEST_FULL("disk-mirror", false, WHEN_ACTIVE); DO_TEST_FULL("disk-mirror", true, WHEN_INACTIVE); + DO_TEST_FULL("disk-active-commit", false, WHEN_ACTIVE); DO_TEST("graphics-listen-network"); DO_TEST("graphics-vnc"); DO_TEST("graphics-vnc-websocket"); -- 1.9.3
On 06/11/14 18:27, Eric Blake wrote:
A future patch is going to wire up qemu active block commit jobs; but as they have similar events and are canceled/pivoted in the same way as block copy jobs, it is easiest to track all bookkeeping for the commit job by reusing the <mirror> element. This patch adds domain XML to track which job was responsible for creating a mirroring situation, and adds a job='copy' attribute to all existing uses of <mirror>. Along the way, it also massages the qemu monitor backend to read the new field in order to generate the correct type of libvirt job (even though it requires a future patch to actually cause a qemu event that can be reported as an active commit).
* docs/schemas/domaincommon.rng: Enhance schema. * docs/formatdomain.html.in: Document it. * src/conf/domain_conf.h (_virDomainDiskDef): Add a field. * src/conf/domain_conf.c (virDomainBlockJobType): String conversion. (virDomainDiskDefParseXML): Parse job type. (virDomainDiskDefFormat): Output job type. * src/qemu/qemu_process.c (qemuProcessHandleBlockJob): Distinguish active from regular commit. * src/qemu/qemu_driver.c (qemuDomainBlockCopy): Set job type. (qemuDomainBlockPivot, qemuDomainBlockJobImpl): Clean up job type on completion. * tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-mirror-old.xml: Update tests. * tests/qemuxml2argvdata/qemuxml2argv-disk-mirror.xml: Likewise. * tests/qemuxml2argvdata/qemuxml2argv-disk-active-commit.xml: New file. * tests/qemuxml2xmltest.c (mymain): Drive new test.
Signed-off-by: Eric Blake <eblake@redhat.com> --- docs/formatdomain.html.in | 20 ++++++------ docs/schemas/domaincommon.rng | 6 ++++ src/conf/domain_conf.c | 24 ++++++++++++++ src/conf/domain_conf.h | 1 + src/qemu/qemu_driver.c | 3 ++ src/qemu/qemu_process.c | 18 +++++++---- .../qemuxml2argv-disk-active-commit.xml | 37 ++++++++++++++++++++++ .../qemuxml2argvdata/qemuxml2argv-disk-mirror.xml | 4 +-- .../qemuxml2xmlout-disk-mirror-old.xml | 4 +-- tests/qemuxml2xmltest.c | 1 + 10 files changed, 98 insertions(+), 20 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-active-commit.xml
ACK, although this patch pointed me to a problem. If you do a block-copy of a network disk to a local file (which seems from the code that it's allowed) and the mirror is being pivoted to the new disk we don't update the disk type in the code and thus would end up with a very damaged VM. Fortunately for block-copy this isn't a major problem as the VM has to be transient, but will be fatal for the active-commit code. Peter
On 06/12/2014 05:51 AM, Peter Krempa wrote:
On 06/11/14 18:27, Eric Blake wrote:
A future patch is going to wire up qemu active block commit jobs; but as they have similar events and are canceled/pivoted in the same way as block copy jobs, it is easiest to track all bookkeeping for the commit job by reusing the <mirror> element. This patch adds domain XML to track which job was responsible for creating a mirroring situation, and adds a job='copy' attribute to all existing uses of <mirror>. Along the way, it also massages the qemu monitor backend to read the new field in order to generate the correct type of libvirt job (even though it requires a future patch to actually cause a qemu event that can be reported as an active commit).
ACK, although this patch pointed me to a problem. If you do a block-copy of a network disk to a local file (which seems from the code that it's allowed) and the mirror is being pivoted to the new disk we don't update the disk type in the code and thus would end up with a very damaged VM. Fortunately for block-copy this isn't a major problem as the VM has to be transient, but will be fatal for the active-commit code.
Oh good point. I'm hoping your series for copying a virStorageSource will help out here (including however we end up resolving the issue at tracking the security manager stuff). -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 06/11/2014 10:27 AM, Eric Blake wrote:
A future patch is going to wire up qemu active block commit jobs; but as they have similar events and are canceled/pivoted in the same way as block copy jobs, it is easiest to track all bookkeeping for the commit job by reusing the <mirror> element. This patch adds domain XML to track which job was responsible for creating a mirroring situation, and adds a job='copy' attribute to all existing uses of <mirror>. Along the way, it also massages the qemu monitor backend to read the new field in order to generate the correct type of libvirt job (even though it requires a future patch to actually cause a qemu event that can be reported as an active commit).
On IRC, Adam Litke pointed out that we have an issue on the event we generate:
+++ b/src/qemu/qemu_process.c @@ -1020,26 +1020,30 @@ qemuProcessHandleBlockJob(qemuMonitorPtr mon ATTRIBUTE_UNUSED, disk = qemuProcessFindDomainDiskByAlias(vm, diskAlias);
if (disk) { + if (type == VIR_DOMAIN_BLOCK_JOB_TYPE_COMMIT && + disk->mirrorJob == VIR_DOMAIN_BLOCK_JOB_TYPE_ACTIVE_COMMIT) + type = disk->mirrorJob; path = virDomainDiskGetSource(disk); event = virDomainEventBlockJobNewFromObj(vm, path, type, status);
for an original chain 'base <- snap', we are generating the 'ready' event with a disk name of 'snap', but the 'complete' event with a disk name of 'base'. Ideally, we WANT to generate both events tied to the same disk name (even better would be generating the events tied to the name 'vda' which is unchanging, rather than the disk source name, but I have to think about whether there are backward-compatibility concerns before making such a change). I'll investigate what it will take to get that naming improvement into the events that we generate. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
With this in place, I can (finally!) now do: virsh blockcommit $dom vda --shallow --wait --verbose --pivot and watch qemu shorten the backing chain by one, followed by libvirt automatically updating the dumpxml output, effectively undoing the work of virsh snapshot-commit --no-metadata --disk-only. Commit is SOOOO much faster than blockpull, when I'm still fairly close in time to when the temporary qcow2 wrapper file was created via a snapshot operation! * src/qemu/qemu_driver.c (qemuDomainBlockCommit): Implement live commit to regular files. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/qemu/qemu_driver.c | 43 ++++++++++++++++++++++++++++++++++++++----- 1 file changed, 38 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 728aa85..60dc9c6 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15529,9 +15529,11 @@ qemuDomainBlockCommit(virDomainPtr dom, char *topPath = NULL; char *basePath = NULL; char *backingPath = NULL; + virStorageSourcePtr mirror = NULL; - /* XXX Add support for COMMIT_ACTIVE, COMMIT_DELETE */ + /* XXX Add support for COMMIT_DELETE */ virCheckFlags(VIR_DOMAIN_BLOCK_COMMIT_SHALLOW | + VIR_DOMAIN_BLOCK_COMMIT_ACTIVE | VIR_DOMAIN_BLOCK_COMMIT_RELATIVE, -1); if (!(vm = qemuDomObjFromDomain(dom))) @@ -15578,13 +15580,14 @@ qemuDomainBlockCommit(virDomainPtr dom, &top_parent))) goto endjob; - /* FIXME: qemu 2.0 supports active commit, but as a two-stage - * process; qemu 2.1 is further improving active commit. We need - * to start supporting it in libvirt. */ if (topSource == disk->src) { /* We assume that no one will backport qemu 2.0 active commit * to an earlier qemu without also backporting the block job - * ready event; but this makes sure of that fact */ + * ready event; but this makes sure of that fact. + * + * XXX Also need to check other capability bit(s): qemu 1.7 + * supports async blockjob but not active commit; and qemu 2.0 + * active commit misbehaves on 0-byte file. */ if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKJOB_ASYNC)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("active commit not supported with this QEMU binary")); @@ -15597,6 +15600,14 @@ qemuDomainBlockCommit(virDomainPtr dom, disk->dst); goto endjob; } + if (disk->mirror) { + virReportError(VIR_ERR_BLOCK_COPY_ACTIVE, + _("disk '%s' already in active block job"), + disk->dst); + goto endjob; + } + if (VIR_ALLOC(mirror) < 0) + goto endjob; } else if (flags & VIR_DOMAIN_BLOCK_COMMIT_ACTIVE) { virReportError(VIR_ERR_INVALID_ARG, _("active commit requested but '%s' is not active"), @@ -15667,6 +15678,21 @@ qemuDomainBlockCommit(virDomainPtr dom, } } + /* For an active commit, clone enough of the base to act as the mirror */ + if (mirror) { + /* XXX Support network commits */ + if (baseSource->type != VIR_STORAGE_TYPE_FILE && + baseSource->type != VIR_STORAGE_TYPE_BLOCK) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("active commit to non-file not supported yet")); + goto endjob; + } + mirror->type = baseSource->type; + if (VIR_STRDUP(mirror->path, baseSource->path) < 0) + goto endjob; + mirror->format = baseSource->format; + } + /* Start the commit operation. Pass the user's original spelling, * if any, through to qemu, since qemu may behave differently * depending on whether the input was specified as relative or @@ -15678,6 +15704,12 @@ qemuDomainBlockCommit(virDomainPtr dom, bandwidth); qemuDomainObjExitMonitor(driver, vm); + if (ret == 0 && mirror) { + disk->mirror = mirror; + mirror = NULL; + disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_ACTIVE_COMMIT; + } + endjob: if (ret < 0 && clean_access) { /* Revert access to read-only, if possible. */ @@ -15688,6 +15720,7 @@ qemuDomainBlockCommit(virDomainPtr dom, top_parent, VIR_DISK_CHAIN_READ_ONLY); } + virStorageSourceFree(mirror); if (!qemuDomainObjEndJob(driver, vm)) vm = NULL; -- 1.9.3
On 06/11/14 18:27, Eric Blake wrote:
With this in place, I can (finally!) now do:
virsh blockcommit $dom vda --shallow --wait --verbose --pivot
and watch qemu shorten the backing chain by one, followed by libvirt automatically updating the dumpxml output, effectively undoing the work of virsh snapshot-commit --no-metadata --disk-only. Commit is SOOOO much faster than blockpull, when I'm still fairly close in time to when the temporary qcow2 wrapper file was created via a snapshot operation!
* src/qemu/qemu_driver.c (qemuDomainBlockCommit): Implement live commit to regular files.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/qemu/qemu_driver.c | 43 ++++++++++++++++++++++++++++++++++++++----- 1 file changed, 38 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 728aa85..60dc9c6 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15529,9 +15529,11 @@ qemuDomainBlockCommit(virDomainPtr dom, char *topPath = NULL; char *basePath = NULL; char *backingPath = NULL; + virStorageSourcePtr mirror = NULL;
- /* XXX Add support for COMMIT_ACTIVE, COMMIT_DELETE */ + /* XXX Add support for COMMIT_DELETE */ virCheckFlags(VIR_DOMAIN_BLOCK_COMMIT_SHALLOW | + VIR_DOMAIN_BLOCK_COMMIT_ACTIVE |
Rebase needed if this should go before my series.
VIR_DOMAIN_BLOCK_COMMIT_RELATIVE, -1);
if (!(vm = qemuDomObjFromDomain(dom))) @@ -15578,13 +15580,14 @@ qemuDomainBlockCommit(virDomainPtr dom, &top_parent))) goto endjob;
- /* FIXME: qemu 2.0 supports active commit, but as a two-stage - * process; qemu 2.1 is further improving active commit. We need - * to start supporting it in libvirt. */ if (topSource == disk->src) { /* We assume that no one will backport qemu 2.0 active commit * to an earlier qemu without also backporting the block job - * ready event; but this makes sure of that fact */ + * ready event; but this makes sure of that fact. + * + * XXX Also need to check other capability bit(s): qemu 1.7 + * supports async blockjob but not active commit; and qemu 2.0 + * active commit misbehaves on 0-byte file. */
How about tying the support to qemu 2.1 if it makes things easier? I don't see any problem with supporting stuff from later-than-introduced versions. The only question that remains is whether there's anything relevant to trigger the capability of.
if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKJOB_ASYNC)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("active commit not supported with this QEMU binary"));
@@ -15667,6 +15678,21 @@ qemuDomainBlockCommit(virDomainPtr dom, } }
+ /* For an active commit, clone enough of the base to act as the mirror */ + if (mirror) { + /* XXX Support network commits */ + if (baseSource->type != VIR_STORAGE_TYPE_FILE && + baseSource->type != VIR_STORAGE_TYPE_BLOCK) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("active commit to non-file not supported yet")); + goto endjob; + } + mirror->type = baseSource->type; + if (VIR_STRDUP(mirror->path, baseSource->path) < 0) + goto endjob; + mirror->format = baseSource->format; + } + /* Start the commit operation. Pass the user's original spelling, * if any, through to qemu, since qemu may behave differently * depending on whether the input was specified as relative or
Hmmm, this commit is invalid here as we are not passing user's spelling to qemu. It also doesn't make sense to do that. I should revert that patch explicitly apparently as a part of my series. Still needs the capability bit figured out. Otherwise looks good. Peter
On 06/12/2014 06:01 AM, Peter Krempa wrote:
On 06/11/14 18:27, Eric Blake wrote:
With this in place, I can (finally!) now do:
virsh blockcommit $dom vda --shallow --wait --verbose --pivot
- /* FIXME: qemu 2.0 supports active commit, but as a two-stage - * process; qemu 2.1 is further improving active commit. We need - * to start supporting it in libvirt. */ if (topSource == disk->src) { /* We assume that no one will backport qemu 2.0 active commit * to an earlier qemu without also backporting the block job - * ready event; but this makes sure of that fact */ + * ready event; but this makes sure of that fact. + * + * XXX Also need to check other capability bit(s): qemu 1.7 + * supports async blockjob but not active commit; and qemu 2.0 + * active commit misbehaves on 0-byte file. */
How about tying the support to qemu 2.1 if it makes things easier? I don't see any problem with supporting stuff from later-than-introduced versions. The only question that remains is whether there's anything relevant to trigger the capability of.
Testing qemu 2.1 is easier - merely check whether 'block-commit' with an omitted top argument is accepted or rejected. I just realized - rather than trying to go for 2.0 to begin with, it may be easier to just require 2.1 for now; we can always relax things later if people complain that they are still stuck on 2.0, but right now, there's enough other things going on at once (such as your relative backing naming stuff) that also depends on 2.1 that it just feels easier to stick to a single version as our baseline, rather than delaying this further. But if I require qemu 2.1, then I'm in the same boat as you as not having anything to push until I have an actual commit in qemu.git to point to. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 06/11/2014 10:27 AM, Eric Blake wrote:
I still don't have qemu capability detection working reliably, but want to post this series right now so that it can be built into a scratch build containing Peter's and my changes. (Or put another way, I was testing what conflict resolutions are required - patch 2/5 (virsh) and 5/5 (qemu_driver) has some conflicts with Peter's addition of relative backing name; and I think the resolutions were fairly simple).
These patches are tested on top of: https://www.redhat.com/archives/libvir-list/2014-June/msg00492.html
I may still end up posting a v4 and/or pushing my series before Peter's, once I get capability detection working the way I want.
Phooey, I forgot to send my v3 changelog.
Eric Blake (5): virsh: improve blockcopy UI
new in v3
virsh: expose new active commit controls
resolves Peter's comments in v2 about ?:, rename --finish to --keep-overlay, and copy UI enhancements from 1/5. Conflict resolution with --keep-relative.
blockcommit: update error messages related to block jobs
unchanged from v2
blockcommit: track job type in xml
Reflow html source, tweak comment about why only 2 of the 5 job types are mapped to string
blockcommit: turn on active commit
Conflict resolution with --keep-relative; still needs proper qemu capability detection -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 11/06/14 10:27 -0600, Eric Blake wrote:
I still don't have qemu capability detection working reliably, but want to post this series right now so that it can be built into a scratch build containing Peter's and my changes. (Or put another way, I was testing what conflict resolutions are required - patch 2/5 (virsh) and 5/5 (qemu_driver) has some conflicts with Peter's addition of relative backing name; and I think the resolutions were fairly simple).
These patches are tested on top of: https://www.redhat.com/archives/libvir-list/2014-June/msg00492.html
I may still end up posting a v4 and/or pushing my series before Peter's, once I get capability detection working the way I want.
Eric Blake (5): virsh: improve blockcopy UI virsh: expose new active commit controls blockcommit: update error messages related to block jobs blockcommit: track job type in xml blockcommit: turn on active commit
I've tested this with the following script and it seems that specifying base and top explicitly does not work. Using --shallow does work for me though. Running the included script yields the following output: $ sudo bash /home/alitke/test/test.sh Formatting 'base.img', fmt=raw size=1073741824 Formatting 'snap1.img', fmt=qcow2 size=1073741824 backing_file='base.img' backing_fmt='raw' encryption=off cluster_size=65536 lazy_refcounts=off Formatting 'snap2.img', fmt=qcow2 size=1073741824 backing_file='snap1.img' backing_fmt='qcow2' encryption=off cluster_size=65536 lazy_refcounts=off image: /tmp/snap2.img file format: qcow2 virtual size: 1.0G (1073741824 bytes) disk size: 196K cluster_size: 65536 backing file: snap1.img (actual path: /tmp/snap1.img) backing file format: qcow2 Format specific information: compat: 1.1 lazy refcounts: false Domain testvm1 created from /dev/stdin error: invalid argument: could not find image '/tmp/snap2.img' in chain for '/tmp/snap2.img' Domain testvm1 destroyed -- #!/bin/sh rm -f base.img snap1.img snap2.img # base.img <- snap1.img <- snap2.img qemu-img create -f raw base.img 1G qemu-img create -f qcow2 -b base.img -o backing_fmt=raw snap1.img qemu-img create -f qcow2 -b snap1.img -o backing_fmt=qcow2 snap2.img chcon -t svirt_image_t base.img snap1.img snap2.img chmod 666 base.img snap1.img snap2.img qemu-img info $PWD/snap2.img virsh create /dev/stdin <<EOF <domain type='kvm'> <name>testvm1</name> <memory unit='MiB'>256</memory> <vcpu>1</vcpu> <os> <type arch='x86_64'>hvm</type> </os> <devices> <disk type='file' device='disk'> <driver name='qemu' type='qcow2'/> <source file='$PWD/snap2.img'/> <target dev='vda' bus='virtio'/> </disk> <graphics type='vnc'/> </devices> </domain> EOF base=$PWD/snap1.img top=$PWD/snap2.img virsh blockcommit testvm1 vda 0 $base $top --wait --verbose --pivot virsh destroy testvm1 -- Adam Litke
On 06/11/2014 01:53 PM, Adam Litke wrote:
I've tested this with the following script and it seems that specifying base and top explicitly does not work. Using --shallow does work for me though. Running the included script yields the following output:
$ sudo bash /home/alitke/test/test.sh Formatting 'base.img', fmt=raw
error: invalid argument: could not find image '/tmp/snap2.img' in chain for '/tmp/snap2.img'
base=$PWD/snap1.img top=$PWD/snap2.img virsh blockcommit testvm1 vda 0 $base $top --wait --verbose --pivot
Okay, I see the flaw - when looking up an explicit top, we aren't starting from the active layer. Try omitting the --top argument as a workaround. Meanwhile, this patch should fix it: diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c index 60dc9c6..aded8e0 100644 --- i/src/qemu/qemu_driver.c +++ w/src/qemu/qemu_driver.c @@ -15575,7 +15575,7 @@ qemuDomainBlockCommit(virDomainPtr dom, topSource = disk->src; else if (virStorageFileParseChainIndex(disk->dst, top, &topIndex) < 0 || !(topSource = virStorageFileChainLookup(disk->src, - disk->src->backingStore, + disk->src, top, topIndex, &top_parent))) goto endjob; -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (3)
-
Adam Litke -
Eric Blake -
Peter Krempa