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

[Shanzhi Yu <shyu@xxxxxxxxxx>]

---

From: "Peter Krempa" <pkrempa@xxxxxxxxxx>
To: "Shanzhi Yu" <shyu@xxxxxxxxxx>
Cc: libvir-list@xxxxxxxxxx
Sent: Wednesday, March 25, 2015 11:24:21 PM
Subject: Re: [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@xxxxxxxxxx>
> ---
>  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

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list