[PATCH] block-backend: per-device throttling of BLOCK_IO_ERROR reports

[PATCH] block-backend: per-device throttling of BLOCK_IO_ERROR reports

[Vladimir Sementsov-Ogievskiy]

From: Leonid Kaplan <xeor@yandex-team.ru>

BLOCK_IO_ERROR events comes from guest, so we must throttle them.
We still want per-device throttling, so let's use device id as a key.

Signed-off-by: Leonid Kaplan <xeor@yandex-team.ru>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 monitor/monitor.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/monitor/monitor.c b/monitor/monitor.c
index 01ede1babd..ad0243e9d7 100644
--- a/monitor/monitor.c
+++ b/monitor/monitor.c
@@ -309,6 +309,7 @@ int error_printf_unless_qmp(const char *fmt, ...)
 static MonitorQAPIEventConf monitor_qapi_event_conf[QAPI_EVENT__MAX] = {
     /* Limit guest-triggerable events to 1 per second */
     [QAPI_EVENT_RTC_CHANGE]        = { 1000 * SCALE_MS },
+    [QAPI_EVENT_BLOCK_IO_ERROR]    = { 1000 * SCALE_MS },
     [QAPI_EVENT_WATCHDOG]          = { 1000 * SCALE_MS },
     [QAPI_EVENT_BALLOON_CHANGE]    = { 1000 * SCALE_MS },
     [QAPI_EVENT_QUORUM_REPORT_BAD] = { 1000 * SCALE_MS },
@@ -498,6 +499,10 @@ static unsigned int qapi_event_throttle_hash(const void *key)
         hash += g_str_hash(qdict_get_str(evstate->data, "qom-path"));
     }
 
+    if (evstate->event == QAPI_EVENT_BLOCK_IO_ERROR) {
+        hash += g_str_hash(qdict_get_str(evstate->data, "device"));
+    }
+
     return hash;
 }
 
@@ -525,6 +530,11 @@ static gboolean qapi_event_throttle_equal(const void *a, const void *b)
                        qdict_get_str(evb->data, "qom-path"));
     }
 
+    if (eva->event == QAPI_EVENT_BLOCK_IO_ERROR) {
+        return !strcmp(qdict_get_str(eva->data, "device"),
+                       qdict_get_str(evb->data, "device"));
+    }
+
     return TRUE;
 }
 
-- 
2.34.1

Re: [PATCH] block-backend: per-device throttling of BLOCK_IO_ERROR reports

[Eric Blake]

On Tue, Nov 07, 2023 at 11:58:42AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> From: Leonid Kaplan <xeor@yandex-team.ru>
> 
> BLOCK_IO_ERROR events comes from guest, so we must throttle them.
> We still want per-device throttling, so let's use device id as a key.
> 
> Signed-off-by: Leonid Kaplan <xeor@yandex-team.ru>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>  monitor/monitor.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/monitor/monitor.c b/monitor/monitor.c
> index 01ede1babd..ad0243e9d7 100644
> --- a/monitor/monitor.c
> +++ b/monitor/monitor.c
> @@ -309,6 +309,7 @@ int error_printf_unless_qmp(const char *fmt, ...)
>  static MonitorQAPIEventConf monitor_qapi_event_conf[QAPI_EVENT__MAX] = {
>      /* Limit guest-triggerable events to 1 per second */
>      [QAPI_EVENT_RTC_CHANGE]        = { 1000 * SCALE_MS },
> +    [QAPI_EVENT_BLOCK_IO_ERROR]    = { 1000 * SCALE_MS },
>      [QAPI_EVENT_WATCHDOG]          = { 1000 * SCALE_MS },
>      [QAPI_EVENT_BALLOON_CHANGE]    = { 1000 * SCALE_MS },
>      [QAPI_EVENT_QUORUM_REPORT_BAD] = { 1000 * SCALE_MS },
> @@ -498,6 +499,10 @@ static unsigned int qapi_event_throttle_hash(const void *key)
>          hash += g_str_hash(qdict_get_str(evstate->data, "qom-path"));
>      }
>  
> +    if (evstate->event == QAPI_EVENT_BLOCK_IO_ERROR) {
> +        hash += g_str_hash(qdict_get_str(evstate->data, "device"));

Wouldn't ^= be better than += for combining hashes?

> +    }
> +
>      return hash;
>  }
>  
> @@ -525,6 +530,11 @@ static gboolean qapi_event_throttle_equal(const void *a, const void *b)
>                         qdict_get_str(evb->data, "qom-path"));
>      }
>  
> +    if (eva->event == QAPI_EVENT_BLOCK_IO_ERROR) {
> +        return !strcmp(qdict_get_str(eva->data, "device"),
> +                       qdict_get_str(evb->data, "device"));
> +    }
> +

At any rate, the idea makes sense for me.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

Re: [PATCH] block-backend: per-device throttling of BLOCK_IO_ERROR reports

[Vladimir Sementsov-Ogievskiy]

