All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH v8 0/9] discard blockstats
@ 2019-05-16 14:33 Anton Nefedov
  2019-05-16 14:33 ` [Qemu-devel] [PATCH v8 1/9] qapi: group BlockDeviceStats fields Anton Nefedov
                   ` (8 more replies)
  0 siblings, 9 replies; 21+ messages in thread
From: Anton Nefedov @ 2019-05-16 14:33 UTC (permalink / raw)
  To: qemu-block
  Cc: kwolf, vsementsov, berto, den, qemu-devel, mreitz, Anton Nefedov,
	pbonzini, jsnow

..apparently v7 ended up in a weird base64 that would not easily git-am.
Resending.

----

hi,

yet another take for this patch series; please kindly consider these for 4.1

Just a few cosmetic comments were received for v6 so this is mostly
a rebase+ping.

new in v7:
    - general rebase
    - since clauses -> 4.1
    - patch 8: not completely trivial rebase: raw_account_discard moved to
      common raw_do_pdiscard()
    - patch 9: comment wording fixed

v6: http://lists.nongnu.org/archive/html/qemu-devel/2018-11/msg06633.html
v5: http://lists.nongnu.org/archive/html/qemu-devel/2018-10/msg06828.html
v4: http://lists.nongnu.org/archive/html/qemu-devel/2018-08/msg04308.html
v3: http://lists.nongnu.org/archive/html/qemu-devel/2018-06/msg03688.html

----

qmp query-blockstats provides stats info for write/read/flush ops.

Patches 1-7 implement the similar for discard (unmap) command for scsi
and ide disks.
Discard stat "unmap_ops / unmap_bytes" is supposed to account the ops that
have completed without an error.

However, discard operation is advisory. Specifically,
 - common block layer ignores ENOTSUP error code.
   That might be returned if the block driver does not support discard,
   or discard has been configured to be ignored.
 - format drivers such as qcow2 may ignore discard if they were configured
   to ignore that, or if the corresponding area is already marked unused
   (unallocated / zero clusters).

And what is actually useful is the number of bytes actually discarded
down on the host filesystem.
To achieve that, driver-specific statistics has been added to blockstats
(patch 9).
With patch 8, file-posix driver accounts discard operations on its level too.

query-blockstat result:

