[PATCH v2 2/7] conf: expand iotune params if only group name is given

Michal Privoznik posted 7 patches 6 years ago
[PATCH v2 2/7] conf: expand iotune params if only group name is given
Posted by Michal Privoznik 6 years ago
From: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>

Currently, if only iotune group name is given for some disk and
no any params then later start of domain will fail. I guess it
will be convenient to allow such configuration if there is
another disk in the same iotune group with iotune params set. The
meaning is that the first disk have same iotunes and the latter.
Thus one can easily add a disk to iotune group - just add group
name parameter and no need to copy all the params.

Also let's expand iotunes params in the described case so we don't
need to refer to another disk to know iotunes and this will make
logic in many places simple.

Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/conf/domain_conf.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index c77901dfa0..0eeffb6ed0 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -5110,6 +5110,31 @@ virDomainRNGDefPostParse(virDomainRNGDefPtr rng)
 }
 
 
+static void
+virDomainDiskExpandGroupIoTune(virDomainDiskDefPtr disk,
+                               const virDomainDef *def)
+{
+    size_t i;
+
+    if (!disk->blkdeviotune.group_name ||
+        virDomainBlockIoTuneInfoHasAny(&disk->blkdeviotune))
+        return;
+
+    for (i = 0; i < def->ndisks; i++) {
+        virDomainDiskDefPtr d = def->disks[i];
+        char *tmp = disk->blkdeviotune.group_name;
+
+        if (STRNEQ_NULLABLE(disk->blkdeviotune.group_name, d->blkdeviotune.group_name) ||
+            !virDomainBlockIoTuneInfoHasAny(&d->blkdeviotune))
+            continue;
+
+        disk->blkdeviotune = d->blkdeviotune;
+        disk->blkdeviotune.group_name = tmp;
+        return;
+    }
+}
+
+
 static int
 virDomainDiskDefPostParse(virDomainDiskDefPtr disk,
                           const virDomainDef *def,
@@ -5155,6 +5180,8 @@ virDomainDiskDefPostParse(virDomainDiskDefPtr disk,
         return -1;
     }
 
+    virDomainDiskExpandGroupIoTune(disk, def);
+
     return 0;
 }
 
-- 
2.24.1

Re: [PATCH v2 2/7] conf: expand iotune params if only group name is given
Posted by Nikolay Shirokovskiy 6 years ago

On 29.01.2020 10:54, Michal Privoznik wrote:
> From: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
> 
> Currently, if only iotune group name is given for some disk and
> no any params then later start of domain will fail. I guess it
> will be convenient to allow such configuration if there is
> another disk in the same iotune group with iotune params set. The
> meaning is that the first disk have same iotunes and the latter.
> Thus one can easily add a disk to iotune group - just add group
> name parameter and no need to copy all the params.
> 
> Also let's expand iotunes params in the described case so we don't
> need to refer to another disk to know iotunes and this will make
> logic in many places simple.
> 
> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/conf/domain_conf.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index c77901dfa0..0eeffb6ed0 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -5110,6 +5110,31 @@ virDomainRNGDefPostParse(virDomainRNGDefPtr rng)
>  }
>  
>  
> +static void
> +virDomainDiskExpandGroupIoTune(virDomainDiskDefPtr disk,
> +                               const virDomainDef *def)
> +{
> +    size_t i;
> +
> +    if (!disk->blkdeviotune.group_name ||
> +        virDomainBlockIoTuneInfoHasAny(&disk->blkdeviotune))
> +        return;
> +
> +    for (i = 0; i < def->ndisks; i++) {
> +        virDomainDiskDefPtr d = def->disks[i];
> +        char *tmp = disk->blkdeviotune.group_name;
> +
> +        if (STRNEQ_NULLABLE(disk->blkdeviotune.group_name, d->blkdeviotune.group_name) ||
> +            !virDomainBlockIoTuneInfoHasAny(&d->blkdeviotune))
> +            continue;
> +
> +        disk->blkdeviotune = d->blkdeviotune;
> +        disk->blkdeviotune.group_name = tmp;

We can use virDomainBlockIoTuneInfoCopy here too. And it makes sense to add freeing of group_name
in the copy function then I guess. Everything else is fine.

Nikolay

> +        return;
> +    }
> +}
> +
> +
>  static int
>  virDomainDiskDefPostParse(virDomainDiskDefPtr disk,
>                            const virDomainDef *def,
> @@ -5155,6 +5180,8 @@ virDomainDiskDefPostParse(virDomainDiskDefPtr disk,
>          return -1;
>      }
>  
> +    virDomainDiskExpandGroupIoTune(disk, def);
> +
>      return 0;
>  }
>  
> 

Re: [PATCH v2 2/7] conf: expand iotune params if only group name is given
Posted by Michal Privoznik 6 years ago
On 1/29/20 9:37 AM, Nikolay Shirokovskiy wrote:
> 
> 

Pushed now.

Michal