* [PATCH 0/9] block: BLOCK_IO_DELAY event
@ 2026-08-31 13:51 Hanna Czenczek
2026-08-31 13:51 ` [PATCH 1/9] block/accounting: Add offset to BlockAcctCookie Hanna Czenczek
` (9 more replies)
0 siblings, 10 replies; 47+ messages in thread
From: Hanna Czenczek @ 2026-08-31 13:51 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, Hanna Czenczek, Kevin Wolf, John Snow,
Denis V . Lunev, Eric Blake, Markus Armbruster, Stefan Hajnoczi
Based-on: <20260724152315.234183-1-hreitz@redhat.com>
[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
(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.
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.
(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
^ permalink raw reply [flat|nested] 47+ messages in thread* [PATCH 1/9] block/accounting: Add offset to BlockAcctCookie 2026-08-31 13:51 [PATCH 0/9] block: BLOCK_IO_DELAY event Hanna Czenczek @ 2026-08-31 13:51 ` Hanna Czenczek 2026-09-07 10:05 ` Jesper Wendel Devantier 2026-08-31 13:51 ` [PATCH 2/9] qapi/block: Add IoAccountingOperation enum Hanna Czenczek ` (8 subsequent siblings) 9 siblings, 1 reply; 47+ messages in thread From: Hanna Czenczek @ 2026-08-31 13:51 UTC (permalink / raw) To: qemu-block Cc: qemu-devel, Hanna Czenczek, Kevin Wolf, John Snow, Denis V . Lunev, Eric Blake, Markus Armbruster, Stefan Hajnoczi For latency alerts, we will want to generate QMP events from block accounting cookies. These should contain some minimal information about the offending request, i.e., start offset, size, and type. The latter two are already part of the cookie, the former is not. Add it. Signed-off-by: Hanna Czenczek <hreitz@redhat.com> --- include/block/accounting.h | 7 ++++- include/system/dma.h | 2 +- block/accounting.c | 3 ++- 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 +- 15 files changed, 105 insertions(+), 64 deletions(-) diff --git a/include/block/accounting.h b/include/block/accounting.h index 12d32460927..9386440ec89 100644 --- a/include/block/accounting.h +++ b/include/block/accounting.h @@ -95,6 +95,7 @@ struct BlockAcctStats { }; typedef struct BlockAcctCookie { + int64_t offset; int64_t bytes; int64_t start_time_ns; enum BlockAcctType type; @@ -108,8 +109,12 @@ void block_acct_cleanup(BlockAcctStats *stats); void block_acct_add_interval(BlockAcctStats *stats, unsigned interval_length); BlockAcctTimedStats *block_acct_interval_next(BlockAcctStats *stats, BlockAcctTimedStats *s); +/** + * Begin an I/O operation. + * @offset == -1 indicates an unknown offset (e.g. for meta operations). + */ void block_acct_start(BlockAcctStats *stats, BlockAcctCookie *cookie, - int64_t bytes, enum BlockAcctType type); + int64_t offset, int64_t bytes, enum BlockAcctType type); void block_acct_done(BlockAcctStats *stats, BlockAcctCookie *cookie); void block_acct_failed(BlockAcctStats *stats, BlockAcctCookie *cookie); void block_acct_invalid(BlockAcctStats *stats, enum BlockAcctType type); diff --git a/include/system/dma.h b/include/system/dma.h index 82e7ad54374..1291697642e 100644 --- a/include/system/dma.h +++ b/include/system/dma.h @@ -305,7 +305,7 @@ MemTxResult dma_buf_write(void *ptr, dma_addr_t len, dma_addr_t *residual, QEMUSGList *sg, MemTxAttrs attrs); void dma_acct_start(BlockBackend *blk, BlockAcctCookie *cookie, - QEMUSGList *sg, enum BlockAcctType type); + QEMUSGList *sg, int64_t offset, enum BlockAcctType type); /** * dma_aligned_pow2_mask: Return the address bit mask of the largest diff --git a/block/accounting.c b/block/accounting.c index 038af370170..66e5001403f 100644 --- a/block/accounting.c +++ b/block/accounting.c @@ -115,10 +115,11 @@ BlockAcctTimedStats *block_acct_interval_next(BlockAcctStats *stats, } void block_acct_start(BlockAcctStats *stats, BlockAcctCookie *cookie, - int64_t bytes, enum BlockAcctType type) + int64_t offset, int64_t bytes, enum BlockAcctType type) { assert(type < BLOCK_MAX_IOTYPE); + cookie->offset = offset; cookie->bytes = bytes; cookie->start_time_ns = qemu_clock_get_ns(clock_type); cookie->type = type; diff --git a/hw/block/dataplane/xen-block.c b/hw/block/dataplane/xen-block.c index 48c2e315f31..f4dab9b2510 100644 --- a/hw/block/dataplane/xen-block.c +++ b/hw/block/dataplane/xen-block.c @@ -373,7 +373,7 @@ static int xen_block_do_aio(XenBlockRequest *request) case BLKIF_OP_READ: qemu_iovec_add(&request->v, request->buf, request->size); block_acct_start(blk_get_stats(dataplane->blk), &request->acct, - request->v.size, BLOCK_ACCT_READ); + request->start, request->v.size, BLOCK_ACCT_READ); request->aio_inflight++; blk_aio_preadv(dataplane->blk, request->start, &request->v, 0, xen_block_complete_aio, request); @@ -386,7 +386,7 @@ static int xen_block_do_aio(XenBlockRequest *request) qemu_iovec_add(&request->v, request->buf, request->size); block_acct_start(blk_get_stats(dataplane->blk), &request->acct, - request->v.size, + request->start, request->v.size, request->req.operation == BLKIF_OP_WRITE ? BLOCK_ACCT_WRITE : BLOCK_ACCT_FLUSH); request->aio_inflight++; diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c index 384a5ee3f1c..d2605814795 100644 --- a/hw/block/virtio-blk.c +++ b/hw/block/virtio-blk.c @@ -338,7 +338,7 @@ static void virtio_blk_handle_flush(VirtIOBlockReq *req, MultiReqBuffer *mrb) { VirtIOBlock *s = req->dev; - block_acct_start(blk_get_stats(s->blk), &req->acct, 0, + block_acct_start(blk_get_stats(s->blk), &req->acct, -1, 0, BLOCK_ACCT_FLUSH); /* @@ -421,8 +421,8 @@ static uint8_t virtio_blk_handle_discard_write_zeroes(VirtIOBlockReq *req, blk_aio_flags |= BDRV_REQ_MAY_UNMAP; } - block_acct_start(blk_get_stats(s->blk), &req->acct, bytes, - BLOCK_ACCT_WRITE); + block_acct_start(blk_get_stats(s->blk), &req->acct, + sector << BDRV_SECTOR_BITS, bytes, BLOCK_ACCT_WRITE); blk_aio_pwrite_zeroes(s->blk, sector << BDRV_SECTOR_BITS, bytes, blk_aio_flags, @@ -437,8 +437,8 @@ static uint8_t virtio_blk_handle_discard_write_zeroes(VirtIOBlockReq *req, goto err; } - block_acct_start(blk_get_stats(s->blk), &req->acct, bytes, - BLOCK_ACCT_UNMAP); + block_acct_start(blk_get_stats(s->blk), &req->acct, + sector << BDRV_SECTOR_BITS, bytes, BLOCK_ACCT_UNMAP); blk_aio_pdiscard(s->blk, sector << BDRV_SECTOR_BITS, bytes, virtio_blk_discard_write_zeroes_complete, req); @@ -813,7 +813,7 @@ static int virtio_blk_handle_zone_append(VirtIOBlockReq *req, data->zone_append_data.offset = offset; qemu_iovec_init_external(&req->qiov, out_iov, out_num); - block_acct_start(blk_get_stats(s->blk), &req->acct, len, + block_acct_start(blk_get_stats(s->blk), &req->acct, offset, len, BLOCK_ACCT_ZONE_APPEND); blk_aio_zone_append(s->blk, &data->zone_append_data.offset, &req->qiov, 0, @@ -893,7 +893,8 @@ static int virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb) return 0; } - block_acct_start(blk_get_stats(s->blk), &req->acct, req->qiov.size, + block_acct_start(blk_get_stats(s->blk), &req->acct, + req->sector_num * BDRV_SECTOR_SIZE, req->qiov.size, is_write ? BLOCK_ACCT_WRITE : BLOCK_ACCT_READ); /* merge would exceed maximum number of requests or IO direction diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index 6b04762c4ad..cf63c87a013 100644 --- a/hw/ide/ahci.c +++ b/hw/ide/ahci.c @@ -1092,7 +1092,8 @@ static void execute_ncq_command(NCQTransferState *ncq_tfs) trace_execute_ncq_command_read(ad->hba, port, ncq_tfs->tag, ncq_tfs->sector_count, ncq_tfs->lba); dma_acct_start(ide_state->blk, &ncq_tfs->acct, - &ncq_tfs->sglist, BLOCK_ACCT_READ); + &ncq_tfs->sglist, ncq_tfs->lba << BDRV_SECTOR_BITS, + BLOCK_ACCT_READ); ncq_tfs->aiocb = dma_blk_read(ide_state->blk, &ncq_tfs->sglist, ncq_tfs->lba << BDRV_SECTOR_BITS, BDRV_SECTOR_SIZE, @@ -1102,7 +1103,8 @@ static void execute_ncq_command(NCQTransferState *ncq_tfs) trace_execute_ncq_command_write(ad->hba, port, ncq_tfs->tag, ncq_tfs->sector_count, ncq_tfs->lba); dma_acct_start(ide_state->blk, &ncq_tfs->acct, - &ncq_tfs->sglist, BLOCK_ACCT_WRITE); + &ncq_tfs->sglist, ncq_tfs->lba << BDRV_SECTOR_BITS, + BLOCK_ACCT_WRITE); ncq_tfs->aiocb = dma_blk_write(ide_state->blk, &ncq_tfs->sglist, ncq_tfs->lba << BDRV_SECTOR_BITS, BDRV_SECTOR_SIZE, diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c index 0ea149ad8c6..6f1187bce12 100644 --- a/hw/ide/atapi.c +++ b/hw/ide/atapi.c @@ -163,6 +163,7 @@ static int cd_read_sector(IDEState *s) trace_cd_read_sector(s->lba); block_acct_start(blk_get_stats(s->blk), &s->acct, + (int64_t)s->lba << ATAPI_SECTOR_BITS, nsec * ATAPI_SECTOR_SIZE, BLOCK_ACCT_READ); ide_buffered_readv(s, (int64_t)s->lba << 2, &s->qiov, nsec * 4, @@ -297,7 +298,8 @@ static void ide_atapi_cmd_reply(IDEState *s, int size, int max_size) s->elementary_transfer_size = 0; if (s->atapi_dma) { - block_acct_start(blk_get_stats(s->blk), &s->acct, size, + /* No offset available for these commands */ + block_acct_start(blk_get_stats(s->blk), &s->acct, -1, size, BLOCK_ACCT_READ); s->status = READY_STAT | SEEK_STAT | DRQ_STAT; ide_start_dma(s, ide_atapi_cmd_read_dma_cb); @@ -419,8 +421,9 @@ static void ide_atapi_cmd_read_dma(IDEState *s, int lba, int nb_sectors, s->io_buffer_size = 0; s->cd_sector_size = sector_size; - block_acct_start(blk_get_stats(s->blk), &s->acct, s->packet_transfer_size, - BLOCK_ACCT_READ); + block_acct_start(blk_get_stats(s->blk), &s->acct, + (int64_t)lba << ATAPI_SECTOR_BITS, + s->packet_transfer_size, BLOCK_ACCT_READ); /* XXX: check if BUSY_STAT should be set */ s->status = READY_STAT | SEEK_STAT | DRQ_STAT | BUSY_STAT; diff --git a/hw/ide/core.c b/hw/ide/core.c index 06e6bb6067d..4b559cf68d0 100644 --- a/hw/ide/core.c +++ b/hw/ide/core.c @@ -491,6 +491,7 @@ static void coroutine_fn ide_trim_co_entry(void *opaque) } block_acct_start(blk_get_stats(s->blk), &s->acct, + sector << BDRV_SECTOR_BITS, count << BDRV_SECTOR_BITS, BLOCK_ACCT_UNMAP); /* Got an entry! Submit and exit. */ @@ -833,6 +834,7 @@ static void ide_sector_read(IDEState *s) qemu_iovec_init_buf(&s->qiov, s->io_buffer, n * BDRV_SECTOR_SIZE); block_acct_start(blk_get_stats(s->blk), &s->acct, + sector_num << BDRV_SECTOR_BITS, n * BDRV_SECTOR_SIZE, BLOCK_ACCT_READ); s->pio_aiocb = ide_buffered_readv(s, sector_num, &s->qiov, n, ide_sector_read_cb, s); @@ -1009,10 +1011,12 @@ static void ide_sector_start_dma(IDEState *s, enum ide_dma_cmd dma_cmd) switch (dma_cmd) { case IDE_DMA_READ: block_acct_start(blk_get_stats(s->blk), &s->acct, + ide_get_sector(s) << BDRV_SECTOR_BITS, s->nsector * BDRV_SECTOR_SIZE, BLOCK_ACCT_READ); break; case IDE_DMA_WRITE: block_acct_start(blk_get_stats(s->blk), &s->acct, + ide_get_sector(s) << BDRV_SECTOR_BITS, s->nsector * BDRV_SECTOR_SIZE, BLOCK_ACCT_WRITE); break; default: @@ -1112,7 +1116,8 @@ static void ide_sector_write(IDEState *s) qemu_iovec_init_buf(&s->qiov, s->io_buffer, n * BDRV_SECTOR_SIZE); block_acct_start(blk_get_stats(s->blk), &s->acct, - n * BDRV_SECTOR_SIZE, BLOCK_ACCT_WRITE); + sector_num << BDRV_SECTOR_BITS, n * BDRV_SECTOR_SIZE, + BLOCK_ACCT_WRITE); s->pio_aiocb = blk_aio_pwritev(s->blk, sector_num << BDRV_SECTOR_BITS, &s->qiov, 0, ide_sector_write_cb, s); } @@ -1147,7 +1152,7 @@ static void ide_flush_cache(IDEState *s) s->status |= BUSY_STAT; ide_set_retry(s); - block_acct_start(blk_get_stats(s->blk), &s->acct, 0, BLOCK_ACCT_FLUSH); + block_acct_start(blk_get_stats(s->blk), &s->acct, -1, 0, BLOCK_ACCT_FLUSH); s->pio_aiocb = blk_aio_flush(s->blk, ide_flush_cb, s); } diff --git a/hw/ide/macio.c b/hw/ide/macio.c index 40fb4f3b4f3..99d079be916 100644 --- a/hw/ide/macio.c +++ b/hw/ide/macio.c @@ -218,7 +218,14 @@ static void pmac_ide_transfer(DBDMA_io *io) MACIO_DPRINTF("\n"); if (s->drive_kind == IDE_CD) { - block_acct_start(blk_get_stats(s->blk), &s->acct, io->len, + int64_t offset = -1; + + if (s->lba >= 0) { + /* Same hardcoded 11 as in pmac_ide_atapi_transfer_cb() */ + offset = ((int64_t)s->lba << 11) + s->io_buffer_index; + } + + block_acct_start(blk_get_stats(s->blk), &s->acct, offset, io->len, BLOCK_ACCT_READ); pmac_ide_atapi_transfer_cb(io, 0); @@ -227,11 +234,13 @@ static void pmac_ide_transfer(DBDMA_io *io) switch (s->dma_cmd) { case IDE_DMA_READ: - block_acct_start(blk_get_stats(s->blk), &s->acct, io->len, + block_acct_start(blk_get_stats(s->blk), &s->acct, + ide_get_sector(s) << BDRV_SECTOR_BITS, io->len, BLOCK_ACCT_READ); break; case IDE_DMA_WRITE: - block_acct_start(blk_get_stats(s->blk), &s->acct, io->len, + block_acct_start(blk_get_stats(s->blk), &s->acct, + ide_get_sector(s) << BDRV_SECTOR_BITS, io->len, BLOCK_ACCT_WRITE); break; default: diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c index 5168551e115..980816a2145 100644 --- a/hw/nvme/ctrl.c +++ b/hw/nvme/ctrl.c @@ -2754,7 +2754,7 @@ static uint16_t nvme_verify(NvmeCtrl *n, NvmeRequest *req) qemu_iovec_init(&ctx->data.iov, 1); qemu_iovec_add(&ctx->data.iov, ctx->data.bounce, len); - block_acct_start(blk_get_stats(blk), &req->acct, ctx->data.iov.size, + block_acct_start(blk_get_stats(blk), &req->acct, offset, ctx->data.iov.size, BLOCK_ACCT_READ); req->aiocb = blk_aio_preadv(ns->blkconf.blk, offset, &ctx->data.iov, 0, @@ -2983,6 +2983,7 @@ static void nvme_copy_out_cb(void *opaque, int ret) uint32_t nlb; size_t mlen; uint8_t *mbounce; + int64_t offset; if (ret < 0 || iocb->ret < 0) { block_acct_failed(stats, &iocb->acct.write); @@ -3003,10 +3004,10 @@ static void nvme_copy_out_cb(void *opaque, int ret) qemu_iovec_reset(&iocb->iov); qemu_iovec_add(&iocb->iov, mbounce, mlen); - block_acct_start(stats, &iocb->acct.write, mlen, BLOCK_ACCT_WRITE); - iocb->aiocb = blk_aio_pwritev(dns->blkconf.blk, nvme_moff(dns, iocb->slba), - &iocb->iov, 0, nvme_copy_out_completed_cb, - iocb); + offset = nvme_moff(dns, iocb->slba); + block_acct_start(stats, &iocb->acct.write, offset, mlen, BLOCK_ACCT_WRITE); + iocb->aiocb = blk_aio_pwritev(dns->blkconf.blk, offset, &iocb->iov, 0, + nvme_copy_out_completed_cb, iocb); return; @@ -3029,6 +3030,7 @@ static void nvme_copy_in_completed_cb(void *opaque, int ret) uint64_t reftag; size_t len, mlen; uint16_t status; + int64_t offset; if (ret < 0) { iocb->ret = ret; @@ -3113,10 +3115,11 @@ static void nvme_copy_in_completed_cb(void *opaque, int ret) qemu_iovec_reset(&iocb->iov); qemu_iovec_add(&iocb->iov, iocb->bounce, len); - block_acct_start(blk_get_stats(dns->blkconf.blk), &iocb->acct.write, len, - BLOCK_ACCT_WRITE); + offset = nvme_l2b(dns, iocb->slba); + block_acct_start(blk_get_stats(dns->blkconf.blk), &iocb->acct.write, + offset, len, BLOCK_ACCT_WRITE); - iocb->aiocb = blk_aio_pwritev(dns->blkconf.blk, nvme_l2b(dns, iocb->slba), + iocb->aiocb = blk_aio_pwritev(dns->blkconf.blk, offset, &iocb->iov, 0, nvme_copy_out_cb, iocb); return; @@ -3136,6 +3139,7 @@ static void nvme_copy_in_cb(void *opaque, int ret) uint64_t slba; uint32_t nlb; size_t mlen; + int64_t offset; if (ret < 0 || iocb->ret < 0) { block_acct_failed(stats, &iocb->acct.read); @@ -3154,10 +3158,11 @@ static void nvme_copy_in_cb(void *opaque, int ret) qemu_iovec_reset(&iocb->iov); qemu_iovec_add(&iocb->iov, iocb->bounce + nvme_l2b(sns, nlb), mlen); - block_acct_start(stats, &iocb->acct.read, mlen, BLOCK_ACCT_READ); - iocb->aiocb = blk_aio_preadv(sns->blkconf.blk, nvme_moff(sns, slba), - &iocb->iov, 0, nvme_copy_in_completed_cb, - iocb); + offset = nvme_moff(sns, slba); + block_acct_start(stats, &iocb->acct.read, offset, mlen, + BLOCK_ACCT_READ); + iocb->aiocb = blk_aio_preadv(sns->blkconf.blk, offset, &iocb->iov, 0, + nvme_copy_in_completed_cb, iocb); return; out: @@ -3236,6 +3241,7 @@ static void nvme_do_copy(NvmeCopyAIOCB *iocb) uint16_t status; uint32_t dnsid = le32_to_cpu(req->cmd.nsid); uint32_t snsid = dnsid; + int64_t offset; if (iocb->ret < 0) { goto done; @@ -3362,10 +3368,11 @@ static void nvme_do_copy(NvmeCopyAIOCB *iocb) assert(len <= blen); qemu_iovec_add(&iocb->iov, iocb->bounce, len); - block_acct_start(blk_get_stats(sns->blkconf.blk), &iocb->acct.read, len, - BLOCK_ACCT_READ); + offset = nvme_l2b(sns, slba); + block_acct_start(blk_get_stats(sns->blkconf.blk), &iocb->acct.read, + offset, len, BLOCK_ACCT_READ); - iocb->aiocb = blk_aio_preadv(sns->blkconf.blk, nvme_l2b(sns, slba), + iocb->aiocb = blk_aio_preadv(sns->blkconf.blk, offset, &iocb->iov, 0, nvme_copy_in_cb, iocb); return; @@ -3524,7 +3531,7 @@ static uint16_t nvme_compare(NvmeCtrl *n, NvmeRequest *req) qemu_iovec_init(&ctx->data.iov, 1); qemu_iovec_add(&ctx->data.iov, ctx->data.bounce, data_len); - block_acct_start(blk_get_stats(blk), &req->acct, data_len, + block_acct_start(blk_get_stats(blk), &req->acct, offset, data_len, BLOCK_ACCT_READ); req->aiocb = blk_aio_preadv(blk, offset, &ctx->data.iov, 0, nvme_compare_data_cb, req); @@ -3722,7 +3729,7 @@ static uint16_t nvme_read(NvmeCtrl *n, NvmeRequest *req) data_offset = nvme_l2b(ns, slba); - block_acct_start(blk_get_stats(blk), &req->acct, data_size, + block_acct_start(blk_get_stats(blk), &req->acct, data_offset, data_size, BLOCK_ACCT_READ); nvme_blk_read(blk, data_offset, BDRV_SECTOR_SIZE, nvme_rw_cb, req); return NVME_NO_COMPLETE; @@ -3892,7 +3899,7 @@ static uint16_t nvme_do_write(NvmeCtrl *n, NvmeRequest *req, bool append, goto invalid; } - block_acct_start(blk_get_stats(blk), &req->acct, data_size, + block_acct_start(blk_get_stats(blk), &req->acct, data_offset, data_size, BLOCK_ACCT_WRITE); nvme_blk_write(blk, data_offset, BDRV_SECTOR_SIZE, nvme_rw_cb, req); } else { diff --git a/hw/nvme/dif.c b/hw/nvme/dif.c index 4e7874f3223..fbd4d8eb816 100644 --- a/hw/nvme/dif.c +++ b/hw/nvme/dif.c @@ -644,8 +644,8 @@ uint16_t nvme_dif_rw(NvmeCtrl *n, NvmeRequest *req) qemu_iovec_add(&ctx->data.iov, ctx->data.bounce, len); if (req->cmd.opcode == NVME_CMD_READ) { - block_acct_start(blk_get_stats(blk), &req->acct, ctx->data.iov.size, - BLOCK_ACCT_READ); + block_acct_start(blk_get_stats(blk), &req->acct, offset, + ctx->data.iov.size, BLOCK_ACCT_READ); req->aiocb = blk_aio_preadv(ns->blkconf.blk, offset, &ctx->data.iov, 0, nvme_dif_rw_mdata_in_cb, ctx); @@ -690,8 +690,8 @@ uint16_t nvme_dif_rw(NvmeCtrl *n, NvmeRequest *req) } } - block_acct_start(blk_get_stats(blk), &req->acct, ctx->data.iov.size, - BLOCK_ACCT_WRITE); + block_acct_start(blk_get_stats(blk), &req->acct, offset, + ctx->data.iov.size, BLOCK_ACCT_WRITE); req->aiocb = blk_aio_pwritev(ns->blkconf.blk, offset, &ctx->data.iov, 0, nvme_dif_rw_mdata_out_cb, ctx); diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c index 24bd1a78b6e..5b40e404e13 100644 --- a/hw/scsi/scsi-disk.c +++ b/hw/scsi/scsi-disk.c @@ -484,7 +484,8 @@ static void scsi_do_read(SCSIDiskReq *r, int ret) scsi_req_ref(&r->req); if (r->req.sg) { - dma_acct_start(s->qdev.conf.blk, &r->acct, r->req.sg, BLOCK_ACCT_READ); + dma_acct_start(s->qdev.conf.blk, &r->acct, r->req.sg, + r->sector << BDRV_SECTOR_BITS, BLOCK_ACCT_READ); r->req.residual -= r->req.sg->size; r->req.aiocb = dma_blk_io(r->req.sg, r->sector << BDRV_SECTOR_BITS, BDRV_SECTOR_SIZE, @@ -493,7 +494,8 @@ static void scsi_do_read(SCSIDiskReq *r, int ret) } else { scsi_init_iovec(r, SCSI_DMA_BUF_SIZE); block_acct_start(blk_get_stats(s->qdev.conf.blk), &r->acct, - r->qiov.size, BLOCK_ACCT_READ); + r->sector << BDRV_SECTOR_BITS, r->qiov.size, + BLOCK_ACCT_READ); r->req.aiocb = sdc->dma_readv(r->sector << BDRV_SECTOR_BITS, &r->qiov, scsi_read_complete, r, r); } @@ -551,7 +553,7 @@ static void scsi_read_data(SCSIRequest *req) first = !r->started; r->started = true; if (first && r->need_fua) { - block_acct_start(blk_get_stats(s->qdev.conf.blk), &r->acct, 0, + block_acct_start(blk_get_stats(s->qdev.conf.blk), &r->acct, -1, 0, BLOCK_ACCT_FLUSH); r->req.aiocb = blk_aio_flush(s->qdev.conf.blk, scsi_do_read_cb, r); } else { @@ -634,7 +636,7 @@ static void scsi_write_data(SCSIRequest *req) if (r->req.cmd.buf[0] == VERIFY_10 || r->req.cmd.buf[0] == VERIFY_12 || r->req.cmd.buf[0] == VERIFY_16) { - block_acct_start(blk_get_stats(s->qdev.conf.blk), &r->acct, 0, + block_acct_start(blk_get_stats(s->qdev.conf.blk), &r->acct, -1, 0, BLOCK_ACCT_FLUSH); cb = r->req.sg ? scsi_dma_complete : scsi_write_complete; r->req.aiocb = blk_aio_flush(s->qdev.conf.blk, cb, r); @@ -642,7 +644,8 @@ static void scsi_write_data(SCSIRequest *req) } if (r->req.sg) { - dma_acct_start(s->qdev.conf.blk, &r->acct, r->req.sg, BLOCK_ACCT_WRITE); + dma_acct_start(s->qdev.conf.blk, &r->acct, r->req.sg, + r->sector << BDRV_SECTOR_BITS, BLOCK_ACCT_WRITE); r->req.residual -= r->req.sg->size; r->req.aiocb = dma_blk_io(r->req.sg, r->sector << BDRV_SECTOR_BITS, BDRV_SECTOR_SIZE, @@ -650,7 +653,8 @@ static void scsi_write_data(SCSIRequest *req) DMA_DIRECTION_TO_DEVICE); } else { block_acct_start(blk_get_stats(s->qdev.conf.blk), &r->acct, - r->qiov.size, BLOCK_ACCT_WRITE); + r->sector << BDRV_SECTOR_BITS, r->qiov.size, + BLOCK_ACCT_WRITE); r->req.aiocb = sdc->dma_writev(r->sector << BDRV_SECTOR_BITS, &r->qiov, scsi_write_complete, r, r); } @@ -1710,7 +1714,7 @@ static void scsi_disk_emulate_mode_select(SCSIDiskReq *r, uint8_t *inbuf) if (!blk_enable_write_cache(s->qdev.conf.blk)) { /* The request is used as the AIO opaque value, so add a ref. */ scsi_req_ref(&r->req); - block_acct_start(blk_get_stats(s->qdev.conf.blk), &r->acct, 0, + block_acct_start(blk_get_stats(s->qdev.conf.blk), &r->acct, -1, 0, BLOCK_ACCT_FLUSH); r->req.aiocb = blk_aio_flush(s->qdev.conf.blk, scsi_aio_complete, r); return; @@ -1777,6 +1781,7 @@ static void scsi_unmap_complete_noio(UnmapCBData *data, int ret) } block_acct_start(blk_get_stats(s->qdev.conf.blk), &r->acct, + r->sector * BDRV_SECTOR_SIZE, r->sector_count * BDRV_SECTOR_SIZE, BLOCK_ACCT_UNMAP); @@ -1894,7 +1899,8 @@ static void scsi_write_same_complete(void *opaque, int ret) data->iov.iov_len); if (data->iov.iov_len) { block_acct_start(blk_get_stats(s->qdev.conf.blk), &r->acct, - data->iov.iov_len, BLOCK_ACCT_WRITE); + data->sector << BDRV_SECTOR_BITS, data->iov.iov_len, + BLOCK_ACCT_WRITE); /* Reinitialize qiov, to handle unaligned WRITE SAME request * where final qiov may need smaller size */ qemu_iovec_init_external(&data->qiov, &data->iov, 1); @@ -1943,6 +1949,7 @@ static void scsi_disk_emulate_write_same(SCSIDiskReq *r, uint8_t *inbuf) /* The request is used as the AIO opaque value, so add a ref. */ scsi_req_ref(&r->req); block_acct_start(blk_get_stats(s->qdev.conf.blk), &r->acct, + r->req.cmd.lba * s->qdev.blocksize, nb_sectors * s->qdev.blocksize, BLOCK_ACCT_WRITE); r->req.aiocb = blk_aio_pwrite_zeroes(s->qdev.conf.blk, @@ -1969,7 +1976,8 @@ static void scsi_disk_emulate_write_same(SCSIDiskReq *r, uint8_t *inbuf) scsi_req_ref(&r->req); block_acct_start(blk_get_stats(s->qdev.conf.blk), &r->acct, - data->iov.iov_len, BLOCK_ACCT_WRITE); + data->sector << BDRV_SECTOR_BITS, data->iov.iov_len, + BLOCK_ACCT_WRITE); r->req.aiocb = blk_aio_pwritev(s->qdev.conf.blk, data->sector << BDRV_SECTOR_BITS, &data->qiov, 0, @@ -2237,7 +2245,7 @@ static int32_t scsi_disk_emulate_command(SCSIRequest *req, uint8_t *buf) case SYNCHRONIZE_CACHE: /* The request is used as the AIO opaque value, so add a ref. */ scsi_req_ref(&r->req); - block_acct_start(blk_get_stats(s->qdev.conf.blk), &r->acct, 0, + block_acct_start(blk_get_stats(s->qdev.conf.blk), &r->acct, -1, 0, BLOCK_ACCT_FLUSH); r->req.aiocb = blk_aio_flush(s->qdev.conf.blk, scsi_aio_complete, r); return 0; diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c index de4c1966fea..655dfae9057 100644 --- a/qemu-io-cmds.c +++ b/qemu-io-cmds.c @@ -1538,8 +1538,8 @@ static int aio_read_f(BlockBackend *blk, int argc, char **argv) } clock_gettime(CLOCK_MONOTONIC, &ctx->t1); - block_acct_start(blk_get_stats(blk), &ctx->acct, ctx->qiov.size, - BLOCK_ACCT_READ); + block_acct_start(blk_get_stats(blk), &ctx->acct, ctx->offset, + ctx->qiov.size, BLOCK_ACCT_READ); blk_aio_preadv(blk, ctx->offset, &ctx->qiov, ctx->flags, aio_read_done, ctx); return 0; @@ -1693,8 +1693,8 @@ static int aio_write_f(BlockBackend *blk, int argc, char **argv) } clock_gettime(CLOCK_MONOTONIC, &ctx->t1); - block_acct_start(blk_get_stats(blk), &ctx->acct, ctx->qiov.size, - BLOCK_ACCT_WRITE); + block_acct_start(blk_get_stats(blk), &ctx->acct, ctx->offset, + ctx->qiov.size, BLOCK_ACCT_WRITE); blk_aio_pwritev(blk, ctx->offset, &ctx->qiov, ctx->flags, aio_write_done, ctx); @@ -1706,7 +1706,7 @@ static int aio_write_f(BlockBackend *blk, int argc, char **argv) static int aio_flush_f(BlockBackend *blk, int argc, char **argv) { BlockAcctCookie cookie; - block_acct_start(blk_get_stats(blk), &cookie, 0, BLOCK_ACCT_FLUSH); + block_acct_start(blk_get_stats(blk), &cookie, -1, 0, BLOCK_ACCT_FLUSH); blk_drain_all(); block_acct_done(blk_get_stats(blk), &cookie); return 0; @@ -2325,8 +2325,8 @@ static int aio_discard_f(BlockBackend *blk, int argc, char **argv) clock_gettime(CLOCK_MONOTONIC, &ctx->t1); ctx->qiov.size = count; - block_acct_start(blk_get_stats(blk), &ctx->acct, ctx->qiov.size, - BLOCK_ACCT_UNMAP); + block_acct_start(blk_get_stats(blk), &ctx->acct, ctx->offset, + ctx->qiov.size, BLOCK_ACCT_UNMAP); blk_aio_pdiscard(blk, ctx->offset, count, aio_discard_done, ctx); return 0; diff --git a/system/dma-helpers.c b/system/dma-helpers.c index 0d592f64680..7bf41119ebc 100644 --- a/system/dma-helpers.c +++ b/system/dma-helpers.c @@ -315,9 +315,9 @@ MemTxResult dma_buf_write(void *ptr, dma_addr_t len, dma_addr_t *residual, } void dma_acct_start(BlockBackend *blk, BlockAcctCookie *cookie, - QEMUSGList *sg, enum BlockAcctType type) + QEMUSGList *sg, int64_t offset, enum BlockAcctType type) { - block_acct_start(blk_get_stats(blk), cookie, sg->size, type); + block_acct_start(blk_get_stats(blk), cookie, offset, sg->size, type); } uint64_t dma_aligned_pow2_mask(uint64_t start, uint64_t end, int max_addr_bits) diff --git a/tests/unit/test-block-accounting.c b/tests/unit/test-block-accounting.c index 7aae491cfc2..0081502891c 100644 --- a/tests/unit/test-block-accounting.c +++ b/tests/unit/test-block-accounting.c @@ -63,7 +63,7 @@ static void *reader_thread(void *opaque) while (!qatomic_read(&stop_workers)) { BlockAcctCookie cookie; - block_acct_start(stats, &cookie, 4096, BLOCK_ACCT_READ); + block_acct_start(stats, &cookie, -1, 4096, BLOCK_ACCT_READ); block_acct_done(stats, &cookie); } -- 2.55.0 ^ permalink raw reply related [flat|nested] 47+ messages in thread
* Re: [PATCH 1/9] block/accounting: Add offset to BlockAcctCookie 2026-08-31 13:51 ` [PATCH 1/9] block/accounting: Add offset to BlockAcctCookie Hanna Czenczek @ 2026-09-07 10:05 ` Jesper Wendel Devantier 0 siblings, 0 replies; 47+ messages in thread From: Jesper Wendel Devantier @ 2026-09-07 10:05 UTC (permalink / raw) To: Hanna Czenczek Cc: qemu-block, qemu-devel, Kevin Wolf, John Snow, Denis V . Lunev, Eric Blake, Markus Armbruster, Stefan Hajnoczi On 2026-08-31T15:51:57+02:00, Hanna Czenczek <hreitz@redhat.com> wrote: > For latency alerts, we will want to generate QMP events from block > accounting cookies. These should contain some minimal information about > the offending request, i.e., start offset, size, and type. The latter > two are already part of the cookie, the former is not. Add it. > > Signed-off-by: Hanna Czenczek <hreitz@redhat.com> > --- > include/block/accounting.h | 7 ++++- > include/system/dma.h | 2 +- > block/accounting.c | 3 ++- > 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 +- > 15 files changed, 105 insertions(+), 64 deletions(-) > > diff --git a/include/block/accounting.h b/include/block/accounting.h > index 12d32460927..9386440ec89 100644 > --- a/include/block/accounting.h > +++ b/include/block/accounting.h > @@ -95,6 +95,7 @@ struct BlockAcctStats { > }; > > typedef struct BlockAcctCookie { > + int64_t offset; > int64_t bytes; > int64_t start_time_ns; > enum BlockAcctType type; > @@ -108,8 +109,12 @@ void block_acct_cleanup(BlockAcctStats *stats); > void block_acct_add_interval(BlockAcctStats *stats, unsigned interval_length); > BlockAcctTimedStats *block_acct_interval_next(BlockAcctStats *stats, > BlockAcctTimedStats *s); > +/** > + * Begin an I/O operation. > + * @offset == -1 indicates an unknown offset (e.g. for meta operations). > + */ > void block_acct_start(BlockAcctStats *stats, BlockAcctCookie *cookie, > - int64_t bytes, enum BlockAcctType type); > + int64_t offset, int64_t bytes, enum BlockAcctType type); > void block_acct_done(BlockAcctStats *stats, BlockAcctCookie *cookie); > void block_acct_failed(BlockAcctStats *stats, BlockAcctCookie *cookie); > void block_acct_invalid(BlockAcctStats *stats, enum BlockAcctType type); > diff --git a/include/system/dma.h b/include/system/dma.h > index 82e7ad54374..1291697642e 100644 > --- a/include/system/dma.h > +++ b/include/system/dma.h > @@ -305,7 +305,7 @@ MemTxResult dma_buf_write(void *ptr, dma_addr_t len, dma_addr_t *residual, > QEMUSGList *sg, MemTxAttrs attrs); > > void dma_acct_start(BlockBackend *blk, BlockAcctCookie *cookie, > - QEMUSGList *sg, enum BlockAcctType type); > + QEMUSGList *sg, int64_t offset, enum BlockAcctType type); > > /** > * dma_aligned_pow2_mask: Return the address bit mask of the largest > diff --git a/block/accounting.c b/block/accounting.c > index 038af370170..66e5001403f 100644 > --- a/block/accounting.c > +++ b/block/accounting.c > @@ -115,10 +115,11 @@ BlockAcctTimedStats *block_acct_interval_next(BlockAcctStats *stats, > } > > void block_acct_start(BlockAcctStats *stats, BlockAcctCookie *cookie, > - int64_t bytes, enum BlockAcctType type) > + int64_t offset, int64_t bytes, enum BlockAcctType type) > { > assert(type < BLOCK_MAX_IOTYPE); > > + cookie->offset = offset; > cookie->bytes = bytes; > cookie->start_time_ns = qemu_clock_get_ns(clock_type); > cookie->type = type; > diff --git a/hw/block/dataplane/xen-block.c b/hw/block/dataplane/xen-block.c > index 48c2e315f31..f4dab9b2510 100644 > --- a/hw/block/dataplane/xen-block.c > +++ b/hw/block/dataplane/xen-block.c > @@ -373,7 +373,7 @@ static int xen_block_do_aio(XenBlockRequest *request) > case BLKIF_OP_READ: > qemu_iovec_add(&request->v, request->buf, request->size); > block_acct_start(blk_get_stats(dataplane->blk), &request->acct, > - request->v.size, BLOCK_ACCT_READ); > + request->start, request->v.size, BLOCK_ACCT_READ); > request->aio_inflight++; > blk_aio_preadv(dataplane->blk, request->start, &request->v, 0, > xen_block_complete_aio, request); > @@ -386,7 +386,7 @@ static int xen_block_do_aio(XenBlockRequest *request) > > qemu_iovec_add(&request->v, request->buf, request->size); > block_acct_start(blk_get_stats(dataplane->blk), &request->acct, > - request->v.size, > + request->start, request->v.size, > request->req.operation == BLKIF_OP_WRITE ? > BLOCK_ACCT_WRITE : BLOCK_ACCT_FLUSH); > request->aio_inflight++; > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > index 384a5ee3f1c..d2605814795 100644 > --- a/hw/block/virtio-blk.c > +++ b/hw/block/virtio-blk.c > @@ -338,7 +338,7 @@ static void virtio_blk_handle_flush(VirtIOBlockReq *req, MultiReqBuffer *mrb) > { > VirtIOBlock *s = req->dev; > > - block_acct_start(blk_get_stats(s->blk), &req->acct, 0, > + block_acct_start(blk_get_stats(s->blk), &req->acct, -1, 0, > BLOCK_ACCT_FLUSH); > > /* > @@ -421,8 +421,8 @@ static uint8_t virtio_blk_handle_discard_write_zeroes(VirtIOBlockReq *req, > blk_aio_flags |= BDRV_REQ_MAY_UNMAP; > } > > - block_acct_start(blk_get_stats(s->blk), &req->acct, bytes, > - BLOCK_ACCT_WRITE); > + block_acct_start(blk_get_stats(s->blk), &req->acct, > + sector << BDRV_SECTOR_BITS, bytes, BLOCK_ACCT_WRITE); > > blk_aio_pwrite_zeroes(s->blk, sector << BDRV_SECTOR_BITS, > bytes, blk_aio_flags, > @@ -437,8 +437,8 @@ static uint8_t virtio_blk_handle_discard_write_zeroes(VirtIOBlockReq *req, > goto err; > } > > - block_acct_start(blk_get_stats(s->blk), &req->acct, bytes, > - BLOCK_ACCT_UNMAP); > + block_acct_start(blk_get_stats(s->blk), &req->acct, > + sector << BDRV_SECTOR_BITS, bytes, BLOCK_ACCT_UNMAP); > > blk_aio_pdiscard(s->blk, sector << BDRV_SECTOR_BITS, bytes, > virtio_blk_discard_write_zeroes_complete, req); > @@ -813,7 +813,7 @@ static int virtio_blk_handle_zone_append(VirtIOBlockReq *req, > data->zone_append_data.offset = offset; > qemu_iovec_init_external(&req->qiov, out_iov, out_num); > > - block_acct_start(blk_get_stats(s->blk), &req->acct, len, > + block_acct_start(blk_get_stats(s->blk), &req->acct, offset, len, > BLOCK_ACCT_ZONE_APPEND); > > blk_aio_zone_append(s->blk, &data->zone_append_data.offset, &req->qiov, 0, > @@ -893,7 +893,8 @@ static int virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb) > return 0; > } > > - block_acct_start(blk_get_stats(s->blk), &req->acct, req->qiov.size, > + block_acct_start(blk_get_stats(s->blk), &req->acct, > + req->sector_num * BDRV_SECTOR_SIZE, req->qiov.size, > is_write ? BLOCK_ACCT_WRITE : BLOCK_ACCT_READ); > > /* merge would exceed maximum number of requests or IO direction > diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c > index 6b04762c4ad..cf63c87a013 100644 > --- a/hw/ide/ahci.c > +++ b/hw/ide/ahci.c > @@ -1092,7 +1092,8 @@ static void execute_ncq_command(NCQTransferState *ncq_tfs) > trace_execute_ncq_command_read(ad->hba, port, ncq_tfs->tag, > ncq_tfs->sector_count, ncq_tfs->lba); > dma_acct_start(ide_state->blk, &ncq_tfs->acct, > - &ncq_tfs->sglist, BLOCK_ACCT_READ); > + &ncq_tfs->sglist, ncq_tfs->lba << BDRV_SECTOR_BITS, > + BLOCK_ACCT_READ); > ncq_tfs->aiocb = dma_blk_read(ide_state->blk, &ncq_tfs->sglist, > ncq_tfs->lba << BDRV_SECTOR_BITS, > BDRV_SECTOR_SIZE, > @@ -1102,7 +1103,8 @@ static void execute_ncq_command(NCQTransferState *ncq_tfs) > trace_execute_ncq_command_write(ad->hba, port, ncq_tfs->tag, > ncq_tfs->sector_count, ncq_tfs->lba); > dma_acct_start(ide_state->blk, &ncq_tfs->acct, > - &ncq_tfs->sglist, BLOCK_ACCT_WRITE); > + &ncq_tfs->sglist, ncq_tfs->lba << BDRV_SECTOR_BITS, > + BLOCK_ACCT_WRITE); > ncq_tfs->aiocb = dma_blk_write(ide_state->blk, &ncq_tfs->sglist, > ncq_tfs->lba << BDRV_SECTOR_BITS, > BDRV_SECTOR_SIZE, > diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c > index 0ea149ad8c6..6f1187bce12 100644 > --- a/hw/ide/atapi.c > +++ b/hw/ide/atapi.c > @@ -163,6 +163,7 @@ static int cd_read_sector(IDEState *s) > trace_cd_read_sector(s->lba); > > block_acct_start(blk_get_stats(s->blk), &s->acct, > + (int64_t)s->lba << ATAPI_SECTOR_BITS, > nsec * ATAPI_SECTOR_SIZE, BLOCK_ACCT_READ); > > ide_buffered_readv(s, (int64_t)s->lba << 2, &s->qiov, nsec * 4, > @@ -297,7 +298,8 @@ static void ide_atapi_cmd_reply(IDEState *s, int size, int max_size) > s->elementary_transfer_size = 0; > > if (s->atapi_dma) { > - block_acct_start(blk_get_stats(s->blk), &s->acct, size, > + /* No offset available for these commands */ > + block_acct_start(blk_get_stats(s->blk), &s->acct, -1, size, > BLOCK_ACCT_READ); > s->status = READY_STAT | SEEK_STAT | DRQ_STAT; > ide_start_dma(s, ide_atapi_cmd_read_dma_cb); > @@ -419,8 +421,9 @@ static void ide_atapi_cmd_read_dma(IDEState *s, int lba, int nb_sectors, > s->io_buffer_size = 0; > s->cd_sector_size = sector_size; > > - block_acct_start(blk_get_stats(s->blk), &s->acct, s->packet_transfer_size, > - BLOCK_ACCT_READ); > + block_acct_start(blk_get_stats(s->blk), &s->acct, > + (int64_t)lba << ATAPI_SECTOR_BITS, > + s->packet_transfer_size, BLOCK_ACCT_READ); > > /* XXX: check if BUSY_STAT should be set */ > s->status = READY_STAT | SEEK_STAT | DRQ_STAT | BUSY_STAT; > diff --git a/hw/ide/core.c b/hw/ide/core.c > index 06e6bb6067d..4b559cf68d0 100644 > --- a/hw/ide/core.c > +++ b/hw/ide/core.c > @@ -491,6 +491,7 @@ static void coroutine_fn ide_trim_co_entry(void *opaque) > } > > block_acct_start(blk_get_stats(s->blk), &s->acct, > + sector << BDRV_SECTOR_BITS, > count << BDRV_SECTOR_BITS, BLOCK_ACCT_UNMAP); > > /* Got an entry! Submit and exit. */ > @@ -833,6 +834,7 @@ static void ide_sector_read(IDEState *s) > qemu_iovec_init_buf(&s->qiov, s->io_buffer, n * BDRV_SECTOR_SIZE); > > block_acct_start(blk_get_stats(s->blk), &s->acct, > + sector_num << BDRV_SECTOR_BITS, > n * BDRV_SECTOR_SIZE, BLOCK_ACCT_READ); > s->pio_aiocb = ide_buffered_readv(s, sector_num, &s->qiov, n, > ide_sector_read_cb, s); > @@ -1009,10 +1011,12 @@ static void ide_sector_start_dma(IDEState *s, enum ide_dma_cmd dma_cmd) > switch (dma_cmd) { > case IDE_DMA_READ: > block_acct_start(blk_get_stats(s->blk), &s->acct, > + ide_get_sector(s) << BDRV_SECTOR_BITS, > s->nsector * BDRV_SECTOR_SIZE, BLOCK_ACCT_READ); > break; > case IDE_DMA_WRITE: > block_acct_start(blk_get_stats(s->blk), &s->acct, > + ide_get_sector(s) << BDRV_SECTOR_BITS, > s->nsector * BDRV_SECTOR_SIZE, BLOCK_ACCT_WRITE); > break; > default: > @@ -1112,7 +1116,8 @@ static void ide_sector_write(IDEState *s) > qemu_iovec_init_buf(&s->qiov, s->io_buffer, n * BDRV_SECTOR_SIZE); > > block_acct_start(blk_get_stats(s->blk), &s->acct, > - n * BDRV_SECTOR_SIZE, BLOCK_ACCT_WRITE); > + sector_num << BDRV_SECTOR_BITS, n * BDRV_SECTOR_SIZE, > + BLOCK_ACCT_WRITE); > s->pio_aiocb = blk_aio_pwritev(s->blk, sector_num << BDRV_SECTOR_BITS, > &s->qiov, 0, ide_sector_write_cb, s); > } > @@ -1147,7 +1152,7 @@ static void ide_flush_cache(IDEState *s) > > s->status |= BUSY_STAT; > ide_set_retry(s); > - block_acct_start(blk_get_stats(s->blk), &s->acct, 0, BLOCK_ACCT_FLUSH); > + block_acct_start(blk_get_stats(s->blk), &s->acct, -1, 0, BLOCK_ACCT_FLUSH); > s->pio_aiocb = blk_aio_flush(s->blk, ide_flush_cb, s); > } > > diff --git a/hw/ide/macio.c b/hw/ide/macio.c > index 40fb4f3b4f3..99d079be916 100644 > --- a/hw/ide/macio.c > +++ b/hw/ide/macio.c > @@ -218,7 +218,14 @@ static void pmac_ide_transfer(DBDMA_io *io) > MACIO_DPRINTF("\n"); > > if (s->drive_kind == IDE_CD) { > - block_acct_start(blk_get_stats(s->blk), &s->acct, io->len, > + int64_t offset = -1; > + > + if (s->lba >= 0) { > + /* Same hardcoded 11 as in pmac_ide_atapi_transfer_cb() */ > + offset = ((int64_t)s->lba << 11) + s->io_buffer_index; > + } > + > + block_acct_start(blk_get_stats(s->blk), &s->acct, offset, io->len, > BLOCK_ACCT_READ); > > pmac_ide_atapi_transfer_cb(io, 0); > @@ -227,11 +234,13 @@ static void pmac_ide_transfer(DBDMA_io *io) > > switch (s->dma_cmd) { > case IDE_DMA_READ: > - block_acct_start(blk_get_stats(s->blk), &s->acct, io->len, > + block_acct_start(blk_get_stats(s->blk), &s->acct, > + ide_get_sector(s) << BDRV_SECTOR_BITS, io->len, > BLOCK_ACCT_READ); > break; > case IDE_DMA_WRITE: > - block_acct_start(blk_get_stats(s->blk), &s->acct, io->len, > + block_acct_start(blk_get_stats(s->blk), &s->acct, > + ide_get_sector(s) << BDRV_SECTOR_BITS, io->len, > BLOCK_ACCT_WRITE); > break; > default: > diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c > index 5168551e115..980816a2145 100644 > --- a/hw/nvme/ctrl.c > +++ b/hw/nvme/ctrl.c > @@ -2754,7 +2754,7 @@ static uint16_t nvme_verify(NvmeCtrl *n, NvmeRequest *req) > qemu_iovec_init(&ctx->data.iov, 1); > qemu_iovec_add(&ctx->data.iov, ctx->data.bounce, len); > > - block_acct_start(blk_get_stats(blk), &req->acct, ctx->data.iov.size, > + block_acct_start(blk_get_stats(blk), &req->acct, offset, ctx->data.iov.size, > BLOCK_ACCT_READ); > > req->aiocb = blk_aio_preadv(ns->blkconf.blk, offset, &ctx->data.iov, 0, > @@ -2983,6 +2983,7 @@ static void nvme_copy_out_cb(void *opaque, int ret) > uint32_t nlb; > size_t mlen; > uint8_t *mbounce; > + int64_t offset; > > if (ret < 0 || iocb->ret < 0) { > block_acct_failed(stats, &iocb->acct.write); > @@ -3003,10 +3004,10 @@ static void nvme_copy_out_cb(void *opaque, int ret) > qemu_iovec_reset(&iocb->iov); > qemu_iovec_add(&iocb->iov, mbounce, mlen); > > - block_acct_start(stats, &iocb->acct.write, mlen, BLOCK_ACCT_WRITE); > - iocb->aiocb = blk_aio_pwritev(dns->blkconf.blk, nvme_moff(dns, iocb->slba), > - &iocb->iov, 0, nvme_copy_out_completed_cb, > - iocb); > + offset = nvme_moff(dns, iocb->slba); > + block_acct_start(stats, &iocb->acct.write, offset, mlen, BLOCK_ACCT_WRITE); > + iocb->aiocb = blk_aio_pwritev(dns->blkconf.blk, offset, &iocb->iov, 0, > + nvme_copy_out_completed_cb, iocb); > > return; > > @@ -3029,6 +3030,7 @@ static void nvme_copy_in_completed_cb(void *opaque, int ret) > uint64_t reftag; > size_t len, mlen; > uint16_t status; > + int64_t offset; > > if (ret < 0) { > iocb->ret = ret; > @@ -3113,10 +3115,11 @@ static void nvme_copy_in_completed_cb(void *opaque, int ret) > qemu_iovec_reset(&iocb->iov); > qemu_iovec_add(&iocb->iov, iocb->bounce, len); > > - block_acct_start(blk_get_stats(dns->blkconf.blk), &iocb->acct.write, len, > - BLOCK_ACCT_WRITE); > + offset = nvme_l2b(dns, iocb->slba); > + block_acct_start(blk_get_stats(dns->blkconf.blk), &iocb->acct.write, > + offset, len, BLOCK_ACCT_WRITE); > > - iocb->aiocb = blk_aio_pwritev(dns->blkconf.blk, nvme_l2b(dns, iocb->slba), > + iocb->aiocb = blk_aio_pwritev(dns->blkconf.blk, offset, > &iocb->iov, 0, nvme_copy_out_cb, iocb); > > return; > @@ -3136,6 +3139,7 @@ static void nvme_copy_in_cb(void *opaque, int ret) > uint64_t slba; > uint32_t nlb; > size_t mlen; > + int64_t offset; > > if (ret < 0 || iocb->ret < 0) { > block_acct_failed(stats, &iocb->acct.read); > @@ -3154,10 +3158,11 @@ static void nvme_copy_in_cb(void *opaque, int ret) > qemu_iovec_reset(&iocb->iov); > qemu_iovec_add(&iocb->iov, iocb->bounce + nvme_l2b(sns, nlb), mlen); > > - block_acct_start(stats, &iocb->acct.read, mlen, BLOCK_ACCT_READ); > - iocb->aiocb = blk_aio_preadv(sns->blkconf.blk, nvme_moff(sns, slba), > - &iocb->iov, 0, nvme_copy_in_completed_cb, > - iocb); > + offset = nvme_moff(sns, slba); > + block_acct_start(stats, &iocb->acct.read, offset, mlen, > + BLOCK_ACCT_READ); > + iocb->aiocb = blk_aio_preadv(sns->blkconf.blk, offset, &iocb->iov, 0, > + nvme_copy_in_completed_cb, iocb); > return; > > out: > @@ -3236,6 +3241,7 @@ static void nvme_do_copy(NvmeCopyAIOCB *iocb) > uint16_t status; > uint32_t dnsid = le32_to_cpu(req->cmd.nsid); > uint32_t snsid = dnsid; > + int64_t offset; > > if (iocb->ret < 0) { > goto done; > @@ -3362,10 +3368,11 @@ static void nvme_do_copy(NvmeCopyAIOCB *iocb) > assert(len <= blen); > qemu_iovec_add(&iocb->iov, iocb->bounce, len); > > - block_acct_start(blk_get_stats(sns->blkconf.blk), &iocb->acct.read, len, > - BLOCK_ACCT_READ); > + offset = nvme_l2b(sns, slba); > + block_acct_start(blk_get_stats(sns->blkconf.blk), &iocb->acct.read, > + offset, len, BLOCK_ACCT_READ); > > - iocb->aiocb = blk_aio_preadv(sns->blkconf.blk, nvme_l2b(sns, slba), > + iocb->aiocb = blk_aio_preadv(sns->blkconf.blk, offset, > &iocb->iov, 0, nvme_copy_in_cb, iocb); > return; > > @@ -3524,7 +3531,7 @@ static uint16_t nvme_compare(NvmeCtrl *n, NvmeRequest *req) > qemu_iovec_init(&ctx->data.iov, 1); > qemu_iovec_add(&ctx->data.iov, ctx->data.bounce, data_len); > > - block_acct_start(blk_get_stats(blk), &req->acct, data_len, > + block_acct_start(blk_get_stats(blk), &req->acct, offset, data_len, > BLOCK_ACCT_READ); > req->aiocb = blk_aio_preadv(blk, offset, &ctx->data.iov, 0, > nvme_compare_data_cb, req); > @@ -3722,7 +3729,7 @@ static uint16_t nvme_read(NvmeCtrl *n, NvmeRequest *req) > > data_offset = nvme_l2b(ns, slba); > > - block_acct_start(blk_get_stats(blk), &req->acct, data_size, > + block_acct_start(blk_get_stats(blk), &req->acct, data_offset, data_size, > BLOCK_ACCT_READ); > nvme_blk_read(blk, data_offset, BDRV_SECTOR_SIZE, nvme_rw_cb, req); > return NVME_NO_COMPLETE; > @@ -3892,7 +3899,7 @@ static uint16_t nvme_do_write(NvmeCtrl *n, NvmeRequest *req, bool append, > goto invalid; > } > > - block_acct_start(blk_get_stats(blk), &req->acct, data_size, > + block_acct_start(blk_get_stats(blk), &req->acct, data_offset, data_size, > BLOCK_ACCT_WRITE); > nvme_blk_write(blk, data_offset, BDRV_SECTOR_SIZE, nvme_rw_cb, req); > } else { > diff --git a/hw/nvme/dif.c b/hw/nvme/dif.c > index 4e7874f3223..fbd4d8eb816 100644 > --- a/hw/nvme/dif.c > +++ b/hw/nvme/dif.c > @@ -644,8 +644,8 @@ uint16_t nvme_dif_rw(NvmeCtrl *n, NvmeRequest *req) > qemu_iovec_add(&ctx->data.iov, ctx->data.bounce, len); > > if (req->cmd.opcode == NVME_CMD_READ) { > - block_acct_start(blk_get_stats(blk), &req->acct, ctx->data.iov.size, > - BLOCK_ACCT_READ); > + block_acct_start(blk_get_stats(blk), &req->acct, offset, > + ctx->data.iov.size, BLOCK_ACCT_READ); > > req->aiocb = blk_aio_preadv(ns->blkconf.blk, offset, &ctx->data.iov, 0, > nvme_dif_rw_mdata_in_cb, ctx); > @@ -690,8 +690,8 @@ uint16_t nvme_dif_rw(NvmeCtrl *n, NvmeRequest *req) > } > } > > - block_acct_start(blk_get_stats(blk), &req->acct, ctx->data.iov.size, > - BLOCK_ACCT_WRITE); > + block_acct_start(blk_get_stats(blk), &req->acct, offset, > + ctx->data.iov.size, BLOCK_ACCT_WRITE); > > req->aiocb = blk_aio_pwritev(ns->blkconf.blk, offset, &ctx->data.iov, 0, > nvme_dif_rw_mdata_out_cb, ctx); > diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c > index 24bd1a78b6e..5b40e404e13 100644 > --- a/hw/scsi/scsi-disk.c > +++ b/hw/scsi/scsi-disk.c > @@ -484,7 +484,8 @@ static void scsi_do_read(SCSIDiskReq *r, int ret) > scsi_req_ref(&r->req); > > if (r->req.sg) { > - dma_acct_start(s->qdev.conf.blk, &r->acct, r->req.sg, BLOCK_ACCT_READ); > + dma_acct_start(s->qdev.conf.blk, &r->acct, r->req.sg, > + r->sector << BDRV_SECTOR_BITS, BLOCK_ACCT_READ); > r->req.residual -= r->req.sg->size; > r->req.aiocb = dma_blk_io(r->req.sg, r->sector << BDRV_SECTOR_BITS, > BDRV_SECTOR_SIZE, > @@ -493,7 +494,8 @@ static void scsi_do_read(SCSIDiskReq *r, int ret) > } else { > scsi_init_iovec(r, SCSI_DMA_BUF_SIZE); > block_acct_start(blk_get_stats(s->qdev.conf.blk), &r->acct, > - r->qiov.size, BLOCK_ACCT_READ); > + r->sector << BDRV_SECTOR_BITS, r->qiov.size, > + BLOCK_ACCT_READ); > r->req.aiocb = sdc->dma_readv(r->sector << BDRV_SECTOR_BITS, &r->qiov, > scsi_read_complete, r, r); > } > @@ -551,7 +553,7 @@ static void scsi_read_data(SCSIRequest *req) > first = !r->started; > r->started = true; > if (first && r->need_fua) { > - block_acct_start(blk_get_stats(s->qdev.conf.blk), &r->acct, 0, > + block_acct_start(blk_get_stats(s->qdev.conf.blk), &r->acct, -1, 0, > BLOCK_ACCT_FLUSH); > r->req.aiocb = blk_aio_flush(s->qdev.conf.blk, scsi_do_read_cb, r); > } else { > @@ -634,7 +636,7 @@ static void scsi_write_data(SCSIRequest *req) > > if (r->req.cmd.buf[0] == VERIFY_10 || r->req.cmd.buf[0] == VERIFY_12 || > r->req.cmd.buf[0] == VERIFY_16) { > - block_acct_start(blk_get_stats(s->qdev.conf.blk), &r->acct, 0, > + block_acct_start(blk_get_stats(s->qdev.conf.blk), &r->acct, -1, 0, > BLOCK_ACCT_FLUSH); > cb = r->req.sg ? scsi_dma_complete : scsi_write_complete; > r->req.aiocb = blk_aio_flush(s->qdev.conf.blk, cb, r); > @@ -642,7 +644,8 @@ static void scsi_write_data(SCSIRequest *req) > } > > if (r->req.sg) { > - dma_acct_start(s->qdev.conf.blk, &r->acct, r->req.sg, BLOCK_ACCT_WRITE); > + dma_acct_start(s->qdev.conf.blk, &r->acct, r->req.sg, > + r->sector << BDRV_SECTOR_BITS, BLOCK_ACCT_WRITE); > r->req.residual -= r->req.sg->size; > r->req.aiocb = dma_blk_io(r->req.sg, r->sector << BDRV_SECTOR_BITS, > BDRV_SECTOR_SIZE, > @@ -650,7 +653,8 @@ static void scsi_write_data(SCSIRequest *req) > DMA_DIRECTION_TO_DEVICE); > } else { > block_acct_start(blk_get_stats(s->qdev.conf.blk), &r->acct, > - r->qiov.size, BLOCK_ACCT_WRITE); > + r->sector << BDRV_SECTOR_BITS, r->qiov.size, > + BLOCK_ACCT_WRITE); > r->req.aiocb = sdc->dma_writev(r->sector << BDRV_SECTOR_BITS, &r->qiov, > scsi_write_complete, r, r); > } > @@ -1710,7 +1714,7 @@ static void scsi_disk_emulate_mode_select(SCSIDiskReq *r, uint8_t *inbuf) > if (!blk_enable_write_cache(s->qdev.conf.blk)) { > /* The request is used as the AIO opaque value, so add a ref. */ > scsi_req_ref(&r->req); > - block_acct_start(blk_get_stats(s->qdev.conf.blk), &r->acct, 0, > + block_acct_start(blk_get_stats(s->qdev.conf.blk), &r->acct, -1, 0, > BLOCK_ACCT_FLUSH); > r->req.aiocb = blk_aio_flush(s->qdev.conf.blk, scsi_aio_complete, r); > return; > @@ -1777,6 +1781,7 @@ static void scsi_unmap_complete_noio(UnmapCBData *data, int ret) > } > > block_acct_start(blk_get_stats(s->qdev.conf.blk), &r->acct, > + r->sector * BDRV_SECTOR_SIZE, > r->sector_count * BDRV_SECTOR_SIZE, > BLOCK_ACCT_UNMAP); > > @@ -1894,7 +1899,8 @@ static void scsi_write_same_complete(void *opaque, int ret) > data->iov.iov_len); > if (data->iov.iov_len) { > block_acct_start(blk_get_stats(s->qdev.conf.blk), &r->acct, > - data->iov.iov_len, BLOCK_ACCT_WRITE); > + data->sector << BDRV_SECTOR_BITS, data->iov.iov_len, > + BLOCK_ACCT_WRITE); > /* Reinitialize qiov, to handle unaligned WRITE SAME request > * where final qiov may need smaller size */ > qemu_iovec_init_external(&data->qiov, &data->iov, 1); > @@ -1943,6 +1949,7 @@ static void scsi_disk_emulate_write_same(SCSIDiskReq *r, uint8_t *inbuf) > /* The request is used as the AIO opaque value, so add a ref. */ > scsi_req_ref(&r->req); > block_acct_start(blk_get_stats(s->qdev.conf.blk), &r->acct, > + r->req.cmd.lba * s->qdev.blocksize, > nb_sectors * s->qdev.blocksize, > BLOCK_ACCT_WRITE); > r->req.aiocb = blk_aio_pwrite_zeroes(s->qdev.conf.blk, > @@ -1969,7 +1976,8 @@ static void scsi_disk_emulate_write_same(SCSIDiskReq *r, uint8_t *inbuf) > > scsi_req_ref(&r->req); > block_acct_start(blk_get_stats(s->qdev.conf.blk), &r->acct, > - data->iov.iov_len, BLOCK_ACCT_WRITE); > + data->sector << BDRV_SECTOR_BITS, data->iov.iov_len, > + BLOCK_ACCT_WRITE); > r->req.aiocb = blk_aio_pwritev(s->qdev.conf.blk, > data->sector << BDRV_SECTOR_BITS, > &data->qiov, 0, > @@ -2237,7 +2245,7 @@ static int32_t scsi_disk_emulate_command(SCSIRequest *req, uint8_t *buf) > case SYNCHRONIZE_CACHE: > /* The request is used as the AIO opaque value, so add a ref. */ > scsi_req_ref(&r->req); > - block_acct_start(blk_get_stats(s->qdev.conf.blk), &r->acct, 0, > + block_acct_start(blk_get_stats(s->qdev.conf.blk), &r->acct, -1, 0, > BLOCK_ACCT_FLUSH); > r->req.aiocb = blk_aio_flush(s->qdev.conf.blk, scsi_aio_complete, r); > return 0; > diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c > index de4c1966fea..655dfae9057 100644 > --- a/qemu-io-cmds.c > +++ b/qemu-io-cmds.c > @@ -1538,8 +1538,8 @@ static int aio_read_f(BlockBackend *blk, int argc, char **argv) > } > > clock_gettime(CLOCK_MONOTONIC, &ctx->t1); > - block_acct_start(blk_get_stats(blk), &ctx->acct, ctx->qiov.size, > - BLOCK_ACCT_READ); > + block_acct_start(blk_get_stats(blk), &ctx->acct, ctx->offset, > + ctx->qiov.size, BLOCK_ACCT_READ); > blk_aio_preadv(blk, ctx->offset, &ctx->qiov, ctx->flags, aio_read_done, > ctx); > return 0; > @@ -1693,8 +1693,8 @@ static int aio_write_f(BlockBackend *blk, int argc, char **argv) > } > > clock_gettime(CLOCK_MONOTONIC, &ctx->t1); > - block_acct_start(blk_get_stats(blk), &ctx->acct, ctx->qiov.size, > - BLOCK_ACCT_WRITE); > + block_acct_start(blk_get_stats(blk), &ctx->acct, ctx->offset, > + ctx->qiov.size, BLOCK_ACCT_WRITE); > > blk_aio_pwritev(blk, ctx->offset, &ctx->qiov, ctx->flags, > aio_write_done, ctx); > @@ -1706,7 +1706,7 @@ static int aio_write_f(BlockBackend *blk, int argc, char **argv) > static int aio_flush_f(BlockBackend *blk, int argc, char **argv) > { > BlockAcctCookie cookie; > - block_acct_start(blk_get_stats(blk), &cookie, 0, BLOCK_ACCT_FLUSH); > + block_acct_start(blk_get_stats(blk), &cookie, -1, 0, BLOCK_ACCT_FLUSH); > blk_drain_all(); > block_acct_done(blk_get_stats(blk), &cookie); > return 0; > @@ -2325,8 +2325,8 @@ static int aio_discard_f(BlockBackend *blk, int argc, char **argv) > > clock_gettime(CLOCK_MONOTONIC, &ctx->t1); > ctx->qiov.size = count; > - block_acct_start(blk_get_stats(blk), &ctx->acct, ctx->qiov.size, > - BLOCK_ACCT_UNMAP); > + block_acct_start(blk_get_stats(blk), &ctx->acct, ctx->offset, > + ctx->qiov.size, BLOCK_ACCT_UNMAP); > blk_aio_pdiscard(blk, ctx->offset, count, aio_discard_done, ctx); > > return 0; > diff --git a/system/dma-helpers.c b/system/dma-helpers.c > index 0d592f64680..7bf41119ebc 100644 > --- a/system/dma-helpers.c > +++ b/system/dma-helpers.c > @@ -315,9 +315,9 @@ MemTxResult dma_buf_write(void *ptr, dma_addr_t len, dma_addr_t *residual, > } > > void dma_acct_start(BlockBackend *blk, BlockAcctCookie *cookie, > - QEMUSGList *sg, enum BlockAcctType type) > + QEMUSGList *sg, int64_t offset, enum BlockAcctType type) > { > - block_acct_start(blk_get_stats(blk), cookie, sg->size, type); > + block_acct_start(blk_get_stats(blk), cookie, offset, sg->size, type); > } > > uint64_t dma_aligned_pow2_mask(uint64_t start, uint64_t end, int max_addr_bits) > diff --git a/tests/unit/test-block-accounting.c b/tests/unit/test-block-accounting.c > index 7aae491cfc2..0081502891c 100644 > --- a/tests/unit/test-block-accounting.c > +++ b/tests/unit/test-block-accounting.c > @@ -63,7 +63,7 @@ static void *reader_thread(void *opaque) > while (!qatomic_read(&stop_workers)) { > BlockAcctCookie cookie; > > - block_acct_start(stats, &cookie, 4096, BLOCK_ACCT_READ); > + block_acct_start(stats, &cookie, -1, 4096, BLOCK_ACCT_READ); > block_acct_done(stats, &cookie); > } > > -- > 2.55.0 > > > For the parts touching hw/nvme/* Acked-by: Jesper Wendel Devantier <foss@defmacro.it> ^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH 2/9] qapi/block: Add IoAccountingOperation enum 2026-08-31 13:51 [PATCH 0/9] block: BLOCK_IO_DELAY event Hanna Czenczek 2026-08-31 13:51 ` [PATCH 1/9] block/accounting: Add offset to BlockAcctCookie Hanna Czenczek @ 2026-08-31 13:51 ` Hanna Czenczek 2026-09-03 14:32 ` Markus Armbruster 2026-08-31 13:51 ` [PATCH 3/9] qapi/block: Add BLOCK_IO_DELAY event Hanna Czenczek ` (7 subsequent siblings) 9 siblings, 1 reply; 47+ messages in thread From: Hanna Czenczek @ 2026-08-31 13:51 UTC (permalink / raw) To: qemu-block Cc: qemu-devel, Hanna Czenczek, Kevin Wolf, John Snow, Denis V . Lunev, Eric Blake, Markus Armbruster, Stefan Hajnoczi This enum gives more information than IoOperationType (which is used for the rerror/werror distinction) to identify operation types for block accounting. Signed-off-by: Hanna Czenczek <hreitz@redhat.com> --- qapi/block.json | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/qapi/block.json b/qapi/block.json index 46955bbb3e3..456118eb0b4 100644 --- a/qapi/block.json +++ b/qapi/block.json @@ -603,3 +603,25 @@ '*boundaries-zap': ['uint64'], '*boundaries-flush': ['uint64'] }, 'allow-preconfig': true } + +## +# @IoAccountingOperation: +# +# A more finely grained enumeration of I/O operation types beyond what +# `IoOperationType` provides (whose read/write distinction matches the +# rerror/werror policy settings). +# +# @read: Reading data +# +# @write: Writing data +# +# @flush: Flushing data to disk +# +# @zone-append: Appending data to a zone of a zoned block device +# +# @unmap: Discarding a block of data +# +# Since: 11.2 +## +{ 'enum': 'IoAccountingOperation', + 'data': [ 'read', 'write', 'flush', 'zone-append', 'unmap' ] } -- 2.55.0 ^ permalink raw reply related [flat|nested] 47+ messages in thread
* Re: [PATCH 2/9] qapi/block: Add IoAccountingOperation enum 2026-08-31 13:51 ` [PATCH 2/9] qapi/block: Add IoAccountingOperation enum Hanna Czenczek @ 2026-09-03 14:32 ` Markus Armbruster 2026-09-16 8:09 ` Hanna Czenczek 0 siblings, 1 reply; 47+ messages in thread From: Markus Armbruster @ 2026-09-03 14:32 UTC (permalink / raw) To: Hanna Czenczek Cc: qemu-block, qemu-devel, Kevin Wolf, John Snow, Denis V . Lunev, Eric Blake, Stefan Hajnoczi Hanna Czenczek <hreitz@redhat.com> writes: > This enum gives more information than IoOperationType (which is used for > the rerror/werror distinction) to identify operation types for block > accounting. > > Signed-off-by: Hanna Czenczek <hreitz@redhat.com> > --- > qapi/block.json | 22 ++++++++++++++++++++++ > 1 file changed, 22 insertions(+) > > diff --git a/qapi/block.json b/qapi/block.json > index 46955bbb3e3..456118eb0b4 100644 > --- a/qapi/block.json > +++ b/qapi/block.json > @@ -603,3 +603,25 @@ > '*boundaries-zap': ['uint64'], > '*boundaries-flush': ['uint64'] }, > 'allow-preconfig': true } > + > +## > +# @IoAccountingOperation: > +# > +# A more finely grained enumeration of I/O operation types beyond what > +# `IoOperationType` provides (whose read/write distinction matches the > +# rerror/werror policy settings). Why is the comparison to IoOperationType useful? If making the connection between IoOperationType values and "rerror/werror policy setting" is useful, why isn't it made in IoOperationType's docs? What settings exactly? Could this be a link? > +# > +# @read: Reading data > +# > +# @write: Writing data > +# > +# @flush: Flushing data to disk > +# > +# @zone-append: Appending data to a zone of a zoned block device > +# > +# @unmap: Discarding a block of data > +# > +# Since: 11.2 > +## > +{ 'enum': 'IoAccountingOperation', > + 'data': [ 'read', 'write', 'flush', 'zone-append', 'unmap' ] } ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 2/9] qapi/block: Add IoAccountingOperation enum 2026-09-03 14:32 ` Markus Armbruster @ 2026-09-16 8:09 ` Hanna Czenczek 0 siblings, 0 replies; 47+ messages in thread From: Hanna Czenczek @ 2026-09-16 8:09 UTC (permalink / raw) To: Markus Armbruster Cc: qemu-block, qemu-devel, Kevin Wolf, John Snow, Denis V . Lunev, Eric Blake, Stefan Hajnoczi On 03.09.26 16:32, Markus Armbruster wrote: > Hanna Czenczek <hreitz@redhat.com> writes: > >> This enum gives more information than IoOperationType (which is used for >> the rerror/werror distinction) to identify operation types for block >> accounting. >> >> Signed-off-by: Hanna Czenczek <hreitz@redhat.com> >> --- >> qapi/block.json | 22 ++++++++++++++++++++++ >> 1 file changed, 22 insertions(+) >> >> diff --git a/qapi/block.json b/qapi/block.json >> index 46955bbb3e3..456118eb0b4 100644 >> --- a/qapi/block.json >> +++ b/qapi/block.json >> @@ -603,3 +603,25 @@ >> '*boundaries-zap': ['uint64'], >> '*boundaries-flush': ['uint64'] }, >> 'allow-preconfig': true } >> + >> +## >> +# @IoAccountingOperation: >> +# >> +# A more finely grained enumeration of I/O operation types beyond what >> +# `IoOperationType` provides (whose read/write distinction matches the >> +# rerror/werror policy settings). > Why is the comparison to IoOperationType useful? Because the `IoOperationType` exists, and in my mind, it would be better to have a single enum for all types. Now, to reason why a new one is introduced, the commit message here would be sufficient. But I felt it better to document for users also why there are two types. > If making the connection between IoOperationType values and > "rerror/werror policy setting" is useful, why isn't it made in > IoOperationType's docs? That is true! I should move it there. > What settings exactly? Could this be a link? Probably. I will take the look. (The block device’s rerror/werror settings.) Hanna >> +# >> +# @read: Reading data >> +# >> +# @write: Writing data >> +# >> +# @flush: Flushing data to disk >> +# >> +# @zone-append: Appending data to a zone of a zoned block device >> +# >> +# @unmap: Discarding a block of data >> +# >> +# Since: 11.2 >> +## >> +{ 'enum': 'IoAccountingOperation', >> + 'data': [ 'read', 'write', 'flush', 'zone-append', 'unmap' ] } ^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH 3/9] qapi/block: Add BLOCK_IO_DELAY event 2026-08-31 13:51 [PATCH 0/9] block: BLOCK_IO_DELAY event Hanna Czenczek 2026-08-31 13:51 ` [PATCH 1/9] block/accounting: Add offset to BlockAcctCookie Hanna Czenczek 2026-08-31 13:51 ` [PATCH 2/9] qapi/block: Add IoAccountingOperation enum Hanna Czenczek @ 2026-08-31 13:51 ` Hanna Czenczek 2026-09-03 14:36 ` Markus Armbruster 2026-08-31 13:52 ` [PATCH 4/9] block-backend: Public blk_get_attached_dev_path() Hanna Czenczek ` (6 subsequent siblings) 9 siblings, 1 reply; 47+ messages in thread From: Hanna Czenczek @ 2026-08-31 13:51 UTC (permalink / raw) To: qemu-block Cc: qemu-devel, Hanna Czenczek, Kevin Wolf, John Snow, Denis V . Lunev, Eric Blake, Markus Armbruster, Stefan Hajnoczi This event will be emitted when a disk operation takes longer than a pre-defined threshold (as set on the block device). Signed-off-by: Hanna Czenczek <hreitz@redhat.com> --- qapi/block.json | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/qapi/block.json b/qapi/block.json index 456118eb0b4..1cdaffd5520 100644 --- a/qapi/block.json +++ b/qapi/block.json @@ -625,3 +625,33 @@ ## { 'enum': 'IoAccountingOperation', 'data': [ 'read', 'write', 'flush', 'zone-append', 'unmap' ] } + +## +# @BLOCK_IO_DELAY: +# +# Emitted when a disk operation takes longer than the pre-defined +# threshold (as set via the delay-alert-ms property on the block +# device). +# +# Note that delays deliberately incurred by throttling do count +# towards the operation duration. +# +# @qom-path: path to the device object in the QOM tree +# +# @operation: I/O operation +# +# @duration: Ongoing duration of the operation (in seconds) +# +# @offset: Request offset in the guest disk (in bytes), if available +# +# @bytes: Request length +# +# Since: 11.2 +## +{ 'event': 'BLOCK_IO_DELAY', + 'data': { + 'qom-path': 'str', + 'operation': 'IoAccountingOperation', + 'duration': 'number', + '*offset': 'uint64', + 'bytes': 'uint64' } } -- 2.55.0 ^ permalink raw reply related [flat|nested] 47+ messages in thread
* Re: [PATCH 3/9] qapi/block: Add BLOCK_IO_DELAY event 2026-08-31 13:51 ` [PATCH 3/9] qapi/block: Add BLOCK_IO_DELAY event Hanna Czenczek @ 2026-09-03 14:36 ` Markus Armbruster 2026-09-16 8:08 ` Hanna Czenczek 0 siblings, 1 reply; 47+ messages in thread From: Markus Armbruster @ 2026-09-03 14:36 UTC (permalink / raw) To: Hanna Czenczek Cc: qemu-block, qemu-devel, Kevin Wolf, John Snow, Denis V . Lunev, Eric Blake, Stefan Hajnoczi Hanna Czenczek <hreitz@redhat.com> writes: > This event will be emitted when a disk operation takes longer than a > pre-defined threshold (as set on the block device). > > Signed-off-by: Hanna Czenczek <hreitz@redhat.com> > --- > qapi/block.json | 30 ++++++++++++++++++++++++++++++ > 1 file changed, 30 insertions(+) > > diff --git a/qapi/block.json b/qapi/block.json > index 456118eb0b4..1cdaffd5520 100644 > --- a/qapi/block.json > +++ b/qapi/block.json > @@ -625,3 +625,33 @@ > ## > { 'enum': 'IoAccountingOperation', > 'data': [ 'read', 'write', 'flush', 'zone-append', 'unmap' ] } > + > +## > +# @BLOCK_IO_DELAY: > +# > +# Emitted when a disk operation takes longer than the pre-defined > +# threshold (as set via the delay-alert-ms property on the block > +# device). Double-quotes around delay-alert-ms, please. "The block device" is the one at @qom-path? As far as I can tell, this property doesn't yet exist at this point. Fine, but please mention the fact in the commit message. > +# > +# Note that delays deliberately incurred by throttling do count > +# towards the operation duration. > +# > +# @qom-path: path to the device object in the QOM tree > +# > +# @operation: I/O operation > +# > +# @duration: Ongoing duration of the operation (in seconds) > +# > +# @offset: Request offset in the guest disk (in bytes), if available > +# > +# @bytes: Request length > +# > +# Since: 11.2 > +## > +{ 'event': 'BLOCK_IO_DELAY', > + 'data': { > + 'qom-path': 'str', > + 'operation': 'IoAccountingOperation', > + 'duration': 'number', > + '*offset': 'uint64', > + 'bytes': 'uint64' } } ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 3/9] qapi/block: Add BLOCK_IO_DELAY event 2026-09-03 14:36 ` Markus Armbruster @ 2026-09-16 8:08 ` Hanna Czenczek 2026-09-16 9:57 ` Markus Armbruster 0 siblings, 1 reply; 47+ messages in thread From: Hanna Czenczek @ 2026-09-16 8:08 UTC (permalink / raw) To: Markus Armbruster Cc: qemu-block, qemu-devel, Kevin Wolf, John Snow, Denis V . Lunev, Eric Blake, Stefan Hajnoczi On 03.09.26 16:36, Markus Armbruster wrote: > Hanna Czenczek <hreitz@redhat.com> writes: > >> This event will be emitted when a disk operation takes longer than a >> pre-defined threshold (as set on the block device). >> >> Signed-off-by: Hanna Czenczek <hreitz@redhat.com> >> --- >> qapi/block.json | 30 ++++++++++++++++++++++++++++++ >> 1 file changed, 30 insertions(+) >> >> diff --git a/qapi/block.json b/qapi/block.json >> index 456118eb0b4..1cdaffd5520 100644 >> --- a/qapi/block.json >> +++ b/qapi/block.json >> @@ -625,3 +625,33 @@ >> ## >> { 'enum': 'IoAccountingOperation', >> 'data': [ 'read', 'write', 'flush', 'zone-append', 'unmap' ] } >> + >> +## >> +# @BLOCK_IO_DELAY: >> +# >> +# Emitted when a disk operation takes longer than the pre-defined >> +# threshold (as set via the delay-alert-ms property on the block >> +# device). > Double-quotes around delay-alert-ms, please. Sure. > "The block device" is the one at @qom-path? Yes, exactly. > As far as I can tell, this property doesn't yet exist at this point. > Fine, but please mention the fact in the commit message. I will! Thanks for reviewing, Hanna >> +# >> +# Note that delays deliberately incurred by throttling do count >> +# towards the operation duration. >> +# >> +# @qom-path: path to the device object in the QOM tree >> +# >> +# @operation: I/O operation >> +# >> +# @duration: Ongoing duration of the operation (in seconds) >> +# >> +# @offset: Request offset in the guest disk (in bytes), if available >> +# >> +# @bytes: Request length >> +# >> +# Since: 11.2 >> +## >> +{ 'event': 'BLOCK_IO_DELAY', >> + 'data': { >> + 'qom-path': 'str', >> + 'operation': 'IoAccountingOperation', >> + 'duration': 'number', >> + '*offset': 'uint64', >> + 'bytes': 'uint64' } } ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 3/9] qapi/block: Add BLOCK_IO_DELAY event 2026-09-16 8:08 ` Hanna Czenczek @ 2026-09-16 9:57 ` Markus Armbruster 0 siblings, 0 replies; 47+ messages in thread From: Markus Armbruster @ 2026-09-16 9:57 UTC (permalink / raw) To: Hanna Czenczek Cc: qemu-block, qemu-devel, Kevin Wolf, John Snow, Denis V . Lunev, Eric Blake, Stefan Hajnoczi Hanna Czenczek <hreitz@redhat.com> writes: > On 03.09.26 16:36, Markus Armbruster wrote: >> Hanna Czenczek <hreitz@redhat.com> writes: >> >>> This event will be emitted when a disk operation takes longer than a >>> pre-defined threshold (as set on the block device). >>> >>> Signed-off-by: Hanna Czenczek <hreitz@redhat.com> >>> --- >>> qapi/block.json | 30 ++++++++++++++++++++++++++++++ >>> 1 file changed, 30 insertions(+) >>> >>> diff --git a/qapi/block.json b/qapi/block.json >>> index 456118eb0b4..1cdaffd5520 100644 >>> --- a/qapi/block.json >>> +++ b/qapi/block.json >>> @@ -625,3 +625,33 @@ >>> ## >>> { 'enum': 'IoAccountingOperation', >>> 'data': [ 'read', 'write', 'flush', 'zone-append', 'unmap' ] } >>> + >>> +## >>> +# @BLOCK_IO_DELAY: >>> +# >>> +# Emitted when a disk operation takes longer than the pre-defined >>> +# threshold (as set via the delay-alert-ms property on the block >>> +# device). >> Double-quotes around delay-alert-ms, please. > > Sure. > >> "The block device" is the one at @qom-path? > > Yes, exactly. Perhaps tweak @qom-path's description: "the block device object". >> As far as I can tell, this property doesn't yet exist at this point. >> Fine, but please mention the fact in the commit message. > > I will! > > Thanks for reviewing, > > Hanna > >>> +# >>> +# Note that delays deliberately incurred by throttling do count >>> +# towards the operation duration. >>> +# >>> +# @qom-path: path to the device object in the QOM tree >>> +# >>> +# @operation: I/O operation >>> +# >>> +# @duration: Ongoing duration of the operation (in seconds) >>> +# >>> +# @offset: Request offset in the guest disk (in bytes), if available >>> +# >>> +# @bytes: Request length >>> +# >>> +# Since: 11.2 >>> +## >>> +{ 'event': 'BLOCK_IO_DELAY', >>> + 'data': { >>> + 'qom-path': 'str', >>> + 'operation': 'IoAccountingOperation', >>> + 'duration': 'number', >>> + '*offset': 'uint64', >>> + 'bytes': 'uint64' } } ^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH 4/9] block-backend: Public blk_get_attached_dev_path() 2026-08-31 13:51 [PATCH 0/9] block: BLOCK_IO_DELAY event Hanna Czenczek ` (2 preceding siblings ...) 2026-08-31 13:51 ` [PATCH 3/9] qapi/block: Add BLOCK_IO_DELAY event Hanna Czenczek @ 2026-08-31 13:52 ` Hanna Czenczek 2026-08-31 13:52 ` [PATCH 5/9] block/accounting: Add BB field to latency checker Hanna Czenczek ` (5 subsequent siblings) 9 siblings, 0 replies; 47+ messages in thread From: Hanna Czenczek @ 2026-08-31 13:52 UTC (permalink / raw) To: qemu-block Cc: qemu-devel, Hanna Czenczek, Kevin Wolf, John Snow, Denis V . Lunev, Eric Blake, Markus Armbruster, Stefan Hajnoczi Make blk_get_attached_dev_path() public so we can use it in the I/O operation delay QMP event to identify the affected guest block device. Signed-off-by: Hanna Czenczek <hreitz@redhat.com> --- include/system/block-backend-io.h | 9 +++++++++ block/block-backend.c | 6 +----- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/include/system/block-backend-io.h b/include/system/block-backend-io.h index fd84723d9d0..63b2dd2e24f 100644 --- a/include/system/block-backend-io.h +++ b/include/system/block-backend-io.h @@ -41,6 +41,15 @@ bool blk_iostatus_is_enabled(const BlockBackend *blk); */ char *blk_get_attached_dev_id(BlockBackend *blk); +/* + * Return the QOM path of the block device attached to the + * BlockBackend. + * + * The caller is responsible for releasing the value returned + * with g_free() after use. + */ +char *blk_get_attached_dev_path(BlockBackend *blk); + BlockAIOCB *blk_aio_pwrite_zeroes(BlockBackend *blk, int64_t offset, int64_t bytes, BdrvRequestFlags flags, BlockCompletionFunc *cb, void *opaque); diff --git a/block/block-backend.c b/block/block-backend.c index 37ba7e9fc40..164bda846f4 100644 --- a/block/block-backend.c +++ b/block/block-backend.c @@ -1053,11 +1053,7 @@ char *blk_get_attached_dev_id(BlockBackend *blk) return blk_get_attached_dev_id_or_path(blk, true); } -/* - * The caller is responsible for releasing the value returned - * with g_free() after use. - */ -static char *blk_get_attached_dev_path(BlockBackend *blk) +char *blk_get_attached_dev_path(BlockBackend *blk) { return blk_get_attached_dev_id_or_path(blk, false); } -- 2.55.0 ^ permalink raw reply related [flat|nested] 47+ messages in thread
* [PATCH 5/9] block/accounting: Add BB field to latency checker 2026-08-31 13:51 [PATCH 0/9] block: BLOCK_IO_DELAY event Hanna Czenczek ` (3 preceding siblings ...) 2026-08-31 13:52 ` [PATCH 4/9] block-backend: Public blk_get_attached_dev_path() Hanna Czenczek @ 2026-08-31 13:52 ` Hanna Czenczek 2026-09-22 12:14 ` Kevin Wolf 2026-08-31 13:52 ` [PATCH 6/9] block/accounting: Emit BLOCK_IO_DELAY event Hanna Czenczek ` (4 subsequent siblings) 9 siblings, 1 reply; 47+ messages in thread From: Hanna Czenczek @ 2026-08-31 13:52 UTC (permalink / raw) To: qemu-block Cc: qemu-devel, Hanna Czenczek, Kevin Wolf, John Snow, Denis V . Lunev, Eric Blake, Markus Armbruster, Stefan Hajnoczi The BlockBackend reference is required to be able to generate the QOM path as part of emitted events. A weak reference is enough because BlockAcctStats is tied directly to one BlockBackend, and will be deleted via block_acct_cleanup() before the BlockBackend is truly deleted. Signed-off-by: Hanna Czenczek <hreitz@redhat.com> --- include/block/accounting.h | 3 ++- block/accounting.c | 7 ++++++- block/block-backend.c | 2 +- 3 files changed, 9 insertions(+), 3 deletions(-) diff --git a/include/block/accounting.h b/include/block/accounting.h index 9386440ec89..025536239e6 100644 --- a/include/block/accounting.h +++ b/include/block/accounting.h @@ -81,6 +81,7 @@ typedef struct BlockLatencyHistogram { struct BlockAcctStats { QemuMutex lock; + BlockBackend *blk; uint64_t nr_bytes[BLOCK_MAX_IOTYPE]; uint64_t nr_ops[BLOCK_MAX_IOTYPE]; uint64_t invalid_ops[BLOCK_MAX_IOTYPE]; @@ -101,7 +102,7 @@ typedef struct BlockAcctCookie { enum BlockAcctType type; } BlockAcctCookie; -void block_acct_init(BlockAcctStats *stats); +void block_acct_init(BlockBackend *blk, BlockAcctStats *stats); 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); diff --git a/block/accounting.c b/block/accounting.c index 66e5001403f..a74551d41f2 100644 --- a/block/accounting.c +++ b/block/accounting.c @@ -33,7 +33,7 @@ static QEMUClockType clock_type = QEMU_CLOCK_REALTIME; static const int qtest_latency_ns = NANOSECONDS_PER_SECOND / 1000; -void block_acct_init(BlockAcctStats *stats) +void block_acct_init(BlockBackend *blk, BlockAcctStats *stats) { qemu_mutex_init(&stats->lock); if (qtest_enabled()) { @@ -41,6 +41,11 @@ void block_acct_init(BlockAcctStats *stats) } stats->account_invalid = true; stats->account_failed = true; + /* + * No need to blk_ref() because block_acct_cleanup() is called exactly when + * the BB is deleted. + */ + stats->blk = blk; } static bool bool_from_onoffauto(OnOffAuto val, bool def) diff --git a/block/block-backend.c b/block/block-backend.c index 164bda846f4..ca091a8bf5d 100644 --- a/block/block-backend.c +++ b/block/block-backend.c @@ -369,7 +369,7 @@ BlockBackend *blk_new(AioContext *ctx, uint64_t perm, uint64_t shared_perm) blk->on_read_error = BLOCKDEV_ON_ERROR_REPORT; blk->on_write_error = BLOCKDEV_ON_ERROR_ENOSPC; - block_acct_init(&blk->stats); + block_acct_init(blk, &blk->stats); qemu_mutex_init(&blk->queued_requests_lock); qemu_co_queue_init(&blk->queued_requests); -- 2.55.0 ^ permalink raw reply related [flat|nested] 47+ messages in thread
* Re: [PATCH 5/9] block/accounting: Add BB field to latency checker 2026-08-31 13:52 ` [PATCH 5/9] block/accounting: Add BB field to latency checker Hanna Czenczek @ 2026-09-22 12:14 ` Kevin Wolf 2026-09-23 11:05 ` Hanna Czenczek 0 siblings, 1 reply; 47+ messages in thread From: Kevin Wolf @ 2026-09-22 12:14 UTC (permalink / raw) To: Hanna Czenczek Cc: qemu-block, qemu-devel, John Snow, Denis V . Lunev, Eric Blake, Markus Armbruster, Stefan Hajnoczi Am 31.08.2026 um 15:52 hat Hanna Czenczek geschrieben: > The BlockBackend reference is required to be able to generate the QOM > path as part of emitted events. > > A weak reference is enough because BlockAcctStats is tied directly to > one BlockBackend, and will be deleted via block_acct_cleanup() before > the BlockBackend is truly deleted. > > Signed-off-by: Hanna Czenczek <hreitz@redhat.com> The subject line is a bit confusing as there is no latency checker yet. Should it say "to BlockAcctStats"? > include/block/accounting.h | 3 ++- > block/accounting.c | 7 ++++++- > block/block-backend.c | 2 +- > 3 files changed, 9 insertions(+), 3 deletions(-) > diff --git a/block/block-backend.c b/block/block-backend.c > index 164bda846f4..ca091a8bf5d 100644 > --- a/block/block-backend.c > +++ b/block/block-backend.c > @@ -369,7 +369,7 @@ BlockBackend *blk_new(AioContext *ctx, uint64_t perm, uint64_t shared_perm) > blk->on_read_error = BLOCKDEV_ON_ERROR_REPORT; > blk->on_write_error = BLOCKDEV_ON_ERROR_ENOSPC; > > - block_acct_init(&blk->stats); > + block_acct_init(blk, &blk->stats); This is the only caller. I'm not sure what other callers could look like, but now that a BlockBackend is required for it, could the second parameter ever give any useful additional information (as opposed to block_acct_init() just accessing blk->stats internally now that it has blk)? Kevin ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 5/9] block/accounting: Add BB field to latency checker 2026-09-22 12:14 ` Kevin Wolf @ 2026-09-23 11:05 ` Hanna Czenczek 0 siblings, 0 replies; 47+ messages in thread From: Hanna Czenczek @ 2026-09-23 11:05 UTC (permalink / raw) To: Kevin Wolf Cc: qemu-block, qemu-devel, John Snow, Denis V . Lunev, Eric Blake, Markus Armbruster, Stefan Hajnoczi On 22.09.26 14:14, Kevin Wolf wrote: > Am 31.08.2026 um 15:52 hat Hanna Czenczek geschrieben: >> The BlockBackend reference is required to be able to generate the QOM >> path as part of emitted events. >> >> A weak reference is enough because BlockAcctStats is tied directly to >> one BlockBackend, and will be deleted via block_acct_cleanup() before >> the BlockBackend is truly deleted. >> >> Signed-off-by: Hanna Czenczek <hreitz@redhat.com> > The subject line is a bit confusing as there is no latency checker yet. > Should it say "to BlockAcctStats"? Oops. I fixed the body, but not the subject. Yes, it absolutely should, thanks! >> include/block/accounting.h | 3 ++- >> block/accounting.c | 7 ++++++- >> block/block-backend.c | 2 +- >> 3 files changed, 9 insertions(+), 3 deletions(-) >> diff --git a/block/block-backend.c b/block/block-backend.c >> index 164bda846f4..ca091a8bf5d 100644 >> --- a/block/block-backend.c >> +++ b/block/block-backend.c >> @@ -369,7 +369,7 @@ BlockBackend *blk_new(AioContext *ctx, uint64_t perm, uint64_t shared_perm) >> blk->on_read_error = BLOCKDEV_ON_ERROR_REPORT; >> blk->on_write_error = BLOCKDEV_ON_ERROR_ENOSPC; >> >> - block_acct_init(&blk->stats); >> + block_acct_init(blk, &blk->stats); > This is the only caller. I'm not sure what other callers could look > like, but now that a BlockBackend is required for it, could the second > parameter ever give any useful additional information (as opposed to > block_acct_init() just accessing blk->stats internally now that it has > blk)? I mean, to me personally it looks a bit nicer this way, because it makes it a bit clearer *what* is initialized. But you’re right. Functionally, there’s absolutely no point in passing both, so I’ll cut it down (depending on what the next version of this series is going to look like…). Hanna ^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH 6/9] block/accounting: Emit BLOCK_IO_DELAY event 2026-08-31 13:51 [PATCH 0/9] block: BLOCK_IO_DELAY event Hanna Czenczek ` (4 preceding siblings ...) 2026-08-31 13:52 ` [PATCH 5/9] block/accounting: Add BB field to latency checker Hanna Czenczek @ 2026-08-31 13:52 ` Hanna Czenczek 2026-09-03 14:23 ` Stefan Hajnoczi 2026-09-22 12:44 ` Kevin Wolf 2026-08-31 13:52 ` [PATCH 7/9] block: Add delay-alert-ms property Hanna Czenczek ` (3 subsequent siblings) 9 siblings, 2 replies; 47+ messages in thread From: Hanna Czenczek @ 2026-08-31 13:52 UTC (permalink / raw) To: qemu-block Cc: qemu-devel, Hanna Czenczek, Kevin Wolf, John Snow, Denis V . Lunev, Eric Blake, Markus Armbruster, Stefan Hajnoczi 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> --- 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; 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(); + } +} + 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; + + 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]++; diff --git a/blockdev.c b/blockdev.c index 6e86c6262f9..195bac8af01 100644 --- a/blockdev.c +++ b/blockdev.c @@ -618,7 +618,7 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts, bs->detect_zeroes = detect_zeroes; block_acct_setup(blk_get_stats(blk), account_invalid, account_failed, - NULL, 0, NULL); + NULL, 0, 0, NULL); if (!parse_stats_intervals(blk_get_stats(blk), interval_list, errp)) { blk_unref(blk); diff --git a/hw/block/block.c b/hw/block/block.c index f187fa025d0..19301c6f995 100644 --- a/hw/block/block.c +++ b/hw/block/block.c @@ -251,7 +251,7 @@ bool blkconf_apply_backend_options(BlockConf *conf, bool readonly, if (!block_acct_setup(blk_get_stats(blk), conf->account_invalid, conf->account_failed, conf->stats_intervals, - conf->num_stats_intervals, errp)) { + conf->num_stats_intervals, 0, errp)) { return false; } return true; -- 2.55.0 ^ permalink raw reply related [flat|nested] 47+ messages in thread
* Re: [PATCH 6/9] block/accounting: Emit BLOCK_IO_DELAY event 2026-08-31 13:52 ` [PATCH 6/9] block/accounting: Emit BLOCK_IO_DELAY event Hanna Czenczek @ 2026-09-03 14:23 ` Stefan Hajnoczi 2026-09-16 8:14 ` Hanna Czenczek 2026-09-22 12:44 ` Kevin Wolf 1 sibling, 1 reply; 47+ messages in thread From: Stefan Hajnoczi @ 2026-09-03 14:23 UTC (permalink / raw) To: Hanna Czenczek Cc: qemu-block, qemu-devel, Kevin Wolf, John Snow, Denis V . Lunev, Eric Blake, Markus Armbruster [-- Attachment #1: Type: text/plain, Size: 8036 bytes --] On Mon, Aug 31, 2026 at 03:52:02PM +0200, Hanna Czenczek wrote: > 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. I think this patch series already couples the accounting so closely with BlockBackend (i.e. adding the offset field into the cookie struct and adding a BB pointer into the stats struct) that we might as well fully integrate the two. Then callers don't need to manually manage cookies because BlockBackend does that internally and the concerns about lifetimes go away. > > 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> > --- > 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; > 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, There are a few names for the same thing: - delay_threshold_ns - alert_ns - BLOCK_IO_DELAY Pick one and use it consistently? > + 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(); > + } > +} > + > 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; > + > + 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]++; > diff --git a/blockdev.c b/blockdev.c > index 6e86c6262f9..195bac8af01 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -618,7 +618,7 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts, > bs->detect_zeroes = detect_zeroes; > > block_acct_setup(blk_get_stats(blk), account_invalid, account_failed, > - NULL, 0, NULL); > + NULL, 0, 0, NULL); > > if (!parse_stats_intervals(blk_get_stats(blk), interval_list, errp)) { > blk_unref(blk); > diff --git a/hw/block/block.c b/hw/block/block.c > index f187fa025d0..19301c6f995 100644 > --- a/hw/block/block.c > +++ b/hw/block/block.c > @@ -251,7 +251,7 @@ bool blkconf_apply_backend_options(BlockConf *conf, bool readonly, > > if (!block_acct_setup(blk_get_stats(blk), conf->account_invalid, > conf->account_failed, conf->stats_intervals, > - conf->num_stats_intervals, errp)) { > + conf->num_stats_intervals, 0, errp)) { > return false; > } > return true; > -- > 2.55.0 > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 6/9] block/accounting: Emit BLOCK_IO_DELAY event 2026-09-03 14:23 ` Stefan Hajnoczi @ 2026-09-16 8:14 ` Hanna Czenczek 2026-09-16 12:04 ` Hanna Czenczek 0 siblings, 1 reply; 47+ messages in thread From: Hanna Czenczek @ 2026-09-16 8:14 UTC (permalink / raw) To: Stefan Hajnoczi Cc: qemu-block, qemu-devel, Kevin Wolf, John Snow, Denis V . Lunev, Eric Blake, Markus Armbruster On 03.09.26 16:23, Stefan Hajnoczi wrote: > On Mon, Aug 31, 2026 at 03:52:02PM +0200, Hanna Czenczek wrote: >> 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. > I think this patch series already couples the accounting so closely with > BlockBackend (i.e. adding the offset field into the cookie struct and > adding a BB pointer into the stats struct) that we might as well fully > integrate the two. Then callers don't need to manually manage cookies > because BlockBackend does that internally and the concerns about > lifetimes go away. I don’t follow how integrating them into BlockBackend automatically solves the problem. Are you suggesting that blk_* I/O functions should do the accounting instead of the device emulation code? >> 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> >> --- >> 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; >> 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, > There are a few names for the same thing: > - delay_threshold_ns > - alert_ns > - BLOCK_IO_DELAY > > Pick one and use it consistently? Sure. Hanna >> + 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(); >> + } >> +} >> + >> 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; >> + >> + 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]++; >> diff --git a/blockdev.c b/blockdev.c >> index 6e86c6262f9..195bac8af01 100644 >> --- a/blockdev.c >> +++ b/blockdev.c >> @@ -618,7 +618,7 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts, >> bs->detect_zeroes = detect_zeroes; >> >> block_acct_setup(blk_get_stats(blk), account_invalid, account_failed, >> - NULL, 0, NULL); >> + NULL, 0, 0, NULL); >> >> if (!parse_stats_intervals(blk_get_stats(blk), interval_list, errp)) { >> blk_unref(blk); >> diff --git a/hw/block/block.c b/hw/block/block.c >> index f187fa025d0..19301c6f995 100644 >> --- a/hw/block/block.c >> +++ b/hw/block/block.c >> @@ -251,7 +251,7 @@ bool blkconf_apply_backend_options(BlockConf *conf, bool readonly, >> >> if (!block_acct_setup(blk_get_stats(blk), conf->account_invalid, >> conf->account_failed, conf->stats_intervals, >> - conf->num_stats_intervals, errp)) { >> + conf->num_stats_intervals, 0, errp)) { >> return false; >> } >> return true; >> -- >> 2.55.0 >> ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 6/9] block/accounting: Emit BLOCK_IO_DELAY event 2026-09-16 8:14 ` Hanna Czenczek @ 2026-09-16 12:04 ` Hanna Czenczek 2026-09-21 20:48 ` Stefan Hajnoczi 0 siblings, 1 reply; 47+ messages in thread From: Hanna Czenczek @ 2026-09-16 12:04 UTC (permalink / raw) To: Stefan Hajnoczi Cc: qemu-block, qemu-devel, Kevin Wolf, John Snow, Denis V . Lunev, Eric Blake, Markus Armbruster On 16.09.26 10:14, Hanna Czenczek wrote: > On 03.09.26 16:23, Stefan Hajnoczi wrote: >> On Mon, Aug 31, 2026 at 03:52:02PM +0200, Hanna Czenczek wrote: >>> 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. >> I think this patch series already couples the accounting so closely with >> BlockBackend (i.e. adding the offset field into the cookie struct and >> adding a BB pointer into the stats struct) that we might as well fully >> integrate the two. Then callers don't need to manually manage cookies >> because BlockBackend does that internally and the concerns about >> lifetimes go away. > > I don’t follow how integrating them into BlockBackend automatically > solves the problem. > > Are you suggesting that blk_* I/O functions should do the accounting > instead of the device emulation code? The thing is, AFAIU, doing that would cause changes in behavior, because the hardware device requests don’t always line up with the BB requests. What I could do would be to abandon the cookie-based approach altogether, of course; instead creating a completely different tracking object in those BB functions and put those into a list. And then we could decide at a later point if we want to integrate cookies with that. The only downside I see is that this would mean the latency reporting would not line up with the requests reported in the stats, or the histogram. Hanna ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 6/9] block/accounting: Emit BLOCK_IO_DELAY event 2026-09-16 12:04 ` Hanna Czenczek @ 2026-09-21 20:48 ` Stefan Hajnoczi 2026-09-23 10:45 ` Hanna Czenczek 0 siblings, 1 reply; 47+ messages in thread From: Stefan Hajnoczi @ 2026-09-21 20:48 UTC (permalink / raw) To: Hanna Czenczek Cc: qemu-block, qemu-devel, Kevin Wolf, John Snow, Denis V . Lunev, Eric Blake, Markus Armbruster [-- Attachment #1: Type: text/plain, Size: 2958 bytes --] On Wed, Sep 16, 2026 at 02:04:58PM +0200, Hanna Czenczek wrote: > On 16.09.26 10:14, Hanna Czenczek wrote: > > On 03.09.26 16:23, Stefan Hajnoczi wrote: > > > On Mon, Aug 31, 2026 at 03:52:02PM +0200, Hanna Czenczek wrote: > > > > 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. > > > I think this patch series already couples the accounting so closely with > > > BlockBackend (i.e. adding the offset field into the cookie struct and > > > adding a BB pointer into the stats struct) that we might as well fully > > > integrate the two. Then callers don't need to manually manage cookies > > > because BlockBackend does that internally and the concerns about > > > lifetimes go away. > > > > I don’t follow how integrating them into BlockBackend automatically > > solves the problem. > > > > Are you suggesting that blk_* I/O functions should do the accounting > > instead of the device emulation code? Yes. > > The thing is, AFAIU, doing that would cause changes in behavior, because the > hardware device requests don’t always line up with the BB requests. Did you find a fundamental incompatibility that rules out letting the block layer handles stats for blk_aio_*()? > What I could do would be to abandon the cookie-based approach altogether, of > course; instead creating a completely different tracking object in those BB > functions and put those into a list. And then we could decide at a later > point if we want to integrate cookies with that. The only downside I see is > that this would mean the latency reporting would not line up with the > requests reported in the stats, or the histogram. The tracking object could be a timer! :) Stefan [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 6/9] block/accounting: Emit BLOCK_IO_DELAY event 2026-09-21 20:48 ` Stefan Hajnoczi @ 2026-09-23 10:45 ` Hanna Czenczek 0 siblings, 0 replies; 47+ messages in thread From: Hanna Czenczek @ 2026-09-23 10:45 UTC (permalink / raw) To: Stefan Hajnoczi Cc: qemu-block, qemu-devel, Kevin Wolf, John Snow, Denis V . Lunev, Eric Blake, Markus Armbruster On 21.09.26 22:48, Stefan Hajnoczi wrote: > On Wed, Sep 16, 2026 at 02:04:58PM +0200, Hanna Czenczek wrote: >> On 16.09.26 10:14, Hanna Czenczek wrote: >>> On 03.09.26 16:23, Stefan Hajnoczi wrote: >>>> On Mon, Aug 31, 2026 at 03:52:02PM +0200, Hanna Czenczek wrote: >>>>> 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. >>>> I think this patch series already couples the accounting so closely with >>>> BlockBackend (i.e. adding the offset field into the cookie struct and >>>> adding a BB pointer into the stats struct) that we might as well fully >>>> integrate the two. Then callers don't need to manually manage cookies >>>> because BlockBackend does that internally and the concerns about >>>> lifetimes go away. >>> I don’t follow how integrating them into BlockBackend automatically >>> solves the problem. >>> >>> Are you suggesting that blk_* I/O functions should do the accounting >>> instead of the device emulation code? > Yes. OK, I see. >> The thing is, AFAIU, doing that would cause changes in behavior, because the >> hardware device requests don’t always line up with the BB requests. > Did you find a fundamental incompatibility that rules out letting the > block layer handles stats for blk_aio_*()? I mean, a simple thing is that reset requests are currently just not accounted, but they would be accounted then. Or repeated requests after VM stop/cont. There is also the fact that rerror/werror set to IGNORE means errors are currently accounted as 'done', not 'failed'. That makes… a little sense in the device models, but not really much sense in BB, I feel like. So I am not sure what you mean by “fundamental incompatibility”. I’m sure if we make it sufficiently ugly, we could reproduce all current peculiarities. We could use the chance to make it cleaner, but that would change the interface, and “cleaner” is always in the eye of the beholder. (Besides the fact that I feel like you are trying to have me open a can of worms, I feel like. :) ) >> What I could do would be to abandon the cookie-based approach altogether, of >> course; instead creating a completely different tracking object in those BB >> functions and put those into a list. And then we could decide at a later >> point if we want to integrate cookies with that. The only downside I see is >> that this would mean the latency reporting would not line up with the >> requests reported in the stats, or the histogram. > The tracking object could be a timer! :) The problem is not timer or sleep, the problem is the lifecycle. Hanna ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 6/9] block/accounting: Emit BLOCK_IO_DELAY event 2026-08-31 13:52 ` [PATCH 6/9] block/accounting: Emit BLOCK_IO_DELAY event Hanna Czenczek 2026-09-03 14:23 ` Stefan Hajnoczi @ 2026-09-22 12:44 ` Kevin Wolf 2026-09-23 11:11 ` Hanna Czenczek 1 sibling, 1 reply; 47+ messages in thread From: Kevin Wolf @ 2026-09-22 12:44 UTC (permalink / raw) To: Hanna Czenczek Cc: qemu-block, qemu-devel, John Snow, Denis V . Lunev, Eric Blake, Markus Armbruster, Stefan Hajnoczi 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. > 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? > 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? > 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. > + 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 ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 6/9] block/accounting: Emit BLOCK_IO_DELAY event 2026-09-22 12:44 ` Kevin Wolf @ 2026-09-23 11:11 ` Hanna Czenczek 0 siblings, 0 replies; 47+ messages in thread From: Hanna Czenczek @ 2026-09-23 11:11 UTC (permalink / raw) To: Kevin Wolf Cc: qemu-block, qemu-devel, John Snow, Denis V . Lunev, Eric Blake, Markus Armbruster, Stefan Hajnoczi 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 ^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH 7/9] block: Add delay-alert-ms property 2026-08-31 13:51 [PATCH 0/9] block: BLOCK_IO_DELAY event Hanna Czenczek ` (5 preceding siblings ...) 2026-08-31 13:52 ` [PATCH 6/9] block/accounting: Emit BLOCK_IO_DELAY event Hanna Czenczek @ 2026-08-31 13:52 ` Hanna Czenczek 2026-09-03 14:30 ` Stefan Hajnoczi 2026-08-31 13:52 ` [PATCH 8/9] block/accounting: Move latency_ns override down Hanna Czenczek ` (2 subsequent siblings) 9 siblings, 1 reply; 47+ messages in thread From: Hanna Czenczek @ 2026-08-31 13:52 UTC (permalink / raw) To: qemu-block Cc: qemu-devel, Hanna Czenczek, Kevin Wolf, John Snow, Denis V . Lunev, Eric Blake, Markus Armbruster, Stefan Hajnoczi This device property allows setting up an I/O latency threshold for when to emit a delay QMP event. Signed-off-by: Hanna Czenczek <hreitz@redhat.com> --- include/hw/block/block.h | 5 ++++- blockdev.c | 16 +++++++++++++++- hw/block/block.c | 4 +++- tests/qemu-iotests/172.out | 38 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 60 insertions(+), 3 deletions(-) diff --git a/include/hw/block/block.h b/include/hw/block/block.h index df941df19f2..e7e401dd303 100644 --- a/include/hw/block/block.h +++ b/include/hw/block/block.h @@ -36,6 +36,7 @@ typedef struct BlockConf { BlockdevOnError werror; uint32_t num_stats_intervals; uint32_t *stats_intervals; + uint32_t delay_alert_ms; } BlockConf; static inline unsigned int get_physical_block_exp(BlockConf *conf) @@ -83,7 +84,9 @@ static inline unsigned int get_physical_block_exp(BlockConf *conf) _conf.account_failed, ON_OFF_AUTO_AUTO), \ DEFINE_PROP_ARRAY("stats-intervals", _state, \ _conf.num_stats_intervals, _conf.stats_intervals, \ - qdev_prop_uint32, uint32_t) + qdev_prop_uint32, uint32_t), \ + DEFINE_PROP_UINT32("delay-alert-ms", _state, _conf.delay_alert_ms, \ + 0) \ #define DEFINE_BLOCK_PROPERTIES(_state, _conf) \ DEFINE_PROP_DRIVE("drive", _state, _conf.blk), \ diff --git a/blockdev.c b/blockdev.c index 195bac8af01..9c76e89ef38 100644 --- a/blockdev.c +++ b/blockdev.c @@ -496,6 +496,7 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts, BlockdevDetectZeroesOptions detect_zeroes = BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF; const char *throttling_group = NULL; + uint64_t delay_alert_ns; /* Check common options by copying from bs_opts to opts, all other options * stay in bs_opts for processing by bdrv_open(). */ @@ -580,6 +581,14 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts, read_only = qemu_opt_get_bool(opts, BDRV_OPT_READ_ONLY, false); + delay_alert_ns = qemu_opt_get_number(opts, "delay-alert-ms", 0); + if (delay_alert_ns > (uint64_t)INT64_MAX / SCALE_MS) { + error_setg(errp, "delay-alert-ms must not exceed %" PRId64, + INT64_MAX / SCALE_MS); + goto early_err; + } + delay_alert_ns *= SCALE_MS; + /* init */ if ((!file || !*file) && !qdict_size(bs_opts)) { BlockBackendRootState *blk_rs; @@ -618,7 +627,7 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts, bs->detect_zeroes = detect_zeroes; block_acct_setup(blk_get_stats(blk), account_invalid, account_failed, - NULL, 0, 0, NULL); + NULL, 0, delay_alert_ns, NULL); if (!parse_stats_intervals(blk_get_stats(blk), interval_list, errp)) { blk_unref(blk); @@ -3753,6 +3762,11 @@ QemuOptsList qemu_common_drive_opts = { .type = QEMU_OPT_BOOL, .help = "whether to account for failed I/O operations " "in the statistics", + },{ + .name = "delay-alert-ms", + .type = QEMU_OPT_NUMBER, + .help = "threshold in ms when to emit an I/O operation " + "delay QMP event", }, { /* end of list */ } }, diff --git a/hw/block/block.c b/hw/block/block.c index 19301c6f995..be0ea89cb79 100644 --- a/hw/block/block.c +++ b/hw/block/block.c @@ -207,6 +207,7 @@ bool blkconf_apply_backend_options(BlockConf *conf, bool readonly, BlockBackend *blk = conf->blk; BlockdevOnError rerror, werror; uint64_t perm, shared_perm; + uint64_t delay_alert_ns; bool wce; int ret; @@ -249,9 +250,10 @@ bool blkconf_apply_backend_options(BlockConf *conf, bool readonly, blk_set_enable_write_cache(blk, wce); blk_set_on_error(blk, rerror, werror); + delay_alert_ns = (uint64_t)conf->delay_alert_ms * SCALE_MS; if (!block_acct_setup(blk_get_stats(blk), conf->account_invalid, conf->account_failed, conf->stats_intervals, - conf->num_stats_intervals, 0, errp)) { + conf->num_stats_intervals, delay_alert_ns, errp)) { return false; } return true; diff --git a/tests/qemu-iotests/172.out b/tests/qemu-iotests/172.out index a023cd407de..ea6a1875eb3 100644 --- a/tests/qemu-iotests/172.out +++ b/tests/qemu-iotests/172.out @@ -31,6 +31,7 @@ Testing: account-invalid = "auto" account-failed = "auto" stats-intervals = <null> + delay-alert-ms = 0 (0x0) drive-type = "288" @@ -61,6 +62,7 @@ Testing: -fda TEST_DIR/t.qcow2 account-invalid = "auto" account-failed = "auto" stats-intervals = <null> + delay-alert-ms = 0 (0x0) drive-type = "144" floppy0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) Attached to: /machine/unattached/device[N] @@ -98,6 +100,7 @@ Testing: -fdb TEST_DIR/t.qcow2 account-invalid = "auto" account-failed = "auto" stats-intervals = <null> + delay-alert-ms = 0 (0x0) drive-type = "144" dev: floppy, id "" unit = 0 (0x0) @@ -113,6 +116,7 @@ Testing: -fdb TEST_DIR/t.qcow2 account-invalid = "auto" account-failed = "auto" stats-intervals = <null> + delay-alert-ms = 0 (0x0) drive-type = "288" floppy1 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) Attached to: /machine/unattached/device[N] @@ -154,6 +158,7 @@ Testing: -fda TEST_DIR/t.qcow2 -fdb TEST_DIR/t.qcow2.2 account-invalid = "auto" account-failed = "auto" stats-intervals = <null> + delay-alert-ms = 0 (0x0) drive-type = "144" dev: floppy, id "" unit = 0 (0x0) @@ -169,6 +174,7 @@ Testing: -fda TEST_DIR/t.qcow2 -fdb TEST_DIR/t.qcow2.2 account-invalid = "auto" account-failed = "auto" stats-intervals = <null> + delay-alert-ms = 0 (0x0) drive-type = "144" floppy0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) Attached to: /machine/unattached/device[N] @@ -211,6 +217,7 @@ Testing: -fdb account-invalid = "auto" account-failed = "auto" stats-intervals = <null> + delay-alert-ms = 0 (0x0) drive-type = "288" dev: floppy, id "" unit = 0 (0x0) @@ -226,6 +233,7 @@ Testing: -fdb account-invalid = "auto" account-failed = "auto" stats-intervals = <null> + delay-alert-ms = 0 (0x0) drive-type = "288" @@ -256,6 +264,7 @@ Testing: -drive if=floppy,file=TEST_DIR/t.qcow2 account-invalid = "auto" account-failed = "auto" stats-intervals = <null> + delay-alert-ms = 0 (0x0) drive-type = "144" floppy0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) Attached to: /machine/unattached/device[N] @@ -293,6 +302,7 @@ Testing: -drive if=floppy,file=TEST_DIR/t.qcow2,index=1 account-invalid = "auto" account-failed = "auto" stats-intervals = <null> + delay-alert-ms = 0 (0x0) drive-type = "144" dev: floppy, id "" unit = 0 (0x0) @@ -308,6 +318,7 @@ Testing: -drive if=floppy,file=TEST_DIR/t.qcow2,index=1 account-invalid = "auto" account-failed = "auto" stats-intervals = <null> + delay-alert-ms = 0 (0x0) drive-type = "288" floppy1 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) Attached to: /machine/unattached/device[N] @@ -349,6 +360,7 @@ Testing: -drive if=floppy,file=TEST_DIR/t.qcow2 -drive if=floppy,file=TEST_DIR/t account-invalid = "auto" account-failed = "auto" stats-intervals = <null> + delay-alert-ms = 0 (0x0) drive-type = "144" dev: floppy, id "" unit = 0 (0x0) @@ -364,6 +376,7 @@ Testing: -drive if=floppy,file=TEST_DIR/t.qcow2 -drive if=floppy,file=TEST_DIR/t account-invalid = "auto" account-failed = "auto" stats-intervals = <null> + delay-alert-ms = 0 (0x0) drive-type = "144" floppy0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) Attached to: /machine/unattached/device[N] @@ -409,6 +422,7 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -device floppy,drive=none0 account-invalid = "auto" account-failed = "auto" stats-intervals = <null> + delay-alert-ms = 0 (0x0) drive-type = "144" none0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) Attached to: /machine/peripheral-anon/device[N] @@ -446,6 +460,7 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -device floppy,drive=none0,unit=1 account-invalid = "auto" account-failed = "auto" stats-intervals = <null> + delay-alert-ms = 0 (0x0) drive-type = "144" none0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) Attached to: /machine/peripheral-anon/device[N] @@ -483,6 +498,7 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qco account-invalid = "auto" account-failed = "auto" stats-intervals = <null> + delay-alert-ms = 0 (0x0) drive-type = "144" dev: floppy, id "" unit = 0 (0x0) @@ -498,6 +514,7 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qco account-invalid = "auto" account-failed = "auto" stats-intervals = <null> + delay-alert-ms = 0 (0x0) drive-type = "144" none0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) Attached to: /machine/peripheral-anon/device[N] @@ -549,6 +566,7 @@ Testing: -fda TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -device fl account-invalid = "auto" account-failed = "auto" stats-intervals = <null> + delay-alert-ms = 0 (0x0) drive-type = "144" dev: floppy, id "" unit = 0 (0x0) @@ -564,6 +582,7 @@ Testing: -fda TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -device fl account-invalid = "auto" account-failed = "auto" stats-intervals = <null> + delay-alert-ms = 0 (0x0) drive-type = "144" floppy0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) Attached to: /machine/unattached/device[N] @@ -606,6 +625,7 @@ Testing: -fda TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -device fl account-invalid = "auto" account-failed = "auto" stats-intervals = <null> + delay-alert-ms = 0 (0x0) drive-type = "144" dev: floppy, id "" unit = 0 (0x0) @@ -621,6 +641,7 @@ Testing: -fda TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -device fl account-invalid = "auto" account-failed = "auto" stats-intervals = <null> + delay-alert-ms = 0 (0x0) drive-type = "144" floppy0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) Attached to: /machine/unattached/device[N] @@ -663,6 +684,7 @@ Testing: -fdb TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -device fl account-invalid = "auto" account-failed = "auto" stats-intervals = <null> + delay-alert-ms = 0 (0x0) drive-type = "144" dev: floppy, id "" unit = 1 (0x1) @@ -678,6 +700,7 @@ Testing: -fdb TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -device fl account-invalid = "auto" account-failed = "auto" stats-intervals = <null> + delay-alert-ms = 0 (0x0) drive-type = "144" floppy1 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) Attached to: /machine/unattached/device[N] @@ -720,6 +743,7 @@ Testing: -fdb TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -device fl account-invalid = "auto" account-failed = "auto" stats-intervals = <null> + delay-alert-ms = 0 (0x0) drive-type = "144" dev: floppy, id "" unit = 1 (0x1) @@ -735,6 +759,7 @@ Testing: -fdb TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -device fl account-invalid = "auto" account-failed = "auto" stats-intervals = <null> + delay-alert-ms = 0 (0x0) drive-type = "144" floppy1 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) Attached to: /machine/unattached/device[N] @@ -786,6 +811,7 @@ Testing: -drive if=floppy,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.q account-invalid = "auto" account-failed = "auto" stats-intervals = <null> + delay-alert-ms = 0 (0x0) drive-type = "144" dev: floppy, id "" unit = 0 (0x0) @@ -801,6 +827,7 @@ Testing: -drive if=floppy,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.q account-invalid = "auto" account-failed = "auto" stats-intervals = <null> + delay-alert-ms = 0 (0x0) drive-type = "144" floppy0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) Attached to: /machine/unattached/device[N] @@ -843,6 +870,7 @@ Testing: -drive if=floppy,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.q account-invalid = "auto" account-failed = "auto" stats-intervals = <null> + delay-alert-ms = 0 (0x0) drive-type = "144" dev: floppy, id "" unit = 0 (0x0) @@ -858,6 +886,7 @@ Testing: -drive if=floppy,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.q account-invalid = "auto" account-failed = "auto" stats-intervals = <null> + delay-alert-ms = 0 (0x0) drive-type = "144" floppy0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) Attached to: /machine/unattached/device[N] @@ -906,6 +935,7 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -global floppy.drive=none0 -device account-invalid = "auto" account-failed = "auto" stats-intervals = <null> + delay-alert-ms = 0 (0x0) drive-type = "144" none0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) Attached to: /machine/peripheral-anon/device[N] @@ -973,6 +1003,7 @@ Testing: -device floppy account-invalid = "auto" account-failed = "auto" stats-intervals = <null> + delay-alert-ms = 0 (0x0) drive-type = "288" Testing: -device floppy,drive-type=120 @@ -1000,6 +1031,7 @@ Testing: -device floppy,drive-type=120 account-invalid = "auto" account-failed = "auto" stats-intervals = <null> + delay-alert-ms = 0 (0x0) drive-type = "120" Testing: -device floppy,drive-type=144 @@ -1027,6 +1059,7 @@ Testing: -device floppy,drive-type=144 account-invalid = "auto" account-failed = "auto" stats-intervals = <null> + delay-alert-ms = 0 (0x0) drive-type = "144" Testing: -device floppy,drive-type=288 @@ -1054,6 +1087,7 @@ Testing: -device floppy,drive-type=288 account-invalid = "auto" account-failed = "auto" stats-intervals = <null> + delay-alert-ms = 0 (0x0) drive-type = "288" @@ -1084,6 +1118,7 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -device floppy,drive=none0,drive-t account-invalid = "auto" account-failed = "auto" stats-intervals = <null> + delay-alert-ms = 0 (0x0) drive-type = "120" none0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) Attached to: /machine/peripheral-anon/device[N] @@ -1121,6 +1156,7 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -device floppy,drive=none0,drive-t account-invalid = "auto" account-failed = "auto" stats-intervals = <null> + delay-alert-ms = 0 (0x0) drive-type = "288" none0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) Attached to: /machine/peripheral-anon/device[N] @@ -1161,6 +1197,7 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -device floppy,drive=none0,logical account-invalid = "auto" account-failed = "auto" stats-intervals = <null> + delay-alert-ms = 0 (0x0) drive-type = "144" none0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) Attached to: /machine/peripheral-anon/device[N] @@ -1198,6 +1235,7 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -device floppy,drive=none0,physica account-invalid = "auto" account-failed = "auto" stats-intervals = <null> + delay-alert-ms = 0 (0x0) drive-type = "144" none0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) Attached to: /machine/peripheral-anon/device[N] -- 2.55.0 ^ permalink raw reply related [flat|nested] 47+ messages in thread
* Re: [PATCH 7/9] block: Add delay-alert-ms property 2026-08-31 13:52 ` [PATCH 7/9] block: Add delay-alert-ms property Hanna Czenczek @ 2026-09-03 14:30 ` Stefan Hajnoczi 2026-09-16 8:17 ` Hanna Czenczek 0 siblings, 1 reply; 47+ messages in thread From: Stefan Hajnoczi @ 2026-09-03 14:30 UTC (permalink / raw) To: Hanna Czenczek Cc: qemu-block, qemu-devel, Kevin Wolf, John Snow, Denis V . Lunev, Eric Blake, Markus Armbruster [-- Attachment #1: Type: text/plain, Size: 20093 bytes --] On Mon, Aug 31, 2026 at 03:52:03PM +0200, Hanna Czenczek wrote: > This device property allows setting up an I/O latency threshold for when > to emit a delay QMP event. > > Signed-off-by: Hanna Czenczek <hreitz@redhat.com> > --- > include/hw/block/block.h | 5 ++++- > blockdev.c | 16 +++++++++++++++- > hw/block/block.c | 4 +++- > tests/qemu-iotests/172.out | 38 ++++++++++++++++++++++++++++++++++++++ > 4 files changed, 60 insertions(+), 3 deletions(-) > > diff --git a/include/hw/block/block.h b/include/hw/block/block.h > index df941df19f2..e7e401dd303 100644 > --- a/include/hw/block/block.h > +++ b/include/hw/block/block.h > @@ -36,6 +36,7 @@ typedef struct BlockConf { > BlockdevOnError werror; > uint32_t num_stats_intervals; > uint32_t *stats_intervals; > + uint32_t delay_alert_ms; > } BlockConf; > > static inline unsigned int get_physical_block_exp(BlockConf *conf) > @@ -83,7 +84,9 @@ static inline unsigned int get_physical_block_exp(BlockConf *conf) > _conf.account_failed, ON_OFF_AUTO_AUTO), \ > DEFINE_PROP_ARRAY("stats-intervals", _state, \ > _conf.num_stats_intervals, _conf.stats_intervals, \ > - qdev_prop_uint32, uint32_t) > + qdev_prop_uint32, uint32_t), \ > + DEFINE_PROP_UINT32("delay-alert-ms", _state, _conf.delay_alert_ms, \ > + 0) \ The latency histogram feature uses nanoseconds (64-bit integer) whereas this patch series uses delay-alert-ms (uint32) and a double floating-point seconds value in the QMP event. Maybe stick to 64-bit integer nanoseconds everywhere for consistency? > > #define DEFINE_BLOCK_PROPERTIES(_state, _conf) \ > DEFINE_PROP_DRIVE("drive", _state, _conf.blk), \ > diff --git a/blockdev.c b/blockdev.c > index 195bac8af01..9c76e89ef38 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -496,6 +496,7 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts, > BlockdevDetectZeroesOptions detect_zeroes = > BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF; > const char *throttling_group = NULL; > + uint64_t delay_alert_ns; > > /* Check common options by copying from bs_opts to opts, all other options > * stay in bs_opts for processing by bdrv_open(). */ > @@ -580,6 +581,14 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts, > > read_only = qemu_opt_get_bool(opts, BDRV_OPT_READ_ONLY, false); > > + delay_alert_ns = qemu_opt_get_number(opts, "delay-alert-ms", 0); > + if (delay_alert_ns > (uint64_t)INT64_MAX / SCALE_MS) { > + error_setg(errp, "delay-alert-ms must not exceed %" PRId64, > + INT64_MAX / SCALE_MS); > + goto early_err; > + } > + delay_alert_ns *= SCALE_MS; > + > /* init */ > if ((!file || !*file) && !qdict_size(bs_opts)) { > BlockBackendRootState *blk_rs; > @@ -618,7 +627,7 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts, > bs->detect_zeroes = detect_zeroes; > > block_acct_setup(blk_get_stats(blk), account_invalid, account_failed, > - NULL, 0, 0, NULL); > + NULL, 0, delay_alert_ns, NULL); > > if (!parse_stats_intervals(blk_get_stats(blk), interval_list, errp)) { > blk_unref(blk); > @@ -3753,6 +3762,11 @@ QemuOptsList qemu_common_drive_opts = { > .type = QEMU_OPT_BOOL, > .help = "whether to account for failed I/O operations " > "in the statistics", > + },{ > + .name = "delay-alert-ms", > + .type = QEMU_OPT_NUMBER, > + .help = "threshold in ms when to emit an I/O operation " > + "delay QMP event", > }, > { /* end of list */ } > }, > diff --git a/hw/block/block.c b/hw/block/block.c > index 19301c6f995..be0ea89cb79 100644 > --- a/hw/block/block.c > +++ b/hw/block/block.c > @@ -207,6 +207,7 @@ bool blkconf_apply_backend_options(BlockConf *conf, bool readonly, > BlockBackend *blk = conf->blk; > BlockdevOnError rerror, werror; > uint64_t perm, shared_perm; > + uint64_t delay_alert_ns; > bool wce; > int ret; > > @@ -249,9 +250,10 @@ bool blkconf_apply_backend_options(BlockConf *conf, bool readonly, > blk_set_enable_write_cache(blk, wce); > blk_set_on_error(blk, rerror, werror); > > + delay_alert_ns = (uint64_t)conf->delay_alert_ms * SCALE_MS; > if (!block_acct_setup(blk_get_stats(blk), conf->account_invalid, > conf->account_failed, conf->stats_intervals, > - conf->num_stats_intervals, 0, errp)) { > + conf->num_stats_intervals, delay_alert_ns, errp)) { > return false; > } > return true; > diff --git a/tests/qemu-iotests/172.out b/tests/qemu-iotests/172.out > index a023cd407de..ea6a1875eb3 100644 > --- a/tests/qemu-iotests/172.out > +++ b/tests/qemu-iotests/172.out > @@ -31,6 +31,7 @@ Testing: > account-invalid = "auto" > account-failed = "auto" > stats-intervals = <null> > + delay-alert-ms = 0 (0x0) > drive-type = "288" > > > @@ -61,6 +62,7 @@ Testing: -fda TEST_DIR/t.qcow2 > account-invalid = "auto" > account-failed = "auto" > stats-intervals = <null> > + delay-alert-ms = 0 (0x0) > drive-type = "144" > floppy0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) > Attached to: /machine/unattached/device[N] > @@ -98,6 +100,7 @@ Testing: -fdb TEST_DIR/t.qcow2 > account-invalid = "auto" > account-failed = "auto" > stats-intervals = <null> > + delay-alert-ms = 0 (0x0) > drive-type = "144" > dev: floppy, id "" > unit = 0 (0x0) > @@ -113,6 +116,7 @@ Testing: -fdb TEST_DIR/t.qcow2 > account-invalid = "auto" > account-failed = "auto" > stats-intervals = <null> > + delay-alert-ms = 0 (0x0) > drive-type = "288" > floppy1 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) > Attached to: /machine/unattached/device[N] > @@ -154,6 +158,7 @@ Testing: -fda TEST_DIR/t.qcow2 -fdb TEST_DIR/t.qcow2.2 > account-invalid = "auto" > account-failed = "auto" > stats-intervals = <null> > + delay-alert-ms = 0 (0x0) > drive-type = "144" > dev: floppy, id "" > unit = 0 (0x0) > @@ -169,6 +174,7 @@ Testing: -fda TEST_DIR/t.qcow2 -fdb TEST_DIR/t.qcow2.2 > account-invalid = "auto" > account-failed = "auto" > stats-intervals = <null> > + delay-alert-ms = 0 (0x0) > drive-type = "144" > floppy0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) > Attached to: /machine/unattached/device[N] > @@ -211,6 +217,7 @@ Testing: -fdb > account-invalid = "auto" > account-failed = "auto" > stats-intervals = <null> > + delay-alert-ms = 0 (0x0) > drive-type = "288" > dev: floppy, id "" > unit = 0 (0x0) > @@ -226,6 +233,7 @@ Testing: -fdb > account-invalid = "auto" > account-failed = "auto" > stats-intervals = <null> > + delay-alert-ms = 0 (0x0) > drive-type = "288" > > > @@ -256,6 +264,7 @@ Testing: -drive if=floppy,file=TEST_DIR/t.qcow2 > account-invalid = "auto" > account-failed = "auto" > stats-intervals = <null> > + delay-alert-ms = 0 (0x0) > drive-type = "144" > floppy0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) > Attached to: /machine/unattached/device[N] > @@ -293,6 +302,7 @@ Testing: -drive if=floppy,file=TEST_DIR/t.qcow2,index=1 > account-invalid = "auto" > account-failed = "auto" > stats-intervals = <null> > + delay-alert-ms = 0 (0x0) > drive-type = "144" > dev: floppy, id "" > unit = 0 (0x0) > @@ -308,6 +318,7 @@ Testing: -drive if=floppy,file=TEST_DIR/t.qcow2,index=1 > account-invalid = "auto" > account-failed = "auto" > stats-intervals = <null> > + delay-alert-ms = 0 (0x0) > drive-type = "288" > floppy1 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) > Attached to: /machine/unattached/device[N] > @@ -349,6 +360,7 @@ Testing: -drive if=floppy,file=TEST_DIR/t.qcow2 -drive if=floppy,file=TEST_DIR/t > account-invalid = "auto" > account-failed = "auto" > stats-intervals = <null> > + delay-alert-ms = 0 (0x0) > drive-type = "144" > dev: floppy, id "" > unit = 0 (0x0) > @@ -364,6 +376,7 @@ Testing: -drive if=floppy,file=TEST_DIR/t.qcow2 -drive if=floppy,file=TEST_DIR/t > account-invalid = "auto" > account-failed = "auto" > stats-intervals = <null> > + delay-alert-ms = 0 (0x0) > drive-type = "144" > floppy0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) > Attached to: /machine/unattached/device[N] > @@ -409,6 +422,7 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -device floppy,drive=none0 > account-invalid = "auto" > account-failed = "auto" > stats-intervals = <null> > + delay-alert-ms = 0 (0x0) > drive-type = "144" > none0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) > Attached to: /machine/peripheral-anon/device[N] > @@ -446,6 +460,7 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -device floppy,drive=none0,unit=1 > account-invalid = "auto" > account-failed = "auto" > stats-intervals = <null> > + delay-alert-ms = 0 (0x0) > drive-type = "144" > none0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) > Attached to: /machine/peripheral-anon/device[N] > @@ -483,6 +498,7 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qco > account-invalid = "auto" > account-failed = "auto" > stats-intervals = <null> > + delay-alert-ms = 0 (0x0) > drive-type = "144" > dev: floppy, id "" > unit = 0 (0x0) > @@ -498,6 +514,7 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qco > account-invalid = "auto" > account-failed = "auto" > stats-intervals = <null> > + delay-alert-ms = 0 (0x0) > drive-type = "144" > none0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) > Attached to: /machine/peripheral-anon/device[N] > @@ -549,6 +566,7 @@ Testing: -fda TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -device fl > account-invalid = "auto" > account-failed = "auto" > stats-intervals = <null> > + delay-alert-ms = 0 (0x0) > drive-type = "144" > dev: floppy, id "" > unit = 0 (0x0) > @@ -564,6 +582,7 @@ Testing: -fda TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -device fl > account-invalid = "auto" > account-failed = "auto" > stats-intervals = <null> > + delay-alert-ms = 0 (0x0) > drive-type = "144" > floppy0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) > Attached to: /machine/unattached/device[N] > @@ -606,6 +625,7 @@ Testing: -fda TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -device fl > account-invalid = "auto" > account-failed = "auto" > stats-intervals = <null> > + delay-alert-ms = 0 (0x0) > drive-type = "144" > dev: floppy, id "" > unit = 0 (0x0) > @@ -621,6 +641,7 @@ Testing: -fda TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -device fl > account-invalid = "auto" > account-failed = "auto" > stats-intervals = <null> > + delay-alert-ms = 0 (0x0) > drive-type = "144" > floppy0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) > Attached to: /machine/unattached/device[N] > @@ -663,6 +684,7 @@ Testing: -fdb TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -device fl > account-invalid = "auto" > account-failed = "auto" > stats-intervals = <null> > + delay-alert-ms = 0 (0x0) > drive-type = "144" > dev: floppy, id "" > unit = 1 (0x1) > @@ -678,6 +700,7 @@ Testing: -fdb TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -device fl > account-invalid = "auto" > account-failed = "auto" > stats-intervals = <null> > + delay-alert-ms = 0 (0x0) > drive-type = "144" > floppy1 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) > Attached to: /machine/unattached/device[N] > @@ -720,6 +743,7 @@ Testing: -fdb TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -device fl > account-invalid = "auto" > account-failed = "auto" > stats-intervals = <null> > + delay-alert-ms = 0 (0x0) > drive-type = "144" > dev: floppy, id "" > unit = 1 (0x1) > @@ -735,6 +759,7 @@ Testing: -fdb TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -device fl > account-invalid = "auto" > account-failed = "auto" > stats-intervals = <null> > + delay-alert-ms = 0 (0x0) > drive-type = "144" > floppy1 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) > Attached to: /machine/unattached/device[N] > @@ -786,6 +811,7 @@ Testing: -drive if=floppy,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.q > account-invalid = "auto" > account-failed = "auto" > stats-intervals = <null> > + delay-alert-ms = 0 (0x0) > drive-type = "144" > dev: floppy, id "" > unit = 0 (0x0) > @@ -801,6 +827,7 @@ Testing: -drive if=floppy,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.q > account-invalid = "auto" > account-failed = "auto" > stats-intervals = <null> > + delay-alert-ms = 0 (0x0) > drive-type = "144" > floppy0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) > Attached to: /machine/unattached/device[N] > @@ -843,6 +870,7 @@ Testing: -drive if=floppy,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.q > account-invalid = "auto" > account-failed = "auto" > stats-intervals = <null> > + delay-alert-ms = 0 (0x0) > drive-type = "144" > dev: floppy, id "" > unit = 0 (0x0) > @@ -858,6 +886,7 @@ Testing: -drive if=floppy,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.q > account-invalid = "auto" > account-failed = "auto" > stats-intervals = <null> > + delay-alert-ms = 0 (0x0) > drive-type = "144" > floppy0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) > Attached to: /machine/unattached/device[N] > @@ -906,6 +935,7 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -global floppy.drive=none0 -device > account-invalid = "auto" > account-failed = "auto" > stats-intervals = <null> > + delay-alert-ms = 0 (0x0) > drive-type = "144" > none0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) > Attached to: /machine/peripheral-anon/device[N] > @@ -973,6 +1003,7 @@ Testing: -device floppy > account-invalid = "auto" > account-failed = "auto" > stats-intervals = <null> > + delay-alert-ms = 0 (0x0) > drive-type = "288" > > Testing: -device floppy,drive-type=120 > @@ -1000,6 +1031,7 @@ Testing: -device floppy,drive-type=120 > account-invalid = "auto" > account-failed = "auto" > stats-intervals = <null> > + delay-alert-ms = 0 (0x0) > drive-type = "120" > > Testing: -device floppy,drive-type=144 > @@ -1027,6 +1059,7 @@ Testing: -device floppy,drive-type=144 > account-invalid = "auto" > account-failed = "auto" > stats-intervals = <null> > + delay-alert-ms = 0 (0x0) > drive-type = "144" > > Testing: -device floppy,drive-type=288 > @@ -1054,6 +1087,7 @@ Testing: -device floppy,drive-type=288 > account-invalid = "auto" > account-failed = "auto" > stats-intervals = <null> > + delay-alert-ms = 0 (0x0) > drive-type = "288" > > > @@ -1084,6 +1118,7 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -device floppy,drive=none0,drive-t > account-invalid = "auto" > account-failed = "auto" > stats-intervals = <null> > + delay-alert-ms = 0 (0x0) > drive-type = "120" > none0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) > Attached to: /machine/peripheral-anon/device[N] > @@ -1121,6 +1156,7 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -device floppy,drive=none0,drive-t > account-invalid = "auto" > account-failed = "auto" > stats-intervals = <null> > + delay-alert-ms = 0 (0x0) > drive-type = "288" > none0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) > Attached to: /machine/peripheral-anon/device[N] > @@ -1161,6 +1197,7 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -device floppy,drive=none0,logical > account-invalid = "auto" > account-failed = "auto" > stats-intervals = <null> > + delay-alert-ms = 0 (0x0) > drive-type = "144" > none0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) > Attached to: /machine/peripheral-anon/device[N] > @@ -1198,6 +1235,7 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -device floppy,drive=none0,physica > account-invalid = "auto" > account-failed = "auto" > stats-intervals = <null> > + delay-alert-ms = 0 (0x0) > drive-type = "144" > none0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) > Attached to: /machine/peripheral-anon/device[N] > -- > 2.55.0 > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 7/9] block: Add delay-alert-ms property 2026-09-03 14:30 ` Stefan Hajnoczi @ 2026-09-16 8:17 ` Hanna Czenczek 2026-09-22 12:51 ` Kevin Wolf 0 siblings, 1 reply; 47+ messages in thread From: Hanna Czenczek @ 2026-09-16 8:17 UTC (permalink / raw) To: Stefan Hajnoczi Cc: qemu-block, qemu-devel, Kevin Wolf, John Snow, Denis V . Lunev, Eric Blake, Markus Armbruster On 03.09.26 16:30, Stefan Hajnoczi wrote: > On Mon, Aug 31, 2026 at 03:52:03PM +0200, Hanna Czenczek wrote: >> This device property allows setting up an I/O latency threshold for when >> to emit a delay QMP event. >> >> Signed-off-by: Hanna Czenczek <hreitz@redhat.com> >> --- >> include/hw/block/block.h | 5 ++++- >> blockdev.c | 16 +++++++++++++++- >> hw/block/block.c | 4 +++- >> tests/qemu-iotests/172.out | 38 ++++++++++++++++++++++++++++++++++++++ >> 4 files changed, 60 insertions(+), 3 deletions(-) >> >> diff --git a/include/hw/block/block.h b/include/hw/block/block.h >> index df941df19f2..e7e401dd303 100644 >> --- a/include/hw/block/block.h >> +++ b/include/hw/block/block.h >> @@ -36,6 +36,7 @@ typedef struct BlockConf { >> BlockdevOnError werror; >> uint32_t num_stats_intervals; >> uint32_t *stats_intervals; >> + uint32_t delay_alert_ms; >> } BlockConf; >> >> static inline unsigned int get_physical_block_exp(BlockConf *conf) >> @@ -83,7 +84,9 @@ static inline unsigned int get_physical_block_exp(BlockConf *conf) >> _conf.account_failed, ON_OFF_AUTO_AUTO), \ >> DEFINE_PROP_ARRAY("stats-intervals", _state, \ >> _conf.num_stats_intervals, _conf.stats_intervals, \ >> - qdev_prop_uint32, uint32_t) >> + qdev_prop_uint32, uint32_t), \ >> + DEFINE_PROP_UINT32("delay-alert-ms", _state, _conf.delay_alert_ms, \ >> + 0) \ > The latency histogram feature uses nanoseconds (64-bit integer) whereas > this patch series uses delay-alert-ms (uint32) and a double > floating-point seconds value in the QMP event. Maybe stick to 64-bit > integer nanoseconds everywhere for consistency? I don’t have technical objections because doing so is easy, of course. Personally, I found nanoseconds really unwieldy to specify here. But I suppose if @block-latency-histogram-set does the same, then I might as well. Hanna >> >> #define DEFINE_BLOCK_PROPERTIES(_state, _conf) \ >> DEFINE_PROP_DRIVE("drive", _state, _conf.blk), \ >> diff --git a/blockdev.c b/blockdev.c >> index 195bac8af01..9c76e89ef38 100644 >> --- a/blockdev.c >> +++ b/blockdev.c >> @@ -496,6 +496,7 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts, >> BlockdevDetectZeroesOptions detect_zeroes = >> BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF; >> const char *throttling_group = NULL; >> + uint64_t delay_alert_ns; >> >> /* Check common options by copying from bs_opts to opts, all other options >> * stay in bs_opts for processing by bdrv_open(). */ >> @@ -580,6 +581,14 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts, >> >> read_only = qemu_opt_get_bool(opts, BDRV_OPT_READ_ONLY, false); >> >> + delay_alert_ns = qemu_opt_get_number(opts, "delay-alert-ms", 0); >> + if (delay_alert_ns > (uint64_t)INT64_MAX / SCALE_MS) { >> + error_setg(errp, "delay-alert-ms must not exceed %" PRId64, >> + INT64_MAX / SCALE_MS); >> + goto early_err; >> + } >> + delay_alert_ns *= SCALE_MS; >> + >> /* init */ >> if ((!file || !*file) && !qdict_size(bs_opts)) { >> BlockBackendRootState *blk_rs; >> @@ -618,7 +627,7 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts, >> bs->detect_zeroes = detect_zeroes; >> >> block_acct_setup(blk_get_stats(blk), account_invalid, account_failed, >> - NULL, 0, 0, NULL); >> + NULL, 0, delay_alert_ns, NULL); >> >> if (!parse_stats_intervals(blk_get_stats(blk), interval_list, errp)) { >> blk_unref(blk); >> @@ -3753,6 +3762,11 @@ QemuOptsList qemu_common_drive_opts = { >> .type = QEMU_OPT_BOOL, >> .help = "whether to account for failed I/O operations " >> "in the statistics", >> + },{ >> + .name = "delay-alert-ms", >> + .type = QEMU_OPT_NUMBER, >> + .help = "threshold in ms when to emit an I/O operation " >> + "delay QMP event", >> }, >> { /* end of list */ } >> }, >> diff --git a/hw/block/block.c b/hw/block/block.c >> index 19301c6f995..be0ea89cb79 100644 >> --- a/hw/block/block.c >> +++ b/hw/block/block.c >> @@ -207,6 +207,7 @@ bool blkconf_apply_backend_options(BlockConf *conf, bool readonly, >> BlockBackend *blk = conf->blk; >> BlockdevOnError rerror, werror; >> uint64_t perm, shared_perm; >> + uint64_t delay_alert_ns; >> bool wce; >> int ret; >> >> @@ -249,9 +250,10 @@ bool blkconf_apply_backend_options(BlockConf *conf, bool readonly, >> blk_set_enable_write_cache(blk, wce); >> blk_set_on_error(blk, rerror, werror); >> >> + delay_alert_ns = (uint64_t)conf->delay_alert_ms * SCALE_MS; >> if (!block_acct_setup(blk_get_stats(blk), conf->account_invalid, >> conf->account_failed, conf->stats_intervals, >> - conf->num_stats_intervals, 0, errp)) { >> + conf->num_stats_intervals, delay_alert_ns, errp)) { >> return false; >> } >> return true; >> diff --git a/tests/qemu-iotests/172.out b/tests/qemu-iotests/172.out >> index a023cd407de..ea6a1875eb3 100644 >> --- a/tests/qemu-iotests/172.out >> +++ b/tests/qemu-iotests/172.out >> @@ -31,6 +31,7 @@ Testing: >> account-invalid = "auto" >> account-failed = "auto" >> stats-intervals = <null> >> + delay-alert-ms = 0 (0x0) >> drive-type = "288" >> >> >> @@ -61,6 +62,7 @@ Testing: -fda TEST_DIR/t.qcow2 >> account-invalid = "auto" >> account-failed = "auto" >> stats-intervals = <null> >> + delay-alert-ms = 0 (0x0) >> drive-type = "144" >> floppy0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) >> Attached to: /machine/unattached/device[N] >> @@ -98,6 +100,7 @@ Testing: -fdb TEST_DIR/t.qcow2 >> account-invalid = "auto" >> account-failed = "auto" >> stats-intervals = <null> >> + delay-alert-ms = 0 (0x0) >> drive-type = "144" >> dev: floppy, id "" >> unit = 0 (0x0) >> @@ -113,6 +116,7 @@ Testing: -fdb TEST_DIR/t.qcow2 >> account-invalid = "auto" >> account-failed = "auto" >> stats-intervals = <null> >> + delay-alert-ms = 0 (0x0) >> drive-type = "288" >> floppy1 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) >> Attached to: /machine/unattached/device[N] >> @@ -154,6 +158,7 @@ Testing: -fda TEST_DIR/t.qcow2 -fdb TEST_DIR/t.qcow2.2 >> account-invalid = "auto" >> account-failed = "auto" >> stats-intervals = <null> >> + delay-alert-ms = 0 (0x0) >> drive-type = "144" >> dev: floppy, id "" >> unit = 0 (0x0) >> @@ -169,6 +174,7 @@ Testing: -fda TEST_DIR/t.qcow2 -fdb TEST_DIR/t.qcow2.2 >> account-invalid = "auto" >> account-failed = "auto" >> stats-intervals = <null> >> + delay-alert-ms = 0 (0x0) >> drive-type = "144" >> floppy0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) >> Attached to: /machine/unattached/device[N] >> @@ -211,6 +217,7 @@ Testing: -fdb >> account-invalid = "auto" >> account-failed = "auto" >> stats-intervals = <null> >> + delay-alert-ms = 0 (0x0) >> drive-type = "288" >> dev: floppy, id "" >> unit = 0 (0x0) >> @@ -226,6 +233,7 @@ Testing: -fdb >> account-invalid = "auto" >> account-failed = "auto" >> stats-intervals = <null> >> + delay-alert-ms = 0 (0x0) >> drive-type = "288" >> >> >> @@ -256,6 +264,7 @@ Testing: -drive if=floppy,file=TEST_DIR/t.qcow2 >> account-invalid = "auto" >> account-failed = "auto" >> stats-intervals = <null> >> + delay-alert-ms = 0 (0x0) >> drive-type = "144" >> floppy0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) >> Attached to: /machine/unattached/device[N] >> @@ -293,6 +302,7 @@ Testing: -drive if=floppy,file=TEST_DIR/t.qcow2,index=1 >> account-invalid = "auto" >> account-failed = "auto" >> stats-intervals = <null> >> + delay-alert-ms = 0 (0x0) >> drive-type = "144" >> dev: floppy, id "" >> unit = 0 (0x0) >> @@ -308,6 +318,7 @@ Testing: -drive if=floppy,file=TEST_DIR/t.qcow2,index=1 >> account-invalid = "auto" >> account-failed = "auto" >> stats-intervals = <null> >> + delay-alert-ms = 0 (0x0) >> drive-type = "288" >> floppy1 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) >> Attached to: /machine/unattached/device[N] >> @@ -349,6 +360,7 @@ Testing: -drive if=floppy,file=TEST_DIR/t.qcow2 -drive if=floppy,file=TEST_DIR/t >> account-invalid = "auto" >> account-failed = "auto" >> stats-intervals = <null> >> + delay-alert-ms = 0 (0x0) >> drive-type = "144" >> dev: floppy, id "" >> unit = 0 (0x0) >> @@ -364,6 +376,7 @@ Testing: -drive if=floppy,file=TEST_DIR/t.qcow2 -drive if=floppy,file=TEST_DIR/t >> account-invalid = "auto" >> account-failed = "auto" >> stats-intervals = <null> >> + delay-alert-ms = 0 (0x0) >> drive-type = "144" >> floppy0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) >> Attached to: /machine/unattached/device[N] >> @@ -409,6 +422,7 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -device floppy,drive=none0 >> account-invalid = "auto" >> account-failed = "auto" >> stats-intervals = <null> >> + delay-alert-ms = 0 (0x0) >> drive-type = "144" >> none0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) >> Attached to: /machine/peripheral-anon/device[N] >> @@ -446,6 +460,7 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -device floppy,drive=none0,unit=1 >> account-invalid = "auto" >> account-failed = "auto" >> stats-intervals = <null> >> + delay-alert-ms = 0 (0x0) >> drive-type = "144" >> none0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) >> Attached to: /machine/peripheral-anon/device[N] >> @@ -483,6 +498,7 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qco >> account-invalid = "auto" >> account-failed = "auto" >> stats-intervals = <null> >> + delay-alert-ms = 0 (0x0) >> drive-type = "144" >> dev: floppy, id "" >> unit = 0 (0x0) >> @@ -498,6 +514,7 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qco >> account-invalid = "auto" >> account-failed = "auto" >> stats-intervals = <null> >> + delay-alert-ms = 0 (0x0) >> drive-type = "144" >> none0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) >> Attached to: /machine/peripheral-anon/device[N] >> @@ -549,6 +566,7 @@ Testing: -fda TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -device fl >> account-invalid = "auto" >> account-failed = "auto" >> stats-intervals = <null> >> + delay-alert-ms = 0 (0x0) >> drive-type = "144" >> dev: floppy, id "" >> unit = 0 (0x0) >> @@ -564,6 +582,7 @@ Testing: -fda TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -device fl >> account-invalid = "auto" >> account-failed = "auto" >> stats-intervals = <null> >> + delay-alert-ms = 0 (0x0) >> drive-type = "144" >> floppy0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) >> Attached to: /machine/unattached/device[N] >> @@ -606,6 +625,7 @@ Testing: -fda TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -device fl >> account-invalid = "auto" >> account-failed = "auto" >> stats-intervals = <null> >> + delay-alert-ms = 0 (0x0) >> drive-type = "144" >> dev: floppy, id "" >> unit = 0 (0x0) >> @@ -621,6 +641,7 @@ Testing: -fda TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -device fl >> account-invalid = "auto" >> account-failed = "auto" >> stats-intervals = <null> >> + delay-alert-ms = 0 (0x0) >> drive-type = "144" >> floppy0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) >> Attached to: /machine/unattached/device[N] >> @@ -663,6 +684,7 @@ Testing: -fdb TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -device fl >> account-invalid = "auto" >> account-failed = "auto" >> stats-intervals = <null> >> + delay-alert-ms = 0 (0x0) >> drive-type = "144" >> dev: floppy, id "" >> unit = 1 (0x1) >> @@ -678,6 +700,7 @@ Testing: -fdb TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -device fl >> account-invalid = "auto" >> account-failed = "auto" >> stats-intervals = <null> >> + delay-alert-ms = 0 (0x0) >> drive-type = "144" >> floppy1 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) >> Attached to: /machine/unattached/device[N] >> @@ -720,6 +743,7 @@ Testing: -fdb TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -device fl >> account-invalid = "auto" >> account-failed = "auto" >> stats-intervals = <null> >> + delay-alert-ms = 0 (0x0) >> drive-type = "144" >> dev: floppy, id "" >> unit = 1 (0x1) >> @@ -735,6 +759,7 @@ Testing: -fdb TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -device fl >> account-invalid = "auto" >> account-failed = "auto" >> stats-intervals = <null> >> + delay-alert-ms = 0 (0x0) >> drive-type = "144" >> floppy1 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) >> Attached to: /machine/unattached/device[N] >> @@ -786,6 +811,7 @@ Testing: -drive if=floppy,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.q >> account-invalid = "auto" >> account-failed = "auto" >> stats-intervals = <null> >> + delay-alert-ms = 0 (0x0) >> drive-type = "144" >> dev: floppy, id "" >> unit = 0 (0x0) >> @@ -801,6 +827,7 @@ Testing: -drive if=floppy,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.q >> account-invalid = "auto" >> account-failed = "auto" >> stats-intervals = <null> >> + delay-alert-ms = 0 (0x0) >> drive-type = "144" >> floppy0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) >> Attached to: /machine/unattached/device[N] >> @@ -843,6 +870,7 @@ Testing: -drive if=floppy,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.q >> account-invalid = "auto" >> account-failed = "auto" >> stats-intervals = <null> >> + delay-alert-ms = 0 (0x0) >> drive-type = "144" >> dev: floppy, id "" >> unit = 0 (0x0) >> @@ -858,6 +886,7 @@ Testing: -drive if=floppy,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.q >> account-invalid = "auto" >> account-failed = "auto" >> stats-intervals = <null> >> + delay-alert-ms = 0 (0x0) >> drive-type = "144" >> floppy0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) >> Attached to: /machine/unattached/device[N] >> @@ -906,6 +935,7 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -global floppy.drive=none0 -device >> account-invalid = "auto" >> account-failed = "auto" >> stats-intervals = <null> >> + delay-alert-ms = 0 (0x0) >> drive-type = "144" >> none0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) >> Attached to: /machine/peripheral-anon/device[N] >> @@ -973,6 +1003,7 @@ Testing: -device floppy >> account-invalid = "auto" >> account-failed = "auto" >> stats-intervals = <null> >> + delay-alert-ms = 0 (0x0) >> drive-type = "288" >> >> Testing: -device floppy,drive-type=120 >> @@ -1000,6 +1031,7 @@ Testing: -device floppy,drive-type=120 >> account-invalid = "auto" >> account-failed = "auto" >> stats-intervals = <null> >> + delay-alert-ms = 0 (0x0) >> drive-type = "120" >> >> Testing: -device floppy,drive-type=144 >> @@ -1027,6 +1059,7 @@ Testing: -device floppy,drive-type=144 >> account-invalid = "auto" >> account-failed = "auto" >> stats-intervals = <null> >> + delay-alert-ms = 0 (0x0) >> drive-type = "144" >> >> Testing: -device floppy,drive-type=288 >> @@ -1054,6 +1087,7 @@ Testing: -device floppy,drive-type=288 >> account-invalid = "auto" >> account-failed = "auto" >> stats-intervals = <null> >> + delay-alert-ms = 0 (0x0) >> drive-type = "288" >> >> >> @@ -1084,6 +1118,7 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -device floppy,drive=none0,drive-t >> account-invalid = "auto" >> account-failed = "auto" >> stats-intervals = <null> >> + delay-alert-ms = 0 (0x0) >> drive-type = "120" >> none0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) >> Attached to: /machine/peripheral-anon/device[N] >> @@ -1121,6 +1156,7 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -device floppy,drive=none0,drive-t >> account-invalid = "auto" >> account-failed = "auto" >> stats-intervals = <null> >> + delay-alert-ms = 0 (0x0) >> drive-type = "288" >> none0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) >> Attached to: /machine/peripheral-anon/device[N] >> @@ -1161,6 +1197,7 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -device floppy,drive=none0,logical >> account-invalid = "auto" >> account-failed = "auto" >> stats-intervals = <null> >> + delay-alert-ms = 0 (0x0) >> drive-type = "144" >> none0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) >> Attached to: /machine/peripheral-anon/device[N] >> @@ -1198,6 +1235,7 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -device floppy,drive=none0,physica >> account-invalid = "auto" >> account-failed = "auto" >> stats-intervals = <null> >> + delay-alert-ms = 0 (0x0) >> drive-type = "144" >> none0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) >> Attached to: /machine/peripheral-anon/device[N] >> -- >> 2.55.0 >> ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 7/9] block: Add delay-alert-ms property 2026-09-16 8:17 ` Hanna Czenczek @ 2026-09-22 12:51 ` Kevin Wolf 0 siblings, 0 replies; 47+ messages in thread From: Kevin Wolf @ 2026-09-22 12:51 UTC (permalink / raw) To: Hanna Czenczek Cc: Stefan Hajnoczi, qemu-block, qemu-devel, John Snow, Denis V . Lunev, Eric Blake, Markus Armbruster Am 16.09.2026 um 10:17 hat Hanna Czenczek geschrieben: > On 03.09.26 16:30, Stefan Hajnoczi wrote: > > On Mon, Aug 31, 2026 at 03:52:03PM +0200, Hanna Czenczek wrote: > > > This device property allows setting up an I/O latency threshold for when > > > to emit a delay QMP event. > > > > > > Signed-off-by: Hanna Czenczek <hreitz@redhat.com> > > > --- > > > include/hw/block/block.h | 5 ++++- > > > blockdev.c | 16 +++++++++++++++- > > > hw/block/block.c | 4 +++- > > > tests/qemu-iotests/172.out | 38 ++++++++++++++++++++++++++++++++++++++ > > > 4 files changed, 60 insertions(+), 3 deletions(-) > > > > > > diff --git a/include/hw/block/block.h b/include/hw/block/block.h > > > index df941df19f2..e7e401dd303 100644 > > > --- a/include/hw/block/block.h > > > +++ b/include/hw/block/block.h > > > @@ -36,6 +36,7 @@ typedef struct BlockConf { > > > BlockdevOnError werror; > > > uint32_t num_stats_intervals; > > > uint32_t *stats_intervals; > > > + uint32_t delay_alert_ms; > > > } BlockConf; > > > static inline unsigned int get_physical_block_exp(BlockConf *conf) > > > @@ -83,7 +84,9 @@ static inline unsigned int get_physical_block_exp(BlockConf *conf) > > > _conf.account_failed, ON_OFF_AUTO_AUTO), \ > > > DEFINE_PROP_ARRAY("stats-intervals", _state, \ > > > _conf.num_stats_intervals, _conf.stats_intervals, \ > > > - qdev_prop_uint32, uint32_t) > > > + qdev_prop_uint32, uint32_t), \ > > > + DEFINE_PROP_UINT32("delay-alert-ms", _state, _conf.delay_alert_ms, \ > > > + 0) \ > > The latency histogram feature uses nanoseconds (64-bit integer) whereas > > this patch series uses delay-alert-ms (uint32) and a double > > floating-point seconds value in the QMP event. Maybe stick to 64-bit > > integer nanoseconds everywhere for consistency? > > I don’t have technical objections because doing so is easy, of course. > Personally, I found nanoseconds really unwieldy to specify here. But I > suppose if @block-latency-histogram-set does the same, then I might as well. I don't expect this to be used much by human users, and for a machine interface, what makes things unwieldy is having to keep track of which value has which unit instead of just sticking to one unit everywhere. I had the same comment before seeing that Stefan already pointed it out. Kevin ^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH 8/9] block/accounting: Move latency_ns override down 2026-08-31 13:51 [PATCH 0/9] block: BLOCK_IO_DELAY event Hanna Czenczek ` (6 preceding siblings ...) 2026-08-31 13:52 ` [PATCH 7/9] block: Add delay-alert-ms property Hanna Czenczek @ 2026-08-31 13:52 ` Hanna Czenczek 2026-09-03 15:08 ` Stefan Hajnoczi 2026-08-31 13:52 ` [PATCH 9/9] iotests: Add delay-alert test Hanna Czenczek 2026-09-03 14:08 ` [PATCH 0/9] block: BLOCK_IO_DELAY event Stefan Hajnoczi 9 siblings, 1 reply; 47+ messages in thread From: Hanna Czenczek @ 2026-08-31 13:52 UTC (permalink / raw) To: qemu-block Cc: qemu-devel, Hanna Czenczek, Kevin Wolf, John Snow, Denis V . Lunev, Eric Blake, Markus Armbruster, Stefan Hajnoczi I am not quite sure why `latency_ns` is overridden by a fixed value in qtest mode because personally, I find it much better if I can individually change requests' latency by modifying the qtest clock. But I'm not going to change existing behavior for the histogram and such, so I will just move this override after the latency has been evaluated regarding a potential BLOCK_IO_DELAY event. Signed-off-by: Hanna Czenczek <hreitz@redhat.com> --- block/accounting.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/block/accounting.c b/block/accounting.c index debf1924455..223becd2e04 100644 --- a/block/accounting.c +++ b/block/accounting.c @@ -271,10 +271,6 @@ static void block_account_one_io(BlockAcctStats *stats, BlockAcctCookie *cookie, int64_t time_ns = qemu_clock_get_ns(clock_type); int64_t latency_ns = time_ns - cookie->start_time_ns; - if (qtest_enabled()) { - latency_ns = qtest_latency_ns; - } - assert(cookie->type < BLOCK_MAX_IOTYPE); if (cookie->type == BLOCK_ACCT_NONE) { @@ -292,6 +288,10 @@ static void block_account_one_io(BlockAcctStats *stats, BlockAcctCookie *cookie, cookie->bytes); } + if (qtest_enabled()) { + latency_ns = qtest_latency_ns; + } + WITH_QEMU_LOCK_GUARD(&stats->lock) { if (failed) { stats->failed_ops[cookie->type]++; -- 2.55.0 ^ permalink raw reply related [flat|nested] 47+ messages in thread
* Re: [PATCH 8/9] block/accounting: Move latency_ns override down 2026-08-31 13:52 ` [PATCH 8/9] block/accounting: Move latency_ns override down Hanna Czenczek @ 2026-09-03 15:08 ` Stefan Hajnoczi 2026-09-16 8:21 ` Hanna Czenczek 0 siblings, 1 reply; 47+ messages in thread From: Stefan Hajnoczi @ 2026-09-03 15:08 UTC (permalink / raw) To: Hanna Czenczek Cc: qemu-block, qemu-devel, Kevin Wolf, John Snow, Denis V . Lunev, Eric Blake, Markus Armbruster [-- Attachment #1: Type: text/plain, Size: 2011 bytes --] On Mon, Aug 31, 2026 at 03:52:04PM +0200, Hanna Czenczek wrote: > I am not quite sure why `latency_ns` is overridden by a fixed value in > qtest mode because personally, I find it much better if I can > individually change requests' latency by modifying the qtest clock. It looks like tests/qemu-iotests/136 relies on a hardcoded constant so it can check min/max/avg against known values. > > But I'm not going to change existing behavior for the histogram and > such, so I will just move this override after the latency has been > evaluated regarding a potential BLOCK_IO_DELAY event. That's fine if you aren't taking the same testing approach as tests/qemu-iotests/136. I think the benefit of hardcoding the value for testing is that it would become possible to trigger the latency threshold without worrying about timing in the test environment. > > Signed-off-by: Hanna Czenczek <hreitz@redhat.com> > --- > block/accounting.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/block/accounting.c b/block/accounting.c > index debf1924455..223becd2e04 100644 > --- a/block/accounting.c > +++ b/block/accounting.c > @@ -271,10 +271,6 @@ static void block_account_one_io(BlockAcctStats *stats, BlockAcctCookie *cookie, > int64_t time_ns = qemu_clock_get_ns(clock_type); > int64_t latency_ns = time_ns - cookie->start_time_ns; > > - if (qtest_enabled()) { > - latency_ns = qtest_latency_ns; > - } > - > assert(cookie->type < BLOCK_MAX_IOTYPE); > > if (cookie->type == BLOCK_ACCT_NONE) { > @@ -292,6 +288,10 @@ static void block_account_one_io(BlockAcctStats *stats, BlockAcctCookie *cookie, > cookie->bytes); > } > > + if (qtest_enabled()) { > + latency_ns = qtest_latency_ns; > + } > + > WITH_QEMU_LOCK_GUARD(&stats->lock) { > if (failed) { > stats->failed_ops[cookie->type]++; > -- > 2.55.0 > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 8/9] block/accounting: Move latency_ns override down 2026-09-03 15:08 ` Stefan Hajnoczi @ 2026-09-16 8:21 ` Hanna Czenczek 2026-09-21 20:42 ` Stefan Hajnoczi 0 siblings, 1 reply; 47+ messages in thread From: Hanna Czenczek @ 2026-09-16 8:21 UTC (permalink / raw) To: Stefan Hajnoczi Cc: qemu-block, qemu-devel, Kevin Wolf, John Snow, Denis V . Lunev, Eric Blake, Markus Armbruster On 03.09.26 17:08, Stefan Hajnoczi wrote: > On Mon, Aug 31, 2026 at 03:52:04PM +0200, Hanna Czenczek wrote: >> I am not quite sure why `latency_ns` is overridden by a fixed value in >> qtest mode because personally, I find it much better if I can >> individually change requests' latency by modifying the qtest clock. > It looks like tests/qemu-iotests/136 relies on a hardcoded constant so > it can check min/max/avg against known values. > >> But I'm not going to change existing behavior for the histogram and >> such, so I will just move this override after the latency has been >> evaluated regarding a potential BLOCK_IO_DELAY event. > That's fine if you aren't taking the same testing approach as > tests/qemu-iotests/136. I think the benefit of hardcoding the value for > testing is that it would become possible to trigger the latency > threshold without worrying about timing in the test environment. FWIW, as far as I understand, in qtest mode, `clock_type` is `QEMU_CLOCK_VIRTUAL`, so it is not dependent on the test environment anyway, but on the qtest clock. That’s why I don’t understand the fixed latency value, but maybe it was just easier this way for 136 because this way it does not have to do a clock_step for each request. Hanna >> Signed-off-by: Hanna Czenczek <hreitz@redhat.com> >> --- >> block/accounting.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/block/accounting.c b/block/accounting.c >> index debf1924455..223becd2e04 100644 >> --- a/block/accounting.c >> +++ b/block/accounting.c >> @@ -271,10 +271,6 @@ static void block_account_one_io(BlockAcctStats *stats, BlockAcctCookie *cookie, >> int64_t time_ns = qemu_clock_get_ns(clock_type); >> int64_t latency_ns = time_ns - cookie->start_time_ns; >> >> - if (qtest_enabled()) { >> - latency_ns = qtest_latency_ns; >> - } >> - >> assert(cookie->type < BLOCK_MAX_IOTYPE); >> >> if (cookie->type == BLOCK_ACCT_NONE) { >> @@ -292,6 +288,10 @@ static void block_account_one_io(BlockAcctStats *stats, BlockAcctCookie *cookie, >> cookie->bytes); >> } >> >> + if (qtest_enabled()) { >> + latency_ns = qtest_latency_ns; >> + } >> + >> WITH_QEMU_LOCK_GUARD(&stats->lock) { >> if (failed) { >> stats->failed_ops[cookie->type]++; >> -- >> 2.55.0 >> ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 8/9] block/accounting: Move latency_ns override down 2026-09-16 8:21 ` Hanna Czenczek @ 2026-09-21 20:42 ` Stefan Hajnoczi 2026-09-22 13:03 ` Kevin Wolf 0 siblings, 1 reply; 47+ messages in thread From: Stefan Hajnoczi @ 2026-09-21 20:42 UTC (permalink / raw) To: Hanna Czenczek Cc: qemu-block, qemu-devel, Kevin Wolf, John Snow, Denis V . Lunev, Eric Blake, Markus Armbruster [-- Attachment #1: Type: text/plain, Size: 1449 bytes --] On Wed, Sep 16, 2026 at 10:21:22AM +0200, Hanna Czenczek wrote: > On 03.09.26 17:08, Stefan Hajnoczi wrote: > > On Mon, Aug 31, 2026 at 03:52:04PM +0200, Hanna Czenczek wrote: > > > I am not quite sure why `latency_ns` is overridden by a fixed value in > > > qtest mode because personally, I find it much better if I can > > > individually change requests' latency by modifying the qtest clock. > > It looks like tests/qemu-iotests/136 relies on a hardcoded constant so > > it can check min/max/avg against known values. > > > > > But I'm not going to change existing behavior for the histogram and > > > such, so I will just move this override after the latency has been > > > evaluated regarding a potential BLOCK_IO_DELAY event. > > That's fine if you aren't taking the same testing approach as > > tests/qemu-iotests/136. I think the benefit of hardcoding the value for > > testing is that it would become possible to trigger the latency > > threshold without worrying about timing in the test environment. > > FWIW, as far as I understand, in qtest mode, `clock_type` is > `QEMU_CLOCK_VIRTUAL`, so it is not dependent on the test environment anyway, > but on the qtest clock. That’s why I don’t understand the fixed latency > value, but maybe it was just easier this way for 136 because this way it > does not have to do a clock_step for each request. Yes, I think you're right. I'm not sure either. :/ Stefan [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 8/9] block/accounting: Move latency_ns override down 2026-09-21 20:42 ` Stefan Hajnoczi @ 2026-09-22 13:03 ` Kevin Wolf 2026-09-22 13:44 ` Alberto Garcia 0 siblings, 1 reply; 47+ messages in thread From: Kevin Wolf @ 2026-09-22 13:03 UTC (permalink / raw) To: Stefan Hajnoczi Cc: Hanna Czenczek, qemu-block, qemu-devel, John Snow, Denis V . Lunev, Eric Blake, Markus Armbruster, berto [-- Attachment #1: Type: text/plain, Size: 1932 bytes --] Am 21.09.2026 um 22:42 hat Stefan Hajnoczi geschrieben: > On Wed, Sep 16, 2026 at 10:21:22AM +0200, Hanna Czenczek wrote: > > On 03.09.26 17:08, Stefan Hajnoczi wrote: > > > On Mon, Aug 31, 2026 at 03:52:04PM +0200, Hanna Czenczek wrote: > > > > I am not quite sure why `latency_ns` is overridden by a fixed value in > > > > qtest mode because personally, I find it much better if I can > > > > individually change requests' latency by modifying the qtest clock. > > > It looks like tests/qemu-iotests/136 relies on a hardcoded constant so > > > it can check min/max/avg against known values. > > > > > > > But I'm not going to change existing behavior for the histogram and > > > > such, so I will just move this override after the latency has been > > > > evaluated regarding a potential BLOCK_IO_DELAY event. > > > That's fine if you aren't taking the same testing approach as > > > tests/qemu-iotests/136. I think the benefit of hardcoding the value for > > > testing is that it would become possible to trigger the latency > > > threshold without worrying about timing in the test environment. > > > > FWIW, as far as I understand, in qtest mode, `clock_type` is > > `QEMU_CLOCK_VIRTUAL`, so it is not dependent on the test environment anyway, > > but on the qtest clock. That’s why I don’t understand the fixed latency > > value, but maybe it was just easier this way for 136 because this way it > > does not have to do a clock_step for each request. > > Yes, I think you're right. I'm not sure either. :/ What's even stranger is that both things were introduced in the same commit. But it seems clearly related to 136 because the change was made right before the test was added. Berto, do you remember why you didn't rely just on clock_step? I suppose we could just try to change the behaviour and fix up 136. Behaviour under qtest is something I'd be okay with changing. Kevin [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 8/9] block/accounting: Move latency_ns override down 2026-09-22 13:03 ` Kevin Wolf @ 2026-09-22 13:44 ` Alberto Garcia 2026-09-22 13:52 ` Kevin Wolf 0 siblings, 1 reply; 47+ messages in thread From: Alberto Garcia @ 2026-09-22 13:44 UTC (permalink / raw) To: Kevin Wolf, Stefan Hajnoczi Cc: Hanna Czenczek, qemu-block, qemu-devel, John Snow, Denis V . Lunev, Eric Blake, Markus Armbruster On Tue 22 Sep 2026 03:03:56 PM +02, Kevin Wolf wrote: > Am 21.09.2026 um 22:42 hat Stefan Hajnoczi geschrieben: >> On Wed, Sep 16, 2026 at 10:21:22AM +0200, Hanna Czenczek wrote: > What's even stranger is that both things were introduced in the same > commit. But it seems clearly related to 136 because the change was made > right before the test was added. > > Berto, do you remember why you didn't rely just on clock_step? No, sorry, I don't remember :-/ I guess I wasn't familiar with that function? 136 was one of the first tests that I wrote. Berto ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 8/9] block/accounting: Move latency_ns override down 2026-09-22 13:44 ` Alberto Garcia @ 2026-09-22 13:52 ` Kevin Wolf 2026-09-23 10:50 ` Hanna Czenczek 0 siblings, 1 reply; 47+ messages in thread From: Kevin Wolf @ 2026-09-22 13:52 UTC (permalink / raw) To: Alberto Garcia Cc: Stefan Hajnoczi, Hanna Czenczek, qemu-block, qemu-devel, John Snow, Denis V . Lunev, Eric Blake, Markus Armbruster Am 22.09.2026 um 15:44 hat Alberto Garcia geschrieben: > On Tue 22 Sep 2026 03:03:56 PM +02, Kevin Wolf wrote: > > Am 21.09.2026 um 22:42 hat Stefan Hajnoczi geschrieben: > >> On Wed, Sep 16, 2026 at 10:21:22AM +0200, Hanna Czenczek wrote: > > What's even stranger is that both things were introduced in the same > > commit. But it seems clearly related to 136 because the change was made > > right before the test was added. > > > > Berto, do you remember why you didn't rely just on clock_step? > > No, sorry, I don't remember :-/ I guess I wasn't familiar with that > function? 136 was one of the first tests that I wrote. I thought so at first, too, but then I saw that other places in the same test do use it. Anyway, thanks for the quick reply. Then let's just try out what breaks if we change it. :-) Kevin ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 8/9] block/accounting: Move latency_ns override down 2026-09-22 13:52 ` Kevin Wolf @ 2026-09-23 10:50 ` Hanna Czenczek 0 siblings, 0 replies; 47+ messages in thread From: Hanna Czenczek @ 2026-09-23 10:50 UTC (permalink / raw) To: Kevin Wolf, Alberto Garcia Cc: Stefan Hajnoczi, qemu-block, qemu-devel, John Snow, Denis V . Lunev, Eric Blake, Markus Armbruster On 22.09.26 15:52, Kevin Wolf wrote: > Am 22.09.2026 um 15:44 hat Alberto Garcia geschrieben: >> On Tue 22 Sep 2026 03:03:56 PM +02, Kevin Wolf wrote: >>> Am 21.09.2026 um 22:42 hat Stefan Hajnoczi geschrieben: >>>> On Wed, Sep 16, 2026 at 10:21:22AM +0200, Hanna Czenczek wrote: >>> What's even stranger is that both things were introduced in the same >>> commit. But it seems clearly related to 136 because the change was made >>> right before the test was added. >>> >>> Berto, do you remember why you didn't rely just on clock_step? >> No, sorry, I don't remember :-/ I guess I wasn't familiar with that >> function? 136 was one of the first tests that I wrote. > I thought so at first, too, but then I saw that other places in the same > test do use it. > > Anyway, thanks for the quick reply. Then let's just try out what breaks > if we change it. :-) Yup, I’ll take a look. (Thanks for chiming in, Berto!) Hanna ^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH 9/9] iotests: Add delay-alert test 2026-08-31 13:51 [PATCH 0/9] block: BLOCK_IO_DELAY event Hanna Czenczek ` (7 preceding siblings ...) 2026-08-31 13:52 ` [PATCH 8/9] block/accounting: Move latency_ns override down Hanna Czenczek @ 2026-08-31 13:52 ` Hanna Czenczek 2026-09-03 15:16 ` Stefan Hajnoczi 2026-09-03 14:08 ` [PATCH 0/9] block: BLOCK_IO_DELAY event Stefan Hajnoczi 9 siblings, 1 reply; 47+ messages in thread From: Hanna Czenczek @ 2026-08-31 13:52 UTC (permalink / raw) To: qemu-block Cc: qemu-devel, Hanna Czenczek, Kevin Wolf, John Snow, Denis V . Lunev, Eric Blake, Markus Armbruster, Stefan Hajnoczi Test delay-alert-ms via HMP qemu-io (break, aio_read/write, resume) on a null node (with blkdebug). Signed-off-by: Hanna Czenczek <hreitz@redhat.com> --- tests/qemu-iotests/tests/delay-alert | 136 +++++++++++++++++++++++ tests/qemu-iotests/tests/delay-alert.out | 46 ++++++++ 2 files changed, 182 insertions(+) create mode 100755 tests/qemu-iotests/tests/delay-alert create mode 100644 tests/qemu-iotests/tests/delay-alert.out diff --git a/tests/qemu-iotests/tests/delay-alert b/tests/qemu-iotests/tests/delay-alert new file mode 100755 index 00000000000..d98f537e802 --- /dev/null +++ b/tests/qemu-iotests/tests/delay-alert @@ -0,0 +1,136 @@ +#!/usr/bin/env python3 +# group: rw +# +# Test the I/O delay QMP event. +# +# Copyright (C) 2026 Red Hat, Inc. +# +# SPDX-License-Identifier: GPL-2.0-or-later +# +# Creator/Owner: Hanna Czenczek <hreitz@redhat.com> + +import asyncio +import iotests +from iotests import filter_qmp_event, log + +DELAY_ALERT_MS = 5000 # ms +EPSILON_MS = 50 # ms -- small delta that can safely be added/subtracted +NS_PER_MS = 1000 * 1000 + +qtest_clock = 0 # ms + +iotests.script_initialize() + +def vm_qemu_io(qvm, virtio_blk_id, cmd): + log(f'[HMP] qemu-io to {virtio_blk_id}: {cmd}') + cmd = f'qemu-io -d {virtio_blk_id}/virtio-backend "{cmd}"' + log(qvm.qmp('human-monitor-command', command_line=cmd)) + +def advance_clock_to(qvm, clock_ms): + # Retain the current absolute clock time so we can step *to* a specific + # time via `clock_step`. pylint does not like global much, but this is a + # test script, and not using global would just make this more complicated. + global qtest_clock # pylint: disable=global-statement + + msecs = clock_ms - qtest_clock + log(f'[qtest] clock_step {msecs}ms to {clock_ms}ms') + qvm.qtest(f'clock_step {msecs * NS_PER_MS}') + qtest_clock += msecs + +def assert_no_event(qvm, event_name): + try: + evt = qvm.event_wait(event_name, timeout=0.1) + assert evt is None, f'Unexpected {event_name} event: {evt}' + except asyncio.TimeoutError: + pass + log(f'(No {event_name} event)') + +with iotests.VM() as vm: + # Cannot use null-co.latency-ns, as that uses realtime (not qtest time) + # Need to use raw to get the read_aio event working + vm.add_blockdev(vm.qmp_to_opts({ + 'driver': 'raw', + 'node-name': 'test-node', + 'file': { + 'driver': 'blkdebug', + 'image': { + 'driver': 'null-co', + }, + }, + })) + + vm.launch() + + log(f'[QMP] device_add id=vblk delay-alert-ms={DELAY_ALERT_MS}') + log(vm.qmp('device_add', { + 'driver': 'virtio-blk', + 'id': 'vblk', + 'drive': 'test-node', + 'delay-alert-ms': DELAY_ALERT_MS, + })) + + try: + # Test different operation types. Flushing and discarding would also be + # nice but `aio_flush` does not actually execute a flush, and there is + # no blkdebug event for discarding. + + vm_qemu_io(vm, 'vblk', 'break read_aio read_0') + vm_qemu_io(vm, 'vblk', 'aio_read 0k 4k') + vm_qemu_io(vm, 'vblk', 'wait_break read_0') + + vm_qemu_io(vm, 'vblk', 'break write_aio write_0') + vm_qemu_io(vm, 'vblk', 'aio_write 4k 4k') + vm_qemu_io(vm, 'vblk', 'wait_break write_0') + + vm_qemu_io(vm, 'vblk', 'break read_aio read_1') + vm_qemu_io(vm, 'vblk', 'aio_read 8k 4k') + vm_qemu_io(vm, 'vblk', 'wait_break read_1') + + vm_qemu_io(vm, 'vblk', 'break write_aio write_1') + vm_qemu_io(vm, 'vblk', 'aio_write 12k 4k') + vm_qemu_io(vm, 'vblk', 'wait_break write_1') + + # Assert there is no event before the step + advance_clock_to(vm, DELAY_ALERT_MS - EPSILON_MS) + assert_no_event(vm, 'BLOCK_IO_DELAY') + + # Even when a request finishes here + vm_qemu_io(vm, 'vblk', 'resume read_0') + assert_no_event(vm, 'BLOCK_IO_DELAY') + + # Crossing the threshold alone does not cause an event to fire + advance_clock_to(vm, DELAY_ALERT_MS + EPSILON_MS) + assert_no_event(vm, 'BLOCK_IO_DELAY') + + # Only when the request finishes past the threshold do we see one + vm_qemu_io(vm, 'vblk', 'resume write_0') + event = vm.event_wait('BLOCK_IO_DELAY') + assert event is not None + log(event, filters=[filter_qmp_event]) + assert_no_event(vm, 'BLOCK_IO_DELAY') + + # Check that we get the event immediately on completion + vm_qemu_io(vm, 'vblk', 'resume read_1') + event = vm.event_wait('BLOCK_IO_DELAY') + assert event is not None + log(event, filters=[filter_qmp_event]) + assert_no_event(vm, 'BLOCK_IO_DELAY') + + # And the same for the final request, but add another bit of delay + advance_clock_to(vm, DELAY_ALERT_MS + 2 * EPSILON_MS) + vm_qemu_io(vm, 'vblk', 'resume write_1') + event = vm.event_wait('BLOCK_IO_DELAY') + assert event is not None + log(event, filters=[filter_qmp_event]) + assert_no_event(vm, 'BLOCK_IO_DELAY') + except Exception as exc: + # The VM will not be able to quit without these requests + # finished, which (without this block) would make the test + # hang instead of exit if something unexpected goes wrong + vm_qemu_io(vm, 'vblk', 'resume read_0') + vm_qemu_io(vm, 'vblk', 'resume write_0') + vm_qemu_io(vm, 'vblk', 'resume read_1') + vm_qemu_io(vm, 'vblk', 'resume write_1') + raise exc + + vm.shutdown() diff --git a/tests/qemu-iotests/tests/delay-alert.out b/tests/qemu-iotests/tests/delay-alert.out new file mode 100644 index 00000000000..fe6cd71c6b1 --- /dev/null +++ b/tests/qemu-iotests/tests/delay-alert.out @@ -0,0 +1,46 @@ +[QMP] device_add id=vblk delay-alert-ms=5000 +{"return": {}} +[HMP] qemu-io to vblk: break read_aio read_0 +{"return": ""} +[HMP] qemu-io to vblk: aio_read 0k 4k +{"return": ""} +[HMP] qemu-io to vblk: wait_break read_0 +{"return": ""} +[HMP] qemu-io to vblk: break write_aio write_0 +{"return": ""} +[HMP] qemu-io to vblk: aio_write 4k 4k +{"return": ""} +[HMP] qemu-io to vblk: wait_break write_0 +{"return": ""} +[HMP] qemu-io to vblk: break read_aio read_1 +{"return": ""} +[HMP] qemu-io to vblk: aio_read 8k 4k +{"return": ""} +[HMP] qemu-io to vblk: wait_break read_1 +{"return": ""} +[HMP] qemu-io to vblk: break write_aio write_1 +{"return": ""} +[HMP] qemu-io to vblk: aio_write 12k 4k +{"return": ""} +[HMP] qemu-io to vblk: wait_break write_1 +{"return": ""} +[qtest] clock_step 4950ms to 4950ms +(No BLOCK_IO_DELAY event) +[HMP] qemu-io to vblk: resume read_0 +{"return": ""} +(No BLOCK_IO_DELAY event) +[qtest] clock_step 100ms to 5050ms +(No BLOCK_IO_DELAY event) +[HMP] qemu-io to vblk: resume write_0 +{"return": ""} +{"data": {"bytes": 4096, "duration": 5.05, "offset": 4096, "operation": "write", "qom-path": "/machine/peripheral/vblk/virtio-backend"}, "event": "BLOCK_IO_DELAY", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}} +(No BLOCK_IO_DELAY event) +[HMP] qemu-io to vblk: resume read_1 +{"return": ""} +{"data": {"bytes": 4096, "duration": 5.05, "offset": 8192, "operation": "read", "qom-path": "/machine/peripheral/vblk/virtio-backend"}, "event": "BLOCK_IO_DELAY", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}} +(No BLOCK_IO_DELAY event) +[qtest] clock_step 50ms to 5100ms +[HMP] qemu-io to vblk: resume write_1 +{"return": ""} +{"data": {"bytes": 4096, "duration": 5.1, "offset": 12288, "operation": "write", "qom-path": "/machine/peripheral/vblk/virtio-backend"}, "event": "BLOCK_IO_DELAY", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}} +(No BLOCK_IO_DELAY event) -- 2.55.0 ^ permalink raw reply related [flat|nested] 47+ messages in thread
* Re: [PATCH 9/9] iotests: Add delay-alert test 2026-08-31 13:52 ` [PATCH 9/9] iotests: Add delay-alert test Hanna Czenczek @ 2026-09-03 15:16 ` Stefan Hajnoczi 0 siblings, 0 replies; 47+ messages in thread From: Stefan Hajnoczi @ 2026-09-03 15:16 UTC (permalink / raw) To: Hanna Czenczek Cc: qemu-block, qemu-devel, Kevin Wolf, John Snow, Denis V . Lunev, Eric Blake, Markus Armbruster [-- Attachment #1: Type: text/plain, Size: 587 bytes --] On Mon, Aug 31, 2026 at 03:52:05PM +0200, Hanna Czenczek wrote: > Test delay-alert-ms via HMP qemu-io (break, aio_read/write, resume) on a > null node (with blkdebug). > > Signed-off-by: Hanna Czenczek <hreitz@redhat.com> > --- > tests/qemu-iotests/tests/delay-alert | 136 +++++++++++++++++++++++ > tests/qemu-iotests/tests/delay-alert.out | 46 ++++++++ > 2 files changed, 182 insertions(+) > create mode 100755 tests/qemu-iotests/tests/delay-alert > create mode 100644 tests/qemu-iotests/tests/delay-alert.out Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 0/9] block: BLOCK_IO_DELAY event 2026-08-31 13:51 [PATCH 0/9] block: BLOCK_IO_DELAY event Hanna Czenczek ` (8 preceding siblings ...) 2026-08-31 13:52 ` [PATCH 9/9] iotests: Add delay-alert test Hanna Czenczek @ 2026-09-03 14:08 ` Stefan Hajnoczi 2026-09-16 8:04 ` Hanna Czenczek 9 siblings, 1 reply; 47+ messages in thread From: Stefan Hajnoczi @ 2026-09-03 14:08 UTC (permalink / raw) To: Hanna Czenczek Cc: qemu-block, qemu-devel, Kevin Wolf, John Snow, Denis V . Lunev, Eric Blake, Markus Armbruster [-- Attachment #1: Type: text/plain, Size: 5858 bytes --] On Mon, Aug 31, 2026 at 03:51:56PM +0200, Hanna Czenczek wrote: > Based-on: <20260724152315.234183-1-hreitz@redhat.com> > [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. > (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? 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. 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? 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. > > > (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 > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 0/9] block: BLOCK_IO_DELAY event 2026-09-03 14:08 ` [PATCH 0/9] block: BLOCK_IO_DELAY event Stefan Hajnoczi @ 2026-09-16 8:04 ` Hanna Czenczek 2026-09-21 20:41 ` Stefan Hajnoczi 0 siblings, 1 reply; 47+ messages in thread From: Hanna Czenczek @ 2026-09-16 8:04 UTC (permalink / raw) To: Stefan Hajnoczi Cc: qemu-block, qemu-devel, Kevin Wolf, John Snow, Denis V . Lunev, Eric Blake, Markus Armbruster On 03.09.26 16:08, Stefan Hajnoczi wrote: > On Mon, Aug 31, 2026 at 03:51:56PM +0200, Hanna Czenczek wrote: >> Based-on:<20260724152315.234183-1-hreitz@redhat.com> >> [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 >> ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 0/9] block: BLOCK_IO_DELAY event 2026-09-16 8:04 ` Hanna Czenczek @ 2026-09-21 20:41 ` Stefan Hajnoczi 2026-09-22 13:31 ` Kevin Wolf 2026-09-23 10:48 ` Hanna Czenczek 0 siblings, 2 replies; 47+ messages in thread From: Stefan Hajnoczi @ 2026-09-21 20:41 UTC (permalink / raw) To: Hanna Czenczek Cc: qemu-block, qemu-devel, Kevin Wolf, John Snow, Denis V . Lunev, Eric Blake, Markus Armbruster [-- Attachment #1: Type: text/plain, Size: 9668 bytes --] 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:<20260724152315.234183-1-hreitz@redhat.com> > > > [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. > > > (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. 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. > 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. > > > 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? Libvirt could add an API to notify when a particular threshold is reached without any QEMU changes. Stefan > > 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 > > > > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 0/9] block: BLOCK_IO_DELAY event 2026-09-21 20:41 ` Stefan Hajnoczi @ 2026-09-22 13:31 ` Kevin Wolf 2026-09-22 17:18 ` Stefan Hajnoczi 2026-09-23 10:48 ` Hanna Czenczek 1 sibling, 1 reply; 47+ messages in thread From: Kevin Wolf @ 2026-09-22 13:31 UTC (permalink / raw) To: Stefan Hajnoczi Cc: Hanna Czenczek, qemu-block, qemu-devel, John Snow, Denis V . Lunev, Eric Blake, Markus Armbruster [-- Attachment #1: Type: text/plain, Size: 5771 bytes --] Am 21.09.2026 um 22:41 hat Stefan Hajnoczi geschrieben: > 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:<20260724152315.234183-1-hreitz@redhat.com> > > > > [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 think we'll need to do this eventually. There was also talk about stopping the VM if a request hangs for too long, which will definitely need a timer. But I think this specific series can work without it for now. Once we do have the timer anyway, reporting the latency right when the threshold is crossed can still be done. We should be careful with the wording in the documentation to allow both behaviours so we can make this change in the future. > > > > (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. > > 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. > > > 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. Apart from cases where requests are completed entirely within the device (like for all block_acct_invalid() callers), there are also cases where a single device-level requests involves multiple backend-level requests. I was thinking of IDE TRIM initially, but actually I think splitting can happen for any request that uses the DMA helpers. Conversely, virtio-blk can merge requests, so you get a single request in the backend that covers multiple requests in the device. Kevin [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 0/9] block: BLOCK_IO_DELAY event 2026-09-22 13:31 ` Kevin Wolf @ 2026-09-22 17:18 ` Stefan Hajnoczi 2026-09-23 10:52 ` Hanna Czenczek 0 siblings, 1 reply; 47+ messages in thread From: Stefan Hajnoczi @ 2026-09-22 17:18 UTC (permalink / raw) To: Kevin Wolf Cc: Hanna Czenczek, qemu-block, qemu-devel, John Snow, Denis V . Lunev, Eric Blake, Markus Armbruster [-- Attachment #1: Type: text/plain, Size: 1723 bytes --] On Tue, Sep 22, 2026 at 03:31:49PM +0200, Kevin Wolf wrote: > Am 21.09.2026 um 22:41 hat Stefan Hajnoczi geschrieben: > > 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: > > > 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. > > Apart from cases where requests are completed entirely within the > device (like for all block_acct_invalid() callers), there are also cases > where a single device-level requests involves multiple backend-level > requests. I was thinking of IDE TRIM initially, but actually I think > splitting can happen for any request that uses the DMA helpers. > > Conversely, virtio-blk can merge requests, so you get a single request > in the backend that covers multiple requests in the device. Sticking to the requests as seen by the device seems like the cleanest solution rather than cheating and counting host requests in some places. The idea to move the accounting into blk_aio_*() doesn't work well in light of this. Stefan [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 0/9] block: BLOCK_IO_DELAY event 2026-09-22 17:18 ` Stefan Hajnoczi @ 2026-09-23 10:52 ` Hanna Czenczek 2026-09-24 14:51 ` Stefan Hajnoczi 0 siblings, 1 reply; 47+ messages in thread From: Hanna Czenczek @ 2026-09-23 10:52 UTC (permalink / raw) To: Stefan Hajnoczi, Kevin Wolf Cc: qemu-block, qemu-devel, John Snow, Denis V . Lunev, Eric Blake, Markus Armbruster On 22.09.26 19:18, Stefan Hajnoczi wrote: > On Tue, Sep 22, 2026 at 03:31:49PM +0200, Kevin Wolf wrote: >> Am 21.09.2026 um 22:41 hat Stefan Hajnoczi geschrieben: >>> 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: >>>> 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. >> Apart from cases where requests are completed entirely within the >> device (like for all block_acct_invalid() callers), there are also cases >> where a single device-level requests involves multiple backend-level >> requests. I was thinking of IDE TRIM initially, but actually I think >> splitting can happen for any request that uses the DMA helpers. >> >> Conversely, virtio-blk can merge requests, so you get a single request >> in the backend that covers multiple requests in the device. > Sticking to the requests as seen by the device seems like the cleanest > solution rather than cheating and counting host requests in some places. > > The idea to move the accounting into blk_aio_*() doesn't work well in > light of this. Does this mean you would be against separating delay monitoring from the rest of accounting? Because to me that still sounds reasonable: To do delay monitoring in the BB layer, separate from accounting cookies, to have a simple lifecycle and timely reporting. (And as you did say, if one wants the guest-device-request-level delay information, the histogram is there O:) ) Hanna ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 0/9] block: BLOCK_IO_DELAY event 2026-09-23 10:52 ` Hanna Czenczek @ 2026-09-24 14:51 ` Stefan Hajnoczi 0 siblings, 0 replies; 47+ messages in thread From: Stefan Hajnoczi @ 2026-09-24 14:51 UTC (permalink / raw) To: Hanna Czenczek Cc: Kevin Wolf, qemu-block, qemu-devel, John Snow, Denis V . Lunev, Eric Blake, Markus Armbruster [-- Attachment #1: Type: text/plain, Size: 2474 bytes --] On Wed, Sep 23, 2026 at 12:52:17PM +0200, Hanna Czenczek wrote: > On 22.09.26 19:18, Stefan Hajnoczi wrote: > > On Tue, Sep 22, 2026 at 03:31:49PM +0200, Kevin Wolf wrote: > > > Am 21.09.2026 um 22:41 hat Stefan Hajnoczi geschrieben: > > > > 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: > > > > > 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. > > > Apart from cases where requests are completed entirely within the > > > device (like for all block_acct_invalid() callers), there are also cases > > > where a single device-level requests involves multiple backend-level > > > requests. I was thinking of IDE TRIM initially, but actually I think > > > splitting can happen for any request that uses the DMA helpers. > > > > > > Conversely, virtio-blk can merge requests, so you get a single request > > > in the backend that covers multiple requests in the device. > > Sticking to the requests as seen by the device seems like the cleanest > > solution rather than cheating and counting host requests in some places. > > > > The idea to move the accounting into blk_aio_*() doesn't work well in > > light of this. > > Does this mean you would be against separating delay monitoring from the > rest of accounting? I'm not against it. I just don't think we can push accounting down into the block layer since accounting operates at the emulated device's request level and there isn't a 1:1 correspondence between block layer I/O requests and device level requests. > Because to me that still sounds reasonable: To do delay monitoring in the BB > layer, separate from accounting cookies, to have a simple lifecycle and > timely reporting. Yes. Stefan [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 0/9] block: BLOCK_IO_DELAY event 2026-09-21 20:41 ` Stefan Hajnoczi 2026-09-22 13:31 ` Kevin Wolf @ 2026-09-23 10:48 ` Hanna Czenczek 2026-09-23 14:50 ` Kevin Wolf 2026-09-24 15:17 ` Stefan Hajnoczi 1 sibling, 2 replies; 47+ messages in thread From: Hanna Czenczek @ 2026-09-23 10:48 UTC (permalink / raw) To: Stefan Hajnoczi Cc: qemu-block, qemu-devel, Kevin Wolf, John Snow, Denis V . Lunev, Eric Blake, Markus Armbruster 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:<20260724152315.234183-1-hreitz@redhat.com> >>>> [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 ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 0/9] block: BLOCK_IO_DELAY event 2026-09-23 10:48 ` Hanna Czenczek @ 2026-09-23 14:50 ` Kevin Wolf 2026-09-24 15:17 ` Stefan Hajnoczi 1 sibling, 0 replies; 47+ messages in thread From: Kevin Wolf @ 2026-09-23 14:50 UTC (permalink / raw) To: Hanna Czenczek Cc: Stefan Hajnoczi, qemu-block, qemu-devel, John Snow, Denis V . Lunev, Eric Blake, Markus Armbruster Am 23.09.2026 um 12:48 hat Hanna Czenczek geschrieben: > > > > 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. It's the difference between an event and active polling. I would agree that active polling isn't the right answer in most cases. Kevin ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 0/9] block: BLOCK_IO_DELAY event 2026-09-23 10:48 ` Hanna Czenczek 2026-09-23 14:50 ` Kevin Wolf @ 2026-09-24 15:17 ` Stefan Hajnoczi 2026-09-24 18:31 ` Kevin Wolf 1 sibling, 1 reply; 47+ messages in thread From: Stefan Hajnoczi @ 2026-09-24 15:17 UTC (permalink / raw) To: Hanna Czenczek Cc: qemu-block, qemu-devel, Kevin Wolf, John Snow, Denis V . Lunev, Eric Blake, Markus Armbruster [-- Attachment #1: Type: text/plain, Size: 10011 bytes --] On Wed, Sep 23, 2026 at 12:48:36PM +0200, Hanna Czenczek wrote: > 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:<20260724152315.234183-1-hreitz@redhat.com> > > > > > [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. Regarding the lifecycle, I don't see a fundamental problem. It's possible to add a timer to block layer I/O requests and know for certain that leaks, use-after-free, etc are not possible because the caller doesn't need to juggle anything. Even if the caller has to juggle something, this is a C codebase where APIs are not always safe. That's not a blocker as long as they API design allows disciplined users to use it correctly. If I misunderstood your concerns and there is a fundamental reason why the lifetime cannot be made correct, maybe you can explain? > > > > > (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. Why at the QEMU level though? Libvirt can offer an event based on the block histogram. The utility of this new feature is not clear to me. How is this QMP event actually going to be used? Usually a feature has a clear use case and the cost of committing to the QMP API and maintaining backwards compatibility is easy to justify. In this case I'm not sure what the real use case is or whether this is something someone thought might be nice to have but may never use - we'll have to maintain it forever either way. (Timeliness makes the need for this feature clear to me. QEMU has to be involved either by pausing the guest itself or by emitting a QMP event. This is why I'm particularly interested in timeliness.) Stefan [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 0/9] block: BLOCK_IO_DELAY event 2026-09-24 15:17 ` Stefan Hajnoczi @ 2026-09-24 18:31 ` Kevin Wolf 0 siblings, 0 replies; 47+ messages in thread From: Kevin Wolf @ 2026-09-24 18:31 UTC (permalink / raw) To: Stefan Hajnoczi Cc: Hanna Czenczek, qemu-block, qemu-devel, John Snow, Denis V . Lunev, Eric Blake, Markus Armbruster [-- Attachment #1: Type: text/plain, Size: 6819 bytes --] Am 24.09.2026 um 17:17 hat Stefan Hajnoczi geschrieben: > On Wed, Sep 23, 2026 at 12:48:36PM +0200, Hanna Czenczek wrote: > > 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:<20260724152315.234183-1-hreitz@redhat.com> > > > > > > [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. > > Regarding the lifecycle, I don't see a fundamental problem. It's > possible to add a timer to block layer I/O requests and know for certain > that leaks, use-after-free, etc are not possible because the caller > doesn't need to juggle anything. Even if the caller has to juggle > something, this is a C codebase where APIs are not always safe. That's > not a blocker as long as they API design allows disciplined users to use > it correctly. If I misunderstood your concerns and there is a > fundamental reason why the lifetime cannot be made correct, maybe you > can explain? I don't think there is a fundamental reason other than that humans are bad at writing correct C code, but that this is based more on Hanna's finding that our existing code has bugs in this respect and she isn't sure if she caught all of them. The current consequence of missing the end of a request is that it isn't accounted for, and that's it. After the change, it means that a timer callback will fire long after the request is gone, and therefore work on a stale pointer, so it may turn into memory corruption and crashes. Maybe this can be mitigated by not referencing data bound to the request lifetime, but just keeping a copy of type/offset/length or whatever is needed for emitting the QAPI event, at the cost of a heap allocation as Hanna explained in the cover letter. It would still mean that a bogus timeout is reported. So I agree that we'd want to have those cases fixed before depending on correct block_acct_start()/block_account_one_io() pairing. And I'm afraid that Hanna is also right that it might be impossible to get this done by static analysis in our callback-heavy device implementations. If the full request control flow were in a coroutine, TSA could probably do it, but I don't think it can work with our actual code. Maybe simple cases like virtio-blk could be covered with the latest TSA improvements coming from the kernel (I would have to check if they cover callbacks well enough), but the scsi-disk state machine looks too complicated for this. > > > > > 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. > > Why at the QEMU level though? Libvirt can offer an event based on the > block histogram. At what intervals should libvirt poll QEMU? And why should polling be better than using an event? > The utility of this new feature is not clear to me. How is this QMP > event actually going to be used? Usually a feature has a clear use case > and the cost of committing to the QMP API and maintaining backwards > compatibility is easy to justify. In this case I'm not sure what the > real use case is or whether this is something someone thought might be > nice to have but may never use - we'll have to maintain it forever > either way. The idea is to show a warning to the user that their storage has latency issues and they need to look into that. Ideally before the latency reaches critical values that result in errors. > (Timeliness makes the need for this feature clear to me. QEMU has to be > involved either by pausing the guest itself or by emitting a QMP event. > This is why I'm particularly interested in timeliness.) If you want to prevent a in-flight request from timing out in the guest, then yes, QEMU would have pause the guest. I think emitting a QMP event alone wouldn't be good enough for this. (Even more so considering that a 'stop' command actually involves draining all requests.) Kevin [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 47+ messages in thread
end of thread, other threads:[~2026-09-24 18:32 UTC | newest] Thread overview: 47+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-08-31 13:51 [PATCH 0/9] block: BLOCK_IO_DELAY event Hanna Czenczek 2026-08-31 13:51 ` [PATCH 1/9] block/accounting: Add offset to BlockAcctCookie Hanna Czenczek 2026-09-07 10:05 ` Jesper Wendel Devantier 2026-08-31 13:51 ` [PATCH 2/9] qapi/block: Add IoAccountingOperation enum Hanna Czenczek 2026-09-03 14:32 ` Markus Armbruster 2026-09-16 8:09 ` Hanna Czenczek 2026-08-31 13:51 ` [PATCH 3/9] qapi/block: Add BLOCK_IO_DELAY event Hanna Czenczek 2026-09-03 14:36 ` Markus Armbruster 2026-09-16 8:08 ` Hanna Czenczek 2026-09-16 9:57 ` Markus Armbruster 2026-08-31 13:52 ` [PATCH 4/9] block-backend: Public blk_get_attached_dev_path() Hanna Czenczek 2026-08-31 13:52 ` [PATCH 5/9] block/accounting: Add BB field to latency checker Hanna Czenczek 2026-09-22 12:14 ` Kevin Wolf 2026-09-23 11:05 ` Hanna Czenczek 2026-08-31 13:52 ` [PATCH 6/9] block/accounting: Emit BLOCK_IO_DELAY event Hanna Czenczek 2026-09-03 14:23 ` Stefan Hajnoczi 2026-09-16 8:14 ` Hanna Czenczek 2026-09-16 12:04 ` Hanna Czenczek 2026-09-21 20:48 ` Stefan Hajnoczi 2026-09-23 10:45 ` Hanna Czenczek 2026-09-22 12:44 ` Kevin Wolf 2026-09-23 11:11 ` Hanna Czenczek 2026-08-31 13:52 ` [PATCH 7/9] block: Add delay-alert-ms property Hanna Czenczek 2026-09-03 14:30 ` Stefan Hajnoczi 2026-09-16 8:17 ` Hanna Czenczek 2026-09-22 12:51 ` Kevin Wolf 2026-08-31 13:52 ` [PATCH 8/9] block/accounting: Move latency_ns override down Hanna Czenczek 2026-09-03 15:08 ` Stefan Hajnoczi 2026-09-16 8:21 ` Hanna Czenczek 2026-09-21 20:42 ` Stefan Hajnoczi 2026-09-22 13:03 ` Kevin Wolf 2026-09-22 13:44 ` Alberto Garcia 2026-09-22 13:52 ` Kevin Wolf 2026-09-23 10:50 ` Hanna Czenczek 2026-08-31 13:52 ` [PATCH 9/9] iotests: Add delay-alert test Hanna Czenczek 2026-09-03 15:16 ` Stefan Hajnoczi 2026-09-03 14:08 ` [PATCH 0/9] block: BLOCK_IO_DELAY event Stefan Hajnoczi 2026-09-16 8:04 ` Hanna Czenczek 2026-09-21 20:41 ` Stefan Hajnoczi 2026-09-22 13:31 ` Kevin Wolf 2026-09-22 17:18 ` Stefan Hajnoczi 2026-09-23 10:52 ` Hanna Czenczek 2026-09-24 14:51 ` Stefan Hajnoczi 2026-09-23 10:48 ` Hanna Czenczek 2026-09-23 14:50 ` Kevin Wolf 2026-09-24 15:17 ` Stefan Hajnoczi 2026-09-24 18:31 ` Kevin Wolf
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.