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

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

 



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

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]