Re: [libvirt] [PATCH] blockcommit: document semantics of committing active layer

Now that qemu 2.0 allows commit of the active layer, people are attempting to use virsh blockcommit and getting into a stuck state, because libvirt is unprepared to handle the two-phase commit required by qemu. Stepping back a bit, there are two valid semantics for a commit operation: 1. Maintain a 'golden' base, and a transient overlay. Make changes in the overlay, and if everything appears to work, commit those changes into the base, but still keep the overlay for the next round of changes; repeat the cycle as desired. 2. Create an external snapshot, then back up the stable state in the backing file. Once the backup is complete, commit the overlay back into the base, and delete the temporary snapshot. Since qemu doesn't know up front which of the two styles is preferred, a block commit of the active layer merely gets the job into a synchronized state, and sends an event; then the user must either cancel (case 1) or complete (case 2), where qemu then sends a second event that actually ends the job. However, libvirt was blindly assuming the semantics that apply to a commit of an intermediate image, where there is only one sane conclusion (the job automatically ends with fewer elements in the chain); and getting stuck because it wasn't prepared for qemu to enter a second phase of the job. This patch adds a flag to the libvirt API that a user MUST supply in order to acknowledge that they will be using two-phase semantics. It might be possible to have a mode where if the flag is omitted, we automatically do the case 2 semantics on the user's behalf; but before that happens, I must do additional patches to track the fact that we are doing an active commit in the domain XML. So the safest thing for now is to reject the new flag in qemu, as well as prevent commits of the active layer without the flag. Later patches will add support of the flag, and once 2-phase semantics are working, we can then decide whether to relax things to allow an omitted flag to cause an automatic pivot. * include/libvirt/libvirt.h.in (VIR_DOMAIN_BLOCK_COMMIT_ACTIVE) (VIR_DOMAIN_BLOCK_JOB_TYPE_ACTIVE_COMMIT): New enums. * src/libvirt.c (virDomainBlockCommit): Document two-phase job when committing active layer, through new flag. (virDomainBlockJobAbort): Document that pivot also occurs after active commit. * src/qemu/qemu_driver.c (qemuDomainBlockCommit): Explicitly reject active copy; later patches will add it in. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- This patch should probably be backported, even if later patches in the series are not, so that users avoid getting hung. include/libvirt/libvirt.h.in | 12 ++++++---- src/libvirt.c | 55 +++++++++++++++++++++++++++++--------------- src/qemu/qemu_driver.c | 9 ++++++++ 3 files changed, 54 insertions(+), 22 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 260c971..127de11 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2513,6 +2513,7 @@ typedef enum { VIR_DOMAIN_BLOCK_JOB_TYPE_PULL = 1, VIR_DOMAIN_BLOCK_JOB_TYPE_COPY = 2, VIR_DOMAIN_BLOCK_JOB_TYPE_COMMIT = 3, + VIR_DOMAIN_BLOCK_JOB_TYPE_ACTIVE_COMMIT = 4, #ifdef VIR_ENUM_SENTINELS VIR_DOMAIN_BLOCK_JOB_TYPE_LAST @@ -2523,7 +2524,8 @@ typedef enum { * virDomainBlockJobAbortFlags: * * VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC: Request only, do not wait for completion - * VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT: Pivot to mirror when ending a copy job + * VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT: Pivot to new file when ending a copy or + * active commit job */ typedef enum { VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC = 1 << 0, @@ -2588,6 +2590,8 @@ typedef enum { VIR_DOMAIN_BLOCK_COMMIT_DELETE = 1 << 1, /* Delete any files that are now invalid after their contents have been committed */ + VIR_DOMAIN_BLOCK_COMMIT_ACTIVE = 1 << 2, /* Allow a two-phase commit when + top is the active layer */ } virDomainBlockCommitFlags; int virDomainBlockCommit(virDomainPtr dom, const char *disk, const char *base, @@ -4823,8 +4827,8 @@ typedef void (*virConnectDomainEventGraphicsCallback)(virConnectPtr conn, /** * virConnectDomainEventBlockJobStatus: * - * The final status of a virDomainBlockPull() or virDomainBlockRebase() - * operation + * Tracks status of a virDomainBlockPull(), virDomainBlockRebase(), + * or virDomainBlockCommit() operation */ typedef enum { VIR_DOMAIN_BLOCK_JOB_COMPLETED = 0, @@ -4843,7 +4847,7 @@ typedef enum { * @dom: domain on which the event occurred * @disk: fully-qualified filename of the affected disk * @type: type of block job (virDomainBlockJobType) - * @status: final status of the operation (virConnectDomainEventBlockJobStatus) + * @status: status of the operation (virConnectDomainEventBlockJobStatus) * @opaque: application specified data * * The callback signature to use when registering for an event of type diff --git a/src/libvirt.c b/src/libvirt.c index 19fa18b..20abfce 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -19467,13 +19467,16 @@ virDomainOpenChannel(virDomainPtr dom, * * If the current block job for @disk is VIR_DOMAIN_BLOCK_JOB_TYPE_COPY, then * the default is to abort the mirroring and revert to the source disk; - * adding @flags of VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT causes this call to - * fail with VIR_ERR_BLOCK_COPY_ACTIVE if the copy is not fully populated, - * otherwise it will swap the disk over to the copy to end the mirroring. An - * event will be issued when the job is ended, and it is possible to use - * VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC to control whether this command waits - * for the completion of the job. Restarting this job requires starting - * over from the beginning of the first phase. + * likewise, if the current job is VIR_DOMAIN_BLOCK_JOB_TYPE_ACTIVE_COMMIT, + * the default is to abort without changing the active layer of @disk. + * Adding @flags of VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT causes this call to + * fail with VIR_ERR_BLOCK_COPY_ACTIVE if the copy or commit is not yet + * ready; otherwise it will swap the disk over to the new active image + * to end the mirroring or active commit. An event will be issued when the + * job is ended, and it is possible to use VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC + * to control whether this command waits for the completion of the job. + * Restarting a copy or active commit job requires starting over from the + * beginning of the first phase. * * Returns -1 in case of failure, 0 when successful. */ @@ -19843,17 +19846,32 @@ virDomainBlockRebase(virDomainPtr dom, const char *disk, * the job is aborted, it is up to the hypervisor whether starting a new * job will resume from the same point, or start over. * + * As a special case, if @top is the active image (or NULL), and @flags + * includes VIR_DOMAIN_BLOCK_COMMIT_ACTIVE, the block job will have a type + * of VIR_DOMAIN_BLOCK_JOB_TYPE_ACTIVE_COMMIT, and operates in two phases. + * In the first phase, the contents are being committed into @base, and the + * job can only be canceled. The job transitions to the second phase when + * the job info states cur == end, and remains alive to keep all further + * changes to @top synchronized into @base; an event with status + * VIR_DOMAIN_BLOCK_JOB_READY is also issued to mark the job transition. + * Once in the second phase, the user must choose whether to cancel the job + * (keeping @top as the active image, but now containing only the changes + * since the time the job ended) or to pivot the job (adjusting to @base as + * the active image, and invalidating @top). + * * Be aware that this command may invalidate files even if it is aborted; * the user is cautioned against relying on the contents of invalidated - * intermediate files such as @top without manually rebasing those files - * to use a backing file of a read-only copy of @base prior to the point - * where the commit operation was started (although such a rebase cannot - * be safely done until the commit has successfully completed). However, - * the domain itself will not have any issues; the active layer remains - * valid throughout the entire commit operation. As a convenience, - * if @flags contains VIR_DOMAIN_BLOCK_COMMIT_DELETE, this command will - * unlink all files that were invalidated, after the commit successfully - * completes. + * intermediate files such as @top (when @top is not the active image) + * without manually rebasing those files to use a backing file of a + * read-only copy of @base prior to the point where the commit operation + * was started (and such a rebase cannot be safely done until the commit + * has successfully completed). However, the domain itself will not have + * any issues; the active layer remains valid throughout the entire commit + * operation. + * + * Some hypervisors may support a shortcut where if @flags contains + * VIR_DOMAIN_BLOCK_COMMIT_DELETE, then this command will unlink all files + * that were invalidated, after the commit successfully completes. * * By default, if @base is NULL, the commit target will be the bottom of * the backing chain; if @flags contains VIR_DOMAIN_BLOCK_COMMIT_SHALLOW, @@ -19861,8 +19879,9 @@ virDomainBlockRebase(virDomainPtr dom, const char *disk, * is NULL, the active image at the top of the chain will be used. Some * hypervisors place restrictions on how much can be committed, and might * fail if @base is not the immediate backing file of @top, or if @top is - * the active layer in use by a running domain, or if @top is not the - * top-most file; restrictions may differ for online vs. offline domains. + * the active layer in use by a running domain but @flags did not include + * VIR_DOMAIN_BLOCK_COMMIT_ACTIVE, or if @top is not the top-most file; + * restrictions may differ for online vs. offline domains. * * The @disk parameter is either an unambiguous source name of the * block device (the <source file='...'/> sub-element, such as

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 55b4755..75c59e0 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15377,6 +15377,7 @@ qemuDomainBlockCommit(virDomainPtr dom, const char *top_parent = NULL; bool clean_access = false; + /* XXX Add support for COMMIT_ACTIVE, COMMIT_DELETE */ virCheckFlags(VIR_DOMAIN_BLOCK_COMMIT_SHALLOW, -1); if (!(vm = qemuDomObjFromDomain(dom))) @@ -15423,6 +15424,14 @@ qemuDomainBlockCommit(virDomainPtr dom, &top_parent))) goto endjob; + /* XXX Should we auto-pivot when COMMIT_ACTIVE is not specified? */

+ if (topSource == &disk->src && !(flags & VIR_DOMAIN_BLOCK_COPY_ACTIVE)) {

+ virReportError(VIR_ERR_INVALID_ARG, + _("commit of '%s' active layer requires active flag"), + disk->dst); + goto endjob; + } + if (!topSource->backingStore) { virReportError(VIR_ERR_INVALID_ARG, _("top '%s' in chain for '%s' has no backing file"),