On 21.09.26 22:41, Stefan Hajnoczi wrote:
On Wed, Sep 16, 2026 at 10:04:51AM +0200, Hanna Czenczek wrote:
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).
I wasn't thinking about the lifecycle here, but about the current
limitation that users aren't notified until request completion. Requests
can be stuck for a very long time or forever. Using a timer solves the
timeliness problem.
I still don’t follow. The lifecycle *is* the problem if the timeliness.
How to implement repeated querying is not the problem.
FWIW, the series that I had with timely waking did use a timer. But the
timer is just not the problem.
(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.
I meant that each I/O request should contain its own cookie and there is
never a need to create a cookie separately from the request. That way
the lifetime issue is solved.
Okay, I understood with your other email, because to me, “I/O request”
sounded like some kind of object and I couldn’t think of any. My mind
did not jump to the fact that you meant the actual execution thread of
the request, i.e. code, not structure.
This would require API changes because device emulation code currently
has some of the logic for cookies. Devices would no longer call
block_acct_done() once they have called blk_aio_pwritev(), for
example. I haven't looked in detail and am not sure if it's feasible.
As said in the other email, doing this would most likely change behavior
because we can’t or don’t want to reproduce all currently existing
quirks. Which may be good or bad. Bad for me in any case because it
would be more work, but, well…
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.
This is the part I'm asking about: can accounting be done by the block
layer? There might be cases that are purely handled in device emulation
code without a call into the block layer. In that case the accounting
still needs to be done in device emulation code. But when device
emulation calls blk_aio_*(), it should not do accounting itself.
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.
The issue says one purpose of this feature is to pause the guest to
avoid BSODs. This won't work if the request has to finish first, because
the BSOD occurs sometime after the treshold is reached but before QEMU
or any other component can react to the QMP event.
I know. I linked to a specific comment chain that says it is enough for now.
Whether the pausing is something anyone actually would ever want is a
completely different story, honestly. It is an idea I had for the
original design, but nothing that has actually ever been requested.
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.
What does the current approach solve that is not already possible with
the block latency histogram?
See what I linked above. Apparently having to repeatedly query the
histogram repeatedly is not deemed nice enough.
Hanna
Libvirt could add an API to notify when a particular threshold is
reached without any QEMU changes.
Stefan
