[Qemu-devel] [PATCH] qmp-commands.hx: document minimum speed for block jobs

[Qemu-devel] [PATCH] qmp-commands.hx: document minimum speed for block jobs

[Sascha Silbe]

The current rate limit implementation for block jobs is ineffective
below a certain minimum rate. It will permit writes at least once per
time slice. The resulting minimum write speed (assuming source and
sink are fast enough in the first place) is high enough that it may
surprise some users, so document it. Mention that this will be fixed
in the future, otherwise some users might misguidedly rely on it or
clamp their configuration settings to the documented value.

Signed-off-by: Sascha Silbe <silbe@linux.vnet.ibm.com>
---
Noticed this while figuring out why qemu-iotests #141 failed on one of
my systems. I for one was quite surprised, so I went ahead and
documented it.


 qmp-commands.hx | 36 ++++++++++++++++++++++++++++--------
 1 file changed, 28 insertions(+), 8 deletions(-)

diff --git a/qmp-commands.hx b/qmp-commands.hx
index de896a5..e0e519a 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1107,7 +1107,10 @@ Arguments:
                   obvious choice.  Care should be taken when specifying the
                   string, to specify a valid filename or protocol.
                   (json-string, optional) (Since 2.1)
-- "speed":  the maximum speed, in bytes per second (json-int, optional)
+- "speed": The maximum speed, in bytes per second. Note: In the current
+           implementation, at least 5MiB/s will be written, even if a lower
+           speed has been set. This will be fixed in the future. (json-int,
+           optional)
 - "on-error": the action to take on an error (default 'report').  'stop' and
               'enospc' can only be used if the block device supports io-status.
               (json-string, optional) (Since 2.1)
@@ -1172,7 +1175,10 @@ Arguments:
           size of the smaller top, you can safely truncate it
           yourself once the commit operation successfully completes.
           (json-string)
-- "speed":  the maximum speed, in bytes per second (json-int, optional)
+- "speed": The maximum speed, in bytes per second. Note: In the current
+           implementation, at least 5MiB/s will be written, even if a lower
+           speed has been set. This will be fixed in the future. (json-int,
+           optional)
 
 
 Example:
@@ -1219,7 +1225,10 @@ Arguments:
             is "incremental", must NOT be present otherwise.
 - "mode": whether and how QEMU should create a new image
           (NewImageMode, optional, default 'absolute-paths')
-- "speed": the maximum speed, in bytes per second (json-int, optional)
+- "speed": The maximum speed, in bytes per second. Note: The current
+           implementation will write at a minimum speed that depends on device
+           and format, even if a lower speed is configured. This will be fixed
+           in the future. (json-int, optional)
 - "on-source-error": the action to take on an error on the source, default
                      'report'.  'stop' and 'enospc' can only be used
                      if the block device supports io-status.
@@ -1260,7 +1269,10 @@ Arguments:
           possibilities include "full" for all the disk, "top" for only the
           sectors allocated in the topmost image, or "none" to only replicate
           new I/O (MirrorSyncMode).
-- "speed": the maximum speed, in bytes per second (json-int, optional)
+- "speed": The maximum speed, in bytes per second. Note: The current
+           implementation will write at a minimum speed that depends on device
+           and format, even if a lower speed is configured. This will be fixed
+           in the future. (json-int, optional)
 - "on-source-error": the action to take on an error on the source, default
                      'report'.  'stop' and 'enospc' can only be used
                      if the block device supports io-status.
@@ -1659,8 +1671,12 @@ Arguments:
               (json-string, optional)
 - "mode": how an image file should be created into the target
   file/device (NewImageMode, optional, default 'absolute-paths')
-- "speed": maximum speed of the streaming job, in bytes per second
-  (json-int)
+- "speed": The maximum speed, in bytes per second. Note: In the
+           current implementation, the buffer will be written at least
+           once per 100ms. So with the default buffer size of 10MiB,
+           at least 10MiB/s will be written, even if a lower speed is
+           configured. This will be fixed in the future. (json-int,
+           optional)
 - "granularity": granularity of the dirty bitmap, in bytes (json-int, optional)
 - "buf-size": maximum amount of data in flight from source to target, in bytes
   (json-int, default 10M)
@@ -1712,8 +1728,12 @@ Arguments:
 - "target": device name to mirror to (json-string)
 - "replaces": the block driver node name to replace when finished
               (json-string, optional)
