[PATCH v2] qga: add guest-get-diskstats command for Linux guests

luzhipeng posted 1 patch 1 week ago
qga/commands-posix.c | 97 ++++++++++++++++++++++++++++++++++++++++++++
qga/commands-win32.c |  6 +++
qga/qapi-schema.json | 86 +++++++++++++++++++++++++++++++++++++++
3 files changed, 189 insertions(+)
[PATCH v2] qga: add guest-get-diskstats command for Linux guests
Posted by luzhipeng 1 week ago
Add a new 'guest-get-diskstats' command for report disk io statistics
for Linux guests. This can be usefull for getting io flow or handling
IO fault, no need to enter guests.

Signed-off-by: luzhipeng <luzhipeng@cestc.cn>

---
Changes v1->v2:
 v1:https://patchew.org/QEMU/20220512011930.214-1-luzhipeng@cestc.cn/

 qga/commands-posix.c | 97 ++++++++++++++++++++++++++++++++++++++++++++
 qga/commands-win32.c |  6 +++
 qga/qapi-schema.json | 86 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 189 insertions(+)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 69f209af87..7a16d84e3a 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -2783,6 +2783,96 @@ GuestMemoryBlockInfo *qmp_guest_get_memory_block_info(Error **errp)
     return info;
 }
 
+#define MAX_NAME_LEN 128
+static GuestDiskStatsInfoList *guest_get_diskstats(Error **errp)
+{
+#ifdef CONFIG_LINUX
+    GuestDiskStatsInfoList *head = NULL, **tail = &head;
+    const char *diskstats = "/proc/diskstats";
+    FILE *fp;
+    size_t n;
+    char *line = NULL;
+    char dev_name[MAX_NAME_LEN];
+    int i;
+    unsigned int ios_pgr, tot_ticks, rq_ticks, wr_ticks, dc_ticks, fl_ticks;
+    unsigned long rd_ios, rd_merges_or_rd_sec, rd_ticks_or_wr_sec, wr_ios;
+    unsigned long wr_merges, rd_sec_or_wr_ios, wr_sec;
+    unsigned long dc_ios, dc_merges, dc_sec, fl_ios;
+    unsigned int major, minor;
+
+    fp = fopen(diskstats, "r");
+    if (fp  == NULL) {
+        error_setg_errno(errp, errno, "open(\"%s\")", diskstats);
+        return NULL;
+    }
+    while (getline(&line, &n, fp) != -1) {
+        GuestDiskStatsInfo *diskstatinfo;
+        GuestDiskStats *diskstat;
+        i = sscanf(line, "%u %u %s %lu %lu %lu"
+                   "%lu %lu %lu %lu %u %u %u %u"
+                   "%lu %lu %lu %u %lu %u",
+                  &major, &minor, dev_name,
+                  &rd_ios, &rd_merges_or_rd_sec, &rd_sec_or_wr_ios,
+                  &rd_ticks_or_wr_sec, &wr_ios, &wr_merges, &wr_sec,
+                  &wr_ticks, &ios_pgr, &tot_ticks, &rq_ticks,
+                  &dc_ios, &dc_merges, &dc_sec, &dc_ticks,
+                  &fl_ios, &fl_ticks);
+
+        diskstatinfo = g_new0(GuestDiskStatsInfo, 1);
+        diskstat = g_new0(GuestDiskStats, 1);
+        if (i < 7) {
+            continue;
+        }
+        diskstatinfo->name = g_strdup(dev_name);
+        diskstatinfo->major = major;
+        diskstatinfo->minor = minor;
+        if (i == 7) {
+            diskstat->read_ios = rd_ios;
+            diskstat->read_sectors = rd_merges_or_rd_sec;
+            diskstat->write_ios = rd_sec_or_wr_ios;
+            diskstat->write_sectors = rd_ticks_or_wr_sec;
+        }
+        if (i >= 14) {
+            diskstat->read_ios = rd_ios;
+            diskstat->read_sectors = rd_sec_or_wr_ios;
+            diskstat->read_merges = rd_merges_or_rd_sec;
+            diskstat->read_ticks = rd_ticks_or_wr_sec;
+            diskstat->write_ios = wr_ios;
+            diskstat->write_sectors = wr_sec;
+            diskstat->write_merges = wr_merges;
+            diskstat->write_ticks = wr_ticks;
+            diskstat->ios_pgr = ios_pgr;
+            diskstat->total_ticks = tot_ticks;
+            diskstat->weight_ticks = rq_ticks;
+        }
+        if (i >= 18) {
+            diskstat->discard_ios = dc_ios;
+            diskstat->discard_merges = dc_merges;
+            diskstat->discard_sectors = dc_sec;
+            diskstat->discard_ticks = dc_ticks;
+        }
+        if (i >= 20) {
+            diskstat->flush_ios = fl_ios;
+            diskstat->flush_ticks = fl_ticks;
+        }
+
+        diskstatinfo->stats = diskstat;
+        QAPI_LIST_APPEND(tail, diskstatinfo);
+    }
+    g_free(line);
+    fclose(fp);
+    return head;
+#else
+    g_debug("disk stats reporting available only for Linux");
+    return NULL;
+#endif
+}
+
+GuestDiskStatsInfoList *qmp_guest_get_diskstats(Error **errp)
+{
+    return guest_get_diskstats(errp);
+}
+
 #else /* defined(__linux__) */
 
 void qmp_guest_suspend_disk(Error **errp)
