Re: [Qemu-devel] [RFC 2/3] QMP: rate limit BLOCK_IO_ERROR

[Luiz Capitulino]

On Mon, 11 Aug 2014 09:17:19 +0100
"Daniel P. Berrange" <address@hidden> wrote:

> On Wed, Jul 23, 2014 at 09:17:17AM -0400, Luiz Capitulino wrote:
> > This event has the same characteristics of the other rate-limited
> > events, mainly we can emit dozens of it. Rate limit it then.
> > 
> > Signed-off-by: Luiz Capitulino <address@hidden>
> > ---
> >  monitor.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/monitor.c b/monitor.c
> > index 5bc70a6..33abe6c 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -589,6 +589,7 @@ static void monitor_qapi_event_init(void)
> >      monitor_qapi_event_throttle(QAPI_EVENT_QUORUM_REPORT_BAD, 1000);
> >      monitor_qapi_event_throttle(QAPI_EVENT_QUORUM_FAILURE, 1000);
> >      monitor_qapi_event_throttle(QAPI_EVENT_VSERPORT_CHANGE, 1000);
> > +    monitor_qapi_event_throttle(QAPI_EVENT_BLOCK_IO_ERROR, 1000);
> 
> 
> The rate limiting code only rate limits at the granularity of
> individual event types. If there is context sensitive data associated
> with events then the rate limiting will cause problems for applications
> tracking the events.
> 
> eg consider with the simpler RTC CHANGE events if we get
> 
>    QAPI_EVENT_RTC_CHANGE offset=30
>    QAPI_EVENT_RTC_CHANGE offset=700
>    QAPI_EVENT_RTC_CHANGE offset=340
> 
> then rate limiting will mean that the application only receives
> 
>    QAPI_EVENT_RTC_CHANGE offset=340
> 
> This is fine because the application will always end up with a correct
> view of the current system state.
> 
> 
> For the BLOCK IO ERROR events this does not work because the events are
> device and operation specific.
> 
>   QAPI_EVENT_BLOCK_IO_ERROR dev=ide0-hd1 op=read action=stop
>   QAPI_EVENT_BLOCK_IO_ERROR dev=scsi1-hd2 op=write action=stop
>   QAPI_EVENT_BLOCK_IO_ERROR dev=ide0-hd1 op=write action=stop
> 
> with throttling the app wll only receive
> 
>   QAPI_EVENT_BLOCK_IO_ERROR dev=ide0-hd1 op=write action=stop
> 
> which means it will have an *incorrect* view of the system state because
> the info about  scsi1-hd2 is irretrievably lost, likewise info about the
> read operation of ide0-hd1.

You're completely right, of course. Thanks for reviewing!

I think I'll just drop this patch for now.

> 
> If you want to throttle BLOCK IO ERROR events, then you need to make the
> monitor throttling more intelligent, so that it hashes on all the contextual
> state. In this case you'd have to throttle based on (event, dev, op) to get
> correct application behaviour.
> 
> Regards,
> Daniel