On 05/16/14 22:17, Eric Blake wrote:
> 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@xxxxxxxxxx>
> ---
>
> 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
Doc changes seem okay to me.
> 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? */
I think we should. But I agree in postponing it to later patch.
> + if (topSource == &disk->src && !(flags & VIR_DOMAIN_BLOCK_COPY_ACTIVE)) {
As mentioned in the previous mail, this breaks build.
s/BLOCK_COPY/BLOCK_COMMIT/
Also shouldn't this hunk actually go in the patch that adds the active
block commit feature? It seems a bit out of place here.
> + 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"),
>
I think that this patch should also export this new flag in virsh as we
usually do with API flag additions.
Additionally a change in virsh is missing again breaks the build process:
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 84a6706..94688ef 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -1968,7 +1968,8 @@ VIR_ENUM_IMPL(vshDomainBlockJob,
N_("Unknown job"),
N_("Block Pull"),
N_("Block Copy"),
- N_("Block Commit"))
+ N_("Block Commit"),
+ N_("Block commit from active layer"))
static const char *
vshDomainBlockJobToString(int type)
Peter
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list