On 03.09.26 16:08, Stefan Hajnoczi wrote:
On Mon, Aug 31, 2026 at 03:51:56PM +0200, Hanna Czenczek wrote:
Based-on:<[email protected]>
[PATCH 0/6] hw/[block]: Fix missing accounting
Fri, 24 Jul 2026 17:23:09 +0200
Hi,
I’m told there are installations where storage is very slow, and some
would like the VM stack to report this proactively. To do so, we should
(via QAPI events) report on extremely slow I/O requests.
We already have the latency histogram, but this is not deemed sufficient
because it is not proactively reporting and would require repeated
querying. Therefore, this series introduces the event still.
Now, from a user's perspective, it would be nice if this event could be
raised exactly when an I/O request crosses the user-defined threshold,
but this would require keeping all active requests in a list and
checking it periodically. Now, if we used latency cookies for this
Did you consider a per-request timer? The QEMUTimerList active_timers
sorted list does not support efficient insertion, but improving it would
benefit all timer API users.
I'm not sure how that would address the problem. Does that not just move
the list elsewhere?
I.e. it still has the problem that every request needs to be put into a
list (starting a timer), and be removed when the request is done, or the
timer will fire and the below problems would occur if the request is
done but we failed to remove it (either use-after-free; or spurious
events plus memory leaks on top of a heap allocation per request).
(which makes sense), then that would require that every cookie set up is
also finalized when the request is done, because if we don't, results
could well be catastrophic:
- Either we use cookies as-is, which are often allocated on the stack or
in some other structure managed by the device; then this would result
in use-after-free,
- Or we allocate something specifically for this checking, so lingering
requests would at most create spurious latency events and memory
leaks, but this would require an additional heap allocation per
request that we would probably want to avoid.
So ideally we could use latency cookies and could statically verify that
they are always finalized when the request is done, but doing this in C
may well be impossible.
I think you are saying that the cookie API is unsafe because cookie
lifetime is not bounded by the request lifetime?
Yes, because it is not statically proven to be so.
Maybe the block_acct_*() API can be integrated into the actual request
so there is no way to leak the cookie. In other words, directly
associate requests with a BlockAcctStats and stop requiring the user to
manually manage a separate BlockAcctCookie.
I don't follow what you mean concretely. If you are suggesting a list of
requests in BlockAcctStats, then that is exactly what I had.
The problem is that if the destructor has to be called explicitly, we
may forget to do so; and accounting is done on the device emulation
level, so there is no central place where the pairing of constructor and
destructor would be obvious and trivial to verify.
I.e. it’s not like our functions current look like
````
run_co_ide_request() {
start_cookie();
run_co_request();
finalize_cookie();
}
```
Where the pairing of start and finalize are obvious; instead, start and
stop are in very different parts of the code, in all device emulation code.
The API is already weird because devices use:
block_acct_failed(blk_get_stats(s->blk), &req->acct);
i.e. why does the device have to reach into s->blk to access the stats?
If the stats belong to s->blk, then s->blk should do the accounting
during the request lifetime.
This would require a redesign of not just the cookie API, but also the
error policy API. There is also a wrinkle in that virtio-blk merges I/O
requests and accounts the merges.
So, because it is basically impossible (or at least it would be very
hard, and presumably require a large refactoring) to guarantee, without
additional heap allocations, that a list of active requests won’t run
into catastrophic use-after-frees, this series does the much simpler
version first, which is to just raise an event when a request *finishes*
and took more than a user-defined latency threshold.
Does this achieve the goal of warning when requests exceed a threshold?
According to
https://redhat.atlassian.net/browse/RHEL-141617?focusedCommentId=18127333
(and the following two comments), yes.
When an I/O request hangs for a long time, the management tool will be
unable to detect that the threshold has been exceeded in a timely
manner.
Yes. But why would that be a real problem?
Given the request so far is only to notify users of their storage block
showing problematic latencies at all, there is no need to provide the
alerts in actual real time.
Hanna
(PS: The nice thing about throwing an alert while the request is still
going on would be that it could allow us to also stop the VM in case of
excessive latency, before the request completes, so the guest would be
shielded from such excessive latency. This might be useful for Windows
guests that just have a maximum request lantency before throwing a
BSOD.)
Hanna Czenczek (9):
block/accounting: Add offset to BlockAcctCookie
qapi/block: Add IoAccountingOperation enum
qapi/block: Add BLOCK_IO_DELAY event
block-backend: Public blk_get_attached_dev_path()
block/accounting: Add BB field to latency checker
block/accounting: Emit BLOCK_IO_DELAY event
block: Add delay-alert-ms property
block/accounting: Move latency_ns override down
iotests: Add delay-alert test
qapi/block.json | 52 +++++++++
include/block/accounting.h | 20 +++-
include/hw/block/block.h | 5 +-
include/system/block-backend-io.h | 9 ++
include/system/dma.h | 2 +-
block/accounting.c | 60 ++++++++--
block/block-backend.c | 8 +-
blockdev.c | 16 ++-
hw/block/block.c | 4 +-
hw/block/dataplane/xen-block.c | 4 +-
hw/block/virtio-blk.c | 15 +--
hw/ide/ahci.c | 6 +-
hw/ide/atapi.c | 9 +-
hw/ide/core.c | 9 +-
hw/ide/macio.c | 15 ++-
hw/nvme/ctrl.c | 43 ++++---
hw/nvme/dif.c | 8 +-
hw/scsi/scsi-disk.c | 28 +++--
qemu-io-cmds.c | 14 +--
system/dma-helpers.c | 4 +-
tests/unit/test-block-accounting.c | 2 +-
tests/qemu-iotests/172.out | 38 +++++++
tests/qemu-iotests/tests/delay-alert | 136 +++++++++++++++++++++++
tests/qemu-iotests/tests/delay-alert.out | 46 ++++++++
24 files changed, 472 insertions(+), 81 deletions(-)
create mode 100755 tests/qemu-iotests/tests/delay-alert
create mode 100644 tests/qemu-iotests/tests/delay-alert.out
--
2.55.0
