Re: [PATCH] block/nvme: Add driver statistics for access alignment and hw errors

2020-10-01 Thread Philippe Mathieu-Daudé
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.





Re: [PATCH] block/nvme: Add driver statistics for access alignment and hw errors

2020-10-01 Thread Stefan Hajnoczi
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.


signature.asc
Description: PGP signature


[PATCH] block/nvme: Add driver statistics for access alignment and hw errors

2020-09-30 Thread Philippe Mathieu-Daudé
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 
Signed-off-by: Philippe Mathieu-Daudé 
---
 qapi/block-core.json | 24 +++-
 block/nvme.c | 27 +++
 2 files changed, 50 insertions(+), 1 deletion(-)

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,