[Qemu-devel] [PATCH 0/2] improve block-latency-histogram-set

[Qemu-devel] [PATCH 0/2] improve block-latency-histogram-set

[Vladimir Sementsov-Ogievskiy]

Support QOM path for block-latency-histogram-set and drop x- prefix.

Libvirt discussion:
https://www.redhat.com/archives/libvir-list/2018-September/msg00011.html

Vladimir Sementsov-Ogievskiy (2):
  qapi: support device id for x-block-latency-histogram-set
  qapi: drop x- from x-block-latency-histogram-set

 qapi/block-core.json | 10 ++++++----
 blockdev.c           | 10 ++++++----
 2 files changed, 12 insertions(+), 8 deletions(-)

-- 
2.18.0

[Qemu-devel] [PATCH 1/2] qapi: support device id for x-block-latency-histogram-set

[Vladimir Sementsov-Ogievskiy]

Support modern way of device selecting.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 qapi/block-core.json | 6 ++++--
 blockdev.c           | 8 +++++---
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index ac3b48ee54..4efd60d8ab 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -489,7 +489,9 @@
 # If only @device parameter is specified, remove all present latency histograms
 # for the device. Otherwise, add/reset some of (or all) latency histograms.
 #
-# @device: device name to set latency histogram for.
+# @device: device name to set latency histogram for (better use @id).
+#
+# @id: The name or QOM path of the guest device.
 #
 # @boundaries: list of interval boundary values (see description in
 #              BlockLatencyHistogramInfo definition). If specified, all
