Re: [PATCH 6/9] block/accounting: Emit BLOCK_IO_DELAY event

[Hanna Czenczek <hreitz@redhat.com>]

On 22.09.26 14:44, Kevin Wolf wrote:
> Am 31.08.2026 um 15:52 hat Hanna Czenczek geschrieben:
>> When a request finishes with a higher latency than a predefined
>> threshold, emit the BLOCK_IO_DELAY event.
>>
>> Note there would be an alternative, more precise solution: We could keep
>> all active cookies per BlockBackend in a list and repeatedly iterate
>> over it in a background coroutine (woken on a timer so it would wake
>> always exactly when the next request would time out, so it generally
>> stays asleep until there is actually a timeout). This way, we could emit
>> the event exactly when a request crosses the delay threshold, while it
>> is still running; and we could hypothetically even take actions like
>> pausing the VM until the request is done so the guest operating system
>> is shielded from extreme latency spikes.
>>
>> In practice, this is very complicated because latency cookies are
>> created and finalized all over the place, so it is very hard to
>> guarantee that every `block_acct_start()` is matched by the right
>> finalization to ensure that cookies are properly removed from the list
>> when they are done. Even if we fix all non-matching places now, there is
>> hardly a guarantee this will be kept in order in the future.
>>
>> So, for now, just implement the simpler solution of only notifying the
>> management layer when the request does complete, so VMs with high
>> latency spikes can at least be identified when they happen without
>> having to regularly check the latency histogram.
>>
>> Signed-off-by: Hanna Czenczek <hreitz@redhat.com>
> Checking only at request completion for now is fine with me; however,
> the documentation the previous patches added (And possibly their commit
> messages? Not sure any more.) suggests that the event is emitted while
> the request is still in flight. So these description need to change to
> be consistent with the actual behaviour.

I tried being a little bit ambiguous so that behavior could change in 
the future.

But now that you make me think about it, that was a terrible idea. I 
should fix it to reflect the current behavior and explicitly state that 
it might do something else, too, in the future. (*If* we think changing 
behavior is OK, then it should at least be explicit now.)

>>   include/block/accounting.h | 10 ++++++++-
>>   block/accounting.c         | 42 +++++++++++++++++++++++++++++++++++++-
>>   blockdev.c                 |  2 +-
>>   hw/block/block.c           |  2 +-
>>   4 files changed, 52 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/block/accounting.h b/include/block/accounting.h
>> index 025536239e6..ba3a6859cf4 100644
>> --- a/include/block/accounting.h
>> +++ b/include/block/accounting.h
>> @@ -92,6 +92,7 @@ struct BlockAcctStats {
>>       QSLIST_HEAD(, BlockAcctTimedStats) intervals;
>>       bool account_invalid;
>>       bool account_failed;
>> +    int64_t delay_threshold_ns;
> Why signed?

Because block/accounting.c uses int64_t for all latency values (except 
for block_acct_queue_depth()).

>>       BlockLatencyHistogram latency_histogram[BLOCK_MAX_IOTYPE];
>>   };
>>   
>> @@ -103,9 +104,16 @@ typedef struct BlockAcctCookie {
>>   } BlockAcctCookie;
>>   
>>   void block_acct_init(BlockBackend *blk, BlockAcctStats *stats);
>> +/**
>> + * Set up accounting for a block device in @stats.
>> + * @alert_ns specifies a latency so that if any request takes longer than that
>> + * threshold, a BLOCK_IO_DELAY event will be generated (when that request
>> + * completes). Pass 0 to disable.
>> + */
>>   bool block_acct_setup(BlockAcctStats *stats, enum OnOffAuto account_invalid,
>>                         enum OnOffAuto account_failed, uint32_t *stats_intervals,
>> -                      uint32_t num_stats_intervals, Error **errp);
>> +                      uint32_t num_stats_intervals, int64_t alert_ns,
>> +                      Error **errp);
>>   void block_acct_cleanup(BlockAcctStats *stats);
>>   void block_acct_add_interval(BlockAcctStats *stats, unsigned interval_length);
>>   BlockAcctTimedStats *block_acct_interval_next(BlockAcctStats *stats,
>> diff --git a/block/accounting.c b/block/accounting.c
>> index a74551d41f2..debf1924455 100644
>> --- a/block/accounting.c
>> +++ b/block/accounting.c
>> @@ -27,8 +27,10 @@
>>   #include "block/accounting.h"
>>   #include "block/block_int.h"
>>   #include "qemu/timer.h"
>> +#include "system/block-backend.h"
>>   #include "system/qtest.h"
>>   #include "qapi/error.h"
>> +#include "qapi/qapi-events-block.h"
>>   
>>   static QEMUClockType clock_type = QEMU_CLOCK_REALTIME;
>>   static const int qtest_latency_ns = NANOSECONDS_PER_SECOND / 1000;
>> @@ -62,9 +64,35 @@ static bool bool_from_onoffauto(OnOffAuto val, bool def)
>>       }
>>   }
>>   
>> +/**
>> + * Convert a BlockAcctType into its QAPI equivalent IoAccountingOperation.
>> + *
>> + * Must only be called for valid BlockAcctType values, i.e. specifically not for
>> + * `BLOCK_ACCT_NONE`.
>> + */
>> +static IoAccountingOperation block_acct_qapi_type(enum BlockAcctType type)
>> +{
>> +    switch (type) {
>> +    case BLOCK_ACCT_READ:
>> +        return IO_ACCOUNTING_OPERATION_READ;
>> +    case BLOCK_ACCT_WRITE:
>> +        return IO_ACCOUNTING_OPERATION_WRITE;
>> +    case BLOCK_ACCT_FLUSH:
>> +        return IO_ACCOUNTING_OPERATION_FLUSH;
>> +    case BLOCK_ACCT_ZONE_APPEND:
>> +        return IO_ACCOUNTING_OPERATION_ZONE_APPEND;
>> +    case BLOCK_ACCT_UNMAP:
>> +        return IO_ACCOUNTING_OPERATION_UNMAP;
>> +    case BLOCK_ACCT_NONE:
>> +    default:
>> +        g_assert_not_reached();
>> +    }
>> +}
> Wouldn't it be nice if QAPI could generate the shorter BlockAcctType
> with BLOCK_ACCT_* on the C side while still keeping the nicer
> IoAccountingOperation name externally? :-)
>
> (Not a request to change it now, but a wishlist item for Markus.)
>
> Though actually BLOCK_ACCT_* seems to already be possible with 'prefix'.
> Maybe we could live with the longer IoAccountingOperation type name
> everywhere and avoid having two separate enums?