-- "speed": maximum speed of the streaming job, in bytes per second
-  (json-int)
+- "speed": The maximum speed, in bytes per second. Note: In the
+           current implementation, the buffer will be written at least
+           once per 100ms. So with the default buffer size of 10MiB,
+           at least 10MiB/s will be written, even if a lower speed is
+           configured. This will be fixed in the future. (json-int,
+           optional)
 - "granularity": granularity of the dirty bitmap, in bytes (json-int, optional)
 - "buf_size": maximum amount of data in flight from source to target, in bytes
   (json-int, default 10M)
-- 
1.9.1

[Qemu-devel] [for-2.6] Re: [PATCH] qmp-commands.hx: document minimum speed for block jobs

[Sascha Silbe]

Dear developers,

Sascha Silbe <silbe@linux.vnet.ibm.com> writes:

> The current rate limit implementation for block jobs is ineffective
> below a certain minimum rate. It will permit writes at least once per
> time slice. The resulting minimum write speed (assuming source and
> sink are fast enough in the first place) is high enough that it may
> surprise some users, so document it. Mention that this will be fixed
> in the future, otherwise some users might misguidedly rely on it or
> clamp their configuration settings to the documented value.

Forgot to mention: This is 2.6 material as it documents a known
shortcoming that will hopefully be fixed in 2.7.

Sascha
-- 
Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg
https://se-silbe.de/
USt-IdNr. DE281696641

Re: [Qemu-devel] [for-2.6] Re: [PATCH] qmp-commands.hx: document minimum speed for block jobs

[Kevin Wolf]

Am 14.04.2016 um 13:08 hat Sascha Silbe geschrieben:
> Dear developers,
> 
> Sascha Silbe <silbe@linux.vnet.ibm.com> writes:
> 
> > The current rate limit implementation for block jobs is ineffective
> > below a certain minimum rate. It will permit writes at least once per
> > time slice. The resulting minimum write speed (assuming source and
> > sink are fast enough in the first place) is high enough that it may
> > surprise some users, so document it. Mention that this will be fixed
> > in the future, otherwise some users might misguidedly rely on it or
> > clamp their configuration settings to the documented value.
> 
> Forgot to mention: This is 2.6 material as it documents a known
> shortcoming that will hopefully be fixed in 2.7.

Isn't the JSON schema (qapi/block-core.json) considered the
authoritative source for interface specifications these days? I think if
we want to document the shortcomings for the time being, we should
document it in both places.

Kevin

Re: [Qemu-devel] [for-2.6] Re: [PATCH] qmp-commands.hx: document minimum speed for block jobs

[Sascha Silbe]

Dear Kevin,

Kevin Wolf <kwolf@redhat.com> writes:

[...]
> Isn't the JSON schema (qapi/block-core.json) considered the
> authoritative source for interface specifications these days? I think if
> we want to document the shortcomings for the time being, we should
> document it in both places.

Interesting. What exactly is the relationship between qmp-commands.hx
and qapi/block-core.json?

At any rate, you're right that we should update both. See v2.