@@ -3131,6 +3221,13 @@ GuestDiskInfoList *qmp_guest_get_disks(Error **errp)
     return NULL;
 }
 
+GuestDiskStatsInfoList *qmp_guest_get_diskstats(Error **errp)
+{
+    error_setg(errp, QERR_UNSUPPORTED);
+    return NULL;
+}
+
+
 #endif /* CONFIG_FSFREEZE */
 
 #if !defined(CONFIG_FSTRIM)
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index d56b5fd2a7..dcdeb76a68 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -2532,3 +2532,9 @@ char *qga_get_host_name(Error **errp)
 
     return g_utf16_to_utf8(tmp, size, NULL, NULL, NULL);
 }
+
+GuestDiskStatsInfoList *qmp_guest_get_diskstats(Error **errp)
+{
+    error_setg(errp, QERR_UNSUPPORTED);
+    return NULL;
+}
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index 4d8e506c9e..94aad4f2ae 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -1490,3 +1490,89 @@
 { 'command': 'guest-ssh-remove-authorized-keys',
   'data': { 'username': 'str', 'keys': ['str'] },
   'if': 'CONFIG_POSIX' }
+
+##
+# @GuestDiskStats:
+#
+# @read-sectors: sectors read
+#
+# @write-sectors: sectors written
+#
+# @discard-sectors: sectors discarded
+#
+# @read-ios: reads completed successfully
+#
+# @read-merges: Number of read requests merged
+#
+# @write-ios: writes completed
+#
+# @write-merges: Number of write requests merged
+#
+# @discard-ios: Number of discards completed successfully
+#
+# @discard-merges: NUmber of discard requests merged
+#
+# @flush-ios: Number of flush requests completed successfully
+#
+# @read-ticks: time spent reading(ms)
+#
+# @write-ticks: time spent writing(ms)
+#
+# @discard-ticks: time spent discarding(ms)
+#
+# @flush-ticks: time spent flushing(ms)
+#
+# @ios-pgr: Number of I/Os currently in flight
+#
+# @total-ticks: time spent doing I/Os (ms)
+#
+# @weight-ticks: weighted time spent doing I/Os since the last update of this field(ms)
+#
+# Since: 7.1
+##
+{ 'struct': 'GuestDiskStats',
+  'data': {'read-sectors': 'uint64',
+            'write-sectors': 'uint64',
+            'discard-sectors': 'uint64',
+            'read-ios': 'uint64',
+            'read-merges': 'uint64',
+            'write-ios': 'uint64',
+            'write-merges': 'uint64',
+            'discard-ios': 'uint64',
+            'discard-merges': 'uint64',
+            'flush-ios': 'uint64',
+            'read-ticks': 'uint64',
+            'write-ticks': 'uint64',
+            'discard-ticks': 'uint64',
+            'flush-ticks': 'uint64',
+            'ios-pgr': 'uint64',
+            'total-ticks': 'uint64',
+            'weight-ticks': 'uint64'
+           } }
+
+##
+# @GuestDiskStatsInfo:
+#
+# @name disk name
+#
+# @major major of disk
+#
+# @minor minor of disk
+##
+{ 'struct': 'GuestDiskStatsInfo',
+  'data': {'name': 'str',
+           'major': 'uint64',
+           'minor': 'uint64',
+           'stats': 'GuestDiskStats' } }
+
+##
+# @guest-get-diskstats:
+#
+# Retrieve information about disk stats.
+# Returns: List of disk stats of guest.
+#
+# Since: 7.1
+##
+{ 'command': 'guest-get-diskstats',
+  'returns': ['GuestDiskStatsInfo']
+}
-- 
2.31.1
Re: [PATCH v2] qga: add guest-get-diskstats command for Linux guests
Posted by Konstantin Kostiuk 1 week ago
On Fri, May 13, 2022 at 1:40 PM luzhipeng <luzhipeng@cestc.cn> wrote:

