[libvirt] [PATCH] conf: refact virDomainHasDiskMirror and rename it to virDomainHasBlockjob

Shanzhi Yu shyu at redhat.com
Thu Mar 26 12:29:39 UTC 2015


----- Original Message -----

| From: "Peter Krempa" <pkrempa at redhat.com>
| To: "Shanzhi Yu" <shyu at redhat.com>
| Cc: libvir-list at redhat.com
| Sent: Wednesday, March 25, 2015 11:24:21 PM
| Subject: Re: [libvirt] [PATCH] conf: refact virDomainHasDiskMirror and rename
| it to virDomainHasBlockjob

| On Tue, Mar 24, 2015 at 18:08:00 +0800, Shanzhi Yu wrote:
| > Create external snapshot or migrate a vm when there is a blockpull
| > job running should be forbidden by libvirt, otherwise qemu try to
| > create external snapshot and failed with error "unable to execute
| > QEMU command 'transaction': Device 'drive-virtio-disk0' is busy:
| > block device is in use by block job: stream", and migration will
| > succeed which will lead the blockpull job failed.
| >
| > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1203628
| > Signed-off-by: Shanzhi Yu <shyu at redhat.com>
| > ---
| > src/conf/domain_conf.c | 6 +++---
| > src/conf/domain_conf.h | 2 +-
| > src/libvirt_private.syms | 2 +-
| > src/qemu/qemu_driver.c | 6 +++---
| > src/qemu/qemu_migration.c | 2 +-
| > 5 files changed, 9 insertions(+), 9 deletions(-)
| >
| > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
| > index d633f93..24445af 100644
| > --- a/src/conf/domain_conf.c
| > +++ b/src/conf/domain_conf.c
| > @@ -12264,13 +12264,13 @@ virDomainDiskRemoveByName(virDomainDefPtr def,
| > const char *name)
| > }
| >
| > /* Return true if VM has at least one disk involved in a current block
| > - * copy/commit job (that is, with a <mirror> element in the disk xml). */
| > + * copy/commit/pull job */
| > bool
| > -virDomainHasDiskMirror(virDomainObjPtr vm)
| > +virDomainHasBlockjob(virDomainObjPtr vm)
| > {
| > size_t i;
| > for (i = 0; i < vm->def->ndisks; i++)
| > - if (vm->def->disks[i]->mirror)
| > + if (vm->def->disks[i]->blockjob)
| > return true;
| > return false;
| > }
| > diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
| > index bceb2d7..32674f3 100644
| > --- a/src/conf/domain_conf.h
| > +++ b/src/conf/domain_conf.h
| > @@ -2645,7 +2645,7 @@ int virDomainDiskSourceParse(xmlNodePtr node,
| > xmlXPathContextPtr ctxt,
| > virStorageSourcePtr src);
| >
| > -bool virDomainHasDiskMirror(virDomainObjPtr vm);
| > +bool virDomainHasBlockjob(virDomainObjPtr vm);
| >
| > int virDomainNetFindIdx(virDomainDefPtr def, virDomainNetDefPtr net);
| > virDomainNetDefPtr virDomainNetFind(virDomainDefPtr def, const char
| > *device);
| > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
| > index 33222f0..9ebaf4a 100644
| > --- a/src/libvirt_private.syms
| > +++ b/src/libvirt_private.syms
| > @@ -297,7 +297,7 @@ virDomainGraphicsTypeFromString;
| > virDomainGraphicsTypeToString;
| > virDomainGraphicsVNCSharePolicyTypeFromString;
| > virDomainGraphicsVNCSharePolicyTypeToString;
| > -virDomainHasDiskMirror;
| > +virDomainHasBlockjob;
| > virDomainHasNet;
| > virDomainHostdevCapsTypeToString;
| > virDomainHostdevDefAlloc;
| > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
| > index 80a3d77..51e302f 100644
| > --- a/src/qemu/qemu_driver.c
| > +++ b/src/qemu/qemu_driver.c
| > @@ -7398,7 +7398,7 @@ static virDomainPtr
| > qemuDomainDefineXMLFlags(virConnectPtr conn, const char *xml
| >
| > virObjectRef(vm);
| > def = NULL;
| > - if (virDomainHasDiskMirror(vm)) {
| > + if (virDomainHasBlockjob(vm)) {
| > virReportError(VIR_ERR_BLOCK_COPY_ACTIVE, "%s",
| > _("domain has active block job"));
| > virDomainObjAssignDef(vm, NULL, false, NULL);

| This code here has two effects:

| 1) if the VM is started, this is to forbid defining it back
| 2) if the VM is not started it forbids defining one with a disk mirror
| element

| Additionally this check (in the existing form) is wrong since the active
| block copy job should not interlock domain definition.

| Whith this patch the check would be even more wrong as it would disallow
| changes to the persistent defintion if any block job was active.
Understood 

| This problem needs to be fixed separately, before attempting such
| change.
| > @@ -14583,7 +14583,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
| > "%s", _("domain is marked for auto destroy"));
| > goto cleanup;
| > }
| > - if (virDomainHasDiskMirror(vm)) {
| > + if (virDomainHasBlockjob(vm)) {
| > virReportError(VIR_ERR_BLOCK_COPY_ACTIVE, "%s",
| > _("domain has active block job"));
| > goto cleanup;

| I think we should reconsider whether we want to block snapshots by a
| single copy job or we want to do the check in a more granular (per-disk)
Yes, it's better check per disk. 

| fashion.

| > @@ -15245,7 +15245,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr
| > snapshot,
| > if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
| > goto cleanup;
| >
| > - if (virDomainHasDiskMirror(vm)) {
| > + if (virDomainHasBlockjob(vm)) {
| > virReportError(VIR_ERR_BLOCK_COPY_ACTIVE, "%s",
| > _("domain has active block job"));
| > goto cleanup;

| Here it's questionable whether combining the check for

| > diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
| > index d34bb02..27a76ec 100644
| > --- a/src/qemu/qemu_migration.c
| > +++ b/src/qemu/qemu_migration.c
| > @@ -1977,7 +1977,7 @@ qemuMigrationIsAllowed(virQEMUDriverPtr driver,
| > virDomainObjPtr vm,
| >
| > }
| >
| > - if (virDomainHasDiskMirror(vm)) {
| > + if (virDomainHasBlockjob(vm)) {
| > virReportError(VIR_ERR_OPERATION_INVALID, "%s",
| > _("domain has an active block job"));
| > return false;

| Here it should be okay.

| > --

| I'll take this patch over and use it along when fixing the rest of the
| problems I've pointed out here.
OK, thanks 

| Peter

-- 
Regards 
shyu 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150326/533fc04c/attachment-0001.htm>


More information about the libvir-list mailing list