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

[Markus Armbruster]

"Daniel P. Berrange" <address@hidden> writes:

> 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.

... since the intermediate states no longer matter.

> 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.

Even when the event is lost, the information should not be lost.  There
should be a way to poll for it (libvirt needs that anyway, to cope with
possible event loss during a libvirt restart).

> 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.

I think there's more than one  to skin this cat:

1. Don't throttle.  Client can rely on events as long as it keeps the
   QMP connection alive.  Client should poll after establishing the QMP
   connection.

2. Throttle more smartly, so that events only get dropped when they're
   semantically superseded.  I figure that's what you proposed in your
   last paragraph.

3. Throttle, but accumulate the information carried by the event, i.e.
   any dropped events' data is sent with the next non-dropped event.

4. Throttle without smarts or accumulation.

   a. The event's additional information may be incomplete, thus
      worthless.  Client needs too poll after getting an event.

   b. Add a flag "throttling has dropped some events".  The additional
      information is incomplete when the flag is set.  Client needs to
      poll then.

Backward compatibility considerations may narrow our choice.