> Add a new 'guest-get-diskstats' command for report disk io statistics
> for Linux guests. This can be usefull for getting io flow or handling
> IO fault, no need to enter guests.
>
> Signed-off-by: luzhipeng <luzhipeng@cestc.cn>
>
> ---
> Changes v1->v2:
>  v1:https://patchew.org/QEMU/20220512011930.214-1-luzhipeng@cestc.cn/
>
>  qga/commands-posix.c | 97 ++++++++++++++++++++++++++++++++++++++++++++
>  qga/commands-win32.c |  6 +++
>  qga/qapi-schema.json | 86 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 189 insertions(+)
>
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 69f209af87..7a16d84e3a 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -2783,6 +2783,96 @@ GuestMemoryBlockInfo
> *qmp_guest_get_memory_block_info(Error **errp)
>      return info;
>  }
>
> +#define MAX_NAME_LEN 128
> +static GuestDiskStatsInfoList *guest_get_diskstats(Error **errp)
> +{
> +#ifdef CONFIG_LINUX
> +    GuestDiskStatsInfoList *head = NULL, **tail = &head;
> +    const char *diskstats = "/proc/diskstats";
> +    FILE *fp;
> +    size_t n;
> +    char *line = NULL;
> +    char dev_name[MAX_NAME_LEN];
> +    int i;
> +    unsigned int ios_pgr, tot_ticks, rq_ticks, wr_ticks, dc_ticks,
> fl_ticks;
> +    unsigned long rd_ios, rd_merges_or_rd_sec, rd_ticks_or_wr_sec, wr_ios;
> +    unsigned long wr_merges, rd_sec_or_wr_ios, wr_sec;
> +    unsigned long dc_ios, dc_merges, dc_sec, fl_ios;
> +    unsigned int major, minor;
> +
> +    fp = fopen(diskstats, "r");
> +    if (fp  == NULL) {
> +        error_setg_errno(errp, errno, "open(\"%s\")", diskstats);
> +        return NULL;
> +    }
> +    while (getline(&line, &n, fp) != -1) {
> +        GuestDiskStatsInfo *diskstatinfo;
> +        GuestDiskStats *diskstat;
> +        i = sscanf(line, "%u %u %s %lu %lu %lu"
> +                   "%lu %lu %lu %lu %u %u %u %u"
> +                   "%lu %lu %lu %u %lu %u",
> +                  &major, &minor, dev_name,
> +                  &rd_ios, &rd_merges_or_rd_sec, &rd_sec_or_wr_ios,
> +                  &rd_ticks_or_wr_sec, &wr_ios, &wr_merges, &wr_sec,
> +                  &wr_ticks, &ios_pgr, &tot_ticks, &rq_ticks,
> +                  &dc_ios, &dc_merges, &dc_sec, &dc_ticks,
> +                  &fl_ios, &fl_ticks);
> +
> +        diskstatinfo = g_new0(GuestDiskStatsInfo, 1);
> +        diskstat = g_new0(GuestDiskStats, 1);
>
+        if (i < 7) {
>

diskstatinfo and diskstat pointers do not have g_autoptr attribute and
will be not added to the results list.
So, looks like we have a memory leak in this case.


> +            continue;
> +        }
> +        diskstatinfo->name = g_strdup(dev_name);
> +        diskstatinfo->major = major;
> +        diskstatinfo->minor = minor;
> +        if (i == 7) {
> +            diskstat->read_ios = rd_ios;
> +            diskstat->read_sectors = rd_merges_or_rd_sec;
> +            diskstat->write_ios = rd_sec_or_wr_ios;
> +            diskstat->write_sectors = rd_ticks_or_wr_sec;
> +        }
> +        if (i >= 14) {
> +            diskstat->read_ios = rd_ios;
> +            diskstat->read_sectors = rd_sec_or_wr_ios;
> +            diskstat->read_merges = rd_merges_or_rd_sec;
> +            diskstat->read_ticks = rd_ticks_or_wr_sec;
> +            diskstat->write_ios = wr_ios;
> +            diskstat->write_sectors = wr_sec;
> +            diskstat->write_merges = wr_merges;
> +            diskstat->write_ticks = wr_ticks;
> +            diskstat->ios_pgr = ios_pgr;
> +            diskstat->total_ticks = tot_ticks;
> +            diskstat->weight_ticks = rq_ticks;
> +        }
> +        if (i >= 18) {
> +            diskstat->discard_ios = dc_ios;
> +            diskstat->discard_merges = dc_merges;
> +            diskstat->discard_sectors = dc_sec;
> +            diskstat->discard_ticks = dc_ticks;
> +        }
> +        if (i >= 20) {
> +            diskstat->flush_ios = fl_ios;
> +            diskstat->flush_ticks = fl_ticks;
> +        }
> +
> +        diskstatinfo->stats = diskstat;
> +        QAPI_LIST_APPEND(tail, diskstatinfo);
> +    }
> +    g_free(line);
> +    fclose(fp);
> +    return head;
> +#else
> +    g_debug("disk stats reporting available only for Linux");
> +    return NULL;
> +#endif
> +}
> +
> +GuestDiskStatsInfoList *qmp_guest_get_diskstats(Error **errp)
> +{
> +    return guest_get_diskstats(errp);
> +}
> +
>  #else /* defined(__linux__) */
>
>  void qmp_guest_suspend_disk(Error **errp)
> @@ -3131,6 +3221,13 @@ GuestDiskInfoList *qmp_guest_get_disks(Error **errp)
>      return NULL;
>  }
>
> +GuestDiskStatsInfoList *qmp_guest_get_diskstats(Error **errp)
> +{
> +    error_setg(errp, QERR_UNSUPPORTED);
> +    return NULL;
> +}
> +
> +
>  #endif /* CONFIG_FSFREEZE */
>
>  #if !defined(CONFIG_FSTRIM)
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index d56b5fd2a7..dcdeb76a68 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -2532,3 +2532,9 @@ char *qga_get_host_name(Error **errp)
>
>      return g_utf16_to_utf8(tmp, size, NULL, NULL, NULL);
>  }
> +
> +GuestDiskStatsInfoList *qmp_guest_get_diskstats(Error **errp)
> +{
> +    error_setg(errp, QERR_UNSUPPORTED);
> +    return NULL;
> +}
> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> index 4d8e506c9e..94aad4f2ae 100644
> --- a/qga/qapi-schema.json
> +++ b/qga/qapi-schema.json
> @@ -1490,3 +1490,89 @@
>  { 'command': 'guest-ssh-remove-authorized-keys',
>    'data': { 'username': 'str', 'keys': ['str'] },
>    'if': 'CONFIG_POSIX' }
> +
> +##
> +# @GuestDiskStats:
> +#
> +# @read-sectors: sectors read
> +#
> +# @write-sectors: sectors written
> +#
> +# @discard-sectors: sectors discarded
> +#
> +# @read-ios: reads completed successfully
> +#
> +# @read-merges: Number of read requests merged
> +#
> +# @write-ios: writes completed
> +#
> +# @write-merges: Number of write requests merged
> +#
> +# @discard-ios: Number of discards completed successfully
> +#
> +# @discard-merges: NUmber of discard requests merged
> +#
> +# @flush-ios: Number of flush requests completed successfully
> +#
> +# @read-ticks: time spent reading(ms)
> +#
> +# @write-ticks: time spent writing(ms)
> +#
> +# @discard-ticks: time spent discarding(ms)
> +#
> +# @flush-ticks: time spent flushing(ms)
> +#
> +# @ios-pgr: Number of I/Os currently in flight
> +#
> +# @total-ticks: time spent doing I/Os (ms)
> +#
> +# @weight-ticks: weighted time spent doing I/Os since the last update of
> this field(ms)
> +#
> +# Since: 7.1
> +##
> +{ 'struct': 'GuestDiskStats',
> +  'data': {'read-sectors': 'uint64',
> +            'write-sectors': 'uint64',
> +            'discard-sectors': 'uint64',
> +            'read-ios': 'uint64',
> +            'read-merges': 'uint64',
> +            'write-ios': 'uint64',
> +            'write-merges': 'uint64',
> +            'discard-ios': 'uint64',
> +            'discard-merges': 'uint64',
> +            'flush-ios': 'uint64',
> +            'read-ticks': 'uint64',
> +            'write-ticks': 'uint64',
> +            'discard-ticks': 'uint64',
> +            'flush-ticks': 'uint64',
> +            'ios-pgr': 'uint64',
> +            'total-ticks': 'uint64',
> +            'weight-ticks': 'uint64'
> +           } }
> +
> +##
> +# @GuestDiskStatsInfo:
> +#
> +# @name disk name
> +#
> +# @major major of disk
> +#
> +# @minor minor of disk
> +##
> +{ 'struct': 'GuestDiskStatsInfo',
> +  'data': {'name': 'str',
> +           'major': 'uint64',
> +           'minor': 'uint64',
> +           'stats': 'GuestDiskStats' } }
> +
> +##
> +# @guest-get-diskstats:
> +#
> +# Retrieve information about disk stats.
> +# Returns: List of disk stats of guest.
> +#
> +# Since: 7.1
> +##
> +{ 'command': 'guest-get-diskstats',
> +  'returns': ['GuestDiskStatsInfo']
> +}
> --
> 2.31.1
>
>
>
>
Re: [PATCH v2] qga: add guest-get-diskstats command for Linux guests
Posted by Konstantin Kostiuk 1 week ago
On Fri, May 13, 2022 at 1:50 PM Konstantin Kostiuk <kkostiuk@redhat.com>
wrote:

