[Qemu-devel] [PATCH 0/7] discard blockstats

[Qemu-devel] [PATCH 0/7] discard blockstats

[Anton Nefedov]

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

Patches 1-5 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 7).
With patch 6, 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 a few ops down to the file as the clusters are actually unallocated
on qcow2 level)

{"return": [
{"device": "drive-scsi0-0-0-0"
  "parent": {
    "stats": {
>      "unmap_operations": 0
>      "unmap_merged": 0
      "flush_total_time_ns": 0
      "wr_highest_offset": 8111718400
      [..]
      "invalid_wr_operations": 0
      "invalid_rd_operations": 0}
    "node-name": "#block047"
>    "driver_stats": {
>      "type": "file"
>      "data": {
>        "discard_bytes_ok": 1572864
>        "discard_nb_failed": 0
>        "discard_nb_ok": 5}}}
  "stats": {
>   "unmap_operations": 472
>   "unmap_merged": 0
    "flush_total_time_ns": 44530540
    "wr_highest_offset": 7106662400
    "wr_total_time_ns": 45518856
    "failed_wr_operations": 0
    "failed_rd_operations": 0
    "wr_merged": 0
    "wr_bytes": 889856
    "timed_stats": []
>    "failed_unmap_operations": 0
    "failed_flush_operations": 0
    "account_invalid": true
    "rd_total_time_ns": 3306264098
>    "invalid_unmap_operations": 0
    "flush_operations": 18
    "wr_operations": 120
>    "unmap_bytes": 12312014848
    "rd_merged": 0
    "rd_bytes": 137103360
>    "unmap_total_time_ns": 22664692
    "invalid_flush_operations": 0
    "account_failed": true
    "idle_time_ns": 437316567
    "rd_operations": 5636
    "invalid_wr_operations": 0
    "invalid_rd_operations": 0}
  "node-name": "#block128"}

  {"device": "drive-ide0-0-0"
  [..]

Anton Nefedov (7):
  qapi: add unmap to BlockDeviceStats
  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       | 57 ++++++++++++++++++++++++++++++++++++++++++++++
 include/block/accounting.h |  1 +
 include/block/block.h      |  1 +
 include/block/block_int.h  |  1 +
 block.c                    |  9 ++++++++
 block/file-posix.c         | 42 ++++++++++++++++++++++++++++++++--
 block/qapi.c               | 11 +++++++++
 hw/ide/core.c              | 12 ++++++++++
 hw/scsi/scsi-disk.c        | 29 ++++++++++++++---------
 9 files changed, 150 insertions(+), 13 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 1/7] qapi: add unmap to BlockDeviceStats

[Anton Nefedov]

Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 qapi/block-core.json       | 20 ++++++++++++++++++++
 include/block/accounting.h |  1 +
 block/qapi.c               |  6 ++++++
 3 files changed, 27 insertions(+)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 76bf50f..ba2705d 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -730,6 +730,23 @@
 # @timed_stats: Statistics specific to the set of previously defined
 #               intervals of time (Since 2.5)
 #
+# @unmap_bytes: The number of bytes unmapped by the device (Since 2.12)
+#
+# @unmap_operations: The number of unmap operations performed by the device
+#                    (Since 2.12)
+#
+# @unmap_total_time_ns: Total time spent on unmap operations in nano-seconds
+#                       (Since 2.12)
+#
+# @unmap_merged: Number of unmap requests that have been merged into another
+#                request (Since 2.12)
+#
+# @failed_unmap_operations: The number of failed unmap operations performed
+#                           by the device (Since 2.12)
+#
+# @invalid_unmap_operations: The number of invalid unmap operations performed
+#                            by the device (Since 2.12)
+#
 # Since: 0.14.0
 ##
 { 'struct': 'BlockDeviceStats',
@@ -741,6 +758,9 @@
            'failed_rd_operations': 'int', 'failed_wr_operations': 'int',
            'failed_flush_operations': 'int', 'invalid_rd_operations': 'int',
            'invalid_wr_operations': 'int', 'invalid_flush_operations': 'int',
+           'unmap_bytes' : 'int', 'unmap_operations': 'int',
+           'unmap_total_time_ns': 'int', 'unmap_merged': 'int',
+           'failed_unmap_operations': 'int', 'invalid_unmap_operations': 'int',
            'account_invalid': 'bool', 'account_failed': 'bool',
            'timed_stats': ['BlockDeviceTimedStats'] } }
 
diff --git a/include/block/accounting.h b/include/block/accounting.h
index b833d26..4e53c4a 100644
--- a/include/block/accounting.h
+++ b/include/block/accounting.h
@@ -35,6 +35,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 fc10f0a..6e110f2 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -396,24 +396,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) {
-- 
2.7.4

Re: [Qemu-devel] [Qemu-block] [PATCH 1/7] qapi: add unmap to BlockDeviceStats

[Alberto Garcia]

On Mon 20 Nov 2017 05:50:58 PM CET, Anton Nefedov wrote:
> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  qapi/block-core.json       | 20 ++++++++++++++++++++
>  include/block/accounting.h |  1 +
>  block/qapi.c               |  6 ++++++
>  3 files changed, 27 insertions(+)
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 76bf50f..ba2705d 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -730,6 +730,23 @@
>  # @timed_stats: Statistics specific to the set of previously defined
>  #               intervals of time (Since 2.5)
>  #
> +# @unmap_bytes: The number of bytes unmapped by the device (Since 2.12)
> +#
> +# @unmap_operations: The number of unmap operations performed by the device
> +#                    (Since 2.12)
> +#
> +# @unmap_total_time_ns: Total time spent on unmap operations in nano-seconds
> +#                       (Since 2.12)
> +#
> +# @unmap_merged: Number of unmap requests that have been merged into another
> +#                request (Since 2.12)
> +#
> +# @failed_unmap_operations: The number of failed unmap operations performed
> +#                           by the device (Since 2.12)
> +#
> +# @invalid_unmap_operations: The number of invalid unmap operations performed
> +#                            by the device (Since 2.12)
> +#
>  # Since: 0.14.0

Admittedtly this is just a style issue and I don't know what others
think, but wouldn't it be nicer if the documentation of new fields is
located together with the existing ones?

E.g

 @rd_bytes
 @wr_bytes
+@unmap_bytes
 @rd_operations
 @wr_operations
 @flush_operations
+@unmap_operations

[...]

 @failed_rd_operations
 @failed_wr_operations
 @failed_flush_operations
+@failed_unmap_operations

etc.

>      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];

