block.c | 50 ++++++++++++++++++++++++++++++++ hw/block/virtio-blk.c | 6 ++++ include/block/block_int-common.h | 4 +++ qapi/block-core.json | 6 +++- 4 files changed, 65 insertions(+), 1 deletion(-)
This patch allows the stats-intervals.* flag to be used with
-blockdev. Stats collection is initialized for virtio-blk devices
at their time of creation. However, it is limited to just virtio-blk
devices for now.
Signed-off-by: Chandan <csomani@redhat.com>
---
block.c | 50 ++++++++++++++++++++++++++++++++
hw/block/virtio-blk.c | 6 ++++
include/block/block_int-common.h | 4 +++
qapi/block-core.json | 6 +++-
4 files changed, 65 insertions(+), 1 deletion(-)
diff --git a/block.c b/block.c
index 8848e9a7ed..e455d04e97 100644
--- a/block.c
+++ b/block.c
@@ -38,7 +38,9 @@
#include "qapi/error.h"
#include "qobject/qdict.h"
#include "qobject/qjson.h"
+#include "qobject/qlist.h"
#include "qobject/qnull.h"
+#include "qobject/qnum.h"
#include "qobject/qstring.h"
#include "qapi/qobject-output-visitor.h"
#include "qapi/qapi-visit-block-core.h"
@@ -3956,6 +3958,35 @@ out:
return bs_snapshot;
}
+static bool bdrv_parse_stats_intervals(BlockDriverState *bs, QList *intervals,
+ Error **errp)
+{
+ unsigned i = 0;
+ const QListEntry *entry;
+ bs->num_stats_intervals = qlist_size(intervals);
+
+ if (bs->num_stats_intervals > 0) {
+ bs->stats_intervals = g_new(uint64_t, bs->num_stats_intervals);
+ }
+
+ for (entry = qlist_first(intervals); entry; entry = qlist_next(entry)) {
+ if (qobject_type(entry->value) == QTYPE_QNUM) {
+ uint64_t length = qnum_get_int(qobject_to(QNum, entry->value));
+
+ if (length > 0 && length <= UINT_MAX) {
+ bs->stats_intervals[i++] = length;
+ } else {
+ error_setg(errp, "Invalid interval length: %" PRId64, length);
+ return false;
+ }
+ } else {
+ error_setg(errp, "The specification of stats-intervals is invalid");
+ return false;
+ }
+ }
+ return true;
+}
+
/*
* Opens a disk image (raw, qcow2, vmdk, ...)
*
@@ -3987,6 +4018,8 @@ bdrv_open_inherit(const char *filename, const char *reference, QDict *options,
Error *local_err = NULL;
QDict *snapshot_options = NULL;
int snapshot_flags = 0;
+ QDict *interval_dict = NULL;
+ QList *interval_list = NULL;
assert(!child_class || !flags);
assert(!child_class == !parent);
@@ -4205,6 +4238,19 @@ bdrv_open_inherit(const char *filename, const char *reference, QDict *options,
g_free(child_key_dot);
}
+ qdict_extract_subqdict(options, &interval_dict, "stats-intervals.");
+ qdict_array_split(interval_dict, &interval_list);
+
+ if (qdict_size(interval_dict) != 0) {
+ error_setg(errp, "Invalid option stats-intervals.%s",
+ qdict_first(interval_dict)->key);
+ goto close_and_fail;
+ }
+
+ if (!bdrv_parse_stats_intervals(bs, interval_list, errp)) {
+ goto close_and_fail;
+ }
+
/* Check if any unknown options were used */
if (qdict_size(options) != 0) {
const QDictEntry *entry = qdict_first(options);
@@ -4261,6 +4307,8 @@ close_and_fail:
bdrv_unref(bs);
qobject_unref(snapshot_options);
qobject_unref(options);
+ qobject_unref(interval_dict);
+ qobject_unref(interval_list);
error_propagate(errp, local_err);
return NULL;
}
@@ -5190,6 +5238,8 @@ static void GRAPH_UNLOCKED bdrv_close(BlockDriverState *bs)
bs->full_open_options = NULL;
g_free(bs->block_status_cache);
bs->block_status_cache = NULL;
+ g_free(bs->stats_intervals);
+ bs->stats_intervals = NULL;
bdrv_release_named_dirty_bitmaps(bs);
assert(QLIST_EMPTY(&bs->dirty_bitmaps));
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 9bab2716c1..b730c67940 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -1814,6 +1814,12 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
conf->conf.lcyls,
conf->conf.lheads,
conf->conf.lsecs);
+
+ if (bs->stats_intervals) {
+ for (i = 0; i < bs->num_stats_intervals; i++) {
+ block_acct_add_interval(blk_get_stats(s->blk), bs->stats_intervals[i]);
+ }
+ }
}
static void virtio_blk_device_unrealize(DeviceState *dev)
diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
index 034c0634c8..1b4b7c636d 100644
--- a/include/block/block_int-common.h
+++ b/include/block/block_int-common.h
@@ -1277,6 +1277,10 @@ struct BlockDriverState {
/* array of write pointers' location of each zone in the zoned device. */
BlockZoneWps *wps;
+
+ /* Array of intervals for collecting IO stats */
+ uint64_t *stats_intervals;
+ unsigned int num_stats_intervals;
};
struct BlockBackendRootState {
diff --git a/qapi/block-core.json b/qapi/block-core.json
index dc6eb4ae23..dbb53296b1 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -4771,6 +4771,9 @@
# @force-share: force share all permission on added nodes. Requires
# read-only=true. (Since 2.10)
#
+# @stats-intervals: #optional list of intervals for collecting I/O
+# statistics, in seconds (default: none)
+#
# Since: 2.9
##
{ 'union': 'BlockdevOptions',
@@ -4782,7 +4785,8 @@
'*read-only': 'bool',
'*auto-read-only': 'bool',
'*force-share': 'bool',
- '*detect-zeroes': 'BlockdevDetectZeroesOptions' },
+ '*detect-zeroes': 'BlockdevDetectZeroesOptions',
+ '*stats-intervals': ['int'] },
'discriminator': 'driver',
'data': {
'blkdebug': 'BlockdevOptionsBlkdebug',
--
2.51.0Am 30.09.2025 um 01:52 hat Chandan geschrieben:
> This patch allows the stats-intervals.* flag to be used with
> -blockdev. Stats collection is initialized for virtio-blk devices
> at their time of creation. However, it is limited to just virtio-blk
> devices for now.
>
> Signed-off-by: Chandan <csomani@redhat.com>
Is it intentional that this (and the From: header) says only "Chandan"
and not "Chandan Somani"?
One fundamental question I have with this patch is if statistics should
be collected on the BlockBackend level (which is what we're doing today)
or on the node level. This patch can't seem to decide, so you configure
things on the node level, but then the actual collection happens on the
BlockBackend level and setting on the node are ignored if it isn't
directly used by a device.
If we keep things on the BlockBackend level, then intervals shouldn't be
defined with -blockdev (which configures block nodes), but with -device
(which configures the device that owns the BlockBackend). And
conversely, if we think that things should be done on the node level,
then we need to make sure that accounting actually works on the node
level and is mostly done by the functions in block/io.c instead of the
devices.
> ---
> block.c | 50 ++++++++++++++++++++++++++++++++
> hw/block/virtio-blk.c | 6 ++++
> include/block/block_int-common.h | 4 +++
> qapi/block-core.json | 6 +++-
> 4 files changed, 65 insertions(+), 1 deletion(-)
>
> diff --git a/block.c b/block.c
> index 8848e9a7ed..e455d04e97 100644
> --- a/block.c
> +++ b/block.c
> @@ -38,7 +38,9 @@
> #include "qapi/error.h"
> #include "qobject/qdict.h"
> #include "qobject/qjson.h"
> +#include "qobject/qlist.h"
> #include "qobject/qnull.h"
> +#include "qobject/qnum.h"
> #include "qobject/qstring.h"
> #include "qapi/qobject-output-visitor.h"
> #include "qapi/qapi-visit-block-core.h"
> @@ -3956,6 +3958,35 @@ out:
> return bs_snapshot;
> }
>
> +static bool bdrv_parse_stats_intervals(BlockDriverState *bs, QList *intervals,
> + Error **errp)
> +{
> + unsigned i = 0;
> + const QListEntry *entry;
> + bs->num_stats_intervals = qlist_size(intervals);
> +
> + if (bs->num_stats_intervals > 0) {
> + bs->stats_intervals = g_new(uint64_t, bs->num_stats_intervals);
> + }
> +
> + for (entry = qlist_first(intervals); entry; entry = qlist_next(entry)) {
> + if (qobject_type(entry->value) == QTYPE_QNUM) {
> + uint64_t length = qnum_get_int(qobject_to(QNum, entry->value));
> +
> + if (length > 0 && length <= UINT_MAX) {
> + bs->stats_intervals[i++] = length;
> + } else {
> + error_setg(errp, "Invalid interval length: %" PRId64, length);
> + return false;
> + }
> + } else {
> + error_setg(errp, "The specification of stats-intervals is invalid");
> + return false;
> + }
> + }
> + return true;
> +}
This is essentially a duplication of parse_stats_intervals() in
blockdev.c. Can't we just call the existing function? Or in fact move it
here and remove the existing call from blockdev_init() because -drive
should be able to just use the per-node option that you're introducing
here, too?
> /*
> * Opens a disk image (raw, qcow2, vmdk, ...)
> *
> @@ -3987,6 +4018,8 @@ bdrv_open_inherit(const char *filename, const char *reference, QDict *options,
> Error *local_err = NULL;
> QDict *snapshot_options = NULL;
> int snapshot_flags = 0;
> + QDict *interval_dict = NULL;
> + QList *interval_list = NULL;
>
> assert(!child_class || !flags);
> assert(!child_class == !parent);
> @@ -4205,6 +4238,19 @@ bdrv_open_inherit(const char *filename, const char *reference, QDict *options,
> g_free(child_key_dot);
> }
>
> + qdict_extract_subqdict(options, &interval_dict, "stats-intervals.");
> + qdict_array_split(interval_dict, &interval_list);
> +
> + if (qdict_size(interval_dict) != 0) {
> + error_setg(errp, "Invalid option stats-intervals.%s",
> + qdict_first(interval_dict)->key);
> + goto close_and_fail;
> + }
I think I would move the above lines into bdrv_parse_stats_intervals().
> + if (!bdrv_parse_stats_intervals(bs, interval_list, errp)) {
> + goto close_and_fail;
> + }
> +
> /* Check if any unknown options were used */
> if (qdict_size(options) != 0) {
> const QDictEntry *entry = qdict_first(options);
> @@ -4261,6 +4307,8 @@ close_and_fail:
> bdrv_unref(bs);
> qobject_unref(snapshot_options);
> qobject_unref(options);
> + qobject_unref(interval_dict);
> + qobject_unref(interval_list);
> error_propagate(errp, local_err);
> return NULL;
> }
> @@ -5190,6 +5238,8 @@ static void GRAPH_UNLOCKED bdrv_close(BlockDriverState *bs)
> bs->full_open_options = NULL;
> g_free(bs->block_status_cache);
> bs->block_status_cache = NULL;
> + g_free(bs->stats_intervals);
> + bs->stats_intervals = NULL;
Does this fix a preexisting memory leak for -drive?
> bdrv_release_named_dirty_bitmaps(bs);
> assert(QLIST_EMPTY(&bs->dirty_bitmaps));
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index 9bab2716c1..b730c67940 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -1814,6 +1814,12 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
> conf->conf.lcyls,
> conf->conf.lheads,
> conf->conf.lsecs);
> +
> + if (bs->stats_intervals) {
> + for (i = 0; i < bs->num_stats_intervals; i++) {
> + block_acct_add_interval(blk_get_stats(s->blk), bs->stats_intervals[i]);
> + }
> + }
> }
This could be part of block_acct_setup(), which is called by
blkconf_apply_backend_options(). Then it would be available for more
devices than just virtio-blk.
> static void virtio_blk_device_unrealize(DeviceState *dev)
> diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
> index 034c0634c8..1b4b7c636d 100644
> --- a/include/block/block_int-common.h
> +++ b/include/block/block_int-common.h
> @@ -1277,6 +1277,10 @@ struct BlockDriverState {
>
> /* array of write pointers' location of each zone in the zoned device. */
> BlockZoneWps *wps;
> +
> + /* Array of intervals for collecting IO stats */
> + uint64_t *stats_intervals;
> + unsigned int num_stats_intervals;
> };
>
> struct BlockBackendRootState {
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index dc6eb4ae23..dbb53296b1 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -4771,6 +4771,9 @@
> # @force-share: force share all permission on added nodes. Requires
> # read-only=true. (Since 2.10)
> #
> +# @stats-intervals: #optional list of intervals for collecting I/O
> +# statistics, in seconds (default: none)
#optional is not a marker that is used anywhere else.
Can we document why there are multiple intervals and what effect they
have on the statistics that you can query later?
We also need a '(Since 10.2)' note.
> +#
> # Since: 2.9
> ##
> { 'union': 'BlockdevOptions',
> @@ -4782,7 +4785,8 @@
> '*read-only': 'bool',
> '*auto-read-only': 'bool',
> '*force-share': 'bool',
> - '*detect-zeroes': 'BlockdevDetectZeroesOptions' },
> + '*detect-zeroes': 'BlockdevDetectZeroesOptions',
> + '*stats-intervals': ['int'] },
> 'discriminator': 'driver',
> 'data': {
> 'blkdebug': 'BlockdevOptionsBlkdebug',
Kevin
Kevin Wolf <kwolf@redhat.com> writes:
> Am 30.09.2025 um 01:52 hat Chandan geschrieben:
>> This patch allows the stats-intervals.* flag to be used with
>> -blockdev. Stats collection is initialized for virtio-blk devices
>> at their time of creation. However, it is limited to just virtio-blk
>> devices for now.
>>
>> Signed-off-by: Chandan <csomani@redhat.com>
[...]
>> struct BlockBackendRootState {
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index dc6eb4ae23..dbb53296b1 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -4771,6 +4771,9 @@
>> # @force-share: force share all permission on added nodes. Requires
>> # read-only=true. (Since 2.10)
>> #
>> +# @stats-intervals: #optional list of intervals for collecting I/O
>> +# statistics, in seconds (default: none)
>
> #optional is not a marker that is used anywhere else.
Because it gets rendered like this:
* stats-intervals ([int], *optional*) -- #optional list of
intervals for collecting I/O statistics, in seconds (default:
none)
Drop it, please.
[...]
On Tue, Sep 30, 2025 at 5:12 AM Kevin Wolf <kwolf@redhat.com> wrote:
> Am 30.09.2025 um 01:52 hat Chandan geschrieben:
> > This patch allows the stats-intervals.* flag to be used with
> > -blockdev. Stats collection is initialized for virtio-blk devices
> > at their time of creation. However, it is limited to just virtio-blk
> > devices for now.
> >
> > Signed-off-by: Chandan <csomani@redhat.com>
>
> Is it intentional that this (and the From: header) says only "Chandan"
> and not "Chandan Somani"?
>
> One fundamental question I have with this patch is if statistics should
> be collected on the BlockBackend level (which is what we're doing today)
> or on the node level. This patch can't seem to decide, so you configure
> things on the node level, but then the actual collection happens on the
> BlockBackend level and setting on the node are ignored if it isn't
> directly used by a device.
>
> If we keep things on the BlockBackend level, then intervals shouldn't be
> defined with -blockdev (which configures block nodes), but with -device
> (which configures the device that owns the BlockBackend). And
> conversely, if we think that things should be done on the node level,
> then we need to make sure that accounting actually works on the node
> level and is mostly done by the functions in block/io.c instead of the
> devices.
>
Problem I ran into was that the BlockBackends for the storage devices are
hidden from block.c,
which is why I had to collect the stats in virtio_blk_device_realize()
rather than bdrv_open_inherit().
I think it makes sense to keep it in the BlockBackend level because
the storage devices are what we really want to collect stats for, so in v2,
I add stats-intervals for storage devices with the -device option.
>
> > ---
> > block.c | 50 ++++++++++++++++++++++++++++++++
> > hw/block/virtio-blk.c | 6 ++++
> > include/block/block_int-common.h | 4 +++
> > qapi/block-core.json | 6 +++-
> > 4 files changed, 65 insertions(+), 1 deletion(-)
> >
> > diff --git a/block.c b/block.c
> > index 8848e9a7ed..e455d04e97 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -38,7 +38,9 @@
> > #include "qapi/error.h"
> > #include "qobject/qdict.h"
> > #include "qobject/qjson.h"
> > +#include "qobject/qlist.h"
> > #include "qobject/qnull.h"
> > +#include "qobject/qnum.h"
> > #include "qobject/qstring.h"
> > #include "qapi/qobject-output-visitor.h"
> > #include "qapi/qapi-visit-block-core.h"
> > @@ -3956,6 +3958,35 @@ out:
> > return bs_snapshot;
> > }
> >
> > +static bool bdrv_parse_stats_intervals(BlockDriverState *bs, QList
> *intervals,
> > + Error **errp)
> > +{
> > + unsigned i = 0;
> > + const QListEntry *entry;
> > + bs->num_stats_intervals = qlist_size(intervals);
> > +
> > + if (bs->num_stats_intervals > 0) {
> > + bs->stats_intervals = g_new(uint64_t, bs->num_stats_intervals);
> > + }
> > +
> > + for (entry = qlist_first(intervals); entry; entry =
> qlist_next(entry)) {
> > + if (qobject_type(entry->value) == QTYPE_QNUM) {
> > + uint64_t length = qnum_get_int(qobject_to(QNum,
> entry->value));
> > +
> > + if (length > 0 && length <= UINT_MAX) {
> > + bs->stats_intervals[i++] = length;
> > + } else {
> > + error_setg(errp, "Invalid interval length: %" PRId64,
> length);
> > + return false;
> > + }
> > + } else {
> > + error_setg(errp, "The specification of stats-intervals is
> invalid");
> > + return false;
> > + }
> > + }
> > + return true;
> > +}
>
> This is essentially a duplication of parse_stats_intervals() in
> blockdev.c. Can't we just call the existing function? Or in fact move it
> here and remove the existing call from blockdev_init() because -drive
> should be able to just use the per-node option that you're introducing
> here, too?
>
> > /*
> > * Opens a disk image (raw, qcow2, vmdk, ...)
> > *
> > @@ -3987,6 +4018,8 @@ bdrv_open_inherit(const char *filename, const char
> *reference, QDict *options,
> > Error *local_err = NULL;
> > QDict *snapshot_options = NULL;
> > int snapshot_flags = 0;
> > + QDict *interval_dict = NULL;
> > + QList *interval_list = NULL;
> >
> > assert(!child_class || !flags);
> > assert(!child_class == !parent);
> > @@ -4205,6 +4238,19 @@ bdrv_open_inherit(const char *filename, const
> char *reference, QDict *options,
> > g_free(child_key_dot);
> > }
> >
> > + qdict_extract_subqdict(options, &interval_dict, "stats-intervals.");
> > + qdict_array_split(interval_dict, &interval_list);
> > +
> > + if (qdict_size(interval_dict) != 0) {
> > + error_setg(errp, "Invalid option stats-intervals.%s",
> > + qdict_first(interval_dict)->key);
> > + goto close_and_fail;
> > + }
>
> I think I would move the above lines into bdrv_parse_stats_intervals().
>
> > + if (!bdrv_parse_stats_intervals(bs, interval_list, errp)) {
> > + goto close_and_fail;
> > + }
> > +
> > /* Check if any unknown options were used */
> > if (qdict_size(options) != 0) {
> > const QDictEntry *entry = qdict_first(options);
> > @@ -4261,6 +4307,8 @@ close_and_fail:
> > bdrv_unref(bs);
> > qobject_unref(snapshot_options);
> > qobject_unref(options);
> > + qobject_unref(interval_dict);
> > + qobject_unref(interval_list);
> > error_propagate(errp, local_err);
> > return NULL;
> > }
> > @@ -5190,6 +5238,8 @@ static void GRAPH_UNLOCKED
> bdrv_close(BlockDriverState *bs)
> > bs->full_open_options = NULL;
> > g_free(bs->block_status_cache);
> > bs->block_status_cache = NULL;
> > + g_free(bs->stats_intervals);
> > + bs->stats_intervals = NULL;
>
> Does this fix a preexisting memory leak for -drive?
>
> > bdrv_release_named_dirty_bitmaps(bs);
> > assert(QLIST_EMPTY(&bs->dirty_bitmaps));
> > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> > index 9bab2716c1..b730c67940 100644
> > --- a/hw/block/virtio-blk.c
> > +++ b/hw/block/virtio-blk.c
> > @@ -1814,6 +1814,12 @@ static void virtio_blk_device_realize(DeviceState
> *dev, Error **errp)
> > conf->conf.lcyls,
> > conf->conf.lheads,
> > conf->conf.lsecs);
> > +
> > + if (bs->stats_intervals) {
> > + for (i = 0; i < bs->num_stats_intervals; i++) {
> > + block_acct_add_interval(blk_get_stats(s->blk),
> bs->stats_intervals[i]);
> > + }
> > + }
> > }
>
> This could be part of block_acct_setup(), which is called by
> blkconf_apply_backend_options(). Then it would be available for more
> devices than just virtio-blk.
>
Updated in v2 to do this.
>
> > static void virtio_blk_device_unrealize(DeviceState *dev)
> > diff --git a/include/block/block_int-common.h
> b/include/block/block_int-common.h
> > index 034c0634c8..1b4b7c636d 100644
> > --- a/include/block/block_int-common.h
> > +++ b/include/block/block_int-common.h
> > @@ -1277,6 +1277,10 @@ struct BlockDriverState {
> >
> > /* array of write pointers' location of each zone in the zoned
> device. */
> > BlockZoneWps *wps;
> > +
> > + /* Array of intervals for collecting IO stats */
> > + uint64_t *stats_intervals;
> > + unsigned int num_stats_intervals;
> > };
> >
> > struct BlockBackendRootState {
> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > index dc6eb4ae23..dbb53296b1 100644
> > --- a/qapi/block-core.json
> > +++ b/qapi/block-core.json
> > @@ -4771,6 +4771,9 @@
> > # @force-share: force share all permission on added nodes. Requires
> > # read-only=true. (Since 2.10)
> > #
> > +# @stats-intervals: #optional list of intervals for collecting I/O
> > +# statistics, in seconds (default: none)
>
> #optional is not a marker that is used anywhere else.
>
> Can we document why there are multiple intervals and what effect they
> have on the statistics that you can query later?
>
> We also need a '(Since 10.2)' note.
>
> > +#
> > # Since: 2.9
> > ##
> > { 'union': 'BlockdevOptions',
> > @@ -4782,7 +4785,8 @@
> > '*read-only': 'bool',
> > '*auto-read-only': 'bool',
> > '*force-share': 'bool',
> > - '*detect-zeroes': 'BlockdevDetectZeroesOptions' },
> > + '*detect-zeroes': 'BlockdevDetectZeroesOptions',
> > + '*stats-intervals': ['int'] },
> > 'discriminator': 'driver',
> > 'data': {
> > 'blkdebug': 'BlockdevOptionsBlkdebug',
>
> Kevin
>
>
© 2016 - 2025 Red Hat, Inc.