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