Essentially like this :)

The patch is fine with me either way.

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

Re: [Qemu-devel] [Qemu-block] [PATCH 1/7] qapi: add unmap to BlockDeviceStats

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1323 bytes --]

On 12/05/2017 09:09 AM, Alberto Garcia wrote:
> On Mon 20 Nov 2017 05:50:58 PM CET, Anton Nefedov wrote:
>> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>  qapi/block-core.json       | 20 ++++++++++++++++++++
>>  include/block/accounting.h |  1 +
>>  block/qapi.c               |  6 ++++++
>>  3 files changed, 27 insertions(+)
>>

> 
> Admittedtly this is just a style issue and I don't know what others
> think, but wouldn't it be nicer if the documentation of new fields is
> located together with the existing ones?
> 
> E.g
> 
>  @rd_bytes
>  @wr_bytes
> +@unmap_bytes
>  @rd_operations
>  @wr_operations
>  @flush_operations
> +@unmap_operations
> 

Grouping is fine, and your suggestion makes sense to me.  One of the
nice things of QMP is that since it is name/value based, you don't have
to worry about inserting a name in the middle if that makes more sense
logically (it doesn't break backwards compatibility with old QMP
clients, the way that inserting something into the middle of a C struct
breaks all callers that were using the old struct layout).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [Qemu-block] [PATCH 1/7] qapi: add unmap to BlockDeviceStats

[Anton Nefedov]

On 5/12/2017 6:19 PM, Eric Blake wrote:
> On 12/05/2017 09:09 AM, Alberto Garcia wrote:
>> On Mon 20 Nov 2017 05:50:58 PM CET, Anton Nefedov wrote:
>>> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
>>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>   qapi/block-core.json       | 20 ++++++++++++++++++++
>>>   include/block/accounting.h |  1 +
>>>   block/qapi.c               |  6 ++++++
>>>   3 files changed, 27 insertions(+)
>>>
> 
>>
>> Admittedtly this is just a style issue and I don't know what others
>> think, but wouldn't it be nicer if the documentation of new fields is
>> located together with the existing ones?
>>
>> E.g
>>
>>   @rd_bytes
>>   @wr_bytes
>> +@unmap_bytes
>>   @rd_operations
>>   @wr_operations
>>   @flush_operations
>> +@unmap_operations
>>
> 
> Grouping is fine, and your suggestion makes sense to me.  One of the
> nice things of QMP is that since it is name/value based, you don't have
> to worry about inserting a name in the middle if that makes more sense
> logically (it doesn't break backwards compatibility with old QMP
> clients, the way that inserting something into the middle of a C struct
> breaks all callers that were using the old struct layout).
> 

