[Qemu-devel] [PATCH for-2.7 0/2] Don't allow burst limits to be lower than the normal limits

[Qemu-devel] [PATCH for-2.7 0/2] Don't allow burst limits to be lower than the normal limits

[Alberto Garcia]

Hello,

Gu Nini found this problem and reported it in

https://bugzilla.redhat.com/show_bug.cgi?id=1355665

When setting the throttling configuration, the burst limits can be
lower than the normal limits. This does not making any sense and
behaves oddly, so let's forbid it.

Berto

Alberto Garcia (2):
  throttle: Don't allow burst limits to be lower than the normal limits
  throttle: Test burst limits lower than the normal limits

 tests/test-throttle.c | 8 ++++++++
 util/throttle.c       | 6 ++++++
 2 files changed, 14 insertions(+)

-- 
2.8.1

[Qemu-devel] [PATCH for-2.7 1/2] throttle: Don't allow burst limits to be lower than the normal limits

[Alberto Garcia]

Setting FOO_max to a value that is lower than FOO does not make
sense, and it produces odd results depending on the value of
FOO_max_length. Although the user should not set that configuration
in the first place it's better to reject it explicitly.

https://bugzilla.redhat.com/show_bug.cgi?id=1355665

Signed-off-by: Alberto Garcia <berto@igalia.com>
Reported-by: Gu Nini <ngu@redhat.com>
---
 util/throttle.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/util/throttle.c b/util/throttle.c
index 654f95c..7a5c619 100644
--- a/util/throttle.c
+++ b/util/throttle.c
@@ -348,6 +348,12 @@ bool throttle_is_valid(ThrottleConfig *cfg, Error **errp)
                        " bps/iops values");
             return false;
         }
+
+        if (cfg->buckets[i].max && cfg->buckets[i].max < cfg->buckets[i].avg) {
+            error_setg(errp, "if bps_max/iops_max is set it cannot be lower"
+                       " than the bps/iops values");
+            return false;
+        }
     }
 
     return true;
-- 
2.8.1

Re: [Qemu-devel] [PATCH for-2.7 1/2] throttle: Don't allow burst limits to be lower than the normal limits

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1314 bytes --]

On 07/27/2016 04:16 PM, Alberto Garcia wrote:
> Setting FOO_max to a value that is lower than FOO does not make
> sense, and it produces odd results depending on the value of
> FOO_max_length. Although the user should not set that configuration
> in the first place it's better to reject it explicitly.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1355665
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> Reported-by: Gu Nini <ngu@redhat.com>
> ---
>  util/throttle.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/util/throttle.c b/util/throttle.c
> index 654f95c..7a5c619 100644
> --- a/util/throttle.c
> +++ b/util/throttle.c
> @@ -348,6 +348,12 @@ bool throttle_is_valid(ThrottleConfig *cfg, Error **errp)
>                         " bps/iops values");
>              return false;
>          }
> +
> +        if (cfg->buckets[i].max && cfg->buckets[i].max < cfg->buckets[i].avg) {
> +            error_setg(errp, "if bps_max/iops_max is set it cannot be lower"
> +                       " than the bps/iops values");

Feels a bit long, I'd be fine with "bps_max/iops_max must not be lower
than bps/iops values".  But the idea makes sense.


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH for-2.7 1/2] throttle: Don't allow burst limits to be lower than the normal limits

[Alberto Garcia]

On Thu 28 Jul 2016 12:33:51 AM CEST, Eric Blake <eblake@redhat.com> wrote:
>> +        if (cfg->buckets[i].max && cfg->buckets[i].max < cfg->buckets[i].avg) {
>> +            error_setg(errp, "if bps_max/iops_max is set it cannot be lower"
>> +                       " than the bps/iops values");
>
> Feels a bit long, I'd be fine with "bps_max/iops_max must not be lower
> than bps/iops values".  But the idea makes sense.

That's not strictly true since the default value of bps_max is 0 (unless
we forbid setting it explicitly to 0 as well).

Berto

Re: [Qemu-devel] [PATCH for-2.7 1/2] throttle: Don't allow burst limits to be lower than the normal limits

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1018 bytes --]

On 07/27/2016 04:45 PM, Alberto Garcia wrote:
> On Thu 28 Jul 2016 12:33:51 AM CEST, Eric Blake <eblake@redhat.com> wrote:
>>> +        if (cfg->buckets[i].max && cfg->buckets[i].max < cfg->buckets[i].avg) {
>>> +            error_setg(errp, "if bps_max/iops_max is set it cannot be lower"
>>> +                       " than the bps/iops values");
>>
>> Feels a bit long, I'd be fine with "bps_max/iops_max must not be lower
>> than bps/iops values".  But the idea makes sense.
> 
> That's not strictly true since the default value of bps_max is 0 (unless
> we forbid setting it explicitly to 0 as well).

The message can only be emitted if bps_max was set.  If bps_max is its
default value of 0 (or even if the user explicitly set it to zero), then
this message won't appear.  Therefore, telling the user that "if this is
set" is redundant, because the error is only issued when it IS set.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH for-2.7 2/2] throttle: Test burst limits lower than the normal limits

[Alberto Garcia]

This checks that making FOO_max lower than FOO is not allowed.

We could also forbid having FOO_max == FOO, but that doesn't have
any odd side effects and it would require us to update several other
tests, so let's keep it simple.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 tests/test-throttle.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/tests/test-throttle.c b/tests/test-throttle.c
index afe094b..363b59a 100644
--- a/tests/test-throttle.c
+++ b/tests/test-throttle.c
@@ -394,6 +394,14 @@ static void test_max_is_missing_limit(void)
         cfg.buckets[i].max = 0;
         cfg.buckets[i].avg = 100;
         g_assert(throttle_is_valid(&cfg, NULL));
+
+        cfg.buckets[i].max = 30;
+        cfg.buckets[i].avg = 100;
+        g_assert(!throttle_is_valid(&cfg, NULL));
+
+        cfg.buckets[i].max = 100;
+        cfg.buckets[i].avg = 100;
+        g_assert(throttle_is_valid(&cfg, NULL));
     }
 }
 
-- 
2.8.1