(note the difference between blockdevice unmap and file discard stats. qcow2
sends fewer ops down to the file as the clusters are actually unallocated
on qcow2 level)

    {
      "device": "drive-scsi0-0-0-0",
      "node-name": "#block159",
      "stats": {
>       "invalid_unmap_operations": 0,
>       "failed_unmap_operations": 0,
        "wr_highest_offset": 13411688448,
        "rd_total_time_ns": 2859566315,
        "rd_bytes": 103182336,
        "rd_merged": 0,
        "flush_operations": 19,
        "invalid_wr_operations": 0,
        "flush_total_time_ns": 23111608,
        "failed_rd_operations": 0,
        "failed_flush_operations": 0,
        "invalid_flush_operations": 0,
        "timed_stats": [
          
        ],
        "wr_merged": 0,
        "wr_bytes": 1702912,
>       "unmap_bytes": 11954954240,
>       "unmap_operations": 865,
        "idle_time_ns": 2669508623,
        "account_invalid": true,
>       "unmap_total_time_ns": 19698002,
        "wr_operations": 143,
        "failed_wr_operations": 0,
        "rd_operations": 4816,
        "account_failed": true,
>       "unmap_merged": 0,
        "wr_total_time_ns": 1262686124,
        "invalid_rd_operations": 0
      },
      "parent": {
>       "driver-specific": {
>         "discard-nb-failed": 0,
>         "discard-bytes-ok": 720896,
>         "driver": "file",
>         "discard-nb-ok": 8
>       },
        "node-name": "#block009",
        "stats": {
        [..]
        }
      }
    },
    {
      "device": "floppy0",

Anton Nefedov (9):
  qapi: group BlockDeviceStats fields
  qapi: add unmap to BlockDeviceStats
  block: add empty account cookie type
  ide: account UNMAP (TRIM) operations
  scsi: store unmap offset and nb_sectors in request struct
  scsi: move unmap error checking to the complete callback
  scsi: account unmap operations
  file-posix: account discard operations
  qapi: query-blockstat: add driver specific file-posix stats

 qapi/block-core.json       | 81 ++++++++++++++++++++++++++++++++------
 include/block/accounting.h |  2 +
 include/block/block.h      |  1 +
 include/block/block_int.h  |  1 +
 block.c                    |  9 +++++
 block/accounting.c         |  6 +++
 block/file-posix.c         | 54 ++++++++++++++++++++++++-
 block/qapi.c               | 11 ++++++
 hw/ide/core.c              | 12 ++++++
 hw/scsi/scsi-disk.c        | 32 +++++++++------
 tests/qemu-iotests/227.out | 18 +++++++++
 11 files changed, 204 insertions(+), 23 deletions(-)

-- 
2.17.1



^ permalink raw reply	[flat|nested] 21+ messages in thread

* [Qemu-devel] [PATCH v8 1/9] qapi: group BlockDeviceStats fields
  2019-05-16 14:33 [Qemu-devel] [PATCH v8 0/9] discard blockstats Anton Nefedov
@ 2019-05-16 14:33 ` Anton Nefedov
  2019-05-16 14:33 ` [Qemu-devel] [PATCH v8 2/9] qapi: add unmap to BlockDeviceStats Anton Nefedov
                   ` (7 subsequent siblings)
  8 siblings, 0 replies; 21+ messages in thread
From: Anton Nefedov @ 2019-05-16 14:33 UTC (permalink / raw)
  To: qemu-block
  Cc: kwolf, vsementsov, berto, den, qemu-devel, mreitz, Anton Nefedov,
	pbonzini, jsnow

Make the stat fields definition slightly more readable.
Also reorder total_time_ns stats read-write-flush as done elsewhere.
Cosmetic change only.

Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 qapi/block-core.json | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 7ccbfff9d0..754d07f1fb 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -863,12 +863,12 @@
 # @flush_operations: The number of cache flush operations performed by the
 #                    device (since 0.15.0)
 #
-# @flush_total_time_ns: Total time spend on cache flushes in nano-seconds
-#                       (since 0.15.0).
+# @rd_total_time_ns: Total time spent on reads in nanoseconds (since 0.15.0).
 #
-# @wr_total_time_ns: Total time spend on writes in nano-seconds (since 0.15.0).
+# @wr_total_time_ns: Total time spent on writes in nanoseconds (since 0.15.0).
 #
-# @rd_total_time_ns: Total_time_spend on reads in nano-seconds (since 0.15.0).
+# @flush_total_time_ns: Total time spent on cache flushes in nanoseconds
+#                       (since 0.15.0).
 #
 # @wr_highest_offset: The offset after the greatest byte written to the
 #                     device.  The intended use of this information is for
@@ -921,14 +921,18 @@
 # Since: 0.14.0
 ##
 { 'struct': 'BlockDeviceStats',
-  'data': {'rd_bytes': 'int', 'wr_bytes': 'int', 'rd_operations': 'int',
-           'wr_operations': 'int', 'flush_operations': 'int',
-           'flush_total_time_ns': 'int', 'wr_total_time_ns': 'int',
-           'rd_total_time_ns': 'int', 'wr_highest_offset': 'int',
-           'rd_merged': 'int', 'wr_merged': 'int', '*idle_time_ns': 'int',
+  'data': {'rd_bytes': 'int', 'wr_bytes': 'int',
+           'rd_operations': 'int', 'wr_operations': 'int',
+           'flush_operations': 'int',
+           'rd_total_time_ns': 'int', 'wr_total_time_ns': 'int',
+           'flush_total_time_ns': 'int',
+           'wr_highest_offset': 'int',
+           'rd_merged': 'int', 'wr_merged': 'int',
+           '*idle_time_ns': 'int',
            'failed_rd_operations': 'int', 'failed_wr_operations': 'int',
-           'failed_flush_operations': 'int', 'invalid_rd_operations': 'int',
-           'invalid_wr_operations': 'int', 'invalid_flush_operations': 'int',
+           'failed_flush_operations': 'int',
+           'invalid_rd_operations': 'int', 'invalid_wr_operations': 'int',
+           'invalid_flush_operations': 'int',
            'account_invalid': 'bool', 'account_failed': 'bool',
            'timed_stats': ['BlockDeviceTimedStats'],
            '*rd_latency_histogram': 'BlockLatencyHistogramInfo',
-- 
2.17.1



^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [Qemu-devel] [PATCH v8 2/9] qapi: add unmap to BlockDeviceStats
  2019-05-16 14:33 [Qemu-devel] [PATCH v8 0/9] discard blockstats Anton Nefedov
  2019-05-16 14:33 ` [Qemu-devel] [PATCH v8 1/9] qapi: group BlockDeviceStats fields Anton Nefedov
@ 2019-05-16 14:33 ` Anton Nefedov
  2019-05-16 14:33 ` [Qemu-devel] [PATCH v8 3/9] block: add empty account cookie type Anton Nefedov
                   ` (6 subsequent siblings)
  8 siblings, 0 replies; 21+ messages in thread
From: Anton Nefedov @ 2019-05-16 14:33 UTC (permalink / raw)
  To: qemu-block
  Cc: kwolf, vsementsov, berto, den, qemu-devel, mreitz, Anton Nefedov,
	pbonzini, jsnow

Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 qapi/block-core.json       | 29 +++++++++++++++++++++++------
 include/block/accounting.h |  1 +
 block/qapi.c               |  6 ++++++
 tests/qemu-iotests/227.out | 18 ++++++++++++++++++
 4 files changed, 48 insertions(+), 6 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 754d07f1fb..55194f84ce 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -856,6 +856,8 @@
 #
 # @wr_bytes:      The number of bytes written by the device.
 #
+# @unmap_bytes: The number of bytes unmapped by the device (Since 4.1)
+#
 # @rd_operations: The number of read operations performed by the device.
 #
 # @wr_operations: The number of write operations performed by the device.
@@ -863,6 +865,9 @@
 # @flush_operations: The number of cache flush operations performed by the
 #                    device (since 0.15.0)
 #
+# @unmap_operations: The number of unmap operations performed by the device
+#                    (Since 4.1)
+#
 # @rd_total_time_ns: Total time spent on reads in nanoseconds (since 0.15.0).
 #
 # @wr_total_time_ns: Total time spent on writes in nanoseconds (since 0.15.0).
@@ -870,6 +875,9 @@
 # @flush_total_time_ns: Total time spent on cache flushes in nanoseconds
 #                       (since 0.15.0).
 #
+# @unmap_total_time_ns: Total time spent on unmap operations in nanoseconds
+#                       (Since 4.1)
+#
 # @wr_highest_offset: The offset after the greatest byte written to the
 #                     device.  The intended use of this information is for
 #                     growable sparse files (like qcow2) that are used on top
@@ -881,6 +889,9 @@
 # @wr_merged: Number of write requests that have been merged into another
 #             request (Since 2.3).
 #
+# @unmap_merged: Number of unmap requests that have been merged into another
+#                request (Since 4.1)
+#
 # @idle_time_ns: Time since the last I/O operation, in
 #                nanoseconds. If the field is absent it means that
 #                there haven't been any operations yet (Since 2.5).
@@ -894,6 +905,9 @@
 # @failed_flush_operations: The number of failed flush operations
 #                           performed by the device (Since 2.5)
 #
+# @failed_unmap_operations: The number of failed unmap operations performed
+#                           by the device (Since 4.1)
+#
 # @invalid_rd_operations: The number of invalid read operations
 #                          performed by the device (Since 2.5)
 #
@@ -903,6 +917,9 @@
 # @invalid_flush_operations: The number of invalid flush operations
 #                            performed by the device (Since 2.5)
 #
+# @invalid_unmap_operations: The number of invalid unmap operations performed
+#                            by the device (Since 4.1)
+#
 # @account_invalid: Whether invalid operations are included in the
 #                   last access statistics (Since 2.5)
 #
@@ -921,18 +938,18 @@
 # Since: 0.14.0
 ##
 { 'struct': 'BlockDeviceStats',
-  'data': {'rd_bytes': 'int', 'wr_bytes': 'int',
+  'data': {'rd_bytes': 'int', 'wr_bytes': 'int', 'unmap_bytes' : 'int',
            'rd_operations': 'int', 'wr_operations': 'int',
-           'flush_operations': 'int',
+           'flush_operations': 'int', 'unmap_operations': 'int',
            'rd_total_time_ns': 'int', 'wr_total_time_ns': 'int',
-           'flush_total_time_ns': 'int',
+           'flush_total_time_ns': 'int', 'unmap_total_time_ns': 'int',
            'wr_highest_offset': 'int',
-           'rd_merged': 'int', 'wr_merged': 'int',
+           'rd_merged': 'int', 'wr_merged': 'int', 'unmap_merged': 'int',
            '*idle_time_ns': 'int',
            'failed_rd_operations': 'int', 'failed_wr_operations': 'int',
-           'failed_flush_operations': 'int',
+           'failed_flush_operations': 'int', 'failed_unmap_operations': 'int',
            'invalid_rd_operations': 'int', 'invalid_wr_operations': 'int',
-           'invalid_flush_operations': 'int',
+           'invalid_flush_operations': 'int', 'invalid_unmap_operations': 'int',
            'account_invalid': 'bool', 'account_failed': 'bool',
            'timed_stats': ['BlockDeviceTimedStats'],
            '*rd_latency_histogram': 'BlockLatencyHistogramInfo',
diff --git a/include/block/accounting.h b/include/block/accounting.h
index d1f67b10dd..ba8b04d572 100644
--- a/include/block/accounting.h
+++ b/include/block/accounting.h
@@ -36,6 +36,7 @@ enum BlockAcctType {
     BLOCK_ACCT_READ,
     BLOCK_ACCT_WRITE,
     BLOCK_ACCT_FLUSH,
+    BLOCK_ACCT_UNMAP,
     BLOCK_MAX_IOTYPE,
 };
 
diff --git a/block/qapi.c b/block/qapi.c
index 0c13c86f4e..f9447a3297 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -434,24 +434,30 @@ static void bdrv_query_blk_stats(BlockDeviceStats *ds, BlockBackend *blk)
 
     ds->rd_bytes = stats->nr_bytes[BLOCK_ACCT_READ];
     ds->wr_bytes = stats->nr_bytes[BLOCK_ACCT_WRITE];
+    ds->unmap_bytes = stats->nr_bytes[BLOCK_ACCT_UNMAP];
     ds->rd_operations = stats->nr_ops[BLOCK_ACCT_READ];
     ds->wr_operations = stats->nr_ops[BLOCK_ACCT_WRITE];
+    ds->unmap_operations = stats->nr_ops[BLOCK_ACCT_UNMAP];
 
     ds->failed_rd_operations = stats->failed_ops[BLOCK_ACCT_READ];
     ds->failed_wr_operations = stats->failed_ops[BLOCK_ACCT_WRITE];
     ds->failed_flush_operations = stats->failed_ops[BLOCK_ACCT_FLUSH];
+    ds->failed_unmap_operations = stats->failed_ops[BLOCK_ACCT_UNMAP];
 
     ds->invalid_rd_operations = stats->invalid_ops[BLOCK_ACCT_READ];
     ds->invalid_wr_operations = stats->invalid_ops[BLOCK_ACCT_WRITE];
     ds->invalid_flush_operations =
         stats->invalid_ops[BLOCK_ACCT_FLUSH];
+    ds->invalid_unmap_operations = stats->invalid_ops[BLOCK_ACCT_UNMAP];
 
     ds->rd_merged = stats->merged[BLOCK_ACCT_READ];
     ds->wr_merged = stats->merged[BLOCK_ACCT_WRITE];
+    ds->unmap_merged = stats->merged[BLOCK_ACCT_UNMAP];
     ds->flush_operations = stats->nr_ops[BLOCK_ACCT_FLUSH];
     ds->wr_total_time_ns = stats->total_time_ns[BLOCK_ACCT_WRITE];
     ds->rd_total_time_ns = stats->total_time_ns[BLOCK_ACCT_READ];
     ds->flush_total_time_ns = stats->total_time_ns[BLOCK_ACCT_FLUSH];
+    ds->unmap_total_time_ns = stats->total_time_ns[BLOCK_ACCT_UNMAP];
 
     ds->has_idle_time_ns = stats->last_access_time_ns > 0;
     if (ds->has_idle_time_ns) {
diff --git a/tests/qemu-iotests/227.out b/tests/qemu-iotests/227.out
index e77efaf4cf..e9ab4d21f9 100644
--- a/tests/qemu-iotests/227.out
+++ b/tests/qemu-iotests/227.out
@@ -15,6 +15,8 @@ Testing: -drive driver=null-co,if=virtio
         {
             "device": "virtio0",
             "stats": {
+                "unmap_operations": 0,
+                "unmap_merged": 0,
                 "flush_total_time_ns": 0,
                 "wr_highest_offset": 0,
                 "wr_total_time_ns": 0,
@@ -24,13 +26,17 @@ Testing: -drive driver=null-co,if=virtio
                 "wr_bytes": 0,
                 "timed_stats": [
                 ],
+                "failed_unmap_operations": 0,
                 "failed_flush_operations": 0,
                 "account_invalid": true,
                 "rd_total_time_ns": 0,
+                "invalid_unmap_operations": 0,
                 "flush_operations": 0,
                 "wr_operations": 0,
+                "unmap_bytes": 0,
                 "rd_merged": 0,
                 "rd_bytes": 0,
+                "unmap_total_time_ns": 0,
                 "invalid_flush_operations": 0,
                 "account_failed": true,
                 "rd_operations": 0,
@@ -74,6 +80,8 @@ Testing: -drive driver=null-co,if=none
         {
             "device": "none0",
             "stats": {
+                "unmap_operations": 0,
+                "unmap_merged": 0,
                 "flush_total_time_ns": 0,
                 "wr_highest_offset": 0,
                 "wr_total_time_ns": 0,
@@ -83,13 +91,17 @@ Testing: -drive driver=null-co,if=none
                 "wr_bytes": 0,
                 "timed_stats": [
                 ],
+                "failed_unmap_operations": 0,
                 "failed_flush_operations": 0,
                 "account_invalid": true,
                 "rd_total_time_ns": 0,
+                "invalid_unmap_operations": 0,
                 "flush_operations": 0,
                 "wr_operations": 0,
+                "unmap_bytes": 0,
                 "rd_merged": 0,
                 "rd_bytes": 0,
+                "unmap_total_time_ns": 0,
                 "invalid_flush_operations": 0,
                 "account_failed": true,
                 "rd_operations": 0,
@@ -163,6 +175,8 @@ Testing: -blockdev driver=null-co,node-name=null -device virtio-blk,drive=null,i
         {
             "device": "",
             "stats": {
+                "unmap_operations": 0,
+                "unmap_merged": 0,
                 "flush_total_time_ns": 0,
                 "wr_highest_offset": 0,
                 "wr_total_time_ns": 0,
@@ -172,13 +186,17 @@ Testing: -blockdev driver=null-co,node-name=null -device virtio-blk,drive=null,i
                 "wr_bytes": 0,
                 "timed_stats": [
                 ],
+                "failed_unmap_operations": 0,
                 "failed_flush_operations": 0,
                 "account_invalid": false,
                 "rd_total_time_ns": 0,
+                "invalid_unmap_operations": 0,
                 "flush_operations": 0,
                 "wr_operations": 0,
+                "unmap_bytes": 0,
                 "rd_merged": 0,
                 "rd_bytes": 0,
+                "unmap_total_time_ns": 0,
                 "invalid_flush_operations": 0,
                 "account_failed": false,
                 "rd_operations": 0,
-- 
2.17.1



^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [Qemu-devel] [PATCH v8 3/9] block: add empty account cookie type
  2019-05-16 14:33 [Qemu-devel] [PATCH v8 0/9] discard blockstats Anton Nefedov
  2019-05-16 14:33 ` [Qemu-devel] [PATCH v8 1/9] qapi: group BlockDeviceStats fields Anton Nefedov
  2019-05-16 14:33 ` [Qemu-devel] [PATCH v8 2/9] qapi: add unmap to BlockDeviceStats Anton Nefedov
@ 2019-05-16 14:33 ` Anton Nefedov
  2019-05-16 15:34   ` Vladimir Sementsov-Ogievskiy
  2019-05-16 14:33 ` [Qemu-devel] [PATCH v8 4/9] ide: account UNMAP (TRIM) operations Anton Nefedov
                   ` (5 subsequent siblings)
  8 siblings, 1 reply; 21+ messages in thread
From: Anton Nefedov @ 2019-05-16 14:33 UTC (permalink / raw)
  To: qemu-block
  Cc: kwolf, vsementsov, berto, den, qemu-devel, mreitz, Anton Nefedov,
	pbonzini, jsnow

This adds some protection from accounting uninitialized cookie.
That is, block_acct_failed/done without previous block_acct_start;
in that case, cookie probably holds values from previous operation.

(Note: it might also be uninitialized holding garbage value and there is
 still "< BLOCK_MAX_IOTYPE" assertion for that.
 So block_acct_failed/done without previous block_acct_start should be used
 with caution.)

Currently this is particularly useful in ide code where it's hard to
keep track whether the request started accounting or not. For example,
trim requests do the accounting separately.

Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
---
 include/block/accounting.h | 1 +
 block/accounting.c         | 6 ++++++
 2 files changed, 7 insertions(+)

diff --git a/include/block/accounting.h b/include/block/accounting.h
index ba8b04d572..878b4c3581 100644
--- a/include/block/accounting.h
+++ b/include/block/accounting.h
@@ -33,6 +33,7 @@ typedef struct BlockAcctTimedStats BlockAcctTimedStats;
 typedef struct BlockAcctStats BlockAcctStats;
 
 enum BlockAcctType {
+    BLOCK_ACCT_NONE = 0,
     BLOCK_ACCT_READ,
     BLOCK_ACCT_WRITE,
     BLOCK_ACCT_FLUSH,
diff --git a/block/accounting.c b/block/accounting.c
index 70a3d9a426..8d41c8a83a 100644
--- a/block/accounting.c
+++ b/block/accounting.c
@@ -195,6 +195,10 @@ static void block_account_one_io(BlockAcctStats *stats, BlockAcctCookie *cookie,
 
     assert(cookie->type < BLOCK_MAX_IOTYPE);
 
+    if (cookie->type == BLOCK_ACCT_NONE) {
+        return;
+    }
+
     qemu_mutex_lock(&stats->lock);
 
     if (failed) {
@@ -217,6 +221,8 @@ static void block_account_one_io(BlockAcctStats *stats, BlockAcctCookie *cookie,
     }
 
     qemu_mutex_unlock(&stats->lock);
+
+    cookie->type = BLOCK_ACCT_NONE;
 }
 
 void block_acct_done(BlockAcctStats *stats, BlockAcctCookie *cookie)
-- 
2.17.1



^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [Qemu-devel] [PATCH v8 4/9] ide: account UNMAP (TRIM) operations
  2019-05-16 14:33 [Qemu-devel] [PATCH v8 0/9] discard blockstats Anton Nefedov
                   ` (2 preceding siblings ...)
  2019-05-16 14:33 ` [Qemu-devel] [PATCH v8 3/9] block: add empty account cookie type Anton Nefedov
@ 2019-05-16 14:33 ` Anton Nefedov
  2019-08-12 18:16   ` Max Reitz
  2019-05-16 14:33 ` [Qemu-devel] [PATCH v8 5/9] scsi: store unmap offset and nb_sectors in request struct Anton Nefedov
                   ` (4 subsequent siblings)
  8 siblings, 1 reply; 21+ messages in thread
From: Anton Nefedov @ 2019-05-16 14:33 UTC (permalink / raw)
  To: qemu-block
  Cc: kwolf, vsementsov, berto, den, qemu-devel, mreitz, Anton Nefedov,
	pbonzini, jsnow

Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 hw/ide/core.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 6afadf894f..3a7ac93777 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -441,6 +441,14 @@ static void ide_issue_trim_cb(void *opaque, int ret)
     TrimAIOCB *iocb = opaque;
     IDEState *s = iocb->s;
 
+    if (iocb->i >= 0) {
+        if (ret >= 0) {
+            block_acct_done(blk_get_stats(s->blk), &s->acct);
+        } else {
+            block_acct_failed(blk_get_stats(s->blk), &s->acct);
+        }
+    }
+
     if (ret >= 0) {
         while (iocb->j < iocb->qiov->niov) {
             int j = iocb->j;
@@ -458,10 +466,14 @@ static void ide_issue_trim_cb(void *opaque, int ret)
                 }
 
                 if (!ide_sect_range_ok(s, sector, count)) {
+                    block_acct_invalid(blk_get_stats(s->blk), BLOCK_ACCT_UNMAP);
                     iocb->ret = -EINVAL;
                     goto done;
                 }
 
+                block_acct_start(blk_get_stats(s->blk), &s->acct,
+                                 count << BDRV_SECTOR_BITS, BLOCK_ACCT_UNMAP);
+
                 /* Got an entry! Submit and exit.  */
                 iocb->aiocb = blk_aio_pdiscard(s->blk,
                                                sector << BDRV_SECTOR_BITS,
-- 
2.17.1



^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [Qemu-devel] [PATCH v8 5/9] scsi: store unmap offset and nb_sectors in request struct
  2019-05-16 14:33 [Qemu-devel] [PATCH v8 0/9] discard blockstats Anton Nefedov
                   ` (3 preceding siblings ...)
  2019-05-16 14:33 ` [Qemu-devel] [PATCH v8 4/9] ide: account UNMAP (TRIM) operations Anton Nefedov
@ 2019-05-16 14:33 ` Anton Nefedov
  2019-08-12 17:58   ` Max Reitz
  2019-05-16 14:33 ` [Qemu-devel] [PATCH v8 6/9] scsi: move unmap error checking to the complete callback Anton Nefedov
                   ` (3 subsequent siblings)
  8 siblings, 1 reply; 21+ messages in thread
From: Anton Nefedov @ 2019-05-16 14:33 UTC (permalink / raw)
  To: qemu-block
  Cc: kwolf, vsementsov, berto, den, qemu-devel, mreitz, Anton Nefedov,
	pbonzini, jsnow

it allows to report it in the error handler

Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
 hw/scsi/scsi-disk.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index e7e865ab3b..b43254103c 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -1602,8 +1602,6 @@ static void scsi_unmap_complete_noio(UnmapCBData *data, int ret)
 {
     SCSIDiskReq *r = data->r;
     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
-    uint64_t sector_num;
-    uint32_t nb_sectors;
 
     assert(r->req.aiocb == NULL);
     if (scsi_disk_req_check_error(r, ret, false)) {
@@ -1611,16 +1609,16 @@ static void scsi_unmap_complete_noio(UnmapCBData *data, int ret)
     }
 
     if (data->count > 0) {
-        sector_num = ldq_be_p(&data->inbuf[0]);
-        nb_sectors = ldl_be_p(&data->inbuf[8]) & 0xffffffffULL;
-        if (!check_lba_range(s, sector_num, nb_sectors)) {
+        r->sector = ldq_be_p(&data->inbuf[0]);
+        r->sector_count = ldl_be_p(&data->inbuf[8]) & 0xffffffffULL;
+        if (!check_lba_range(s, r->sector, r->sector_count)) {
             scsi_check_condition(r, SENSE_CODE(LBA_OUT_OF_RANGE));
             goto done;
         }
 
         r->req.aiocb = blk_aio_pdiscard(s->qdev.conf.blk,
-                                        sector_num * s->qdev.blocksize,
-                                        nb_sectors * s->qdev.blocksize,
+                                        r->sector * s->qdev.blocksize,
+                                        r->sector_count * s->qdev.blocksize,
                                         scsi_unmap_complete, data);
         data->count--;
         data->inbuf += 16;
-- 
2.17.1



^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [Qemu-devel] [PATCH v8 6/9] scsi: move unmap error checking to the complete callback
  2019-05-16 14:33 [Qemu-devel] [PATCH v8 0/9] discard blockstats Anton Nefedov
                   ` (4 preceding siblings ...)
  2019-05-16 14:33 ` [Qemu-devel] [PATCH v8 5/9] scsi: store unmap offset and nb_sectors in request struct Anton Nefedov
@ 2019-05-16 14:33 ` Anton Nefedov
  2019-05-16 14:33 ` [Qemu-devel] [PATCH v8 7/9] scsi: account unmap operations Anton Nefedov
                   ` (2 subsequent siblings)
  8 siblings, 0 replies; 21+ messages in thread
From: Anton Nefedov @ 2019-05-16 14:33 UTC (permalink / raw)
  To: qemu-block
  Cc: kwolf, vsementsov, berto, den, qemu-devel, mreitz, Anton Nefedov,
	pbonzini, jsnow

This will help to account the operation in the following commit.

The difference is that we don't call scsi_disk_req_check_error() before
the 1st discard iteration anymore. That function also checks if
the request is cancelled, however it shouldn't get canceled until it
yields in blk_aio() functions anyway.
Same approach is already used for emulate_write_same.

Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
 hw/scsi/scsi-disk.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index b43254103c..6eff496b54 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -1604,9 +1604,6 @@ static void scsi_unmap_complete_noio(UnmapCBData *data, int ret)
     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
 
     assert(r->req.aiocb == NULL);
-    if (scsi_disk_req_check_error(r, ret, false)) {
-        goto done;
-    }
 
     if (data->count > 0) {
         r->sector = ldq_be_p(&data->inbuf[0]);
@@ -1642,7 +1639,12 @@ static void scsi_unmap_complete(void *opaque, int ret)
     r->req.aiocb = NULL;
 
     aio_context_acquire(blk_get_aio_context(s->qdev.conf.blk));
-    scsi_unmap_complete_noio(data, ret);
+    if (scsi_disk_req_check_error(r, ret, false)) {
+        scsi_req_unref(&r->req);
+        g_free(data);
+    } else {
+        scsi_unmap_complete_noio(data, ret);
+    }
     aio_context_release(blk_get_aio_context(s->qdev.conf.blk));
 }
 
-- 
2.17.1



^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [Qemu-devel] [PATCH v8 7/9] scsi: account unmap operations
  2019-05-16 14:33 [Qemu-devel] [PATCH v8 0/9] discard blockstats Anton Nefedov
                   ` (5 preceding siblings ...)
  2019-05-16 14:33 ` [Qemu-devel] [PATCH v8 6/9] scsi: move unmap error checking to the complete callback Anton Nefedov
@ 2019-05-16 14:33 ` Anton Nefedov
  2019-05-16 14:33 ` [Qemu-devel] [PATCH v8 8/9] file-posix: account discard operations Anton Nefedov
  2019-05-16 14:33 ` [Qemu-devel] [PATCH v8 9/9] qapi: query-blockstat: add driver specific file-posix stats Anton Nefedov
  8 siblings, 0 replies; 21+ messages in thread
From: Anton Nefedov @ 2019-05-16 14:33 UTC (permalink / raw)
  To: qemu-block
  Cc: kwolf, vsementsov, berto, den, qemu-devel, mreitz, Anton Nefedov,
	pbonzini, jsnow

Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 hw/scsi/scsi-disk.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 6eff496b54..5c77981d60 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -1609,10 +1609,16 @@ static void scsi_unmap_complete_noio(UnmapCBData *data, int ret)
         r->sector = ldq_be_p(&data->inbuf[0]);
         r->sector_count = ldl_be_p(&data->inbuf[8]) & 0xffffffffULL;
         if (!check_lba_range(s, r->sector, r->sector_count)) {
+            block_acct_invalid(blk_get_stats(s->qdev.conf.blk),
+                               BLOCK_ACCT_UNMAP);
             scsi_check_condition(r, SENSE_CODE(LBA_OUT_OF_RANGE));
             goto done;
         }
 
+        block_acct_start(blk_get_stats(s->qdev.conf.blk), &r->acct,
+                         r->sector_count * s->qdev.blocksize,
+                         BLOCK_ACCT_UNMAP);
+
         r->req.aiocb = blk_aio_pdiscard(s->qdev.conf.blk,
                                         r->sector * s->qdev.blocksize,
                                         r->sector_count * s->qdev.blocksize,
@@ -1639,10 +1645,11 @@ static void scsi_unmap_complete(void *opaque, int ret)
     r->req.aiocb = NULL;
 
     aio_context_acquire(blk_get_aio_context(s->qdev.conf.blk));
-    if (scsi_disk_req_check_error(r, ret, false)) {
+    if (scsi_disk_req_check_error(r, ret, true)) {
         scsi_req_unref(&r->req);
         g_free(data);
     } else {
+        block_acct_done(blk_get_stats(s->qdev.conf.blk), &r->acct);
         scsi_unmap_complete_noio(data, ret);
     }
     aio_context_release(blk_get_aio_context(s->qdev.conf.blk));
@@ -1674,6 +1681,7 @@ static void scsi_disk_emulate_unmap(SCSIDiskReq *r, uint8_t *inbuf)
     }
 
     if (blk_is_read_only(s->qdev.conf.blk)) {
+        block_acct_invalid(blk_get_stats(s->qdev.conf.blk), BLOCK_ACCT_UNMAP);
         scsi_check_condition(r, SENSE_CODE(WRITE_PROTECTED));
         return;
     }
@@ -1689,10 +1697,12 @@ static void scsi_disk_emulate_unmap(SCSIDiskReq *r, uint8_t *inbuf)
     return;
 
 invalid_param_len:
+    block_acct_invalid(blk_get_stats(s->qdev.conf.blk), BLOCK_ACCT_UNMAP);
     scsi_check_condition(r, SENSE_CODE(INVALID_PARAM_LEN));
     return;
 
 invalid_field:
+    block_acct_invalid(blk_get_stats(s->qdev.conf.blk), BLOCK_ACCT_UNMAP);
     scsi_check_condition(r, SENSE_CODE(INVALID_FIELD));
 }
 
-- 
2.17.1



^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [Qemu-devel] [PATCH v8 8/9] file-posix: account discard operations
  2019-05-16 14:33 [Qemu-devel] [PATCH v8 0/9] discard blockstats Anton Nefedov
                   ` (6 preceding siblings ...)
  2019-05-16 14:33 ` [Qemu-devel] [PATCH v8 7/9] scsi: account unmap operations Anton Nefedov
@ 2019-05-16 14:33 ` Anton Nefedov
  2019-05-16 15:23   ` Vladimir Sementsov-Ogievskiy
  2019-05-16 14:33 ` [Qemu-devel] [PATCH v8 9/9] qapi: query-blockstat: add driver specific file-posix stats Anton Nefedov
  8 siblings, 1 reply; 21+ messages in thread
From: Anton Nefedov @ 2019-05-16 14:33 UTC (permalink / raw)
  To: qemu-block
  Cc: kwolf, vsementsov, berto, den, qemu-devel, mreitz, Anton Nefedov,
	pbonzini, jsnow

This will help to identify how many of the user-issued discard operations
(accounted on a device level) have actually suceeded down on the host file
(even though the numbers will not be exactly the same if non-raw format
driver is used (e.g. qcow2 sending metadata discards)).

Note that these numbers will not include discards triggered by
write-zeroes + MAY_UNMAP calls.

Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
---
 block/file-posix.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 1cf4ee49eb..76d54b3a85 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -159,6 +159,11 @@ typedef struct BDRVRawState {
     bool needs_alignment;
     bool drop_cache;
     bool check_cache_dropped;
+    struct {
+        int64_t discard_nb_ok;
+        int64_t discard_nb_failed;
+        int64_t discard_bytes_ok;
+    } stats;
 
     PRManager *pr_mgr;
 } BDRVRawState;
@@ -2630,11 +2635,22 @@ static void coroutine_fn raw_co_invalidate_cache(BlockDriverState *bs,
 #endif /* !__linux__ */
 }
 
+static void raw_account_discard(BDRVRawState *s, uint64_t nbytes, int ret)
+{
+    if (ret) {
+        s->stats.discard_nb_failed++;
+    } else {
+        s->stats.discard_nb_ok++;
+        s->stats.discard_bytes_ok += nbytes;
+    }
+}
+
 static coroutine_fn int
 raw_do_pdiscard(BlockDriverState *bs, int64_t offset, int bytes, bool blkdev)
 {
     BDRVRawState *s = bs->opaque;
     RawPosixAIOData acb;
+    int ret;
 
     acb = (RawPosixAIOData) {
         .bs             = bs,
@@ -2648,7 +2664,9 @@ raw_do_pdiscard(BlockDriverState *bs, int64_t offset, int bytes, bool blkdev)
         acb.aio_type |= QEMU_AIO_BLKDEV;
     }
 
-    return raw_thread_pool_submit(bs, handle_aiocb_discard, &acb);
+    ret = raw_thread_pool_submit(bs, handle_aiocb_discard, &acb);
+    raw_account_discard(s, bytes, ret);
+    return ret;
 }
 
 static coroutine_fn int
@@ -3263,10 +3281,12 @@ static int fd_open(BlockDriverState *bs)
 static coroutine_fn int
 hdev_co_pdiscard(BlockDriverState *bs, int64_t offset, int bytes)
 {
+    BDRVRawState *s = bs->opaque;
     int ret;
 
     ret = fd_open(bs);
     if (ret < 0) {
+        raw_account_discard(s, bytes, ret);
         return ret;
     }
     return raw_do_pdiscard(bs, offset, bytes, true);
-- 
2.17.1



^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [Qemu-devel] [PATCH v8 9/9] qapi: query-blockstat: add driver specific file-posix stats
  2019-05-16 14:33 [Qemu-devel] [PATCH v8 0/9] discard blockstats Anton Nefedov
                   ` (7 preceding siblings ...)
  2019-05-16 14:33 ` [Qemu-devel] [PATCH v8 8/9] file-posix: account discard operations Anton Nefedov
@ 2019-05-16 14:33 ` Anton Nefedov
  2019-08-12 19:04   ` Max Reitz
  8 siblings, 1 reply; 21+ messages in thread
From: Anton Nefedov @ 2019-05-16 14:33 UTC (permalink / raw)
  To: qemu-block
  Cc: kwolf, vsementsov, berto, den, qemu-devel, mreitz, Anton Nefedov,
	pbonzini, jsnow

A block driver can provide a callback to report driver-specific
statistics.

file-posix driver now reports discard statistics

Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Acked-by: Markus Armbruster <armbru@redhat.com>
---
 qapi/block-core.json      | 38 ++++++++++++++++++++++++++++++++++++++
 include/block/block.h     |  1 +
 include/block/block_int.h |  1 +
 block.c                   |  9 +++++++++
 block/file-posix.c        | 38 +++++++++++++++++++++++++++++++++++---
 block/qapi.c              |  5 +++++
 6 files changed, 89 insertions(+), 3 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 55194f84ce..368e09ae37 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -956,6 +956,41 @@
            '*wr_latency_histogram': 'BlockLatencyHistogramInfo',
            '*flush_latency_histogram': 'BlockLatencyHistogramInfo' } }
 