Thank you, agree, will fix

/Anton

[Qemu-devel] [PATCH 2/7] ide: account UNMAP (TRIM) operations

[Anton Nefedov]

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 471d0c9..2e4dea7 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -389,6 +389,7 @@ typedef struct TrimAIOCB {
     QEMUIOVector *qiov;
     BlockAIOCB *aiocb;
     int i, j;
+    BlockAcctCookie acct;
 } TrimAIOCB;
 
 static void trim_aio_cancel(BlockAIOCB *acb)
@@ -426,6 +427,14 @@ static void ide_trim_bh_cb(void *opaque)
 static void ide_issue_trim_cb(void *opaque, int ret)
 {
     TrimAIOCB *iocb = opaque;
+    if (iocb->i >= 0) {
+        if (ret >= 0) {
+            block_acct_done(blk_get_stats(iocb->blk), &iocb->acct);
+        } else {
+            block_acct_failed(blk_get_stats(iocb->blk), &iocb->acct);
+        }
+    }
+
     if (ret >= 0) {
         while (iocb->j < iocb->qiov->niov) {
             int j = iocb->j;
@@ -442,6 +451,9 @@ static void ide_issue_trim_cb(void *opaque, int ret)
                     continue;
                 }
 
+                block_acct_start(blk_get_stats(iocb->blk), &iocb->acct,
+                                 count << BDRV_SECTOR_BITS, BLOCK_ACCT_UNMAP);
+
                 /* Got an entry! Submit and exit.  */
                 iocb->aiocb = blk_aio_pdiscard(iocb->blk,
                                                sector << BDRV_SECTOR_BITS,
-- 
2.7.4

Re: [Qemu-devel] [Qemu-block] [PATCH 2/7] ide: account UNMAP (TRIM) operations

[Alberto Garcia]

On Mon 20 Nov 2017 05:50:59 PM CET, 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 471d0c9..2e4dea7 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -389,6 +389,7 @@ typedef struct TrimAIOCB {
>      QEMUIOVector *qiov;
>      BlockAIOCB *aiocb;
>      int i, j;
> +    BlockAcctCookie acct;
>  } TrimAIOCB;
>  
>  static void trim_aio_cancel(BlockAIOCB *acb)
> @@ -426,6 +427,14 @@ static void ide_trim_bh_cb(void *opaque)
>  static void ide_issue_trim_cb(void *opaque, int ret)
>  {
>      TrimAIOCB *iocb = opaque;
> +    if (iocb->i >= 0) {
> +        if (ret >= 0) {
> +            block_acct_done(blk_get_stats(iocb->blk), &iocb->acct);
> +        } else {
> +            block_acct_failed(blk_get_stats(iocb->blk), &iocb->acct);
> +        }
> +    }

This part looks fine, but don't you also need to account for invalid
requests (in ide_dma_cb() or somewhere else) ?

Berto

Re: [Qemu-devel] [Qemu-block] [PATCH 2/7] ide: account UNMAP (TRIM) operations

[Anton Nefedov]

On 5/12/2017 6:21 PM, Alberto Garcia wrote:
> On Mon 20 Nov 2017 05:50:59 PM CET, 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 471d0c9..2e4dea7 100644
>> --- a/hw/ide/core.c
>> +++ b/hw/ide/core.c
>> @@ -389,6 +389,7 @@ typedef struct TrimAIOCB {
>>       QEMUIOVector *qiov;
>>       BlockAIOCB *aiocb;
>>       int i, j;
>> +    BlockAcctCookie acct;
>>   } TrimAIOCB;
>>   
>>   static void trim_aio_cancel(BlockAIOCB *acb)
>> @@ -426,6 +427,14 @@ static void ide_trim_bh_cb(void *opaque)
>>   static void ide_issue_trim_cb(void *opaque, int ret)
>>   {
>>       TrimAIOCB *iocb = opaque;
>> +    if (iocb->i >= 0) {
>> +        if (ret >= 0) {
>> +            block_acct_done(blk_get_stats(iocb->blk), &iocb->acct);
>> +        } else {
>> +            block_acct_failed(blk_get_stats(iocb->blk), &iocb->acct);
>> +        }
>> +    }
> 
> This part looks fine, but don't you also need to account for invalid
> requests (in ide_dma_cb() or somewhere else) ?
> 
> Berto
> 

Good point; in fact, the TRIM sector range is never checked.
(well it should be, down at the block layer, and then counted as error).

The motivation was:

     commit d66168ed687325aa6d338ce3a3cff18ce3098ed6
     Author: Michael Tokarev <mjt@tls.msk.ru>
     Date:   Wed Aug 13 11:23:31 2014 +0400

     ide: only constrain read/write requests to drive size, not other types

     Commit 58ac321135a introduced a check to ide dma processing which
     constrains all requests to drive size.  However, apparently, some
     valid requests (like TRIM) does not fit in this constraint, and
     fails in 2.1.  So check the range only for reads and writes.


It seems like the removed check was at the wrong place (trim request has
to be parsed first to get offset and nbytes).
Probably it should be put to ide_issue_trim_cb() instead.

cc John (should have done it earlier)

/Anton

Re: [Qemu-devel] [Qemu-block] [PATCH 2/7] ide: account UNMAP (TRIM) operations

[John Snow]

On 12/05/2017 12:14 PM, Anton Nefedov wrote:
> 
> 
> On 5/12/2017 6:21 PM, Alberto Garcia wrote:
>> On Mon 20 Nov 2017 05:50:59 PM CET, 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 471d0c9..2e4dea7 100644
>>> --- a/hw/ide/core.c
>>> +++ b/hw/ide/core.c
>>> @@ -389,6 +389,7 @@ typedef struct TrimAIOCB {
>>>       QEMUIOVector *qiov;
>>>       BlockAIOCB *aiocb;
>>>       int i, j;
>>> +    BlockAcctCookie acct;
>>>   } TrimAIOCB;
>>>     static void trim_aio_cancel(BlockAIOCB *acb)
>>> @@ -426,6 +427,14 @@ static void ide_trim_bh_cb(void *opaque)
>>>   static void ide_issue_trim_cb(void *opaque, int ret)
>>>   {
>>>       TrimAIOCB *iocb = opaque;
>>> +    if (iocb->i >= 0) {
>>> +        if (ret >= 0) {
>>> +            block_acct_done(blk_get_stats(iocb->blk), &iocb->acct);
>>> +        } else {
>>> +            block_acct_failed(blk_get_stats(iocb->blk), &iocb->acct);
>>> +        }
>>> +    }
>>
>> This part looks fine, but don't you also need to account for invalid
>> requests (in ide_dma_cb() or somewhere else) ?
>>

not ide_dma_cb this time, because the command does not use ATA registers
as input, see below :(

>> Berto
>>
> 
> Good point; in fact, the TRIM sector range is never checked.
> (well it should be, down at the block layer, and then counted as error).
> 
> The motivation was:
> 
>     commit d66168ed687325aa6d338ce3a3cff18ce3098ed6
>     Author: Michael Tokarev <mjt@tls.msk.ru>
>     Date:   Wed Aug 13 11:23:31 2014 +0400
> 
>     ide: only constrain read/write requests to drive size, not other types
> 
>     Commit 58ac321135a introduced a check to ide dma processing which
>     constrains all requests to drive size.  However, apparently, some
>     valid requests (like TRIM) does not fit in this constraint, and
>     fails in 2.1.  So check the range only for reads and writes.
> 

I wound up at the same commit. The problem here is that the TRIM command
does not issue contiguous LBA+count requests in the same way using the
ATA registers like DMA R/W functions do, but instead works a bit more
like DMA commands in that it transmits a list of regions separately.

Kevin pointed this out in 2014:

https://lists.gnu.org/archive/html/qemu-devel/2014-08/msg02012.html

"I can't give you a clear answer, but it all depends on the value of
sector_num. This is the contents of the LBA registers and unused for
TRIM (spec says it's reserved, so we shouldn't be looking at it)."

He's right! This means the LBA/Count registers are undefined when we're
using TRIM in ide_dma_cb.

So, you have two things you can do:

(1) Modify ide_sector_start_dma to start accounting for TRIM early, and
then continue to mark it failed/complete where you do, but you'll need
to add in a new error case in ide_issue_trim_cb to check the range and
abort the job. We do not need to preprocess the entire "list" of TRIM
regions upfront as we are within our rights to process some of them
before aborting.

(2) Continue to start accounting where you do, per-region, which keeps
trim requests "per chunk", but add the error range checking almost
immediately after, if this is useful for statistical purposes. It will
look a little funny to start accounting and then immediately and
synchronously mark it invalid, but that's probably the most accurate thing.

I'm basing this off of the ATA8-AC3 spec, which defines the command
"DATA SET MANAGEMENT - 06h, DMA":

> If the Trim bit is set to one and:
> a) the device detects an invalid LBA Range Entry; or
> b) count is greater than IDENTIFY DEVICE data word 105 (see 7.16.7.55),
> then the device shall return command aborted.
> A device may trim one or more LBA Range Entries before it returns
command aborted. See table 209.

ATA8-ACS3 section  seems pretty clear to me that we *may* abort the
command if we detect an "invalid LBA Range Entry" which is defined in
3.1.40 as:

"3.1.40 Invalid LBA Range: A range of LBAs that contains one or more
invalid LBAs."

which references 3.1.39:

3.1.39 Invalid LBA: An LBA that is greater than or equal to the largest
value reported in IDENTIFY
DEVICE data words 60..61 (see 7.16.7.22), IDENTIFY DEVICE data words
100..103
(see 7.16.7.53), or IDENTIFY DEVICE data words 230..233 (see 7.16.7.88).

Of course, we only know if a range could possibly be invalid by the time
we actually process it in ide_issue_trim_cb.

> 
> It seems like the removed check was at the wrong place (trim request has
> to be parsed first to get offset and nbytes).
> Probably it should be put to ide_issue_trim_cb() instead.
> 
> cc John (should have done it earlier)
> 

Hi!

> /Anton
> 

[Qemu-devel] [PATCH 3/7] scsi: store unmap offset and nb_sectors in request struct

[Anton Nefedov]

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>
---
 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 1243117..3882052 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -1625,8 +1625,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)) {
@@ -1634,16 +1632,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.7.4

Re: [Qemu-devel] [Qemu-block] [PATCH 3/7] scsi: store unmap offset and nb_sectors in request struct

[Alberto Garcia]

On Mon 20 Nov 2017 05:51:00 PM CET, 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>

Berto

[Qemu-devel] [PATCH 4/7] scsi: move unmap error checking to the complete callback

[Anton Nefedov]

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>
---
 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 3882052..eca6a15 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -1627,9 +1627,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]);
@@ -1665,7 +1662,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.7.4

[Qemu-devel] [PATCH 5/7] scsi: account unmap operations

[Anton Nefedov]

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

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index eca6a15..6c33418 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -1636,6 +1636,10 @@ static void scsi_unmap_complete_noio(UnmapCBData *data, int ret)
             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,
@@ -1662,10 +1666,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));
@@ -1712,10 +1717,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.7.4