>
>
> On Fri, May 13, 2022 at 1:40 PM luzhipeng <luzhipeng@cestc.cn> wrote:
>
>> Add a new 'guest-get-diskstats' command for report disk io statistics
>> for Linux guests. This can be usefull for getting io flow or handling
>> IO fault, no need to enter guests.
>>
>> Signed-off-by: luzhipeng <luzhipeng@cestc.cn>
>>
>> ---
>> Changes v1->v2:
>>  v1:https://patchew.org/QEMU/20220512011930.214-1-luzhipeng@cestc.cn/
>>
>>  qga/commands-posix.c | 97 ++++++++++++++++++++++++++++++++++++++++++++
>>  qga/commands-win32.c |  6 +++
>>  qga/qapi-schema.json | 86 +++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 189 insertions(+)
>>
>> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
>> index 69f209af87..7a16d84e3a 100644
>> --- a/qga/commands-posix.c
>> +++ b/qga/commands-posix.c
>> @@ -2783,6 +2783,96 @@ GuestMemoryBlockInfo
>> *qmp_guest_get_memory_block_info(Error **errp)
>>      return info;
>>  }
>>
>> +#define MAX_NAME_LEN 128
>> +static GuestDiskStatsInfoList *guest_get_diskstats(Error **errp)
>> +{
>> +#ifdef CONFIG_LINUX
>> +    GuestDiskStatsInfoList *head = NULL, **tail = &head;
>> +    const char *diskstats = "/proc/diskstats";
>> +    FILE *fp;
>> +    size_t n;
>> +    char *line = NULL;
>> +    char dev_name[MAX_NAME_LEN];
>> +    int i;
>> +    unsigned int ios_pgr, tot_ticks, rq_ticks, wr_ticks, dc_ticks,
>> fl_ticks;
>> +    unsigned long rd_ios, rd_merges_or_rd_sec, rd_ticks_or_wr_sec,
>> wr_ios;
>> +    unsigned long wr_merges, rd_sec_or_wr_ios, wr_sec;
>> +    unsigned long dc_ios, dc_merges, dc_sec, fl_ios;
>> +    unsigned int major, minor;
>> +
>> +    fp = fopen(diskstats, "r");
>> +    if (fp  == NULL) {
>> +        error_setg_errno(errp, errno, "open(\"%s\")", diskstats);
>> +        return NULL;
>> +    }
>> +    while (getline(&line, &n, fp) != -1) {
>> +        GuestDiskStatsInfo *diskstatinfo;
>> +        GuestDiskStats *diskstat;
>> +        i = sscanf(line, "%u %u %s %lu %lu %lu"
>> +                   "%lu %lu %lu %lu %u %u %u %u"
>> +                   "%lu %lu %lu %u %lu %u",
>> +                  &major, &minor, dev_name,
>> +                  &rd_ios, &rd_merges_or_rd_sec, &rd_sec_or_wr_ios,
>> +                  &rd_ticks_or_wr_sec, &wr_ios, &wr_merges, &wr_sec,
>> +                  &wr_ticks, &ios_pgr, &tot_ticks, &rq_ticks,
>> +                  &dc_ios, &dc_merges, &dc_sec, &dc_ticks,
>> +                  &fl_ios, &fl_ticks);
>> +
>> +        diskstatinfo = g_new0(GuestDiskStatsInfo, 1);
>> +        diskstat = g_new0(GuestDiskStats, 1);
>>
> +        if (i < 7) {
>>
>
> diskstatinfo and diskstat pointers do not have g_autoptr attribute and
> will be not added to the results list.
> So, looks like we have a memory leak in this case.
>

checked with valgrind.
we have a memory leak of diskstatinfo and diskstat when i < 7

I suggest allocating these variables after this condition.


>
>
>> +            continue;
>> +        }
>> +        diskstatinfo->name = g_strdup(dev_name);
>> +        diskstatinfo->major = major;
>> +        diskstatinfo->minor = minor;
>> +        if (i == 7) {
>> +            diskstat->read_ios = rd_ios;
>> +            diskstat->read_sectors = rd_merges_or_rd_sec;
>> +            diskstat->write_ios = rd_sec_or_wr_ios;
>> +            diskstat->write_sectors = rd_ticks_or_wr_sec;
>> +        }
>> +        if (i >= 14) {
>> +            diskstat->read_ios = rd_ios;
>> +            diskstat->read_sectors = rd_sec_or_wr_ios;
>> +            diskstat->read_merges = rd_merges_or_rd_sec;
>> +            diskstat->read_ticks = rd_ticks_or_wr_sec;
>> +            diskstat->write_ios = wr_ios;
>> +            diskstat->write_sectors = wr_sec;
>> +            diskstat->write_merges = wr_merges;
>> +            diskstat->write_ticks = wr_ticks;
>> +            diskstat->ios_pgr = ios_pgr;
>> +            diskstat->total_ticks = tot_ticks;
>> +            diskstat->weight_ticks = rq_ticks;
>> +        }
>> +        if (i >= 18) {
>> +            diskstat->discard_ios = dc_ios;
>> +            diskstat->discard_merges = dc_merges;
>> +            diskstat->discard_sectors = dc_sec;
>> +            diskstat->discard_ticks = dc_ticks;
>> +        }
>> +        if (i >= 20) {
>> +            diskstat->flush_ios = fl_ios;
>> +            diskstat->flush_ticks = fl_ticks;
>> +        }
>> +
>> +        diskstatinfo->stats = diskstat;
>> +        QAPI_LIST_APPEND(tail, diskstatinfo);
>> +    }
>> +    g_free(line);
>> +    fclose(fp);
>> +    return head;
>> +#else
>> +    g_debug("disk stats reporting available only for Linux");
>> +    return NULL;
>> +#endif
>> +}
>> +
>> +GuestDiskStatsInfoList *qmp_guest_get_diskstats(Error **errp)
>> +{
>> +    return guest_get_diskstats(errp);
>> +}
>> +
>>  #else /* defined(__linux__) */
>>
>>  void qmp_guest_suspend_disk(Error **errp)
>> @@ -3131,6 +3221,13 @@ GuestDiskInfoList *qmp_guest_get_disks(Error
>> **errp)
>>      return NULL;
>>  }
>>
>> +GuestDiskStatsInfoList *qmp_guest_get_diskstats(Error **errp)
>> +{
>> +    error_setg(errp, QERR_UNSUPPORTED);
>> +    return NULL;
>> +}
>> +
>> +
>>  #endif /* CONFIG_FSFREEZE */
>>
>>  #if !defined(CONFIG_FSTRIM)
>> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
>> index d56b5fd2a7..dcdeb76a68 100644
>> --- a/qga/commands-win32.c
>> +++ b/qga/commands-win32.c
>> @@ -2532,3 +2532,9 @@ char *qga_get_host_name(Error **errp)
>>
>>      return g_utf16_to_utf8(tmp, size, NULL, NULL, NULL);
>>  }
>> +
>> +GuestDiskStatsInfoList *qmp_guest_get_diskstats(Error **errp)
>> +{
>> +    error_setg(errp, QERR_UNSUPPORTED);
>> +    return NULL;
>> +}
>> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
>> index 4d8e506c9e..94aad4f2ae 100644
>> --- a/qga/qapi-schema.json
>> +++ b/qga/qapi-schema.json
>> @@ -1490,3 +1490,89 @@
>>  { 'command': 'guest-ssh-remove-authorized-keys',
>>    'data': { 'username': 'str', 'keys': ['str'] },
>>    'if': 'CONFIG_POSIX' }
>> +
>> +##
>> +# @GuestDiskStats:
>> +#
>> +# @read-sectors: sectors read
>> +#
>> +# @write-sectors: sectors written
>> +#
>> +# @discard-sectors: sectors discarded
>> +#
>> +# @read-ios: reads completed successfully
>> +#
>> +# @read-merges: Number of read requests merged
>> +#
>> +# @write-ios: writes completed
>> +#
>> +# @write-merges: Number of write requests merged
>> +#
>> +# @discard-ios: Number of discards completed successfully
>> +#
>> +# @discard-merges: NUmber of discard requests merged
>> +#
>> +# @flush-ios: Number of flush requests completed successfully
>> +#
>> +# @read-ticks: time spent reading(ms)
>> +#
>> +# @write-ticks: time spent writing(ms)
>> +#
>> +# @discard-ticks: time spent discarding(ms)
>> +#
>> +# @flush-ticks: time spent flushing(ms)
>> +#
>> +# @ios-pgr: Number of I/Os currently in flight
>> +#
>> +# @total-ticks: time spent doing I/Os (ms)
>> +#
>> +# @weight-ticks: weighted time spent doing I/Os since the last update of
>> this field(ms)
>> +#
>> +# Since: 7.1
>> +##
>> +{ 'struct': 'GuestDiskStats',
>> +  'data': {'read-sectors': 'uint64',
>> +            'write-sectors': 'uint64',
>> +            'discard-sectors': 'uint64',
>> +            'read-ios': 'uint64',
>> +            'read-merges': 'uint64',
>> +            'write-ios': 'uint64',
>> +            'write-merges': 'uint64',
>> +            'discard-ios': 'uint64',
>> +            'discard-merges': 'uint64',
>> +            'flush-ios': 'uint64',
>> +            'read-ticks': 'uint64',
>> +            'write-ticks': 'uint64',
>> +            'discard-ticks': 'uint64',
>> +            'flush-ticks': 'uint64',
>> +            'ios-pgr': 'uint64',
>> +            'total-ticks': 'uint64',
>> +            'weight-ticks': 'uint64'
>> +           } }
>> +
>> +##
>> +# @GuestDiskStatsInfo:
>> +#
>> +# @name disk name
>> +#
>> +# @major major of disk
>> +#
>> +# @minor minor of disk
>> +##
>> +{ 'struct': 'GuestDiskStatsInfo',
>> +  'data': {'name': 'str',
>> +           'major': 'uint64',
>> +           'minor': 'uint64',
>> +           'stats': 'GuestDiskStats' } }
>> +
>> +##
>> +# @guest-get-diskstats:
>> +#
>> +# Retrieve information about disk stats.
>> +# Returns: List of disk stats of guest.
>> +#
>> +# Since: 7.1
>> +##
>> +{ 'command': 'guest-get-diskstats',
>> +  'returns': ['GuestDiskStatsInfo']
>> +}
>> --
>> 2.31.1
>>
>>
>>
>>
Re:Re: [PATCH v2] qga: add guest-get-diskstats command for Linux guests
Posted by 芦志朋 6 days, 17 hours ago

                
            
