qemu: check iotune params same for all disk in group

Currently it is possible to start a domain which have disks
in same iotune group and at the same time having different iotune
params. Both params set are passed to qemu in command line and the one
that is passed later down command line is get actually set.
Let's prohibit such configurations.

Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index daa145b493b2d802510c429652e4ef43bc2f14f4..2443e408ed8a6d00f537bd27cf6e08c81790c329 100644 (file)
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -31795,3 +31795,29 @@ virDomainBlockIoTuneInfoCopy(const virDomainBlockIoTuneInfo *src,
     *dst = *src;
     dst->group_name = g_strdup(src->group_name);
 }
+
+
+bool
+virDomainBlockIoTuneInfoEqual(const virDomainBlockIoTuneInfo *a,
+                              const virDomainBlockIoTuneInfo *b)
+{
+    return a->total_bytes_sec == b->total_bytes_sec &&
+        a->read_bytes_sec == b->read_bytes_sec &&
+        a->write_bytes_sec == b->write_bytes_sec &&
+        a->total_iops_sec == b->total_iops_sec &&
+        a->read_iops_sec == b->read_iops_sec &&
+        a->write_iops_sec == b->write_iops_sec &&
+        a->total_bytes_sec_max == b->total_bytes_sec_max &&
+        a->read_bytes_sec_max == b->read_bytes_sec_max &&
+        a->write_bytes_sec_max == b->write_bytes_sec_max &&
+        a->total_iops_sec_max == b->total_iops_sec_max &&
+        a->read_iops_sec_max == b->read_iops_sec_max &&
+        a->write_iops_sec_max == b->write_iops_sec_max &&
+        a->size_iops_sec == b->size_iops_sec &&
+        a->total_bytes_sec_max_length == b->total_bytes_sec_max_length &&
+        a->read_bytes_sec_max_length == b->read_bytes_sec_max_length &&
+        a->write_bytes_sec_max_length == b->write_bytes_sec_max_length &&
+        a->total_iops_sec_max_length == b->total_iops_sec_max_length &&
+        a->read_iops_sec_max_length == b->read_iops_sec_max_length &&
+        a->write_iops_sec_max_length == b->write_iops_sec_max_length;
+}

diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index adfa04316981e7e1239acf4009202ceac6d4830a..5feb70938e40b53ea9630c2b89991c76becdd947 100644 (file)
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -489,7 +489,8 @@ struct _virDomainBlockIoTuneInfo {
     unsigned long long total_iops_sec_max_length;
     unsigned long long read_iops_sec_max_length;
     unsigned long long write_iops_sec_max_length;
-    /* Don't forget to update virDomainBlockIoTuneInfoCopy. */
+    /* Don't forget to update virDomainBlockIoTuneInfoCopy and
+     * virDomainBlockIoTuneInfoEqual. */
 };
 
 
@@ -3715,3 +3716,7 @@ virDomainBlockIoTuneInfoHasAny(const virDomainBlockIoTuneInfo *iotune);
 void
 virDomainBlockIoTuneInfoCopy(const virDomainBlockIoTuneInfo *src,
                              virDomainBlockIoTuneInfoPtr dst);
+
+bool
+virDomainBlockIoTuneInfoEqual(const virDomainBlockIoTuneInfo *a,
+                              const virDomainBlockIoTuneInfo *b);

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index c7158f09596e4daa81b5137f536a0963f7be39d6..970f412527339b20bfcae46ebfb985e72cfebd6a 100644 (file)
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -227,6 +227,7 @@ virDomainActualNetDefValidate;
 virDomainBlockedReasonTypeFromString;
 virDomainBlockedReasonTypeToString;
 virDomainBlockIoTuneInfoCopy;
