[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@redhat.com> --- 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"), -- 1.9.0
On 05/16/14 22:17, Eric Blake wrote: ...
Signed-off-by: Eric Blake <eblake@redhat.com> ---
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
@@ -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 */
[1] ...
} virDomainBlockCommitFlags;
int virDomainBlockCommit(virDomainPtr dom, const char *disk, const char *base,
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
...
@@ -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)) {
This fails to compile as VIR_DOMAIN_BLOCK_COPY_ACTIVE differs from the one declared at [1].
+ 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'll do a full review on Monday. Peter
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@redhat.com> ---
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
On 05/19/2014 01:58 AM, Peter Krempa wrote: [Sorry for not putting RFC in the subject line]
+++ 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/
Blah - serves me right for sending a patch at the end of my day. But I'm glad you were able to see the intent.
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.
_This_ patch is fixing the qemu driver to not hang, by acknowledging that we _don't_ support active block commit (the virCheckFlags() at the beginning of the function fails if the new flag is specified, and this check fails if the flag is not specified - which means you cannot do active commits). The next few patches (when I develop it into a full series) will first be to add support for the new flag (fixing the first XXX) and then to relax things to auto-pivot when the flag is not present (fixing the second XXX).
+ 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.
Yes, that's my next task before turning this from an RFC into an actual patch series, even before the qemu implementation is done.
Additionally a change in virsh is missing again breaks the build process:
Yep, my fault for not actually compile-testing in my rush to get it out before the weekend :) -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 05/16/2014 02:17 PM, 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.
And as a reminder (more to myself, perhaps), part of flushing this out into a full series rather than just an RFC on the semantics will be adding a patch that exposes the ability for top-level commit, similar to what was done in commit 85a3eb8 for exposing whether disk snapshot is supported. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Peter Krempa