+##
+# @BlockStatsSpecificFile:
+#
+# File driver statistics
+#
+# @discard-nb-ok: The number of successful discard operations performed by
+#                 the driver.
+#
+# @discard-nb-failed: The number of failed discard operations performed by
+#                     the driver.
+#
+# @discard-bytes-ok: The number of bytes discarded by the driver.
+#
+# Since: 4.1
+##
+{ 'struct': 'BlockStatsSpecificFile',
+  'data': {
+      'discard-nb-ok': 'uint64',
+      'discard-nb-failed': 'uint64',
+      'discard-bytes-ok': 'uint64' } }
+
+##
+# @BlockStatsSpecific:
+#
+# Block driver specific statistics
+#
+# Since: 4.1
+##
+{ 'union': 'BlockStatsSpecific',
+  'base': { 'driver': 'BlockdevDriver' },
+  'discriminator': 'driver',
+  'data': {
+      'file': 'BlockStatsSpecificFile',
+      'host_device': 'BlockStatsSpecificFile' } }
+
 ##
 # @BlockStats:
 #
@@ -971,6 +1006,8 @@
 #
 # @stats:  A @BlockDeviceStats for the device.
 #
+# @driver-specific: Optional driver-specific stats. (Since 4.1)
+#
 # @parent: This describes the file block device if it has one.
 #          Contains recursively the statistics of the underlying
 #          protocol (e.g. the host file for a qcow2 image). If there is
