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>

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;
+}

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);

src/libvirt_private.syms

@@ -227,6 +227,7 @@ virDomainActualNetDefValidate;
 virDomainBlockedReasonTypeFromString;
 virDomainBlockedReasonTypeToString;
 virDomainBlockIoTuneInfoCopy;
+virDomainBlockIoTuneInfoEqual;
 virDomainBlockIoTuneInfoHasAny;
 virDomainBlockIoTuneInfoHasBasic;
 virDomainBlockIoTuneInfoHasMax;

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)

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

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;

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;
     }

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;