Re: Re: [PATCH v2] qga: add guest-get-diskstats command for Linux guests
Posted by luzhipeng@cestc.cn 1 week ago
OK,  indeed leak memory
thanks, 



luzhipeng@cestc.cn
 
From: Konstantin Kostiuk
Date: 2022-05-13 22:33
To: luzhipeng
CC: qemu-devel; Michael Roth; Marc-André Lureau; Daniel P . Berrangé; Michal Privoznik
Subject: Re: [PATCH v2] qga: add guest-get-diskstats command for Linux guests

On Fri, May 13, 2022 at 1:50 PM Konstantin Kostiuk <kkostiuk@redhat.com> wrote:


On Fri, May 13, 2022 at 1:40 PM luzhipeng <luzhipeng@cestc.cn> wrote:
Add a new 'guest-get-diskstats' command for report disk io statistics
for Linux guests. This can be usefull for getting io flow or handling
IO fault, no need to enter guests.

Signed-off-by: luzhipeng <luzhipeng@cestc.cn>

---
Changes v1->v2:
 v1:https://patchew.org/QEMU/20220512011930.214-1-luzhipeng@cestc.cn/

 qga/commands-posix.c | 97 ++++++++++++++++++++++++++++++++++++++++++++
 qga/commands-win32.c |  6 +++
 qga/qapi-schema.json | 86 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 189 insertions(+)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 69f209af87..7a16d84e3a 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -2783,6 +2783,96 @@ GuestMemoryBlockInfo *qmp_guest_get_memory_block_info(Error **errp)
     return info;
 }