@@ -984,6 +1021,7 @@
 { 'struct': 'BlockStats',
   'data': {'*device': 'str', '*qdev': 'str', '*node-name': 'str',
            'stats': 'BlockDeviceStats',
+           '*driver-specific': 'BlockStatsSpecific',
            '*parent': 'BlockStats',
            '*backing': 'BlockStats'} }
 
diff --git a/include/block/block.h b/include/block/block.h
index 5e2b98b0ee..b182f0c7ae 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -490,6 +490,7 @@ int bdrv_get_flags(BlockDriverState *bs);
 int bdrv_get_info(BlockDriverState *bs, BlockDriverInfo *bdi);
 ImageInfoSpecific *bdrv_get_specific_info(BlockDriverState *bs,
                                           Error **errp);
+BlockStatsSpecific *bdrv_get_specific_stats(BlockDriverState *bs);
 void bdrv_round_to_clusters(BlockDriverState *bs,
                             int64_t offset, int64_t bytes,
                             int64_t *cluster_offset,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 94d45c9708..dc3bc97ea3 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -358,6 +358,7 @@ struct BlockDriver {
     int (*bdrv_get_info)(BlockDriverState *bs, BlockDriverInfo *bdi);
     ImageInfoSpecific *(*bdrv_get_specific_info)(BlockDriverState *bs,
                                                  Error **errp);
+    BlockStatsSpecific *(*bdrv_get_specific_stats)(BlockDriverState *bs);
 
     int coroutine_fn (*bdrv_save_vmstate)(BlockDriverState *bs,
                                           QEMUIOVector *qiov,
diff --git a/block.c b/block.c
index 6999aad446..f68fb5aaec 100644
--- a/block.c
+++ b/block.c
@@ -4942,6 +4942,15 @@ ImageInfoSpecific *bdrv_get_specific_info(BlockDriverState *bs,
     return NULL;
 }
 
+BlockStatsSpecific *bdrv_get_specific_stats(BlockDriverState *bs)
+{
+    BlockDriver *drv = bs->drv;
+    if (!drv || !drv->bdrv_get_specific_stats) {
+        return NULL;
+    }
+    return drv->bdrv_get_specific_stats(bs);
+}
+
 void bdrv_debug_event(BlockDriverState *bs, BlkdebugEvent event)
 {
     if (!bs || !bs->drv || !bs->drv->bdrv_debug_event) {
diff --git a/block/file-posix.c b/block/file-posix.c
index 76d54b3a85..a2f01cfe10 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -160,9 +160,9 @@ typedef struct BDRVRawState {
     bool drop_cache;
     bool check_cache_dropped;
     struct {
-        int64_t discard_nb_ok;
-        int64_t discard_nb_failed;
-        int64_t discard_bytes_ok;
+        uint64_t discard_nb_ok;
+        uint64_t discard_nb_failed;
+        uint64_t discard_bytes_ok;
     } stats;
 
     PRManager *pr_mgr;
@@ -2723,6 +2723,36 @@ static int raw_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
     return 0;
 }
 
+static BlockStatsSpecificFile get_blockstats_specific_file(BlockDriverState *bs)
+{
+    BDRVRawState *s = bs->opaque;
+    return (BlockStatsSpecificFile) {
+        .discard_nb_ok = s->stats.discard_nb_ok,
+        .discard_nb_failed = s->stats.discard_nb_failed,
+        .discard_bytes_ok = s->stats.discard_bytes_ok,
+    };
+}
+
+static BlockStatsSpecific *raw_get_specific_stats(BlockDriverState *bs)
+{
+    BlockStatsSpecific *stats = g_new(BlockStatsSpecific, 1);
+
+    stats->driver = BLOCKDEV_DRIVER_FILE;
+    stats->u.file = get_blockstats_specific_file(bs);
+
+    return stats;
+}
+
+static BlockStatsSpecific *hdev_get_specific_stats(BlockDriverState *bs)
+{
+    BlockStatsSpecific *stats = g_new(BlockStatsSpecific, 1);
+
+    stats->driver = BLOCKDEV_DRIVER_HOST_DEVICE;
+    stats->u.host_device = get_blockstats_specific_file(bs);
+
+    return stats;
+}
+
 static QemuOptsList raw_create_opts = {
     .name = "raw-create-opts",
     .head = QTAILQ_HEAD_INITIALIZER(raw_create_opts.head),
@@ -2922,6 +2952,7 @@ BlockDriver bdrv_file = {
     .bdrv_get_info = raw_get_info,
     .bdrv_get_allocated_file_size
                         = raw_get_allocated_file_size,
+    .bdrv_get_specific_stats = raw_get_specific_stats,
     .bdrv_check_perm = raw_check_perm,
     .bdrv_set_perm   = raw_set_perm,
     .bdrv_abort_perm_update = raw_abort_perm_update,
@@ -3400,6 +3431,7 @@ static BlockDriver bdrv_host_device = {
     .bdrv_get_info = raw_get_info,
     .bdrv_get_allocated_file_size
                         = raw_get_allocated_file_size,
+    .bdrv_get_specific_stats = hdev_get_specific_stats,
     .bdrv_check_perm = raw_check_perm,
     .bdrv_set_perm   = raw_set_perm,
     .bdrv_abort_perm_update = raw_abort_perm_update,
diff --git a/block/qapi.c b/block/qapi.c
index f9447a3297..3afcb9dc5c 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -537,6 +537,11 @@ static BlockStats *bdrv_query_bds_stats(BlockDriverState *bs,
 
     s->stats->wr_highest_offset = stat64_get(&bs->wr_highest_offset);
 
+    s->driver_specific = bdrv_get_specific_stats(bs);
+    if (s->driver_specific) {
+        s->has_driver_specific = true;
+    }
+
     if (bs->file) {
         s->has_parent = true;
         s->parent = bdrv_query_bds_stats(bs->file->bs, blk_level);
-- 
2.17.1



^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] [PATCH v8 8/9] file-posix: account discard operations
  2019-05-16 14:33 ` [Qemu-devel] [PATCH v8 8/9] file-posix: account discard operations Anton Nefedov
@ 2019-05-16 15:23   ` Vladimir Sementsov-Ogievskiy
  0 siblings, 0 replies; 21+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-05-16 15:23 UTC (permalink / raw)
  To: Anton Nefedov, qemu-block
  Cc: kwolf, berto, Denis Lunev, qemu-devel, mreitz, pbonzini, jsnow

16.05.2019 17:33, Anton Nefedov wrote:
> This will help to identify how many of the user-issued discard operations
> (accounted on a device level) have actually suceeded down on the host file
> (even though the numbers will not be exactly the same if non-raw format
> driver is used (e.g. qcow2 sending metadata discards)).
> 
> Note that these numbers will not include discards triggered by
> write-zeroes + MAY_UNMAP calls.
> 
> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>



-- 
Best regards,
Vladimir

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] [PATCH v8 3/9] block: add empty account cookie type
  2019-05-16 14:33 ` [Qemu-devel] [PATCH v8 3/9] block: add empty account cookie type Anton Nefedov
@ 2019-05-16 15:34   ` Vladimir Sementsov-Ogievskiy
  2019-05-16 16:00     ` Anton Nefedov
  0 siblings, 1 reply; 21+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-05-16 15:34 UTC (permalink / raw)
  To: Anton Nefedov, qemu-block
  Cc: kwolf, berto, Denis Lunev, qemu-devel, mreitz, pbonzini, jsnow

16.05.2019 17:33, Anton Nefedov wrote:
> This adds some protection from accounting uninitialized cookie.
> That is, block_acct_failed/done without previous block_acct_start;
> in that case, cookie probably holds values from previous operation.
> 
> (Note: it might also be uninitialized holding garbage value and there is
>   still "< BLOCK_MAX_IOTYPE" assertion for that.
>   So block_acct_failed/done without previous block_acct_start should be used
>   with caution.)
> 
> Currently this is particularly useful in ide code where it's hard to
> keep track whether the request started accounting or not. For example,
> trim requests do the accounting separately.
> 
> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
> ---
>   include/block/accounting.h | 1 +
>   block/accounting.c         | 6 ++++++
>   2 files changed, 7 insertions(+)
> 
> diff --git a/include/block/accounting.h b/include/block/accounting.h
> index ba8b04d572..878b4c3581 100644
> --- a/include/block/accounting.h
> +++ b/include/block/accounting.h
> @@ -33,6 +33,7 @@ typedef struct BlockAcctTimedStats BlockAcctTimedStats;
>   typedef struct BlockAcctStats BlockAcctStats;
>   
>   enum BlockAcctType {
> +    BLOCK_ACCT_NONE = 0,
>       BLOCK_ACCT_READ,
>       BLOCK_ACCT_WRITE,
>       BLOCK_ACCT_FLUSH,
> diff --git a/block/accounting.c b/block/accounting.c
> index 70a3d9a426..8d41c8a83a 100644
> --- a/block/accounting.c
> +++ b/block/accounting.c
> @@ -195,6 +195,10 @@ static void block_account_one_io(BlockAcctStats *stats, BlockAcctCookie *cookie,
>   
>       assert(cookie->type < BLOCK_MAX_IOTYPE);
>   
> +    if (cookie->type == BLOCK_ACCT_NONE) {

worth error_report() ?

> +        return;
> +    }
> +
>       qemu_mutex_lock(&stats->lock);
>   
>       if (failed) {
> @@ -217,6 +221,8 @@ static void block_account_one_io(BlockAcctStats *stats, BlockAcctCookie *cookie,
>       }
>   
>       qemu_mutex_unlock(&stats->lock);
> +
> +    cookie->type = BLOCK_ACCT_NONE;
>   }
>   
>   void block_acct_done(BlockAcctStats *stats, BlockAcctCookie *cookie)
> 

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] [PATCH v8 3/9] block: add empty account cookie type
  2019-05-16 15:34   ` Vladimir Sementsov-Ogievskiy
@ 2019-05-16 16:00     ` Anton Nefedov
  0 siblings, 0 replies; 21+ messages in thread
From: Anton Nefedov @ 2019-05-16 16:00 UTC (permalink / raw)
  To: Vladimir Sementsov-Ogievskiy, qemu-block
  Cc: kwolf, berto, Denis Lunev, qemu-devel, mreitz, pbonzini, jsnow

On 16/5/2019 6:34 PM, Vladimir Sementsov-Ogievskiy wrote:
> 16.05.2019 17:33, Anton Nefedov wrote:
>> This adds some protection from accounting uninitialized cookie.
>> That is, block_acct_failed/done without previous block_acct_start;
>> in that case, cookie probably holds values from previous operation.
>>
>> (Note: it might also be uninitialized holding garbage value and there is
>>    still "< BLOCK_MAX_IOTYPE" assertion for that.
>>    So block_acct_failed/done without previous block_acct_start should be used
>>    with caution.)
>>
>> Currently this is particularly useful in ide code where it's hard to
>> keep track whether the request started accounting or not. For example,
>> trim requests do the accounting separately.
>>
>> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
>> ---
>>    include/block/accounting.h | 1 +
>>    block/accounting.c         | 6 ++++++
>>    2 files changed, 7 insertions(+)
>>
>> diff --git a/include/block/accounting.h b/include/block/accounting.h
>> index ba8b04d572..878b4c3581 100644
>> --- a/include/block/accounting.h
>> +++ b/include/block/accounting.h
>> @@ -33,6 +33,7 @@ typedef struct BlockAcctTimedStats BlockAcctTimedStats;
>>    typedef struct BlockAcctStats BlockAcctStats;
>>    
>>    enum BlockAcctType {
>> +    BLOCK_ACCT_NONE = 0,
>>        BLOCK_ACCT_READ,
>>        BLOCK_ACCT_WRITE,
>>        BLOCK_ACCT_FLUSH,
>> diff --git a/block/accounting.c b/block/accounting.c
>> index 70a3d9a426..8d41c8a83a 100644
>> --- a/block/accounting.c
>> +++ b/block/accounting.c
>> @@ -195,6 +195,10 @@ static void block_account_one_io(BlockAcctStats *stats, BlockAcctCookie *cookie,
>>    
>>        assert(cookie->type < BLOCK_MAX_IOTYPE);
>>    
>> +    if (cookie->type == BLOCK_ACCT_NONE) {
> 
> worth error_report() ?
> 

I don't think we should necessarily consider it an error;
as mentioned in the commit message this might be useful in some places
like IDE trim handling.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] [PATCH v8 5/9] scsi: store unmap offset and nb_sectors in request struct
  2019-05-16 14:33 ` [Qemu-devel] [PATCH v8 5/9] scsi: store unmap offset and nb_sectors in request struct Anton Nefedov
@ 2019-08-12 17:58   ` Max Reitz
  2019-08-21 11:03     ` Anton Nefedov
  0 siblings, 1 reply; 21+ messages in thread
From: Max Reitz @ 2019-08-12 17:58 UTC (permalink / raw)
  To: Anton Nefedov, qemu-block
  Cc: kwolf, vsementsov, berto, den, qemu-devel, pbonzini, jsnow


[-- Attachment #1.1: Type: text/plain, Size: 2426 bytes --]

On 16.05.19 16:33, Anton Nefedov wrote:
> it allows to report it in the error handler
> 
> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Alberto Garcia <berto@igalia.com>
> ---
>  hw/scsi/scsi-disk.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)

(Sorry for the late reply :-/)

> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> index e7e865ab3b..b43254103c 100644
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c
> @@ -1602,8 +1602,6 @@ static void scsi_unmap_complete_noio(UnmapCBData *data, int ret)
>  {
>      SCSIDiskReq *r = data->r;
>      SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
> -    uint64_t sector_num;
> -    uint32_t nb_sectors;
>  
>      assert(r->req.aiocb == NULL);
>      if (scsi_disk_req_check_error(r, ret, false)) {
> @@ -1611,16 +1609,16 @@ static void scsi_unmap_complete_noio(UnmapCBData *data, int ret)
>      }
>  
>      if (data->count > 0) {
> -        sector_num = ldq_be_p(&data->inbuf[0]);
> -        nb_sectors = ldl_be_p(&data->inbuf[8]) & 0xffffffffULL;
> -        if (!check_lba_range(s, sector_num, nb_sectors)) {
> +        r->sector = ldq_be_p(&data->inbuf[0]);
> +        r->sector_count = ldl_be_p(&data->inbuf[8]) & 0xffffffffULL;
> +        if (!check_lba_range(s, r->sector, r->sector_count)) {
>              scsi_check_condition(r, SENSE_CODE(LBA_OUT_OF_RANGE));
>              goto done;
>          }
>  
>          r->req.aiocb = blk_aio_pdiscard(s->qdev.conf.blk,
> -                                        sector_num * s->qdev.blocksize,
> -                                        nb_sectors * s->qdev.blocksize,
> +                                        r->sector * s->qdev.blocksize,
> +                                        r->sector_count * s->qdev.blocksize,

This looks to me like these are not necessarily in terms of 512-byte
sectors.  It doesn’t seem to make anything technically wrong, because
patch 7 takes that into account.

But it’s still weird if everything else in this file treats these fields
as being in terms of 512 byte sectors (and they are actually defined
this way in SCSIDiskReq).

Max

>                                          scsi_unmap_complete, data);
>          data->count--;
>          data->inbuf += 16;
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] [PATCH v8 4/9] ide: account UNMAP (TRIM) operations
  2019-05-16 14:33 ` [Qemu-devel] [PATCH v8 4/9] ide: account UNMAP (TRIM) operations Anton Nefedov
@ 2019-08-12 18:16   ` Max Reitz
  2019-08-21 11:06     ` Anton Nefedov
  0 siblings, 1 reply; 21+ messages in thread
From: Max Reitz @ 2019-08-12 18:16 UTC (permalink / raw)
  To: Anton Nefedov, qemu-block
  Cc: kwolf, vsementsov, berto, den, qemu-devel, pbonzini, jsnow


[-- Attachment #1.1: Type: text/plain, Size: 1818 bytes --]

On 16.05.19 16:33, Anton Nefedov wrote:
> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  hw/ide/core.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 6afadf894f..3a7ac93777 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -441,6 +441,14 @@ static void ide_issue_trim_cb(void *opaque, int ret)
>      TrimAIOCB *iocb = opaque;
>      IDEState *s = iocb->s;
>  
> +    if (iocb->i >= 0) {
> +        if (ret >= 0) {
> +            block_acct_done(blk_get_stats(s->blk), &s->acct);
> +        } else {
> +            block_acct_failed(blk_get_stats(s->blk), &s->acct);

Hmm, in other places (ide_handle_rw_error() here or
scsi_handle_rw_error() in scsi-disk) only report this with
BLOCK_ERROR_ACTION_REPORT.

So I’m wondering whether the same should be done here.

Max

> +        }
> +    }
> +
>      if (ret >= 0) {
>          while (iocb->j < iocb->qiov->niov) {
>              int j = iocb->j;
> @@ -458,10 +466,14 @@ static void ide_issue_trim_cb(void *opaque, int ret)
>                  }
>  
>                  if (!ide_sect_range_ok(s, sector, count)) {
> +                    block_acct_invalid(blk_get_stats(s->blk), BLOCK_ACCT_UNMAP);
>                      iocb->ret = -EINVAL;
>                      goto done;
>                  }
>  
> +                block_acct_start(blk_get_stats(s->blk), &s->acct,
> +                                 count << BDRV_SECTOR_BITS, BLOCK_ACCT_UNMAP);
> +
>                  /* Got an entry! Submit and exit.  */
>                  iocb->aiocb = blk_aio_pdiscard(s->blk,
>                                                 sector << BDRV_SECTOR_BITS,
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] [PATCH v8 9/9] qapi: query-blockstat: add driver specific file-posix stats
  2019-05-16 14:33 ` [Qemu-devel] [PATCH v8 9/9] qapi: query-blockstat: add driver specific file-posix stats Anton Nefedov
@ 2019-08-12 19:04   ` Max Reitz
  2019-08-21 11:00     ` Anton Nefedov
  0 siblings, 1 reply; 21+ messages in thread
From: Max Reitz @ 2019-08-12 19:04 UTC (permalink / raw)
  To: Anton Nefedov, qemu-block
  Cc: kwolf, vsementsov, berto, den, qemu-devel, pbonzini, jsnow


[-- Attachment #1.1: Type: text/plain, Size: 5592 bytes --]

On 16.05.19 16:33, Anton Nefedov wrote:
> A block driver can provide a callback to report driver-specific
> statistics.
> 
> file-posix driver now reports discard statistics
> 
> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Acked-by: Markus Armbruster <armbru@redhat.com>
> ---
>  qapi/block-core.json      | 38 ++++++++++++++++++++++++++++++++++++++
>  include/block/block.h     |  1 +
>  include/block/block_int.h |  1 +
>  block.c                   |  9 +++++++++
>  block/file-posix.c        | 38 +++++++++++++++++++++++++++++++++++---
>  block/qapi.c              |  5 +++++
>  6 files changed, 89 insertions(+), 3 deletions(-)


> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 55194f84ce..368e09ae37 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -956,6 +956,41 @@
>             '*wr_latency_histogram': 'BlockLatencyHistogramInfo',
>             '*flush_latency_histogram': 'BlockLatencyHistogramInfo' } }
>  
> +##
> +# @BlockStatsSpecificFile:
> +#
> +# File driver statistics
> +#
> +# @discard-nb-ok: The number of successful discard operations performed by
> +#                 the driver.
> +#
> +# @discard-nb-failed: The number of failed discard operations performed by
> +#                     the driver.
> +#
> +# @discard-bytes-ok: The number of bytes discarded by the driver.
> +#
> +# Since: 4.1
> +##
> +{ 'struct': 'BlockStatsSpecificFile',
> +  'data': {
> +      'discard-nb-ok': 'uint64',
> +      'discard-nb-failed': 'uint64',
> +      'discard-bytes-ok': 'uint64' } }
> +
> +##
> +# @BlockStatsSpecific:
> +#
> +# Block driver specific statistics
> +#
> +# Since: 4.1
> +##
> +{ 'union': 'BlockStatsSpecific',
> +  'base': { 'driver': 'BlockdevDriver' },
> +  'discriminator': 'driver',
> +  'data': {
> +      'file': 'BlockStatsSpecificFile',
> +      'host_device': 'BlockStatsSpecificFile' } }

I would like to use these chance to complain that I find this awkward.
My problem is that I don’t know how any management application is
supposed to reasonably consume this.  It feels weird to potentially have
to recognize the result for every block driver.

I would now like to note that I’m clearly not in a position to block
this at this point, because I’ve had a year to do so, I didn’t, so it
would be unfair to do it now.

(Still, I feel like if I have a concern, I should raise it, even if it’s
too late.)

I know Markus has proposed this, but I don’t understand why.  He set
ImageInfoSpecific as a precedence, but that has a different reasoning
behind it.  The point for that is that it simply doesn’t work any other
way, because it is clearly format-specific information that cannot be
shared between drivers.  Anything that can be shared is put into
ImageInfo (like the cluster size).

We have the same constellation here, BlockStats contains common stuff,
and BlockStatsSpecific would contain driver-specific stuff.  But to me,
BlockStatsSpecificFile doesn’t look very special.  It looks like it just
duplicates fields that already exist in BlockDeviceStats.


(Furthermore, most of ImageInfoSpecific is actually not useful to
management software, but only as an information for humans (and having
such a structure for that is perfectly fine).  But these stats don’t
really look like something for immediate human consumption.)


So I wonder why you don’t just put this information into
BlockDeviceStats.  From what I can tell looking at
bdrv_query_bds_stats() and qmp_query_blockstats(), the @stats field is
currently completely 0 if @query-nodes is true.

(Furthermore, I wonder whether it would make sense to re-add
BlockAcctStats to each BDS and then let the generic block code do the
accounting on it.  I moved it to the BB in 7f0e9da6f13 because we didn’t
care about node-level information at the time, but maybe it’s time to
reconsider.)


Anyway, as I’ve said, I fully understand that complaining about a design
decision is just unfair at this point, so this is not a veto.

> +
>  ##
>  # @BlockStats:
>  #
> @@ -971,6 +1006,8 @@
>  #
>  # @stats:  A @BlockDeviceStats for the device.
>  #
> +# @driver-specific: Optional driver-specific stats. (Since 4.1)
> +#
>  # @parent: This describes the file block device if it has one.
>  #          Contains recursively the statistics of the underlying
>  #          protocol (e.g. the host file for a qcow2 image). If there is
> @@ -984,6 +1021,7 @@
>  { 'struct': 'BlockStats',
>    'data': {'*device': 'str', '*qdev': 'str', '*node-name': 'str',
>             'stats': 'BlockDeviceStats',
> +           '*driver-specific': 'BlockStatsSpecific',
>             '*parent': 'BlockStats',
>             '*backing': 'BlockStats'} }
>  

[...]

> diff --git a/block/file-posix.c b/block/file-posix.c
> index 76d54b3a85..a2f01cfe10 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -160,9 +160,9 @@ typedef struct BDRVRawState {
>      bool drop_cache;
>      bool check_cache_dropped;
>      struct {
> -        int64_t discard_nb_ok;
> -        int64_t discard_nb_failed;
> -        int64_t discard_bytes_ok;
> +        uint64_t discard_nb_ok;
> +        uint64_t discard_nb_failed;
> +        uint64_t discard_bytes_ok;

(I don’t know why you didn’t introduce these fields with these types in
the previous patch then.)

Max

>      } stats;
>  
>      PRManager *pr_mgr;


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] [PATCH v8 9/9] qapi: query-blockstat: add driver specific file-posix stats
  2019-08-12 19:04   ` Max Reitz
@ 2019-08-21 11:00     ` Anton Nefedov
  2019-08-21 11:21       ` Max Reitz
  0 siblings, 1 reply; 21+ messages in thread
From: Anton Nefedov @ 2019-08-21 11:00 UTC (permalink / raw)
  To: Max Reitz, qemu-block
  Cc: kwolf, Vladimir Sementsov-Ogievskiy, berto, Denis Lunev,
	qemu-devel, pbonzini, jsnow

On 12/8/2019 10:04 PM, Max Reitz wrote:
> On 16.05.19 16:33, Anton Nefedov wrote:
>> A block driver can provide a callback to report driver-specific
>> statistics.
>>
>> file-posix driver now reports discard statistics
>>
>> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> Acked-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>   qapi/block-core.json      | 38 ++++++++++++++++++++++++++++++++++++++
>>   include/block/block.h     |  1 +
>>   include/block/block_int.h |  1 +
>>   block.c                   |  9 +++++++++
>>   block/file-posix.c        | 38 +++++++++++++++++++++++++++++++++++---
>>   block/qapi.c              |  5 +++++
>>   6 files changed, 89 insertions(+), 3 deletions(-)
> 
> 
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index 55194f84ce..368e09ae37 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -956,6 +956,41 @@
>>              '*wr_latency_histogram': 'BlockLatencyHistogramInfo',
>>              '*flush_latency_histogram': 'BlockLatencyHistogramInfo' } }
>>   
>> +##
>> +# @BlockStatsSpecificFile:
>> +#
>> +# File driver statistics
>> +#
>> +# @discard-nb-ok: The number of successful discard operations performed by
>> +#                 the driver.
>> +#
>> +# @discard-nb-failed: The number of failed discard operations performed by
>> +#                     the driver.
>> +#
>> +# @discard-bytes-ok: The number of bytes discarded by the driver.
>> +#
>> +# Since: 4.1
>> +##
>> +{ 'struct': 'BlockStatsSpecificFile',
>> +  'data': {
>> +      'discard-nb-ok': 'uint64',
>> +      'discard-nb-failed': 'uint64',
>> +      'discard-bytes-ok': 'uint64' } }
>> +
>> +##
>> +# @BlockStatsSpecific:
>> +#
>> +# Block driver specific statistics
>> +#
>> +# Since: 4.1
>> +##
>> +{ 'union': 'BlockStatsSpecific',
>> +  'base': { 'driver': 'BlockdevDriver' },
>> +  'discriminator': 'driver',
>> +  'data': {
>> +      'file': 'BlockStatsSpecificFile',
>> +      'host_device': 'BlockStatsSpecificFile' } }
> 
> I would like to use these chance to complain that I find this awkward.
> My problem is that I don’t know how any management application is
> supposed to reasonably consume this.  It feels weird to potentially have
> to recognize the result for every block driver.
> 
> I would now like to note that I’m clearly not in a position to block
> this at this point, because I’ve had a year to do so, I didn’t, so it
> would be unfair to do it now.
> 
> (Still, I feel like if I have a concern, I should raise it, even if it’s
> too late.)
> 
> I know Markus has proposed this, but I don’t understand why.  He set
> ImageInfoSpecific as a precedence, but that has a different reasoning
> behind it.  The point for that is that it simply doesn’t work any other
> way, because it is clearly format-specific information that cannot be
> shared between drivers.  Anything that can be shared is put into
> ImageInfo (like the cluster size).
> 
> We have the same constellation here, BlockStats contains common stuff,
> and BlockStatsSpecific would contain driver-specific stuff.  But to me,
> BlockStatsSpecificFile doesn’t look very special.  It looks like it just
> duplicates fields that already exist in BlockDeviceStats.
> 
> 
> (Furthermore, most of ImageInfoSpecific is actually not useful to
> management software, but only as an information for humans (and having
> such a structure for that is perfectly fine).  But these stats don’t
> really look like something for immediate human consumption.)
> 
> 
> So I wonder why you don’t just put this information into
> BlockDeviceStats.  From what I can tell looking at
> bdrv_query_bds_stats() and qmp_query_blockstats(), the @stats field is
> currently completely 0 if @query-nodes is true.
> 
> (Furthermore, I wonder whether it would make sense to re-add
> BlockAcctStats to each BDS and then let the generic block code do the
> accounting on it.  I moved it to the BB in 7f0e9da6f13 because we didn’t
> care about node-level information at the time, but maybe it’s time to
> reconsider.)
> 
> 
> Anyway, as I’ve said, I fully understand that complaining about a design
> decision is just unfair at this point, so this is not a veto.
> 

hi!

Having both "unmap" and "discard" stats in the same list feels weird.
The intention is that unmap belongs to the device level, and discard is
from the driver level. Now we have a separate structure named "driver-
specific". Could also be called "driver-stats".

We could make this structure non-optional, present for all driver
types, as indeed there is nothing special about discard stats. But then
we need some way to distinguish
  - discard_nb_ok == 0 as no request reached the driver level
  - discard_nb_ok == 0 as the driver does not support the accounting

Yes, putting the accounting in the generic code would help, but do we
really want to burden it with accounting too? Tracking that each and
every case is covered with all those block_acct_done() invalid() and
failed() can really be a pain.

And what accounting should be there? All the operations? Measuring
discards at both device and BDS level is useful since discards are
optional. Double-measuring reads&writes is probably not so useful (RMW 
accounting? Read stats for the backing images?)


>> diff --git a/block/file-posix.c b/block/file-posix.c
>> index 76d54b3a85..a2f01cfe10 100644
>> --- a/block/file-posix.c
>> +++ b/block/file-posix.c
>> @@ -160,9 +160,9 @@ typedef struct BDRVRawState {
>>       bool drop_cache;
>>       bool check_cache_dropped;
>>       struct {
>> -        int64_t discard_nb_ok;
>> -        int64_t discard_nb_failed;
>> -        int64_t discard_bytes_ok;
>> +        uint64_t discard_nb_ok;
>> +        uint64_t discard_nb_failed;
>> +        uint64_t discard_bytes_ok;
> 
> (I don’t know why you didn’t introduce these fields with these types in
> the previous patch then.)
> 

Ouch, squashed the changes to the wrong commit, obviously

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] [PATCH v8 5/9] scsi: store unmap offset and nb_sectors in request struct
  2019-08-12 17:58   ` Max Reitz
@ 2019-08-21 11:03     ` Anton Nefedov
  0 siblings, 0 replies; 21+ messages in thread
From: Anton Nefedov @ 2019-08-21 11:03 UTC (permalink / raw)
  To: Max Reitz, qemu-block
  Cc: kwolf, Vladimir Sementsov-Ogievskiy, berto, Denis Lunev,
	qemu-devel, pbonzini, jsnow

On 12/8/2019 8:58 PM, Max Reitz wrote:
> On 16.05.19 16:33, Anton Nefedov wrote:
>> it allows to report it in the error handler
>>
>> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> Reviewed-by: Alberto Garcia <berto@igalia.com>
>> ---
>>   hw/scsi/scsi-disk.c | 12 +++++-------
>>   1 file changed, 5 insertions(+), 7 deletions(-)
> 
> (Sorry for the late reply :-/)
> 
>> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
>> index e7e865ab3b..b43254103c 100644
>> --- a/hw/scsi/scsi-disk.c
>> +++ b/hw/scsi/scsi-disk.c
>> @@ -1602,8 +1602,6 @@ static void scsi_unmap_complete_noio(UnmapCBData *data, int ret)
>>   {
>>       SCSIDiskReq *r = data->r;
>>       SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
>> -    uint64_t sector_num;
>> -    uint32_t nb_sectors;
>>   
>>       assert(r->req.aiocb == NULL);
>>       if (scsi_disk_req_check_error(r, ret, false)) {
>> @@ -1611,16 +1609,16 @@ static void scsi_unmap_complete_noio(UnmapCBData *data, int ret)
>>       }
>>   
>>       if (data->count > 0) {
>> -        sector_num = ldq_be_p(&data->inbuf[0]);
>> -        nb_sectors = ldl_be_p(&data->inbuf[8]) & 0xffffffffULL;
>> -        if (!check_lba_range(s, sector_num, nb_sectors)) {
>> +        r->sector = ldq_be_p(&data->inbuf[0]);
>> +        r->sector_count = ldl_be_p(&data->inbuf[8]) & 0xffffffffULL;
>> +        if (!check_lba_range(s, r->sector, r->sector_count)) {
>>               scsi_check_condition(r, SENSE_CODE(LBA_OUT_OF_RANGE));
>>               goto done;
>>           }
>>   
>>           r->req.aiocb = blk_aio_pdiscard(s->qdev.conf.blk,
>> -                                        sector_num * s->qdev.blocksize,
>> -                                        nb_sectors * s->qdev.blocksize,
>> +                                        r->sector * s->qdev.blocksize,
>> +                                        r->sector_count * s->qdev.blocksize,
> 
> This looks to me like these are not necessarily in terms of 512-byte
> sectors.  It doesn’t seem to make anything technically wrong, because
> patch 7 takes that into account.
> 
> But it’s still weird if everything else in this file treats these fields
> as being in terms of 512 byte sectors (and they are actually defined
> this way in SCSIDiskReq).
> 

Nice that you caught this, thanks! I guess variable names misled me

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] [PATCH v8 4/9] ide: account UNMAP (TRIM) operations
  2019-08-12 18:16   ` Max Reitz
@ 2019-08-21 11:06     ` Anton Nefedov
  0 siblings, 0 replies; 21+ messages in thread
From: Anton Nefedov @ 2019-08-21 11:06 UTC (permalink / raw)
  To: Max Reitz, qemu-block
  Cc: kwolf, Vladimir Sementsov-Ogievskiy, berto, Denis Lunev,
	qemu-devel, pbonzini, jsnow

On 12/8/2019 9:16 PM, Max Reitz wrote:
> On 16.05.19 16:33, Anton Nefedov wrote:
>> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   hw/ide/core.c | 12 ++++++++++++
>>   1 file changed, 12 insertions(+)
>>
>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>> index 6afadf894f..3a7ac93777 100644
>> --- a/hw/ide/core.c
>> +++ b/hw/ide/core.c
>> @@ -441,6 +441,14 @@ static void ide_issue_trim_cb(void *opaque, int ret)
>>       TrimAIOCB *iocb = opaque;
>>       IDEState *s = iocb->s;
>>   
>> +    if (iocb->i >= 0) {
>> +        if (ret >= 0) {
>> +            block_acct_done(blk_get_stats(s->blk), &s->acct);
>> +        } else {
>> +            block_acct_failed(blk_get_stats(s->blk), &s->acct);
> 
> Hmm, in other places (ide_handle_rw_error() here or
> scsi_handle_rw_error() in scsi-disk) only report this with
> BLOCK_ERROR_ACTION_REPORT.
> 
> So I’m wondering whether the same should be done here.
> 

Many other places do not check the action:
scsi_{dma|read|write}_complete(), hw/ide/atapi.c calls

That feels somewhat weird to me, to account the operation complete when
it's not.
(But I don't really know the use cases of BLOCK_ERROR_ACTION_IGNORE).

Can it be that the error action is only checked so that the operation is
not accounted twice (as there might be block_acct_done() later in the
common path with ACTION_IGNORE)

/Anton

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] [PATCH v8 9/9] qapi: query-blockstat: add driver specific file-posix stats
  2019-08-21 11:00     ` Anton Nefedov
@ 2019-08-21 11:21       ` Max Reitz
  2019-08-21 12:22         ` Anton Nefedov
  0 siblings, 1 reply; 21+ messages in thread
From: Max Reitz @ 2019-08-21 11:21 UTC (permalink / raw)
  To: Anton Nefedov, qemu-block
  Cc: kwolf, Vladimir Sementsov-Ogievskiy, berto, Denis Lunev,
	qemu-devel, pbonzini, jsnow


[-- Attachment #1.1: Type: text/plain, Size: 6597 bytes --]

On 21.08.19 13:00, Anton Nefedov wrote:
> On 12/8/2019 10:04 PM, Max Reitz wrote:
>> On 16.05.19 16:33, Anton Nefedov wrote:
>>> A block driver can provide a callback to report driver-specific
>>> statistics.
>>>
>>> file-posix driver now reports discard statistics
>>>
>>> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
>>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> Acked-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>>   qapi/block-core.json      | 38 ++++++++++++++++++++++++++++++++++++++
>>>   include/block/block.h     |  1 +
>>>   include/block/block_int.h |  1 +
>>>   block.c                   |  9 +++++++++
>>>   block/file-posix.c        | 38 +++++++++++++++++++++++++++++++++++---
>>>   block/qapi.c              |  5 +++++
>>>   6 files changed, 89 insertions(+), 3 deletions(-)
>>
>>
>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>> index 55194f84ce..368e09ae37 100644
>>> --- a/qapi/block-core.json
>>> +++ b/qapi/block-core.json
>>> @@ -956,6 +956,41 @@
>>>              '*wr_latency_histogram': 'BlockLatencyHistogramInfo',
>>>              '*flush_latency_histogram': 'BlockLatencyHistogramInfo' } }
>>>   
>>> +##
>>> +# @BlockStatsSpecificFile:
>>> +#
>>> +# File driver statistics
>>> +#
>>> +# @discard-nb-ok: The number of successful discard operations performed by
>>> +#                 the driver.
>>> +#
>>> +# @discard-nb-failed: The number of failed discard operations performed by
>>> +#                     the driver.
>>> +#
>>> +# @discard-bytes-ok: The number of bytes discarded by the driver.
>>> +#
>>> +# Since: 4.1
>>> +##
>>> +{ 'struct': 'BlockStatsSpecificFile',
>>> +  'data': {
>>> +      'discard-nb-ok': 'uint64',
>>> +      'discard-nb-failed': 'uint64',
>>> +      'discard-bytes-ok': 'uint64' } }
>>> +
>>> +##
>>> +# @BlockStatsSpecific:
>>> +#
>>> +# Block driver specific statistics
>>> +#
>>> +# Since: 4.1
>>> +##
>>> +{ 'union': 'BlockStatsSpecific',
>>> +  'base': { 'driver': 'BlockdevDriver' },
>>> +  'discriminator': 'driver',
>>> +  'data': {
>>> +      'file': 'BlockStatsSpecificFile',
>>> +      'host_device': 'BlockStatsSpecificFile' } }
>>
>> I would like to use these chance to complain that I find this awkward.
>> My problem is that I don’t know how any management application is
>> supposed to reasonably consume this.  It feels weird to potentially have
>> to recognize the result for every block driver.
>>
>> I would now like to note that I’m clearly not in a position to block
>> this at this point, because I’ve had a year to do so, I didn’t, so it
>> would be unfair to do it now.
>>
>> (Still, I feel like if I have a concern, I should raise it, even if it’s
>> too late.)
>>
>> I know Markus has proposed this, but I don’t understand why.  He set
>> ImageInfoSpecific as a precedence, but that has a different reasoning
>> behind it.  The point for that is that it simply doesn’t work any other
>> way, because it is clearly format-specific information that cannot be
>> shared between drivers.  Anything that can be shared is put into
>> ImageInfo (like the cluster size).
>>
>> We have the same constellation here, BlockStats contains common stuff,
>> and BlockStatsSpecific would contain driver-specific stuff.  But to me,
>> BlockStatsSpecificFile doesn’t look very special.  It looks like it just
>> duplicates fields that already exist in BlockDeviceStats.
>>
>>
>> (Furthermore, most of ImageInfoSpecific is actually not useful to
>> management software, but only as an information for humans (and having
>> such a structure for that is perfectly fine).  But these stats don’t
>> really look like something for immediate human consumption.)
>>
>>
>> So I wonder why you don’t just put this information into
>> BlockDeviceStats.  From what I can tell looking at
>> bdrv_query_bds_stats() and qmp_query_blockstats(), the @stats field is
>> currently completely 0 if @query-nodes is true.
>>
>> (Furthermore, I wonder whether it would make sense to re-add
>> BlockAcctStats to each BDS and then let the generic block code do the
>> accounting on it.  I moved it to the BB in 7f0e9da6f13 because we didn’t
>> care about node-level information at the time, but maybe it’s time to
>> reconsider.)
>>
>>
>> Anyway, as I’ve said, I fully understand that complaining about a design
>> decision is just unfair at this point, so this is not a veto.
>>
> 
> hi!
> 
> Having both "unmap" and "discard" stats in the same list feels weird.
> The intention is that unmap belongs to the device level, and discard is
> from the driver level.

Sorry, what I meant wasn’t adding a separate “discard” group, but just
filling in the existing “unmap” fields.  As far as I understand, if we
had BlockAcctStats for each BDS, the file node’s unmap stats would be
the same as its discard stats, wouldn’t it?

> Now we have a separate structure named "driver-
> specific". Could also be called "driver-stats".
> 
> We could make this structure non-optional, present for all driver
> types, as indeed there is nothing special about discard stats. But then
> we need some way to distinguish
>   - discard_nb_ok == 0 as no request reached the driver level
>   - discard_nb_ok == 0 as the driver does not support the accounting

You can simply make the fields optional.  (Then the first case is
“present, but 0”, and the second is “not present”.)

> Yes, putting the accounting in the generic code would help, but do we
> really want to burden it with accounting too? Tracking that each and
> every case is covered with all those block_acct_done() invalid() and
> failed() can really be a pain.

That’s indeed a problem, yes. :-)

> And what accounting should be there? All the operations? Measuring
> discards at both device and BDS level is useful since discards are
> optional. Double-measuring reads&writes is probably not so useful (RMW 
> accounting? Read stats for the backing images?)

Yes, if we put BlockAcctStats at the node level, we should track all
operations, I suppose.  That would require adding accounting code in
many places, so it wouldn’t be easy, correct.

I think it would be the better solution, but you’re right in that it’s
probably not worth it.

But I do think it would be good if we could get away from a
driver-specific structure (unless we really need it; and I don’t think
we do if we just make the stats fields optional).

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Qemu-devel] [PATCH v8 9/9] qapi: query-blockstat: add driver specific file-posix stats
  2019-08-21 11:21       ` Max Reitz
@ 2019-08-21 12:22         ` Anton Nefedov
  0 siblings, 0 replies; 21+ messages in thread
From: Anton Nefedov @ 2019-08-21 12:22 UTC (permalink / raw)
  To: Max Reitz, qemu-block
  Cc: kwolf, Vladimir Sementsov-Ogievskiy, berto, Denis Lunev,
	qemu-devel, pbonzini, jsnow

On 21/8/2019 2:21 PM, Max Reitz wrote:
> On 21.08.19 13:00, Anton Nefedov wrote:
>> On 12/8/2019 10:04 PM, Max Reitz wrote:
>>> On 16.05.19 16:33, Anton Nefedov wrote:
>>>> A block driver can provide a callback to report driver-specific
>>>> statistics.
>>>>
>>>> file-posix driver now reports discard statistics
>>>>
>>>> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
>>>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>> Acked-by: Markus Armbruster <armbru@redhat.com>
>>>> ---
>>>>    qapi/block-core.json      | 38 ++++++++++++++++++++++++++++++++++++++
>>>>    include/block/block.h     |  1 +
>>>>    include/block/block_int.h |  1 +
>>>>    block.c                   |  9 +++++++++
>>>>    block/file-posix.c        | 38 +++++++++++++++++++++++++++++++++++---
>>>>    block/qapi.c              |  5 +++++
>>>>    6 files changed, 89 insertions(+), 3 deletions(-)
>>>
>>>
>>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>>> index 55194f84ce..368e09ae37 100644
>>>> --- a/qapi/block-core.json
>>>> +++ b/qapi/block-core.json
>>>> @@ -956,6 +956,41 @@
>>>>               '*wr_latency_histogram': 'BlockLatencyHistogramInfo',
>>>>               '*flush_latency_histogram': 'BlockLatencyHistogramInfo' } }
>>>>    
>>>> +##
>>>> +# @BlockStatsSpecificFile:
>>>> +#
>>>> +# File driver statistics
>>>> +#
>>>> +# @discard-nb-ok: The number of successful discard operations performed by
>>>> +#                 the driver.
>>>> +#
>>>> +# @discard-nb-failed: The number of failed discard operations performed by
>>>> +#                     the driver.
>>>> +#
>>>> +# @discard-bytes-ok: The number of bytes discarded by the driver.
>>>> +#
>>>> +# Since: 4.1
>>>> +##
>>>> +{ 'struct': 'BlockStatsSpecificFile',
>>>> +  'data': {
>>>> +      'discard-nb-ok': 'uint64',
>>>> +      'discard-nb-failed': 'uint64',
>>>> +      'discard-bytes-ok': 'uint64' } }
>>>> +
>>>> +##
>>>> +# @BlockStatsSpecific:
>>>> +#
>>>> +# Block driver specific statistics
>>>> +#
>>>> +# Since: 4.1
>>>> +##
>>>> +{ 'union': 'BlockStatsSpecific',
>>>> +  'base': { 'driver': 'BlockdevDriver' },
>>>> +  'discriminator': 'driver',
>>>> +  'data': {
>>>> +      'file': 'BlockStatsSpecificFile',
>>>> +      'host_device': 'BlockStatsSpecificFile' } }
>>>
>>> I would like to use these chance to complain that I find this awkward.
>>> My problem is that I don’t know how any management application is
>>> supposed to reasonably consume this.  It feels weird to potentially have
>>> to recognize the result for every block driver.
>>>
>>> I would now like to note that I’m clearly not in a position to block
>>> this at this point, because I’ve had a year to do so, I didn’t, so it
>>> would be unfair to do it now.
>>>
>>> (Still, I feel like if I have a concern, I should raise it, even if it’s
>>> too late.)
>>>
>>> I know Markus has proposed this, but I don’t understand why.  He set
>>> ImageInfoSpecific as a precedence, but that has a different reasoning
>>> behind it.  The point for that is that it simply doesn’t work any other
>>> way, because it is clearly format-specific information that cannot be
>>> shared between drivers.  Anything that can be shared is put into
>>> ImageInfo (like the cluster size).
>>>
>>> We have the same constellation here, BlockStats contains common stuff,
>>> and BlockStatsSpecific would contain driver-specific stuff.  But to me,
>>> BlockStatsSpecificFile doesn’t look very special.  It looks like it just
>>> duplicates fields that already exist in BlockDeviceStats.
>>>
>>>
>>> (Furthermore, most of ImageInfoSpecific is actually not useful to
>>> management software, but only as an information for humans (and having
>>> such a structure for that is perfectly fine).  But these stats don’t
>>> really look like something for immediate human consumption.)
>>>
>>>
>>> So I wonder why you don’t just put this information into
>>> BlockDeviceStats.  From what I can tell looking at
>>> bdrv_query_bds_stats() and qmp_query_blockstats(), the @stats field is
>>> currently completely 0 if @query-nodes is true.
>>>
>>> (Furthermore, I wonder whether it would make sense to re-add
>>> BlockAcctStats to each BDS and then let the generic block code do the
>>> accounting on it.  I moved it to the BB in 7f0e9da6f13 because we didn’t
>>> care about node-level information at the time, but maybe it’s time to
>>> reconsider.)
>>>
>>>
>>> Anyway, as I’ve said, I fully understand that complaining about a design
>>> decision is just unfair at this point, so this is not a veto.
>>>
>>
>> hi!
>>
>> Having both "unmap" and "discard" stats in the same list feels weird.
>> The intention is that unmap belongs to the device level, and discard is
>> from the driver level.
> 
> Sorry, what I meant wasn’t adding a separate “discard” group, but just
> filling in the existing “unmap” fields.  As far as I understand, if we
> had BlockAcctStats for each BDS, the file node’s unmap stats would be
> the same as its discard stats, wouldn’t it?
> 

So, you mean count it all on BDS level _instead_ of SCSI/IDE level?

Now it's:

       "device": "drive-scsi0-0-0-0",
       "node-name": "#block151",
       "stats": {
         "unmap_operations": 0, <--- filled by SCSI
       [..]

       "parent": {
         "node-name": "#block056",
         "stats": {
           "unmap_operations": 0, <--- not filled

         "driver-stats": { <--- filled by file-posix driver
           "type": "file",
           "data": {
             "discard_bytes_ok": 0,
             "discard_nb_failed": 0,
             "discard_nb_ok": 0
           }
         }
       },


Every level may drop some requests (i.e. qcow2 won't pass requests
smaller than a cluster size) i.e.
BlockBackend stats >= BDS format driver stats >= BDS protocol driver
stats

and the difference between them (mostly between the top and the bottom 
ones) is interesting here too; good to know whether it's a guest who
doesn't send requests, or QEMU that limits them.

>> Now we have a separate structure named "driver-
>> specific". Could also be called "driver-stats".
>>
>> We could make this structure non-optional, present for all driver
>> types, as indeed there is nothing special about discard stats. But then
>> we need some way to distinguish
>>    - discard_nb_ok == 0 as no request reached the driver level
>>    - discard_nb_ok == 0 as the driver does not support the accounting
> 
> You can simply make the fields optional.  (Then the first case is
> “present, but 0”, and the second is “not present”.)
> 
>> Yes, putting the accounting in the generic code would help, but do we
>> really want to burden it with accounting too? Tracking that each and
>> every case is covered with all those block_acct_done() invalid() and
>> failed() can really be a pain.
> 
> That’s indeed a problem, yes. :-)
> 
>> And what accounting should be there? All the operations? Measuring
>> discards at both device and BDS level is useful since discards are
>> optional. Double-measuring reads&writes is probably not so useful (RMW
>> accounting? Read stats for the backing images?)
> 
> Yes, if we put BlockAcctStats at the node level, we should track all
> operations, I suppose.  That would require adding accounting code in
> many places, so it wouldn’t be easy, correct.
> 
> I think it would be the better solution, but you’re right in that it’s
> probably not worth it.
> 
> But I do think it would be good if we could get away from a
> driver-specific structure (unless we really need it; and I don’t think
> we do if we just make the stats fields optional).
> 


^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2019-08-21 12:23 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-16 14:33 [Qemu-devel] [PATCH v8 0/9] discard blockstats Anton Nefedov
2019-05-16 14:33 ` [Qemu-devel] [PATCH v8 1/9] qapi: group BlockDeviceStats fields Anton Nefedov
2019-05-16 14:33 ` [Qemu-devel] [PATCH v8 2/9] qapi: add unmap to BlockDeviceStats Anton Nefedov
2019-05-16 14:33 ` [Qemu-devel] [PATCH v8 3/9] block: add empty account cookie type Anton Nefedov
2019-05-16 15:34   ` Vladimir Sementsov-Ogievskiy
2019-05-16 16:00     ` Anton Nefedov
2019-05-16 14:33 ` [Qemu-devel] [PATCH v8 4/9] ide: account UNMAP (TRIM) operations Anton Nefedov
2019-08-12 18:16   ` Max Reitz
2019-08-21 11:06     ` Anton Nefedov
2019-05-16 14:33 ` [Qemu-devel] [PATCH v8 5/9] scsi: store unmap offset and nb_sectors in request struct Anton Nefedov
2019-08-12 17:58   ` Max Reitz
2019-08-21 11:03     ` Anton Nefedov
2019-05-16 14:33 ` [Qemu-devel] [PATCH v8 6/9] scsi: move unmap error checking to the complete callback Anton Nefedov
2019-05-16 14:33 ` [Qemu-devel] [PATCH v8 7/9] scsi: account unmap operations Anton Nefedov
2019-05-16 14:33 ` [Qemu-devel] [PATCH v8 8/9] file-posix: account discard operations Anton Nefedov
2019-05-16 15:23   ` Vladimir Sementsov-Ogievskiy
2019-05-16 14:33 ` [Qemu-devel] [PATCH v8 9/9] qapi: query-blockstat: add driver specific file-posix stats Anton Nefedov
2019-08-12 19:04   ` Max Reitz
2019-08-21 11:00     ` Anton Nefedov
2019-08-21 11:21       ` Max Reitz
2019-08-21 12:22         ` Anton Nefedov

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.