| Message ID | 20200930133617.1320941-1-philmd@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | block/nvme: Add driver statistics for access alignment and hw errors | expand |
On Wed, Sep 30, 2020 at 03:36:17PM +0200, Philippe Mathieu-Daudé wrote: > "return": [ > { > "device": "", > "node-name": "drive0", > "stats": { > "flush_total_time_ns": 6026948, > "wr_highest_offset": 3383991230464, > "wr_total_time_ns": 807450995, > "failed_wr_operations": 0, > "failed_rd_operations": 0, > "wr_merged": 3, > "wr_bytes": 50133504, > "failed_unmap_operations": 0, > "failed_flush_operations": 0, > "account_invalid": false, > "rd_total_time_ns": 1846979900, > "flush_operations": 130, > "wr_operations": 659, > "rd_merged": 1192, > "rd_bytes": 218244096, > "account_failed": false, > "idle_time_ns": 2678641497, > "rd_operations": 7406, > }, > "driver-specific": { > "driver": "nvme", > "completion-errors": 0, > "unaligned-access-nb": 2959, > "aligned-access-nb": 4477 "nb" is number? Using plural ("unaligned-accesses" and "aligned-accesses") makes it clear that the value is a count. It's also consistent with the existing "wr_operations" and "failed_operations" stats.
On 10/1/20 11:59 AM, Stefan Hajnoczi wrote: > On Wed, Sep 30, 2020 at 03:36:17PM +0200, Philippe Mathieu-Daudé wrote: >> "return": [ >> { >> "device": "", >> "node-name": "drive0", >> "stats": { >> "flush_total_time_ns": 6026948, >> "wr_highest_offset": 3383991230464, >> "wr_total_time_ns": 807450995, >> "failed_wr_operations": 0, >> "failed_rd_operations": 0, >> "wr_merged": 3, >> "wr_bytes": 50133504, >> "failed_unmap_operations": 0, >> "failed_flush_operations": 0, >> "account_invalid": false, >> "rd_total_time_ns": 1846979900, >> "flush_operations": 130, >> "wr_operations": 659, >> "rd_merged": 1192, >> "rd_bytes": 218244096, >> "account_failed": false, >> "idle_time_ns": 2678641497, >> "rd_operations": 7406, >> }, >> "driver-specific": { >> "driver": "nvme", >> "completion-errors": 0, >> "unaligned-access-nb": 2959, >> "aligned-access-nb": 4477 > > "nb" is number? Yes: # @aligned-access-nb: The number of aligned accesses performed by # the driver. # # @unaligned-access-nb: The number of unaligned accesses performed by # the driver. Copied from: qapi/block-core.json:928:# @discard-nb-ok: The number of successful discard operations performed by qapi/block-core.json:931:# @discard-nb-failed: The number of failed discard operations performed by qapi/block-core.json:940: 'discard-nb-ok': 'uint64', qapi/block-core.json:941: 'discard-nb-failed': 'uint64', > > Using plural ("unaligned-accesses" and "aligned-accesses") makes it > clear that the value is a count. It's also consistent with the existing > "wr_operations" and "failed_operations" stats. OK, I'll update. Thanks for the review, Phil.
diff --git a/qapi/block-core.json b/qapi/block-core.json index 86ed72ef9f..795e4185bd 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -941,6 +941,27 @@ 'discard-nb-failed': 'uint64', 'discard-bytes-ok': 'uint64' } } +## +# @BlockStatsSpecificNvme: +# +# NVMe driver statistics +# +# @completion-errors: The number of completion errors. +# +# @aligned-access-nb: The number of aligned accesses performed by +# the driver. +# +# @unaligned-access-nb: The number of unaligned accesses performed by +# the driver. +# +# Since: 5.2 +## +{ 'struct': 'BlockStatsSpecificNvme', + 'data': { + 'completion-errors': 'uint64', + 'aligned-access-nb': 'uint64', + 'unaligned-access-nb': 'uint64' } } + ## # @BlockStatsSpecific: # @@ -953,7 +974,8 @@ 'discriminator': 'driver', 'data': { 'file': 'BlockStatsSpecificFile', - 'host_device': 'BlockStatsSpecificFile' } } + 'host_device': 'BlockStatsSpecificFile', + 'nvme': 'BlockStatsSpecificNvme' } } ## # @BlockStats: diff --git a/block/nvme.c b/block/nvme.c index f4f27b6da7..382f696202 100644 --- a/block/nvme.c +++ b/block/nvme.c @@ -133,6 +133,12 @@ struct BDRVNVMeState { /* PCI address (required for nvme_refresh_filename()) */ char *device; + + struct { + uint64_t completion_errors; + uint64_t aligned_access_nb; + uint64_t unaligned_access_nb; + } stats; }; #define NVME_BLOCK_OPT_DEVICE "device" @@ -389,6 +395,9 @@ static bool nvme_process_completion(NVMeQueuePair *q) break; } ret = nvme_translate_error(c); + if (ret) { + s->stats.completion_errors++; + } q->cq.head = (q->cq.head + 1) % NVME_QUEUE_SIZE; if (!q->cq.head) { q->cq_phase = !q->cq_phase; @@ -1146,8 +1155,10 @@ static int nvme_co_prw(BlockDriverState *bs, uint64_t offset, uint64_t bytes, assert(QEMU_IS_ALIGNED(bytes, s->page_size)); assert(bytes <= s->max_transfer); if (nvme_qiov_aligned(bs, qiov)) { + s->stats.aligned_access_nb++; return nvme_co_prw_aligned(bs, offset, bytes, qiov, is_write, flags); } + s->stats.unaligned_access_nb++; trace_nvme_prw_buffered(s, offset, bytes, qiov->niov, is_write); buf = qemu_try_memalign(s->page_size, bytes); @@ -1443,6 +1454,21 @@ static void nvme_unregister_buf(BlockDriverState *bs, void *host) qemu_vfio_dma_unmap(s->vfio, host); } +static BlockStatsSpecific *nvme_get_specific_stats(BlockDriverState *bs) +{ + BlockStatsSpecific *stats = g_new(BlockStatsSpecific, 1); + BDRVNVMeState *s = bs->opaque; + + stats->driver = BLOCKDEV_DRIVER_NVME; + stats->u.nvme = (BlockStatsSpecificNvme) { + .completion_errors = s->stats.completion_errors, + .aligned_access_nb = s->stats.aligned_access_nb, + .unaligned_access_nb = s->stats.unaligned_access_nb, + }; + + return stats; +} + static const char *const nvme_strong_runtime_opts[] = { NVME_BLOCK_OPT_DEVICE, NVME_BLOCK_OPT_NAMESPACE, @@ -1476,6 +1502,7 @@ static BlockDriver bdrv_nvme = { .bdrv_refresh_filename = nvme_refresh_filename, .bdrv_refresh_limits = nvme_refresh_limits, .strong_runtime_opts = nvme_strong_runtime_opts, + .bdrv_get_specific_stats = nvme_get_specific_stats, .bdrv_detach_aio_context = nvme_detach_aio_context, .bdrv_attach_aio_context = nvme_attach_aio_context,
Keep statistics of some hardware errors, and number of aligned/unaligned I/O accesses. QMP example booting a full RHEL 8.3 aarch64 guest: { "execute": "query-blockstats" } { "return": [ { "device": "", "node-name": "drive0", "stats": { "flush_total_time_ns": 6026948, "wr_highest_offset": 3383991230464, "wr_total_time_ns": 807450995, "failed_wr_operations": 0, "failed_rd_operations": 0, "wr_merged": 3, "wr_bytes": 50133504, "failed_unmap_operations": 0, "failed_flush_operations": 0, "account_invalid": false, "rd_total_time_ns": 1846979900, "flush_operations": 130, "wr_operations": 659, "rd_merged": 1192, "rd_bytes": 218244096, "account_failed": false, "idle_time_ns": 2678641497, "rd_operations": 7406, }, "driver-specific": { "driver": "nvme", "completion-errors": 0, "unaligned-access-nb": 2959, "aligned-access-nb": 4477 }, "qdev": "/machine/peripheral-anon/device[0]/virtio-backend" } ] } Suggested-by: Stefan Hajnoczi <stefanha@gmail.com> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- qapi/block-core.json | 24 +++++++++++++++++++++++- block/nvme.c | 27 +++++++++++++++++++++++++++ 2 files changed, 50 insertions(+), 1 deletion(-)