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

2023-11-16 Thread Vladimir Sementsov-Ogievskiy 

On 15.11.23 16:46, Markus Armbruster wrote:

Vladimir Sementsov-Ogievskiy  writes:


From: Leonid Kaplan

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

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

2023-11-15 Thread Markus Armbruster 
Vladimir Sementsov-Ogievskiy  writes:

> From: Leonid Kaplan 
>
> 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 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  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

2023-11-09 Thread 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 

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 
Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  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

2023-11-08 Thread Eric Blake 
On Tue, Nov 07, 2023 at 11:58:42AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> From: Leonid Kaplan 
> 
> 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 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  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