+#define MAX_NAME_LEN 128
+static GuestDiskStatsInfoList *guest_get_diskstats(Error **errp)
+{
+#ifdef CONFIG_LINUX
+    GuestDiskStatsInfoList *head = NULL, **tail = &head;
+    const char *diskstats = "/proc/diskstats";
+    FILE *fp;
+    size_t n;
+    char *line = NULL;
+    char dev_name[MAX_NAME_LEN];
+    int i;
+    unsigned int ios_pgr, tot_ticks, rq_ticks, wr_ticks, dc_ticks, fl_ticks;
+    unsigned long rd_ios, rd_merges_or_rd_sec, rd_ticks_or_wr_sec, wr_ios;
+    unsigned long wr_merges, rd_sec_or_wr_ios, wr_sec;
+    unsigned long dc_ios, dc_merges, dc_sec, fl_ios;
+    unsigned int major, minor;
+
+    fp = fopen(diskstats, "r");
+    if (fp  == NULL) {
+        error_setg_errno(errp, errno, "open(\"%s\")", diskstats);
+        return NULL;
+    }
+    while (getline(&line, &n, fp) != -1) {
+        GuestDiskStatsInfo *diskstatinfo;
+        GuestDiskStats *diskstat;
+        i = sscanf(line, "%u %u %s %lu %lu %lu"
+                   "%lu %lu %lu %lu %u %u %u %u"
+                   "%lu %lu %lu %u %lu %u",
+                  &major, &minor, dev_name,
+                  &rd_ios, &rd_merges_or_rd_sec, &rd_sec_or_wr_ios,
+                  &rd_ticks_or_wr_sec, &wr_ios, &wr_merges, &wr_sec,
+                  &wr_ticks, &ios_pgr, &tot_ticks, &rq_ticks,
+                  &dc_ios, &dc_merges, &dc_sec, &dc_ticks,
+                  &fl_ios, &fl_ticks);
+
+        diskstatinfo = g_new0(GuestDiskStatsInfo, 1);
+        diskstat = g_new0(GuestDiskStats, 1);
+        if (i < 7) {

diskstatinfo and diskstat pointers do not have g_autoptr attribute and
will be not added to the results list.
So, looks like we have a memory leak in this case.

checked with valgrind. 
we have a memory leak of diskstatinfo and diskstat when i < 7

I suggest allocating these variables after this condition.
 
 
+            continue;
+        }
+        diskstatinfo->name = g_strdup(dev_name);
+        diskstatinfo->major = major;
+        diskstatinfo->minor = minor;
+        if (i == 7) {
+            diskstat->read_ios = rd_ios;
+            diskstat->read_sectors = rd_merges_or_rd_sec;
+            diskstat->write_ios = rd_sec_or_wr_ios;
+            diskstat->write_sectors = rd_ticks_or_wr_sec;
+        }
+        if (i >= 14) {
+            diskstat->read_ios = rd_ios;
+            diskstat->read_sectors = rd_sec_or_wr_ios;
+            diskstat->read_merges = rd_merges_or_rd_sec;
+            diskstat->read_ticks = rd_ticks_or_wr_sec;
+            diskstat->write_ios = wr_ios;
+            diskstat->write_sectors = wr_sec;
+            diskstat->write_merges = wr_merges;
+            diskstat->write_ticks = wr_ticks;
+            diskstat->ios_pgr = ios_pgr;
+            diskstat->total_ticks = tot_ticks;
+            diskstat->weight_ticks = rq_ticks;
+        }
+        if (i >= 18) {
+            diskstat->discard_ios = dc_ios;
+            diskstat->discard_merges = dc_merges;
+            diskstat->discard_sectors = dc_sec;
+            diskstat->discard_ticks = dc_ticks;
+        }
+        if (i >= 20) {
+            diskstat->flush_ios = fl_ios;
+            diskstat->flush_ticks = fl_ticks;
+        }
+
+        diskstatinfo->stats = diskstat;
+        QAPI_LIST_APPEND(tail, diskstatinfo);
+    }
+    g_free(line);
+    fclose(fp);
+    return head;
+#else
+    g_debug("disk stats reporting available only for Linux");
+    return NULL;
+#endif
+}
+
+GuestDiskStatsInfoList *qmp_guest_get_diskstats(Error **errp)
+{
+    return guest_get_diskstats(errp);
+}
+
 #else /* defined(__linux__) */

 void qmp_guest_suspend_disk(Error **errp)
