Re: [Qemu-devel] [PATCH 1/4] block: add accounting for merged requests

[Max Reitz <mreitz@redhat.com>]

On 20.10.2014 at 16:35, Peter Lieven wrote:
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>   block.c                    |    2 ++
>   block/accounting.c         |    8 +++++++-
>   block/qapi.c               |    2 ++
>   hmp.c                      |    6 +++++-
>   include/block/accounting.h |    3 +++
>   qapi/block-core.json       |    9 ++++++++-
>   6 files changed, 27 insertions(+), 3 deletions(-)
>
> diff --git a/block.c b/block.c
> index 773e87e..ba5281d 100644
> --- a/block.c
> +++ b/block.c
> @@ -4466,6 +4466,8 @@ static int multiwrite_merge(BlockDriverState *bs, BlockRequest *reqs,
>           }
>       }
>   
> +    block_merge_done(&bs->stats, BLOCK_ACCT_WRITE, num_reqs - outidx - 1);
> +
>       return outidx + 1;
>   }
>   
> diff --git a/block/accounting.c b/block/accounting.c
> index edbb1cc..8f6ee81 100644
> --- a/block/accounting.c
> +++ b/block/accounting.c
> @@ -44,7 +44,6 @@ void block_acct_done(BlockAcctStats *stats, BlockAcctCookie *cookie)
>       stats->total_time_ns[cookie->type] += get_clock() - cookie->start_time_ns;
>   }
>   
> -
>   void block_acct_highest_sector(BlockAcctStats *stats, int64_t sector_num,
>                                  unsigned int nb_sectors)
>   {

Was this hunk intended?

> @@ -52,3 +51,10 @@ void block_acct_highest_sector(BlockAcctStats *stats, int64_t sector_num,
>           stats->wr_highest_sector = sector_num + nb_sectors - 1;
>       }
>   }
> +
> +void block_merge_done(BlockAcctStats *stats, enum BlockAcctType type,
> +                      int num_requests)
> +{
> +    assert(type < BLOCK_MAX_IOTYPE);
> +    stats->merged[type] += num_requests;
> +}
> diff --git a/block/qapi.c b/block/qapi.c
> index 1301144..a0f26e9 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -338,6 +338,8 @@ static BlockStats *bdrv_query_stats(const BlockDriverState *bs)
>       s->stats->wr_bytes = bs->stats.nr_bytes[BLOCK_ACCT_WRITE];
>       s->stats->rd_operations = bs->stats.nr_ops[BLOCK_ACCT_READ];
>       s->stats->wr_operations = bs->stats.nr_ops[BLOCK_ACCT_WRITE];
> +    s->stats->rd_merged = bs->stats.merged[BLOCK_ACCT_READ];
> +    s->stats->wr_merged = bs->stats.merged[BLOCK_ACCT_WRITE];
>       s->stats->wr_highest_offset =
>           bs->stats.wr_highest_sector * BDRV_SECTOR_SIZE;
>       s->stats->flush_operations = bs->stats.nr_ops[BLOCK_ACCT_FLUSH];
> diff --git a/hmp.c b/hmp.c
> index 63d7686..baaa9e5 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -419,6 +419,8 @@ void hmp_info_blockstats(Monitor *mon, const QDict *qdict)
>                          " wr_total_time_ns=%" PRId64
>                          " rd_total_time_ns=%" PRId64
>                          " flush_total_time_ns=%" PRId64
> +                       " rd_merged=%" PRId64
> +                       " wr_merged=%" PRId64
>                          "\n",
>                          stats->value->stats->rd_bytes,
>                          stats->value->stats->wr_bytes,
> @@ -427,7 +429,9 @@ void hmp_info_blockstats(Monitor *mon, const QDict *qdict)
>                          stats->value->stats->flush_operations,
>                          stats->value->stats->wr_total_time_ns,
>                          stats->value->stats->rd_total_time_ns,
> -                       stats->value->stats->flush_total_time_ns);
> +                       stats->value->stats->flush_total_time_ns,
> +                       stats->value->stats->rd_merged,
> +                       stats->value->stats->wr_merged);
>       }
>   
>       qapi_free_BlockStatsList(stats_list);
> diff --git a/include/block/accounting.h b/include/block/accounting.h
> index 50b42b3..07394f7 100644
> --- a/include/block/accounting.h
> +++ b/include/block/accounting.h
> @@ -39,6 +39,7 @@ typedef struct BlockAcctStats {
>       uint64_t nr_bytes[BLOCK_MAX_IOTYPE];
>       uint64_t nr_ops[BLOCK_MAX_IOTYPE];
>       uint64_t total_time_ns[BLOCK_MAX_IOTYPE];
> +    uint64_t merged[BLOCK_MAX_IOTYPE];
>       uint64_t wr_highest_sector;
>   } BlockAcctStats;
>   
> @@ -53,5 +54,7 @@ void block_acct_start(BlockAcctStats *stats, BlockAcctCookie *cookie,
>   void block_acct_done(BlockAcctStats *stats, BlockAcctCookie *cookie);
>   void block_acct_highest_sector(BlockAcctStats *stats, int64_t sector_num,
>                                  unsigned int nb_sectors);
> +void block_merge_done(BlockAcctStats *stats, enum BlockAcctType type,
> +                      int num_requests);
>   
>   #endif
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 8f7089e..952d50e 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -392,13 +392,20 @@
>   #                     growable sparse files (like qcow2) that are used on top
>   #                     of a physical device.
>   #
> +# @rd_merged: Reserved (Since 2.3)
> +#
> +# @wr_merged: Number of requests that have been merged into another request
> +#             while performing a multiwrite_merge. (Since 2.3)
> +#
> +#

2.2 or 2.3? ;-)

>   # Since: 0.14.0
>   ##
>   { 'type': '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_total_time_ns': 'int', 'wr_highest_offset': 'int',
> +           'rd_merged': 'int', 'wr_merged': 'int' } }
>   
>   ##
>   # @BlockStats:

With the hunk removing the empty line removed (I know that line should 
not be there, but there's no point in removing it in this patch) and 
with either 2.2 or 2.3 in block-core.json:

Reviewed-by: Max Reitz <mreitz@redhat.com>