@@ -547,7 +549,7 @@
 # <- { "return": {} }
 ##
 { 'command': 'x-block-latency-histogram-set',
-  'data': {'device': 'str',
+  'data': {'*device': 'str', '*id': 'str',
            '*boundaries': ['uint64'],
            '*boundaries-read': ['uint64'],
            '*boundaries-write': ['uint64'],
diff --git a/blockdev.c b/blockdev.c
index a8755bd908..87f4ab3316 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -4368,20 +4368,22 @@ void qmp_x_blockdev_set_iothread(const char *node_name, StrOrNull *iothread,
 }
 
 void qmp_x_block_latency_histogram_set(
-    const char *device,
+    bool has_device, const char *device,
+    bool has_id, const char *id,
     bool has_boundaries, uint64List *boundaries,
     bool has_boundaries_read, uint64List *boundaries_read,
     bool has_boundaries_write, uint64List *boundaries_write,
     bool has_boundaries_flush, uint64List *boundaries_flush,
     Error **errp)
 {
-    BlockBackend *blk = blk_by_name(device);
+    BlockBackend *blk;
     BlockAcctStats *stats;
 
+    blk = qmp_get_blk(has_device ? device : NULL, has_id ? id : NULL, errp);
     if (!blk) {
-        error_setg(errp, "Device '%s' not found", device);
         return;
     }
+
     stats = blk_get_stats(blk);
 
     if (!has_boundaries && !has_boundaries_read && !has_boundaries_write &&
-- 
2.18.0

[Qemu-devel] [PATCH 2/2] qapi: drop x- from x-block-latency-histogram-set

[Vladimir Sementsov-Ogievskiy]

Libvirt part is ready, let's drop x- prefix.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 qapi/block-core.json | 4 ++--
 blockdev.c           | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 4efd60d8ab..e9c0079933 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -482,7 +482,7 @@
   'data': {'boundaries': ['uint64'], 'bins': ['uint64'] } }
 
 ##
-# @x-block-latency-histogram-set:
+# @block-latency-histogram-set:
 #
 # Manage read, write and flush latency histograms for the device.
 #
@@ -548,7 +548,7 @@
 #      "arguments": { "device": "drive0" } }
 # <- { "return": {} }
 ##
-{ 'command': 'x-block-latency-histogram-set',
+{ 'command': 'block-latency-histogram-set',
   'data': {'*device': 'str', '*id': 'str',
            '*boundaries': ['uint64'],
            '*boundaries-read': ['uint64'],
diff --git a/blockdev.c b/blockdev.c
index 87f4ab3316..bc579d8f81 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -4367,7 +4367,7 @@ void qmp_x_blockdev_set_iothread(const char *node_name, StrOrNull *iothread,
     aio_context_release(old_context);
 }
 
-void qmp_x_block_latency_histogram_set(
+void qmp_block_latency_histogram_set(
     bool has_device, const char *device,
     bool has_id, const char *id,
     bool has_boundaries, uint64List *boundaries,
-- 
2.18.0

Re: [Qemu-devel] [PATCH 2/2] qapi: drop x- from x-block-latency-histogram-set

[Vladimir Sementsov-Ogievskiy]

02.10.2018 14:33, Vladimir Sementsov-Ogievskiy wrote:
> Libvirt part is ready, let's drop x- prefix.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   qapi/block-core.json | 4 ++--
>   blockdev.c           | 2 +-
>   2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 4efd60d8ab..e9c0079933 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -482,7 +482,7 @@
>     'data': {'boundaries': ['uint64'], 'bins': ['uint64'] } }
>   
>   ##
> -# @x-block-latency-histogram-set:
> +# @block-latency-histogram-set:
>   #
>   # Manage read, write and flush latency histograms for the device.
>   #
> @@ -548,7 +548,7 @@
>   #      "arguments": { "device": "drive0" } }
>   # <- { "return": {} }
>   ##
> -{ 'command': 'x-block-latency-histogram-set',
> +{ 'command': 'block-latency-histogram-set',
>     'data': {'*device': 'str', '*id': 'str',
>              '*boundaries': ['uint64'],
>              '*boundaries-read': ['uint64'],
> diff --git a/blockdev.c b/blockdev.c
> index 87f4ab3316..bc579d8f81 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -4367,7 +4367,7 @@ void qmp_x_blockdev_set_iothread(const char *node_name, StrOrNull *iothread,
>       aio_context_release(old_context);
>   }
>   
> -void qmp_x_block_latency_histogram_set(
> +void qmp_block_latency_histogram_set(
>       bool has_device, const char *device,
>       bool has_id, const char *id,
>       bool has_boundaries, uint64List *boundaries,

please amend (drop x_ from statistic output too):

diff --git a/qapi/block-core.json b/qapi/block-core.json
index e9c0079933..faaaf339a6 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -835,11 +835,11 @@
  # @timed_stats: Statistics specific to the set of previously defined
  #               intervals of time (Since 2.5)
  #
-# @x_rd_latency_histogram: @BlockLatencyHistogramInfo. (Since 2.12)
+# @rd_latency_histogram: @BlockLatencyHistogramInfo. (Since 2.12)
  #
-# @x_wr_latency_histogram: @BlockLatencyHistogramInfo. (Since 2.12)
+# @wr_latency_histogram: @BlockLatencyHistogramInfo. (Since 2.12)
  #
-# @x_flush_latency_histogram: @BlockLatencyHistogramInfo. (Since 2.12)
+# @flush_latency_histogram: @BlockLatencyHistogramInfo. (Since 2.12)
  #
  # Since: 0.14.0
  ##
@@ -854,9 +854,9 @@
             'invalid_wr_operations': 'int', 'invalid_flush_operations': 
'int',
             'account_invalid': 'bool', 'account_failed': 'bool',
             'timed_stats': ['BlockDeviceTimedStats'],
-           '*x_rd_latency_histogram': 'BlockLatencyHistogramInfo',
-           '*x_wr_latency_histogram': 'BlockLatencyHistogramInfo',
-           '*x_flush_latency_histogram': 'BlockLatencyHistogramInfo' } }
+           '*rd_latency_histogram': 'BlockLatencyHistogramInfo',
+           '*wr_latency_histogram': 'BlockLatencyHistogramInfo',
+           '*flush_latency_histogram': 'BlockLatencyHistogramInfo' } }

  ##
  # @BlockStats:
diff --git a/block/qapi.c b/block/qapi.c
index c66f949db8..dc7508a06d 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -492,14 +492,14 @@ static void bdrv_query_blk_stats(BlockDeviceStats 
*ds, BlockBackend *blk)
      }

bdrv_latency_histogram_stats(&stats->latency_histogram[BLOCK_ACCT_READ],
- &ds->has_x_rd_latency_histogram,
- &ds->x_rd_latency_histogram);
+ &ds->has_rd_latency_histogram,
+ &ds->rd_latency_histogram);
bdrv_latency_histogram_stats(&stats->latency_histogram[BLOCK_ACCT_WRITE],
- &ds->has_x_wr_latency_histogram,
- &ds->x_wr_latency_histogram);
+ &ds->has_wr_latency_histogram,
+ &ds->wr_latency_histogram);
bdrv_latency_histogram_stats(&stats->latency_histogram[BLOCK_ACCT_FLUSH],
- &ds->has_x_flush_latency_histogram,
- &ds->x_flush_latency_histogram);
+ &ds->has_flush_latency_histogram,
+ &ds->flush_latency_histogram);
  }

  static BlockStats *bdrv_query_bds_stats(BlockDriverState *bs,


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 1/2] qapi: support device id for x-block-latency-histogram-set

[Eric Blake]

On 10/2/18 6:33 AM, Vladimir Sementsov-Ogievskiy wrote:
> Support modern way of device selecting.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   qapi/block-core.json | 6 ++++--
>   blockdev.c           | 8 +++++---
>   2 files changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index ac3b48ee54..4efd60d8ab 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -489,7 +489,9 @@
>   # If only @device parameter is specified, remove all present latency histograms
>   # for the device. Otherwise, add/reset some of (or all) latency histograms.
>   #
> -# @device: device name to set latency histogram for.
> +# @device: device name to set latency histogram for (better use @id).
> +#
> +# @id: The name or QOM path of the guest device.

As long as we are renaming the command, there's no need to keep a legacy 
parameter around. Just get rid of device and replace it by id, rather 
than worrying about both.  The introduction of the stable command does 
not have to carry any baggage left over from the x- preliminary version.

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

Re: [Qemu-devel] [PATCH 2/2] qapi: drop x- from x-block-latency-histogram-set

[Eric Blake]

On 10/2/18 6:33 AM, Vladimir Sementsov-Ogievskiy wrote:
> Libvirt part is ready, let's drop x- prefix.

A link to the libvirt list archives would be useful.

> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   qapi/block-core.json | 4 ++--
>   blockdev.c           | 2 +-
>   2 files changed, 3 insertions(+), 3 deletions(-)
> 

> @@ -548,7 +548,7 @@
>   #      "arguments": { "device": "drive0" } }
>   # <- { "return": {} }

As part of renaming this, you need to change the 'Since: 2.12' tag to 
now read 3.1.  That is, we are introducing a brand new command (at the 
same time we delete an experimental one), and that is particularly true 
if you take my advice on the previous commit about getting rid of the 
'device' parameter at the same time.

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

Re: [Qemu-devel] [PATCH 1/2] qapi: support device id for x-block-latency-histogram-set

[Nikolay Shirokovskiy]

On 02.10.2018 17:22, Eric Blake wrote:
> On 10/2/18 6:33 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Support modern way of device selecting.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   qapi/block-core.json | 6 ++++--
>>   blockdev.c           | 8 +++++---
>>   2 files changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index ac3b48ee54..4efd60d8ab 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -489,7 +489,9 @@
>>   # If only @device parameter is specified, remove all present latency histograms
>>   # for the device. Otherwise, add/reset some of (or all) latency histograms.
>>   #
>> -# @device: device name to set latency histogram for.
>> +# @device: device name to set latency histogram for (better use @id).
>> +#
>> +# @id: The name or QOM path of the guest device.
> 
> As long as we are renaming the command, there's no need to keep a legacy parameter around. Just get rid of device and replace it by id, rather than worrying about both.  The introduction of the stable command does not have to carry any baggage left over from the x- preliminary version.
> 

I'm afraid libvirt is not ready for this. It works either thru drive aliases or qom paths depending
on capability that is not turned on yet. It takes some time before capability can be safely turned
on (blockjob code is not ready AFAIK). Until that moment we can not use latency API without "device" parameter.

Nikolay

Re: [Qemu-devel] [PATCH 1/2] qapi: support device id for x-block-latency-histogram-set

[Vladimir Sementsov-Ogievskiy]

02.10.2018 17:22, Eric Blake wrote:
> On 10/2/18 6:33 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Support modern way of device selecting.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   qapi/block-core.json | 6 ++++--
>>   blockdev.c           | 8 +++++---
>>   2 files changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index ac3b48ee54..4efd60d8ab 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -489,7 +489,9 @@
>>   # If only @device parameter is specified, remove all present 
>> latency histograms
>>   # for the device. Otherwise, add/reset some of (or all) latency 
>> histograms.
>>   #
>> -# @device: device name to set latency histogram for.
>> +# @device: device name to set latency histogram for (better use @id).
>> +#
>> +# @id: The name or QOM path of the guest device.
>
> As long as we are renaming the command, there's no need to keep a 
> legacy parameter around. Just get rid of device and replace it by id, 
> rather than worrying about both.  The introduction of the stable 
> command does not have to carry any baggage left over from the x- 
> preliminary version.
>

Libvirt don't need both for now, for different scenarios?

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 1/2] qapi: support device id for x-block-latency-histogram-set

[Eric Blake]

On 10/2/18 9:30 AM, Vladimir Sementsov-Ogievskiy wrote:

>>> -# @device: device name to set latency histogram for.
>>> +# @device: device name to set latency histogram for (better use @id).
>>> +#
>>> +# @id: The name or QOM path of the guest device.
>>
>> As long as we are renaming the command, there's no need to keep a 
>> legacy parameter around. Just get rid of device and replace it by id, 
>> rather than worrying about both.  The introduction of the stable 
>> command does not have to carry any baggage left over from the x- 
>> preliminary version.
>>
> 
> Libvirt don't need both for now, for different scenarios?

If you want both to work because libvirt has reasons to use both, then 
the documentation needs to be more precise. "(better use @id)" does not 
tell me why I shouldn't use @device, or what difference in behavior I 
get by using one instead of the other. Furthermore, if @id is able to 
use the name of the guest device as an alternative to the QOM path, then 
how is that different from @device being the name of the guest device?

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

Re: [Qemu-devel] [PATCH 1/2] qapi: support device id for x-block-latency-histogram-set

[Vladimir Sementsov-Ogievskiy]

02.10.2018 17:35, Eric Blake wrote:
> On 10/2/18 9:30 AM, Vladimir Sementsov-Ogievskiy wrote:
>
>>>> -# @device: device name to set latency histogram for.
>>>> +# @device: device name to set latency histogram for (better use @id).
>>>> +#
>>>> +# @id: The name or QOM path of the guest device.
>>>
>>> As long as we are renaming the command, there's no need to keep a 
>>> legacy parameter around. Just get rid of device and replace it by 
>>> id, rather than worrying about both.  The introduction of the stable 
>>> command does not have to carry any baggage left over from the x- 
>>> preliminary version.
>>>
>>
>> Libvirt don't need both for now, for different scenarios?
>
> If you want both to work because libvirt has reasons to use both, then 
> the documentation needs to be more precise. "(better use @id)" does 
> not tell me why I shouldn't use @device, or what difference in 
> behavior I get by using one instead of the other. Furthermore, if @id 
> is able to use the name of the guest device as an alternative to the 
> QOM path, then how is that different from @device being the name of 
> the guest device?
>

Hm. It all looks a bit weird. I've just duplicated block_set_io_throttle 
interface:

# @device: Block device name (deprecated, use @id instead)
#
# @id: The name or QOM path of the guest device (since: 2.8)

     blk = qmp_get_blk(arg->has_device ? arg->device : NULL,
                       arg->has_id ? arg->id : NULL,
errp);
     if (!blk) {
return;
     }


So, looks like "The name or" is wrong part. @id can be only path.

However, I can call blk_by_name on @id, if blk_by_qdev_id failed..

So, variants:

1. both parameters, current code, but fix documentation (will be @id: 
QOM path of the guest device.)
2. only @id and only QOM path
3. only @id, but fullback to blk_by_name(@id) if failed to find qdev.

Any option is ok for me, I don't care. What do you think?


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 1/2] qapi: support device id for x-block-latency-histogram-set

[Eric Blake]

On 10/2/18 9:58 AM, Vladimir Sementsov-Ogievskiy wrote:

>>>>> -# @device: device name to set latency histogram for.
>>>>> +# @device: device name to set latency histogram for (better use @id).
>>>>> +#
>>>>> +# @id: The name or QOM path of the guest device.
>>>>

> Hm. It all looks a bit weird. I've just duplicated block_set_io_throttle
> interface:

But that interface was pre-existing under a stable name, so it had to 
worry about back-compat. Here, we are adding a new command, so we don't 
have to be stuck with back-compat ugliness.

> 
> # @device: Block device name (deprecated, use @id instead)
> #
> # @id: The name or QOM path of the guest device (since: 2.8)
> 
>       blk = qmp_get_blk(arg->has_device ? arg->device : NULL,
>                         arg->has_id ? arg->id : NULL,
> errp);
>       if (!blk) {
> return;
>       }
> 
> 
> So, looks like "The name or" is wrong part. @id can be only path.
> 
> However, I can call blk_by_name on @id, if blk_by_qdev_id failed..
> 
> So, variants:
> 
> 1. both parameters, current code, but fix documentation (will be @id:
> QOM path of the guest device.)
> 2. only @id and only QOM path
> 3. only @id, but fullback to blk_by_name(@id) if failed to find qdev.
> 
> Any option is ok for me, I don't care. What do you think?

I'm leaning towards 3.

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