+virDomainBlockIoTuneInfoEqual;
 virDomainBlockIoTuneInfoHasAny;
 virDomainBlockIoTuneInfoHasBasic;
 virDomainBlockIoTuneInfoHasMax;

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index f4b7251aab2bc06704c69b47094437d7bf52452c..1668c8566660fc30d7189b6c41de342079cd830e 100644 (file)
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1155,6 +1155,7 @@ qemuDiskConfigBlkdeviotuneEnabled(virDomainDiskDefPtr disk)
  */
 static int
 qemuCheckDiskConfigBlkdeviotune(virDomainDiskDefPtr disk,
+                                const virDomainDef *def,
                                 virQEMUCapsPtr qemuCaps)
 {
     /* group_name by itself is ignored by qemu */
@@ -1166,6 +1167,28 @@ qemuCheckDiskConfigBlkdeviotune(virDomainDiskDefPtr disk,
         return -1;
     }
 
+    /* checking def here is only for calling from tests */
+    if (disk->blkdeviotune.group_name) {
+        size_t i;
+
+        for (i = 0; i < def->ndisks; i++) {
+            virDomainDiskDefPtr d = def->disks[i];
+
+            if (STREQ(d->dst, disk->dst) ||
+                STRNEQ_NULLABLE(d->blkdeviotune.group_name,
+                                disk->blkdeviotune.group_name))
+                continue;
+
+            if (!virDomainBlockIoTuneInfoEqual(&d->blkdeviotune,
+                                               &disk->blkdeviotune)) {
+                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                               _("different iotunes for disks %s and %s"),
+                               disk->dst, d->dst);
+                return -1;
+            }
+        }
+    }
+
     if (disk->blkdeviotune.total_bytes_sec > QEMU_BLOCK_IOTUNE_MAX ||
         disk->blkdeviotune.read_bytes_sec > QEMU_BLOCK_IOTUNE_MAX ||
         disk->blkdeviotune.write_bytes_sec > QEMU_BLOCK_IOTUNE_MAX ||
@@ -1228,9 +1251,10 @@ qemuCheckDiskConfigBlkdeviotune(virDomainDiskDefPtr disk,
  */
 int
 qemuCheckDiskConfig(virDomainDiskDefPtr disk,
+                    const virDomainDef *def,
                     virQEMUCapsPtr qemuCaps)
 {
-    if (qemuCheckDiskConfigBlkdeviotune(disk, qemuCaps) < 0)
+    if (qemuCheckDiskConfigBlkdeviotune(disk, def, qemuCaps) < 0)
         return -1;
 
     if (disk->wwn) {
@@ -1728,6 +1752,7 @@ qemuBuildDiskFrontendAttributes(virDomainDiskDefPtr disk,
 
 static char *
 qemuBuildDriveStr(virDomainDiskDefPtr disk,
+                  const virDomainDef *def,
                   virQEMUCapsPtr qemuCaps)
 {
     g_auto(virBuffer) opt = VIR_BUFFER_INITIALIZER;
@@ -1754,7 +1779,7 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk,
         }
 
         /* if we are using -device this will be checked elsewhere */
-        if (qemuCheckDiskConfig(disk, qemuCaps) < 0)
+        if (qemuCheckDiskConfig(disk, def, qemuCaps) < 0)
             return NULL;
 
         virBufferAsprintf(&opt, "if=%s",
@@ -1896,7 +1921,7 @@ qemuBuildDiskDeviceStr(const virDomainDef *def,
     g_autofree char *scsiVPDDeviceId = NULL;
     int controllerModel;
 
-    if (qemuCheckDiskConfig(disk, qemuCaps) < 0)
+    if (qemuCheckDiskConfig(disk, def, qemuCaps) < 0)
         return NULL;
 
     if (!qemuDomainCheckCCWS390AddressSupport(def, &disk->info, qemuCaps, disk->dst))
@@ -2401,6 +2426,7 @@ qemuBuildBlockStorageSourceAttachDataCommandline(virCommandPtr cmd,
 static int
 qemuBuildDiskSourceCommandLine(virCommandPtr cmd,
                                virDomainDiskDefPtr disk,
+                               const virDomainDef *def,
                                virQEMUCapsPtr qemuCaps)
 {
     g_autoptr(qemuBlockStorageSourceChainData) data = NULL;
@@ -2420,7 +2446,7 @@ qemuBuildDiskSourceCommandLine(virCommandPtr cmd,
             !(copyOnReadProps = qemuBlockStorageGetCopyOnReadProps(disk)))
             return -1;
     } else {
-        if (!(data = qemuBuildStorageSourceChainAttachPrepareDrive(disk, qemuCaps)))
+        if (!(data = qemuBuildStorageSourceChainAttachPrepareDrive(disk, def, qemuCaps)))
             return -1;
     }
 
@@ -2450,7 +2476,7 @@ qemuBuildDiskCommandLine(virCommandPtr cmd,
 {
     g_autofree char *optstr = NULL;
 
-    if (qemuBuildDiskSourceCommandLine(cmd, disk, qemuCaps) < 0)
+    if (qemuBuildDiskSourceCommandLine(cmd, disk, def, qemuCaps) < 0)
         return -1;
 
     if (!qemuDiskBusNeedsDriveArg(disk->bus)) {
@@ -10164,6 +10190,7 @@ qemuBuildHotpluggableCPUProps(const virDomainVcpuDef *vcpu)
  */
 qemuBlockStorageSourceAttachDataPtr
 qemuBuildStorageSourceAttachPrepareDrive(virDomainDiskDefPtr disk,
+                                         const virDomainDef *def,
                                          virQEMUCapsPtr qemuCaps)
 {
     g_autoptr(qemuBlockStorageSourceAttachData) data = NULL;
@@ -10171,7 +10198,7 @@ qemuBuildStorageSourceAttachPrepareDrive(virDomainDiskDefPtr disk,
     if (VIR_ALLOC(data) < 0)
         return NULL;
 
-    if (!(data->driveCmd = qemuBuildDriveStr(disk, qemuCaps)) ||
+    if (!(data->driveCmd = qemuBuildDriveStr(disk, def, qemuCaps)) ||
         !(data->driveAlias = qemuAliasDiskDriveFromDisk(disk)))
         return NULL;
 
@@ -10229,6 +10256,7 @@ qemuBuildStorageSourceAttachPrepareCommon(virStorageSourcePtr src,
  */
 qemuBlockStorageSourceChainDataPtr
 qemuBuildStorageSourceChainAttachPrepareDrive(virDomainDiskDefPtr disk,
+                                              const virDomainDef *def,
                                               virQEMUCapsPtr qemuCaps)
 {
     g_autoptr(qemuBlockStorageSourceAttachData) elem = NULL;
@@ -10237,7 +10265,7 @@ qemuBuildStorageSourceChainAttachPrepareDrive(virDomainDiskDefPtr disk,
     if (VIR_ALLOC(data) < 0)
         return NULL;
 
-    if (!(elem = qemuBuildStorageSourceAttachPrepareDrive(disk, qemuCaps)))
+    if (!(elem = qemuBuildStorageSourceAttachPrepareDrive(disk, def, qemuCaps)))
         return NULL;
 
     if (qemuBuildStorageSourceAttachPrepareCommon(disk->src, elem, qemuCaps) < 0)

diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
index 786991fd3de12a887839dd5ed0ba1b7953dc5d78..d7fdba99420aa5306254633a83733808c0729e68 100644 (file)
--- a/src/qemu/qemu_command.h
+++ b/src/qemu/qemu_command.h
@@ -105,6 +105,7 @@ bool qemuDiskBusNeedsDriveArg(int bus);
 
 qemuBlockStorageSourceAttachDataPtr
 qemuBuildStorageSourceAttachPrepareDrive(virDomainDiskDefPtr disk,
+                                         const virDomainDef *def,
                                          virQEMUCapsPtr qemuCaps);
 int
 qemuBuildStorageSourceAttachPrepareCommon(virStorageSourcePtr src,
@@ -114,6 +115,7 @@ qemuBuildStorageSourceAttachPrepareCommon(virStorageSourcePtr src,
 
 qemuBlockStorageSourceChainDataPtr
 qemuBuildStorageSourceChainAttachPrepareDrive(virDomainDiskDefPtr disk,
+                                              const virDomainDef *def,
                                               virQEMUCapsPtr qemuCaps);
 
 
@@ -199,6 +201,7 @@ bool
 qemuDiskConfigBlkdeviotuneEnabled(virDomainDiskDefPtr disk);
 
 int qemuCheckDiskConfig(virDomainDiskDefPtr disk,
+                        const virDomainDef *def,
                         virQEMUCapsPtr qemuCaps);
 
 bool

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 51bdc74e2d98f3527ef3d93d91b88bb0a4aca443..146be25c8b9855e401900de77ef92bfd2d04ceec 100644 (file)
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -8136,7 +8136,7 @@ qemuDomainAttachDeviceConfig(virDomainDefPtr vmdef,
         }
         if (virDomainDiskTranslateSourcePool(disk) < 0)
             return -1;
-        if (qemuCheckDiskConfig(disk, NULL) < 0)
+        if (qemuCheckDiskConfig(disk, vmdef, NULL) < 0)
             return -1;
         if (qemuCheckDiskConfigAgainstDomain(vmdef, disk) < 0)
             return -1;

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 31d455505b239278744c0e4642398c205846ed91..83cc5468c289c315c32a5386c05a468e0f3cf922 100644 (file)
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -700,7 +700,7 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver,
                                                                       priv->qemuCaps)))
             goto cleanup;
     } else {
-        if (!(data = qemuBuildStorageSourceChainAttachPrepareDrive(disk,
+        if (!(data = qemuBuildStorageSourceChainAttachPrepareDrive(disk, vm->def,
                                                                    priv->qemuCaps)))
             goto cleanup;
     }

diff --git a/tests/qemublocktest.c b/tests/qemublocktest.c
index 7ff6a6b17bea240096886ca024677345e89e450f..3076dc9645df7f0bdbccbf2f439a5506ae796e0f 100644 (file)
--- a/tests/qemublocktest.c
+++ b/tests/qemublocktest.c
@@ -185,6 +185,7 @@ static int
 testQemuDiskXMLToProps(const void *opaque)
 {
     struct testQemuDiskXMLToJSONData *data = (void *) opaque;
+    g_autoptr(virDomainDef) vmdef = NULL;
     virDomainDiskDefPtr disk = NULL;
     virStorageSourcePtr n;
     virJSONValuePtr formatProps = NULL;
@@ -204,7 +205,11 @@ testQemuDiskXMLToProps(const void *opaque)
                                        VIR_DOMAIN_DEF_PARSE_STATUS)))
         goto cleanup;
 
-    if (qemuCheckDiskConfig(disk, data->qemuCaps) < 0 ||
+    if (!(vmdef = virDomainDefNew()) ||
+        virDomainDiskInsert(vmdef, disk) < 0)
+        goto cleanup;
+
+    if (qemuCheckDiskConfig(disk, vmdef, data->qemuCaps) < 0 ||
         qemuDomainDeviceDefValidateDisk(disk, data->qemuCaps) < 0) {
         VIR_TEST_VERBOSE("invalid configuration for disk");
         goto cleanup;
@@ -242,7 +247,6 @@ testQemuDiskXMLToProps(const void *opaque)
  cleanup:
     virJSONValueFree(formatProps);
     virJSONValueFree(storageProps);
-    virDomainDiskDefFree(disk);
     VIR_FREE(xmlpath);
     VIR_FREE(xmlstr);
     return ret;