@@ -3131,6 +3221,13 @@ GuestDiskInfoList *qmp_guest_get_disks(Error **errp)
     return NULL;
 }

+GuestDiskStatsInfoList *qmp_guest_get_diskstats(Error **errp)
+{
+    error_setg(errp, QERR_UNSUPPORTED);
+    return NULL;
+}
+
+
 #endif /* CONFIG_FSFREEZE */

 #if !defined(CONFIG_FSTRIM)
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index d56b5fd2a7..dcdeb76a68 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -2532,3 +2532,9 @@ char *qga_get_host_name(Error **errp)

     return g_utf16_to_utf8(tmp, size, NULL, NULL, NULL);
 }
+
+GuestDiskStatsInfoList *qmp_guest_get_diskstats(Error **errp)
+{
+    error_setg(errp, QERR_UNSUPPORTED);
+    return NULL;
+}
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index 4d8e506c9e..94aad4f2ae 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -1490,3 +1490,89 @@
 { 'command': 'guest-ssh-remove-authorized-keys',
   'data': { 'username': 'str', 'keys': ['str'] },
   'if': 'CONFIG_POSIX' }
+
+##
+# @GuestDiskStats:
+#
+# @read-sectors: sectors read
+#
+# @write-sectors: sectors written
+#
+# @discard-sectors: sectors discarded
+#
+# @read-ios: reads completed successfully
+#
+# @read-merges: Number of read requests merged
+#
+# @write-ios: writes completed
+#
+# @write-merges: Number of write requests merged
+#
+# @discard-ios: Number of discards completed successfully
+#
+# @discard-merges: NUmber of discard requests merged
+#
+# @flush-ios: Number of flush requests completed successfully
+#
+# @read-ticks: time spent reading(ms)
+#
+# @write-ticks: time spent writing(ms)
+#
+# @discard-ticks: time spent discarding(ms)
+#
+# @flush-ticks: time spent flushing(ms)
+#
+# @ios-pgr: Number of I/Os currently in flight
+#
+# @total-ticks: time spent doing I/Os (ms)
+#
+# @weight-ticks: weighted time spent doing I/Os since the last update of this field(ms)
+#
+# Since: 7.1
+##
+{ 'struct': 'GuestDiskStats',
+  'data': {'read-sectors': 'uint64',
+            'write-sectors': 'uint64',
+            'discard-sectors': 'uint64',
+            'read-ios': 'uint64',
+            'read-merges': 'uint64',
+            'write-ios': 'uint64',
+            'write-merges': 'uint64',
+            'discard-ios': 'uint64',
+            'discard-merges': 'uint64',
+            'flush-ios': 'uint64',
+            'read-ticks': 'uint64',
+            'write-ticks': 'uint64',
+            'discard-ticks': 'uint64',
+            'flush-ticks': 'uint64',
+            'ios-pgr': 'uint64',
+            'total-ticks': 'uint64',
+            'weight-ticks': 'uint64'
+           } }
+
+##
+# @GuestDiskStatsInfo:
+#
+# @name disk name
+#
+# @major major of disk
+#
+# @minor minor of disk
+##
+{ 'struct': 'GuestDiskStatsInfo',
+  'data': {'name': 'str',
+           'major': 'uint64',
+           'minor': 'uint64',
+           'stats': 'GuestDiskStats' } }
+
+##
+# @guest-get-diskstats:
+#
+# Retrieve information about disk stats.
+# Returns: List of disk stats of guest.
+#
+# Since: 7.1
+##
+{ 'command': 'guest-get-diskstats',
+  'returns': ['GuestDiskStatsInfo']
+}
-- 
2.31.1