Re: [PATCH v7 3/4] block: add accounting for zone append operation

[Stefan Hajnoczi <stefanha@redhat.com>]

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

On Fri, Mar 10, 2023 at 06:54:30PM +0800, Sam Li wrote:
> Taking account of the new zone append write operation for zoned devices,
> BLOCK_ACCT_APPEND enum is introduced as other I/O request type (read,
> write, flush).

Can it be called BLOCK_ACCT_ZONE_APPEND so it's clear that this
operation is specific to zoned devices? I think people might not make
the connection if they just see "append" and think that regular devices
support this operation.

> 
> Signed-off-by: Sam Li <faithilikerun@gmail.com>
> ---
>  block/qapi-sysemu.c        | 11 ++++++++
>  block/qapi.c               | 15 ++++++++++
>  hw/block/virtio-blk.c      |  4 +++
>  include/block/accounting.h |  1 +
>  qapi/block-core.json       | 56 ++++++++++++++++++++++++++++++--------
>  qapi/block.json            |  4 +++
>  6 files changed, 80 insertions(+), 11 deletions(-)
> 
> diff --git a/block/qapi-sysemu.c b/block/qapi-sysemu.c
> index 7bd7554150..f7e56dfeb2 100644
> --- a/block/qapi-sysemu.c
> +++ b/block/qapi-sysemu.c
> @@ -517,6 +517,7 @@ void qmp_block_latency_histogram_set(
>      bool has_boundaries, uint64List *boundaries,
>      bool has_boundaries_read, uint64List *boundaries_read,
>      bool has_boundaries_write, uint64List *boundaries_write,
> +    bool has_boundaries_append, uint64List *boundaries_append,
>      bool has_boundaries_flush, uint64List *boundaries_flush,
>      Error **errp)
>  {
> @@ -557,6 +558,16 @@ void qmp_block_latency_histogram_set(
>          }
>      }
>  
> +    if (has_boundaries || has_boundaries_append) {
> +        ret = block_latency_histogram_set(
> +                stats, BLOCK_ACCT_APPEND,
> +                has_boundaries_append ? boundaries_append : boundaries);
> +        if (ret) {
> +            error_setg(errp, "Device '%s' set append write boundaries fail", id);
> +            return;
> +        }
> +    }
> +
>      if (has_boundaries || has_boundaries_flush) {
>          ret = block_latency_histogram_set(
>              stats, BLOCK_ACCT_FLUSH,
> diff --git a/block/qapi.c b/block/qapi.c
> index c84147849d..d4be8ad72e 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -533,27 +533,33 @@ 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->zap_bytes = stats->nr_bytes[BLOCK_ACCT_APPEND];

"zone_append_bytes" would be clearer. For a moment I thought "zap" is a
new operation. Since "zap" isn't used anywhere else, let's not introduce
a new name here.

>      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->zap_operations = stats->nr_ops[BLOCK_ACCT_APPEND];
>      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_zap_operations = stats->failed_ops[BLOCK_ACCT_APPEND];
>      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_zap_operations = stats->invalid_ops[BLOCK_ACCT_APPEND];
>      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->zap_merged = stats->merged[BLOCK_ACCT_APPEND];
>      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->zap_total_time_ns = stats->total_time_ns[BLOCK_ACCT_APPEND];
>      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];
> @@ -571,6 +577,7 @@ static void bdrv_query_blk_stats(BlockDeviceStats *ds, BlockBackend *blk)
>  
>          TimedAverage *rd = &ts->latency[BLOCK_ACCT_READ];
>          TimedAverage *wr = &ts->latency[BLOCK_ACCT_WRITE];
> +        TimedAverage *zap = &ts->latency[BLOCK_ACCT_APPEND];
>          TimedAverage *fl = &ts->latency[BLOCK_ACCT_FLUSH];
>  
>          dev_stats->interval_length = ts->interval_length;
> @@ -583,6 +590,10 @@ static void bdrv_query_blk_stats(BlockDeviceStats *ds, BlockBackend *blk)
>          dev_stats->max_wr_latency_ns = timed_average_max(wr);
>          dev_stats->avg_wr_latency_ns = timed_average_avg(wr);
>  
> +        dev_stats->min_zap_latency_ns = timed_average_min(zap);
> +        dev_stats->max_zap_latency_ns = timed_average_max(zap);
> +        dev_stats->avg_zap_latency_ns = timed_average_avg(zap);
> +
>          dev_stats->min_flush_latency_ns = timed_average_min(fl);
>          dev_stats->max_flush_latency_ns = timed_average_max(fl);
>          dev_stats->avg_flush_latency_ns = timed_average_avg(fl);
> @@ -591,6 +602,8 @@ static void bdrv_query_blk_stats(BlockDeviceStats *ds, BlockBackend *blk)
>              block_acct_queue_depth(ts, BLOCK_ACCT_READ);
>          dev_stats->avg_wr_queue_depth =
>              block_acct_queue_depth(ts, BLOCK_ACCT_WRITE);
> +        dev_stats->avg_zap_queue_depth =
> +            block_acct_queue_depth(ts, BLOCK_ACCT_APPEND);
>  
>          QAPI_LIST_PREPEND(ds->timed_stats, dev_stats);
>      }
> @@ -600,6 +613,8 @@ static void bdrv_query_blk_stats(BlockDeviceStats *ds, BlockBackend *blk)
>          = bdrv_latency_histogram_stats(&hgram[BLOCK_ACCT_READ]);
>      ds->wr_latency_histogram
>          = bdrv_latency_histogram_stats(&hgram[BLOCK_ACCT_WRITE]);
> +    ds->zap_latency_histogram
> +        = bdrv_latency_histogram_stats(&hgram[BLOCK_ACCT_APPEND]);
>      ds->flush_latency_histogram
>          = bdrv_latency_histogram_stats(&hgram[BLOCK_ACCT_FLUSH]);
>  }
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index 4ded625732..7605ca4f03 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -925,6 +925,10 @@ static int virtio_blk_handle_zone_append(VirtIOBlockReq *req,
>      data->in_num = in_num;
>      data->zone_append_data.offset = offset;
>      qemu_iovec_init_external(&req->qiov, out_iov, out_num);
> +
> +    block_acct_start(blk_get_stats(s->blk), &req->acct, len,
> +                     BLOCK_ACCT_APPEND);
> +
>      blk_aio_zone_append(s->blk, &data->zone_append_data.offset, &req->qiov, 0,
>                          virtio_blk_zone_append_complete, data);
>      return 0;
> diff --git a/include/block/accounting.h b/include/block/accounting.h
> index b9caad60d5..61cc868666 100644
> --- a/include/block/accounting.h
> +++ b/include/block/accounting.h
> @@ -37,6 +37,7 @@ enum BlockAcctType {
>      BLOCK_ACCT_READ,
>      BLOCK_ACCT_WRITE,
>      BLOCK_ACCT_FLUSH,
> +    BLOCK_ACCT_APPEND,
>      BLOCK_ACCT_UNMAP,
>      BLOCK_MAX_IOTYPE,
>  };
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index c05ad0c07e..76fe9c2fca 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -849,6 +849,9 @@
>  # @min_wr_latency_ns: Minimum latency of write operations in the
>  #                     defined interval, in nanoseconds.
>  #
> +# @min_zap_latency_ns: Minimum latency of zone append operations in the
> +#                      defined interval, in nanoseconds.
> +#
>  # @min_flush_latency_ns: Minimum latency of flush operations in the
>  #                        defined interval, in nanoseconds.
>  #
> @@ -858,6 +861,9 @@
>  # @max_wr_latency_ns: Maximum latency of write operations in the
>  #                     defined interval, in nanoseconds.
>  #
> +# @max_zap_latency_ns: Maximum latency of zone append operations in the
> +#                      defined interval, in nanoseconds.
> +#
>  # @max_flush_latency_ns: Maximum latency of flush operations in the
>  #                        defined interval, in nanoseconds.
>  #
> @@ -867,6 +873,9 @@
>  # @avg_wr_latency_ns: Average latency of write operations in the
>  #                     defined interval, in nanoseconds.
>  #
> +# @avg_zap_latency_ns: Average latency of zone append operations in the
> +#                      defined interval, in nanoseconds.
> +#
>  # @avg_flush_latency_ns: Average latency of flush operations in the
>  #                        defined interval, in nanoseconds.
>  #
> @@ -876,15 +885,20 @@
>  # @avg_wr_queue_depth: Average number of pending write operations
>  #                      in the defined interval.
>  #
> +# @avg_zap_queue_depth: Average number of pending zone append operations
> +#                       in the defined interval.
> +#
>  # Since: 2.5
>  ##
>  { 'struct': 'BlockDeviceTimedStats',
>    'data': { 'interval_length': 'int', 'min_rd_latency_ns': 'int',
>              'max_rd_latency_ns': 'int', 'avg_rd_latency_ns': 'int',
>              'min_wr_latency_ns': 'int', 'max_wr_latency_ns': 'int',
> -            'avg_wr_latency_ns': 'int', 'min_flush_latency_ns': 'int',
> -            'max_flush_latency_ns': 'int', 'avg_flush_latency_ns': 'int',
> -            'avg_rd_queue_depth': 'number', 'avg_wr_queue_depth': 'number' } }
> +            'avg_wr_latency_ns': 'int', 'min_zap_latency_ns': 'int',
> +            'max_zap_latency_ns': 'int', 'avg_zap_latency_ns': 'int',
> +            'min_flush_latency_ns': 'int', 'max_flush_latency_ns': 'int',
> +            'avg_flush_latency_ns': 'int', 'avg_rd_queue_depth': 'number',
> +            'avg_wr_queue_depth': 'number', 'avg_zap_queue_depth': 'number'  } }
>  
>  ##
>  # @BlockDeviceStats:
> @@ -895,12 +909,16 @@
>  #
>  # @wr_bytes: The number of bytes written by the device.
>  #
> +# @zap_bytes: The number of bytes appended by the zoned devices.
> +#
>  # @unmap_bytes: The number of bytes unmapped by the device (Since 4.2)
>  #
>  # @rd_operations: The number of read operations performed by the device.
>  #
>  # @wr_operations: The number of write operations performed by the device.
>  #
> +# @zap_operations: The number of zone append operations performed by the zoned devices.
> +#
>  # @flush_operations: The number of cache flush operations performed by the
>  #                    device (since 0.15)
>  #
> @@ -911,6 +929,8 @@
>  #
>  # @wr_total_time_ns: Total time spent on writes in nanoseconds (since 0.15).
>  #
> +# @zap_total_time_ns: Total time spent on zone append writes in nanoseconds.
> +#
>  # @flush_total_time_ns: Total time spent on cache flushes in nanoseconds
>  #                       (since 0.15).
>  #
> @@ -928,6 +948,9 @@
>  # @wr_merged: Number of write requests that have been merged into another
>  #             request (Since 2.3).
>  #
> +# @zap_merged: Number of zone append requests that have been merged into
> +#              another request.
> +#
>  # @unmap_merged: Number of unmap requests that have been merged into another
>  #                request (Since 4.2)
>  #
> @@ -941,6 +964,9 @@
>  # @failed_wr_operations: The number of failed write operations
>  #                        performed by the device (Since 2.5)
>  #
> +# @failed_zap_operations: The number of failed zone append write
> +#                         operations performed by the zoned devices
> +#
>  # @failed_flush_operations: The number of failed flush operations
>  #                           performed by the device (Since 2.5)
>  #
> @@ -953,6 +979,9 @@
>  # @invalid_wr_operations: The number of invalid write operations
>  #                         performed by the device (Since 2.5)
>  #
> +# @invalid_zap_operations: The number of invalid zone append operations
> +#                          performed by the zoned device
> +#
>  # @invalid_flush_operations: The number of invalid flush operations
>  #                            performed by the device (Since 2.5)
>  #
> @@ -972,27 +1001,32 @@
>  #
>  # @wr_latency_histogram: @BlockLatencyHistogramInfo. (Since 4.0)
>  #
> +# @zap_latency_histogram: @BlockLatencyHistogramInfo.
> +#
>  # @flush_latency_histogram: @BlockLatencyHistogramInfo. (Since 4.0)
>  #
>  # Since: 0.14
>  ##
>  { 'struct': 'BlockDeviceStats',
> -  'data': {'rd_bytes': 'int', 'wr_bytes': 'int', 'unmap_bytes' : 'int',
> -           'rd_operations': 'int', 'wr_operations': 'int',
> +  'data': {'rd_bytes': 'int', 'wr_bytes': 'int', 'zap_bytes': 'int',
> +           'unmap_bytes' : 'int', 'rd_operations': 'int',
> +           'wr_operations': 'int', 'zap_operations': 'int',
>             'flush_operations': 'int', 'unmap_operations': 'int',
>             'rd_total_time_ns': 'int', 'wr_total_time_ns': 'int',
> -           'flush_total_time_ns': 'int', 'unmap_total_time_ns': 'int',
> -           'wr_highest_offset': 'int',
> -           'rd_merged': 'int', 'wr_merged': 'int', 'unmap_merged': 'int',
> -           '*idle_time_ns': 'int',
> +           'zap_total_time_ns': 'int', 'flush_total_time_ns': 'int',
> +           'unmap_total_time_ns': 'int', 'wr_highest_offset': 'int',
> +           'rd_merged': 'int', 'wr_merged': 'int', 'zap_merged': 'int',
> +           'unmap_merged': 'int', '*idle_time_ns': 'int',
>             'failed_rd_operations': 'int', 'failed_wr_operations': 'int',
> -           'failed_flush_operations': 'int', 'failed_unmap_operations': 'int',
> -           'invalid_rd_operations': 'int', 'invalid_wr_operations': 'int',
> +           'failed_zap_operations': 'int', 'failed_flush_operations': 'int',
> +           'failed_unmap_operations': 'int', 'invalid_rd_operations': 'int',
> +           'invalid_wr_operations': 'int', 'invalid_zap_operations': 'int',
>             'invalid_flush_operations': 'int', 'invalid_unmap_operations': 'int',
>             'account_invalid': 'bool', 'account_failed': 'bool',
>             'timed_stats': ['BlockDeviceTimedStats'],
>             '*rd_latency_histogram': 'BlockLatencyHistogramInfo',
>             '*wr_latency_histogram': 'BlockLatencyHistogramInfo',
> +           '*zap_latency_histogram': 'BlockLatencyHistogramInfo',
>             '*flush_latency_histogram': 'BlockLatencyHistogramInfo' } }
>  
>  ##
> diff --git a/qapi/block.json b/qapi/block.json
> index 5fe068f903..5a57ef4a9f 100644
> --- a/qapi/block.json
> +++ b/qapi/block.json
> @@ -525,6 +525,9 @@
>  # @boundaries-write: list of interval boundary values for write latency
>  #                    histogram.
>  #
> +# @boundaries-zap: list of interval boundary values for zone append write
> +#                  latency histogram.
> +#
>  # @boundaries-flush: list of interval boundary values for flush latency
>  #                    histogram.
>  #
> @@ -573,5 +576,6 @@
>             '*boundaries': ['uint64'],
>             '*boundaries-read': ['uint64'],
>             '*boundaries-write': ['uint64'],
> +           '*boundaries-zap': ['uint64'],
>             '*boundaries-flush': ['uint64'] },
>    'allow-preconfig': true }
> -- 
> 2.39.2
> 

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