[Qemu-devel] [PATCH 6/7] file-posix: account discard operations

[Anton Nefedov]

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)).

Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/file-posix.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 36ee89e..544ae58 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -158,6 +158,11 @@ typedef struct BDRVRawState {
     bool page_cache_inconsistent:1;
     bool has_fallocate;
     bool needs_alignment;
+    struct {
+        int64_t discard_nb_ok;
+        int64_t discard_nb_failed;
+        int64_t discard_bytes_ok;
+    } stats;
 
     PRManager *pr_mgr;
 } BDRVRawState;
@@ -1458,6 +1463,16 @@ static ssize_t handle_aiocb_discard(RawPosixAIOData *aiocb)
     return ret;
 }
 
+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 int aio_worker(void *arg)
 {
     RawPosixAIOData *aiocb = arg;
@@ -1494,6 +1509,7 @@ static int aio_worker(void *arg)
         break;
     case QEMU_AIO_DISCARD:
         ret = handle_aiocb_discard(aiocb);
+        raw_account_discard(aiocb->bs->opaque, aiocb->aio_nbytes, ret);
         break;
     case QEMU_AIO_WRITE_ZEROES:
         ret = handle_aiocb_write_zeroes(aiocb);
@@ -2654,8 +2670,9 @@ static coroutine_fn BlockAIOCB *hdev_aio_pdiscard(BlockDriverState *bs,
     BlockCompletionFunc *cb, void *opaque)
 {
     BDRVRawState *s = bs->opaque;
-
-    if (fd_open(bs) < 0) {
+    int ret = fd_open(bs);
+    if (ret < 0) {
+        raw_account_discard(s, bytes, ret);
         return NULL;
     }
     return paio_submit(bs, s->fd, offset, NULL, bytes,
-- 
2.7.4

[Qemu-devel] [PATCH 7/7] qapi: query-blockstat: add driver specific file-posix stats

[Anton Nefedov]

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>
---
 qapi/block-core.json      | 37 +++++++++++++++++++++++++++++++++++++
 include/block/block.h     |  1 +
 include/block/block_int.h |  1 +
 block.c                   |  9 +++++++++
 block/file-posix.c        | 21 +++++++++++++++++++++
 block/qapi.c              |  5 +++++
 6 files changed, 74 insertions(+)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index ba2705d..aefccf6 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -765,6 +765,40 @@
            'timed_stats': ['BlockDeviceTimedStats'] } }
 
 ##
+# @BlockDriverStatsFile:
+#
+# File driver statistics
+#
+# @discard_nb_ok: The number of succeeded 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 2.12
+##
+{ 'struct': 'BlockDriverStatsFile',
+  'data': {
+      'discard_nb_ok': 'int',
+      'discard_nb_failed': 'int',
+      'discard_bytes_ok': 'int'
+  } }
+
+##
+# @BlockDriverStats:
+#
+# Statistics of a block driver (driver-specific)
+#
+# Since: 2.12
+##
+{ 'union': 'BlockDriverStats',
+  'data': {
+      'file': 'BlockDriverStatsFile'
+  } }
+
+##
 # @BlockStats:
 #
 # Statistics of a virtual block device or a block backing device.
@@ -776,6 +810,8 @@
 #
 # @stats:  A @BlockDeviceStats for the device.
 #
+# @driver-stats: Optional driver-specific statistics. (Since 2.12)
+#
 # @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
@@ -789,6 +825,7 @@
 { 'struct': 'BlockStats',
   'data': {'*device': 'str', '*node-name': 'str',
            'stats': 'BlockDeviceStats',
+           '*driver-stats': 'BlockDriverStats',
            '*parent': 'BlockStats',
            '*backing': 'BlockStats'} }
 
diff --git a/include/block/block.h b/include/block/block.h
index c05cac5..e6baead 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -473,6 +473,7 @@ const char *bdrv_get_device_or_node_name(const BlockDriverState *bs);
 int bdrv_get_flags(BlockDriverState *bs);
 int bdrv_get_info(BlockDriverState *bs, BlockDriverInfo *bdi);
 ImageInfoSpecific *bdrv_get_specific_info(BlockDriverState *bs);
+BlockDriverStats *bdrv_get_driver_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 a548277..a127861 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -269,6 +269,7 @@ struct BlockDriver {
                                   Error **errp);
     int (*bdrv_get_info)(BlockDriverState *bs, BlockDriverInfo *bdi);
     ImageInfoSpecific *(*bdrv_get_specific_info)(BlockDriverState *bs);
+    BlockDriverStats *(*bdrv_get_stats)(BlockDriverState *bs);
 
     int coroutine_fn (*bdrv_save_vmstate)(BlockDriverState *bs,
                                           QEMUIOVector *qiov,
diff --git a/block.c b/block.c
index 6c8ef98..7e5822f 100644
--- a/block.c
+++ b/block.c
@@ -4016,6 +4016,15 @@ ImageInfoSpecific *bdrv_get_specific_info(BlockDriverState *bs)
     return NULL;
 }
 
+BlockDriverStats *bdrv_get_driver_stats(BlockDriverState *bs)
+{
+    BlockDriver *drv = bs->drv;
+    if (!drv || !drv->bdrv_get_stats) {
+        return NULL;
+    }
+    return drv->bdrv_get_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 544ae58..3ab92e6 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2240,6 +2240,25 @@ static int raw_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
     return 0;
 }
 
+static BlockDriverStats *raw_get_stats(BlockDriverState *bs)
+{
+    BDRVRawState *s = bs->opaque;
+    BlockDriverStats *stats = g_new(BlockDriverStats, 1);
+
+    *stats = (BlockDriverStats){
+        .type  = BLOCK_DRIVER_STATS_KIND_FILE,
+        .u.file.data = g_new(BlockDriverStatsFile, 1),
+    };
+
+    *stats->u.file.data = (BlockDriverStatsFile){
+        .discard_nb_ok = s->stats.discard_nb_ok,
+        .discard_nb_failed = s->stats.discard_nb_failed,
+        .discard_bytes_ok = s->stats.discard_bytes_ok,
+    };
+
+    return stats;
+}
+
 static QemuOptsList raw_create_opts = {
     .name = "raw-create-opts",
     .head = QTAILQ_HEAD_INITIALIZER(raw_create_opts.head),
@@ -2312,6 +2331,7 @@ BlockDriver bdrv_file = {
     .bdrv_get_info = raw_get_info,
     .bdrv_get_allocated_file_size
                         = raw_get_allocated_file_size,
+    .bdrv_get_stats  = raw_get_stats,
     .bdrv_check_perm = raw_check_perm,
     .bdrv_set_perm   = raw_set_perm,
     .bdrv_abort_perm_update = raw_abort_perm_update,
@@ -2790,6 +2810,7 @@ static BlockDriver bdrv_host_device = {
     .bdrv_get_info = raw_get_info,
     .bdrv_get_allocated_file_size
                         = raw_get_allocated_file_size,
+    .bdrv_get_stats  = raw_get_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 6e110f2..4191e6c 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -489,6 +489,11 @@ static BlockStats *bdrv_query_bds_stats(BlockDriverState *bs,
 
     s->stats->wr_highest_offset = stat64_get(&bs->wr_highest_offset);
 
+    s->driver_stats = bdrv_get_driver_stats(bs);
+    if (s->driver_stats) {
+        s->has_driver_stats = true;
+    }
+
     if (bs->file) {
         s->has_parent = true;
         s->parent = bdrv_query_bds_stats(bs->file->bs, blk_level);
-- 
2.7.4

Re: [Qemu-devel] [PATCH 0/7] discard blockstats

[Paolo Bonzini]

On 20/11/2017 17:50, Anton Nefedov wrote:
> qmp query-blockstats provides stats info for write/read/flush ops.
> 
> Patches 1-5 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 7).
> With patch 6, 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 a few ops down to the file as the clusters are actually unallocated
> on qcow2 level)
> 
> {"return": [
> {"device": "drive-scsi0-0-0-0"
>   "parent": {
>     "stats": {
>>      "unmap_operations": 0
>>      "unmap_merged": 0
>       "flush_total_time_ns": 0
>       "wr_highest_offset": 8111718400
>       [..]
>       "invalid_wr_operations": 0
>       "invalid_rd_operations": 0}
>     "node-name": "#block047"
>>    "driver_stats": {
>>      "type": "file"
>>      "data": {
>>        "discard_bytes_ok": 1572864
>>        "discard_nb_failed": 0
>>        "discard_nb_ok": 5}}}
>   "stats": {
>>   "unmap_operations": 472
>>   "unmap_merged": 0
>     "flush_total_time_ns": 44530540
>     "wr_highest_offset": 7106662400
>     "wr_total_time_ns": 45518856
>     "failed_wr_operations": 0
>     "failed_rd_operations": 0
>     "wr_merged": 0
>     "wr_bytes": 889856
>     "timed_stats": []
>>    "failed_unmap_operations": 0
>     "failed_flush_operations": 0
>     "account_invalid": true
>     "rd_total_time_ns": 3306264098
>>    "invalid_unmap_operations": 0
>     "flush_operations": 18
>     "wr_operations": 120
>>    "unmap_bytes": 12312014848
>     "rd_merged": 0
>     "rd_bytes": 137103360
>>    "unmap_total_time_ns": 22664692
>     "invalid_flush_operations": 0
>     "account_failed": true
>     "idle_time_ns": 437316567
>     "rd_operations": 5636
>     "invalid_wr_operations": 0
>     "invalid_rd_operations": 0}
>   "node-name": "#block128"}
> 
>   {"device": "drive-ide0-0-0"
>   [..]
> 
> Anton Nefedov (7):
>   qapi: add unmap to BlockDeviceStats
>   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       | 57 ++++++++++++++++++++++++++++++++++++++++++++++
>  include/block/accounting.h |  1 +
>  include/block/block.h      |  1 +
>  include/block/block_int.h  |  1 +
>  block.c                    |  9 ++++++++
>  block/file-posix.c         | 42 ++++++++++++++++++++++++++++++++--
>  block/qapi.c               | 11 +++++++++
>  hw/ide/core.c              | 12 ++++++++++
>  hw/scsi/scsi-disk.c        | 29 ++++++++++++++---------
>  9 files changed, 150 insertions(+), 13 deletions(-)
> 

Hey, looks like this series has fallen through the cracks.  Anton, are
you going to send an updated version?

Thanks,

Paolo

Re: [Qemu-devel] [PATCH 0/7] discard blockstats

[Anton Nefedov]

On 17/8/2018 8:27 PM, Paolo Bonzini wrote:
> On 20/11/2017 17:50, Anton Nefedov wrote:
>> qmp query-blockstats provides stats info for write/read/flush ops.
>>
>> Patches 1-5 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 7).
>> With patch 6, 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 a few ops down to the file as the clusters are actually unallocated
>> on qcow2 level)
>>
>> {"return": [
>> {"device": "drive-scsi0-0-0-0"
>>    "parent": {
>>      "stats": {
>>>       "unmap_operations": 0
>>>       "unmap_merged": 0
>>        "flush_total_time_ns": 0
>>        "wr_highest_offset": 8111718400
>>        [..]
>>        "invalid_wr_operations": 0
>>        "invalid_rd_operations": 0}
>>      "node-name": "#block047"
>>>     "driver_stats": {
>>>       "type": "file"
>>>       "data": {
>>>         "discard_bytes_ok": 1572864
>>>         "discard_nb_failed": 0
>>>         "discard_nb_ok": 5}}}
>>    "stats": {
>>>    "unmap_operations": 472
>>>    "unmap_merged": 0
>>      "flush_total_time_ns": 44530540
>>      "wr_highest_offset": 7106662400
>>      "wr_total_time_ns": 45518856
>>      "failed_wr_operations": 0
>>      "failed_rd_operations": 0
>>      "wr_merged": 0
>>      "wr_bytes": 889856
>>      "timed_stats": []
>>>     "failed_unmap_operations": 0
>>      "failed_flush_operations": 0
>>      "account_invalid": true
>>      "rd_total_time_ns": 3306264098
>>>     "invalid_unmap_operations": 0
>>      "flush_operations": 18
>>      "wr_operations": 120
>>>     "unmap_bytes": 12312014848
>>      "rd_merged": 0
>>      "rd_bytes": 137103360
>>>     "unmap_total_time_ns": 22664692
>>      "invalid_flush_operations": 0
>>      "account_failed": true
>>      "idle_time_ns": 437316567
>>      "rd_operations": 5636
>>      "invalid_wr_operations": 0
>>      "invalid_rd_operations": 0}
>>    "node-name": "#block128"}
>>
>>    {"device": "drive-ide0-0-0"
>>    [..]
>>
>> Anton Nefedov (7):
>>    qapi: add unmap to BlockDeviceStats
>>    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       | 57 ++++++++++++++++++++++++++++++++++++++++++++++
>>   include/block/accounting.h |  1 +
>>   include/block/block.h      |  1 +
>>   include/block/block_int.h  |  1 +
>>   block.c                    |  9 ++++++++
>>   block/file-posix.c         | 42 ++++++++++++++++++++++++++++++++--
>>   block/qapi.c               | 11 +++++++++
>>   hw/ide/core.c              | 12 ++++++++++
>>   hw/scsi/scsi-disk.c        | 29 ++++++++++++++---------
>>   9 files changed, 150 insertions(+), 13 deletions(-)
>>
> 
> Hey, looks like this series has fallen through the cracks.  Anton, are
> you going to send an updated version?
> 
> Thanks,
> 
> Paolo
> 

hej,

yes, and v3 was the latest
(http://lists.nongnu.org/archive/html/qemu-devel/2018-06/msg03688.html)

I will make a few rebase fixups and resend it soon.

/Anton