(And I've forgot the for-2.6 prefix for the patch itself again. Bah.)

Sascha
-- 
Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg
https://se-silbe.de/
USt-IdNr. DE281696641

Re: [Qemu-devel] [for-2.6] Re: [PATCH] qmp-commands.hx: document minimum speed for block jobs

[Eric Blake]

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

On 04/19/2016 01:05 PM, Sascha Silbe wrote:
> Dear Kevin,
> 
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> [...]
>> Isn't the JSON schema (qapi/block-core.json) considered the
>> authoritative source for interface specifications these days? I think if
>> we want to document the shortcomings for the time being, we should
>> document it in both places.
> 
> Interesting. What exactly is the relationship between qmp-commands.hx
> and qapi/block-core.json?

Ultimately, we want to make qmp-commands.hx go away, and have everything
generated from qapi/*.json.  Marc-Andre has proposed some patches along
those lines (back in the 2.5 timeframe, but dependent on other qapi
patches that are still pending review).

> 
> At any rate, you're right that we should update both. See v2.

Until we've automated the docs from a single source file, that is the
correct approach.

> 
> (And I've forgot the for-2.6 prefix for the patch itself again. Bah.)
> 
> Sascha
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [for-2.6] Re: [PATCH] qmp-commands.hx: document minimum speed for block jobs

[Sascha Silbe]

Dear Eric,

Eric Blake <eblake@redhat.com> writes:

[...]
> Ultimately, we want to make qmp-commands.hx go away, and have everything
> generated from qapi/*.json.  Marc-Andre has proposed some patches along
> those lines (back in the 2.5 timeframe, but dependent on other qapi
> patches that are still pending review).

OK, thanks for the clarification!

Sascha
-- 
Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg
https://se-silbe.de/
USt-IdNr. DE281696641

[Qemu-devel] [PATCH v2] QMP: document minimum speed for block jobs

[Sascha Silbe]

The current rate limit implementation for block jobs is ineffective
below a certain minimum rate. It will permit writes at least once per
time slice. The resulting minimum write speed (assuming source and
sink are fast enough in the first place) is high enough that it may
surprise some users, so document it. Mention that this will be fixed
in the future, otherwise some users might misguidedly rely on it or
clamp their configuration settings to the documented value.

Signed-off-by: Sascha Silbe <silbe@linux.vnet.ibm.com>
---
v1→v2:
  - Change qapi/block-core.json, too.
---
 qapi/block-core.json | 36 +++++++++++++++++++++++++++---------
 qmp-commands.hx      | 36 ++++++++++++++++++++++++++++--------
 2 files changed, 55 insertions(+), 17 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 1d09079..1bc78cc 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -882,7 +882,10 @@
 # @mode: #optional whether and how QEMU should create a new image, default is
 #        'absolute-paths'.
 #
-# @speed: #optional the maximum speed, in bytes per second
+# @speed: #optional The maximum speed, in bytes per second. Note: The current
+#         implementation will write at a minimum speed that depends on device
+#         and format, even if a lower speed is configured. This will be fixed
+#         in the future.
 #
 # @bitmap: #optional the name of dirty bitmap if sync is "incremental".
 #          Must be present if sync is "incremental", must NOT be present
@@ -920,8 +923,10 @@
 #        (all the disk, only the sectors allocated in the topmost image, or
 #        only new I/O).
 #
-# @speed: #optional the maximum speed, in bytes per second. The default is 0,
-#         for unlimited.
+# @speed: #optional The maximum speed, in bytes per second. The default is 0,
+#         for unlimited. Note: The current implementation will write at a
+#         minimum speed that depends on device and format, even if a lower
+#         speed is configured. This will be fixed in the future.
 #
 # @on-source-error: #optional the action to take on an error on the source,
 #                   default 'report'.  'stop' and 'enospc' can only be used
@@ -1042,7 +1047,9 @@
 #                    size of the smaller top, you can safely truncate it
 #                    yourself once the commit operation successfully completes.
 #
-# @speed:  #optional the maximum speed, in bytes per second
+# @speed: #optional The maximum speed, in bytes per second. Note: In the
+#         current implementation, at least 5MiB/s will be written, even if a
+#         lower speed has been set. This will be fixed in the future.
 #
 # Returns: Nothing on success
 #          If commit or stream is already active on this device, DeviceInUse
@@ -1127,7 +1134,11 @@
 # @mode: #optional whether and how QEMU should create a new image, default is
 #        'absolute-paths'.
 #
-# @speed:  #optional the maximum speed, in bytes per second
+# @speed: #optional The maximum speed, in bytes per second. Note: In the
+#         current implementation, the buffer will be written at least once per
+#         100ms. So with the default buffer size of 10MiB, at least 10MiB/s
+#         will be written, even if a lower speed is configured. This will be
+#         fixed in the future.
 #
 # @sync: what parts of the disk image should be copied to the destination
 #        (all the disk, only the sectors allocated in the topmost image, or
@@ -1252,7 +1263,11 @@
 #            image when a whole image copy is done. This can be used to repair
 #            broken Quorum files.
 #
-# @speed:  #optional the maximum speed, in bytes per second
+# @speed: #optional The maximum speed, in bytes per second. Note: In the
+#         current implementation, the buffer will be written at least once per
+#         100ms. So with the default buffer size of 10MiB, at least 10MiB/s
+#         will be written, even if a lower speed is configured. This will be
+#         fixed in the future.
 #
 # @sync: what parts of the disk image should be copied to the destination
 #        (all the disk, only the sectors allocated in the topmost image, or
@@ -1432,7 +1447,9 @@
 #                          protocol.
 #                          (Since 2.1)
 #
-# @speed:  #optional the maximum speed, in bytes per second
+# @speed: #optional The maximum speed, in bytes per second. Note: In the
+#         current implementation, at least 5MiB/s will be written, even if a
+#         lower speed has been set. This will be fixed in the future.
 #
 # @on-error: #optional the action to take on an error (default report).
 #            'stop' and 'enospc' can only be used if the block device
@@ -1458,8 +1475,9 @@
 #
 # @device: the device name
 #
-# @speed:  the maximum speed, in bytes per second, or 0 for unlimited.
-#          Defaults to 0.
+# @speed: The maximum speed, in bytes per second, or 0 for unlimited. Defaults
+#         to 0. See the documentation of the individual background block
+#         operations for notes about current implementation limitations.
 #
 # Returns: Nothing on success
 #          If no background operation is active on this device, DeviceNotActive
diff --git a/qmp-commands.hx b/qmp-commands.hx
index de896a5..e0e519a 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1107,7 +1107,10 @@ Arguments:
                   obvious choice.  Care should be taken when specifying the
                   string, to specify a valid filename or protocol.
                   (json-string, optional) (Since 2.1)
-- "speed":  the maximum speed, in bytes per second (json-int, optional)
+- "speed": The maximum speed, in bytes per second. Note: In the current
+           implementation, at least 5MiB/s will be written, even if a lower
+           speed has been set. This will be fixed in the future. (json-int,
+           optional)
 - "on-error": the action to take on an error (default 'report').  'stop' and
               'enospc' can only be used if the block device supports io-status.
               (json-string, optional) (Since 2.1)
@@ -1172,7 +1175,10 @@ Arguments:
           size of the smaller top, you can safely truncate it
           yourself once the commit operation successfully completes.
           (json-string)
-- "speed":  the maximum speed, in bytes per second (json-int, optional)
+- "speed": The maximum speed, in bytes per second. Note: In the current
+           implementation, at least 5MiB/s will be written, even if a lower
+           speed has been set. This will be fixed in the future. (json-int,
+           optional)
 
 
 Example:
@@ -1219,7 +1225,10 @@ Arguments:
             is "incremental", must NOT be present otherwise.
 - "mode": whether and how QEMU should create a new image
           (NewImageMode, optional, default 'absolute-paths')
-- "speed": the maximum speed, in bytes per second (json-int, optional)
+- "speed": The maximum speed, in bytes per second. Note: The current
+           implementation will write at a minimum speed that depends on device
+           and format, even if a lower speed is configured. This will be fixed
+           in the future. (json-int, optional)
 - "on-source-error": the action to take on an error on the source, default
                      'report'.  'stop' and 'enospc' can only be used
                      if the block device supports io-status.
@@ -1260,7 +1269,10 @@ Arguments:
           possibilities include "full" for all the disk, "top" for only the
           sectors allocated in the topmost image, or "none" to only replicate
           new I/O (MirrorSyncMode).
-- "speed": the maximum speed, in bytes per second (json-int, optional)
+- "speed": The maximum speed, in bytes per second. Note: The current
+           implementation will write at a minimum speed that depends on device
+           and format, even if a lower speed is configured. This will be fixed
+           in the future. (json-int, optional)
 - "on-source-error": the action to take on an error on the source, default
                      'report'.  'stop' and 'enospc' can only be used
                      if the block device supports io-status.
@@ -1659,8 +1671,12 @@ Arguments:
               (json-string, optional)
 - "mode": how an image file should be created into the target
   file/device (NewImageMode, optional, default 'absolute-paths')
-- "speed": maximum speed of the streaming job, in bytes per second
-  (json-int)
+- "speed": The maximum speed, in bytes per second. Note: In the
+           current implementation, the buffer will be written at least
+           once per 100ms. So with the default buffer size of 10MiB,
+           at least 10MiB/s will be written, even if a lower speed is
+           configured. This will be fixed in the future. (json-int,
+           optional)
 - "granularity": granularity of the dirty bitmap, in bytes (json-int, optional)
 - "buf-size": maximum amount of data in flight from source to target, in bytes
   (json-int, default 10M)
@@ -1712,8 +1728,12 @@ Arguments:
 - "target": device name to mirror to (json-string)
 - "replaces": the block driver node name to replace when finished
               (json-string, optional)
-- "speed": maximum speed of the streaming job, in bytes per second
-  (json-int)
+- "speed": The maximum speed, in bytes per second. Note: In the
+           current implementation, the buffer will be written at least
+           once per 100ms. So with the default buffer size of 10MiB,
+           at least 10MiB/s will be written, even if a lower speed is
+           configured. This will be fixed in the future. (json-int,
+           optional)
 - "granularity": granularity of the dirty bitmap, in bytes (json-int, optional)
 - "buf_size": maximum amount of data in flight from source to target, in bytes
   (json-int, default 10M)
-- 
1.9.1