Probably.

(If not, then IoAccountingOperation were not doing what it’s supposed to 
be doing.)

I’ll take a look.

>>   bool block_acct_setup(BlockAcctStats *stats, enum OnOffAuto account_invalid,
>>                         enum OnOffAuto account_failed, uint32_t *stats_intervals,
>> -                      uint32_t num_stats_intervals, Error **errp)
>> +                      uint32_t num_stats_intervals, int64_t alert_ns,
>> +                      Error **errp)
>>   {
>>       stats->account_invalid = bool_from_onoffauto(account_invalid,
>>                                                    stats->account_invalid);
>> @@ -79,6 +107,7 @@ bool block_acct_setup(BlockAcctStats *stats, enum OnOffAuto account_invalid,
>>               block_acct_add_interval(stats, stats_intervals[i]);
>>           }
>>       }
>> +    stats->delay_threshold_ns = alert_ns;
>>       return true;
>>   }
>>   
>> @@ -252,6 +281,17 @@ static void block_account_one_io(BlockAcctStats *stats, BlockAcctCookie *cookie,
>>           return;
>>       }
>>   
>> +    if (stats->delay_threshold_ns && latency_ns > stats->delay_threshold_ns) {
>> +        g_autofree char *dev_path = blk_get_attached_dev_path(stats->blk);
>> +        double latency = latency_ns / (double)NANOSECONDS_PER_SECOND;
> Wouldn't it be both easier and more reliable to just expose an integer
> latency-ns in QAPI? I think all other time related values in the block
> layer interfaces work this way, too.

Yup, noted to use nanoseconds exclusively.

>> +        qapi_event_send_block_io_delay(dev_path,
>> +                                       block_acct_qapi_type(cookie->type),
>> +                                       latency,
>> +                                       cookie->offset >= 0, cookie->offset,
>> +                                       cookie->bytes);
>> +    }
>> +
>>       WITH_QEMU_LOCK_GUARD(&stats->lock) {
>>           if (failed) {
>>               stats->failed_ops[cookie->type]++;
> Kevin
>

Thanks for reviewing!

Hanna