On 09.11.23 00:22, Eric Blake wrote:
> On Tue, Nov 07, 2023 at 11:58:42AM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> From: Leonid Kaplan <xeor@yandex-team.ru>
>>
>> BLOCK_IO_ERROR events comes from guest, so we must throttle them.
>> We still want per-device throttling, so let's use device id as a key.
>>
>> Signed-off-by: Leonid Kaplan <xeor@yandex-team.ru>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> ---
>>   monitor/monitor.c | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/monitor/monitor.c b/monitor/monitor.c
>> index 01ede1babd..ad0243e9d7 100644
>> --- a/monitor/monitor.c
>> +++ b/monitor/monitor.c
>> @@ -309,6 +309,7 @@ int error_printf_unless_qmp(const char *fmt, ...)
>>   static MonitorQAPIEventConf monitor_qapi_event_conf[QAPI_EVENT__MAX] = {
>>       /* Limit guest-triggerable events to 1 per second */
>>       [QAPI_EVENT_RTC_CHANGE]        = { 1000 * SCALE_MS },
>> +    [QAPI_EVENT_BLOCK_IO_ERROR]    = { 1000 * SCALE_MS },
>>       [QAPI_EVENT_WATCHDOG]          = { 1000 * SCALE_MS },
>>       [QAPI_EVENT_BALLOON_CHANGE]    = { 1000 * SCALE_MS },
>>       [QAPI_EVENT_QUORUM_REPORT_BAD] = { 1000 * SCALE_MS },
>> @@ -498,6 +499,10 @@ static unsigned int qapi_event_throttle_hash(const void *key)
>>           hash += g_str_hash(qdict_get_str(evstate->data, "qom-path"));
>>       }
>>   
>> +    if (evstate->event == QAPI_EVENT_BLOCK_IO_ERROR) {
>> +        hash += g_str_hash(qdict_get_str(evstate->data, "device"));
> 
> Wouldn't ^= be better than += for combining hashes?

As I understand (after googling a bit), XOR is a kind of default, and is obviously better than OR and AND.
Adding is not bitwise and should be slower, but I don't think we care about it here. Still adding is better at least in fact that it doesn't map pairs of equal string all to zero hash.
Adding is preexisting in this function, I don't think we should change it now.

> 
>> +    }
>> +
>>       return hash;
>>   }
>>   
>> @@ -525,6 +530,11 @@ static gboolean qapi_event_throttle_equal(const void *a, const void *b)
>>                          qdict_get_str(evb->data, "qom-path"));
>>       }
>>   
>> +    if (eva->event == QAPI_EVENT_BLOCK_IO_ERROR) {
>> +        return !strcmp(qdict_get_str(eva->data, "device"),
>> +                       qdict_get_str(evb->data, "device"));
>> +    }
>> +
> 
> At any rate, the idea makes sense for me.
> 

-- 
Best regards,
Vladimir

Re: [PATCH] block-backend: per-device throttling of BLOCK_IO_ERROR reports

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:

> From: Leonid Kaplan <xeor@yandex-team.ru>
>
> BLOCK_IO_ERROR events comes from guest, so we must throttle them.

Really?  Can you describe how a guest can trigger these errors?

> We still want per-device throttling, so let's use device id as a key.
>
> Signed-off-by: Leonid Kaplan <xeor@yandex-team.ru>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>  monitor/monitor.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/monitor/monitor.c b/monitor/monitor.c
> index 01ede1babd..ad0243e9d7 100644
> --- a/monitor/monitor.c
> +++ b/monitor/monitor.c
> @@ -309,6 +309,7 @@ int error_printf_unless_qmp(const char *fmt, ...)
>  static MonitorQAPIEventConf monitor_qapi_event_conf[QAPI_EVENT__MAX] = {
>      /* Limit guest-triggerable events to 1 per second */
>      [QAPI_EVENT_RTC_CHANGE]        = { 1000 * SCALE_MS },
> +    [QAPI_EVENT_BLOCK_IO_ERROR]    = { 1000 * SCALE_MS },
>      [QAPI_EVENT_WATCHDOG]          = { 1000 * SCALE_MS },
>      [QAPI_EVENT_BALLOON_CHANGE]    = { 1000 * SCALE_MS },
>      [QAPI_EVENT_QUORUM_REPORT_BAD] = { 1000 * SCALE_MS },
> @@ -498,6 +499,10 @@ static unsigned int qapi_event_throttle_hash(const void *key)
>          hash += g_str_hash(qdict_get_str(evstate->data, "qom-path"));
>      }
>  
> +    if (evstate->event == QAPI_EVENT_BLOCK_IO_ERROR) {
> +        hash += g_str_hash(qdict_get_str(evstate->data, "device"));
> +    }
> +
>      return hash;
>  }
>  
> @@ -525,6 +530,11 @@ static gboolean qapi_event_throttle_equal(const void *a, const void *b)
>                         qdict_get_str(evb->data, "qom-path"));
>      }
>  
> +    if (eva->event == QAPI_EVENT_BLOCK_IO_ERROR) {
> +        return !strcmp(qdict_get_str(eva->data, "device"),
> +                       qdict_get_str(evb->data, "device"));
> +    }
> +
>      return TRUE;
>  }

Missing:

  diff --git a/qapi/block-core.json b/qapi/block-core.json
  index ca390c5700..32c2c2f030 100644
  --- a/qapi/block-core.json
  +++ b/qapi/block-core.json
  @@ -5559,6 +5559,8 @@
   # Note: If action is "stop", a STOP event will eventually follow the
   #     BLOCK_IO_ERROR event
   #
  +# Note: This event is rate-limited.
  +#
   # Since: 0.13
   #
   # Example:

Re: [PATCH] block-backend: per-device throttling of BLOCK_IO_ERROR reports

[Vladimir Sementsov-Ogievskiy]

On 15.11.23 16:46, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy<vsementsov@yandex-team.ru>  writes:
> 
>> From: Leonid Kaplan<xeor@yandex-team.ru>
>>
>> BLOCK_IO_ERROR events comes from guest, so we must throttle them.
> Really?  Can you describe how a guest can trigger these errors?

When disk (for examaple vhost-user) is unavailable, every Guest request may map into BLOCK_IO_ERROR, as I understand.

-- 
Best regards,
Vladimir