diff mbox

[1/4] block-commit: Expose granularity

Message ID 1396891800-8627-2-git-send-email-mreitz@redhat.com
State New
Headers show

Commit Message

Max Reitz April 7, 2014, 5:29 p.m. UTC 
Allow QMP users to manipulate the granularity used in the block-commit
command.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/commit.c            | 16 +++++++++++++---
 block/mirror.c            |  4 ++--
 blockdev.c                | 21 +++++++++++++++------
 include/block/block_int.h |  6 ++++--
 qapi-schema.json          |  5 ++++-
 5 files changed, 38 insertions(+), 14 deletions(-)

Comments

Eric Blake April 7, 2014, 7:06 p.m. UTC | #1 
On 04/07/2014 11:29 AM, Max Reitz wrote:
> Allow QMP users to manipulate the granularity used in the block-commit
> command.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---

> @@ -214,6 +215,13 @@ void commit_start(BlockDriverState *bs, BlockDriverState *base,
>      orig_base_flags    = bdrv_get_flags(base);
>      orig_overlay_flags = bdrv_get_flags(overlay_bs);
>  
> +    if (!granularity) {
> +        granularity = COMMIT_BUFFER_SIZE;
> +    }

Default granularity of 0 becomes buffer size...

> @@ -1876,6 +1876,15 @@ void qmp_block_commit(const char *device,
>       */
>      BlockdevOnError on_error = BLOCKDEV_ON_ERROR_REPORT;
>  
> +    granularity = has_granularity ? granularity : 0;
> +
> +    if (has_granularity &&
> +        (granularity < BDRV_SECTOR_SIZE || (granularity & (granularity - 1))))
> +    {
> +        error_set(errp, QERR_INVALID_PARAMETER, "granularity");

...but this code rejects attempts for me to explicitly set granularity
to the default.  Should it be 'if (granularity &&' instead of your
current wording?

Also, is it worth using qemu-common.h's is_power_of_2 instead of
inlining it yourself?  (I don't care, as I recognize the bit
manipulations, but other readers might prefer the named version for its
legibility)

> +++ b/qapi-schema.json
> @@ -2112,6 +2112,9 @@
>  #
>  # @speed:  #optional the maximum speed, in bytes per second
>  #
> +# @granularity: #optional the granularity to be used for the operation, in
> +#               bytes; has to be a power of two and at least 512 (since 2.1)
> +#

At least you documented here that an explicit '0' is rejected, even
though it might be nicer to allow it for the sake of requesting the
default even if the default later changes in size.

Overall, though, I'm liking this series.
Max Reitz April 7, 2014, 7:10 p.m. UTC | #2 
On 07.04.2014 21:06, Eric Blake wrote:
> On 04/07/2014 11:29 AM, Max Reitz wrote:
>> Allow QMP users to manipulate the granularity used in the block-commit
>> command.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>> @@ -214,6 +215,13 @@ void commit_start(BlockDriverState *bs, BlockDriverState *base,
>>       orig_base_flags    = bdrv_get_flags(base);
>>       orig_overlay_flags = bdrv_get_flags(overlay_bs);
>>   
>> +    if (!granularity) {
>> +        granularity = COMMIT_BUFFER_SIZE;
>> +    }
> Default granularity of 0 becomes buffer size...
>
>> @@ -1876,6 +1876,15 @@ void qmp_block_commit(const char *device,
>>        */
>>       BlockdevOnError on_error = BLOCKDEV_ON_ERROR_REPORT;
>>   
>> +    granularity = has_granularity ? granularity : 0;
>> +
>> +    if (has_granularity &&
>> +        (granularity < BDRV_SECTOR_SIZE || (granularity & (granularity - 1))))
>> +    {
>> +        error_set(errp, QERR_INVALID_PARAMETER, "granularity");
> ...but this code rejects attempts for me to explicitly set granularity
> to the default.  Should it be 'if (granularity &&' instead of your
> current wording?

I intentionally rejected a granularity of 0, as I thought the user were 
always capable of dropping this parameter for obtaining the default. But 
since you are bringing this up, I guess there may be cases where a user 
is forced to give the parameter but wants to keep the default value 
nonetheless; I will change it to accept 0 in v2.

> Also, is it worth using qemu-common.h's is_power_of_2 instead of
> inlining it yourself?  (I don't care, as I recognize the bit
> manipulations, but other readers might prefer the named version for its
> legibility)

You are right, I will use the function from qemu-common.h.

Max

>> +++ b/qapi-schema.json
>> @@ -2112,6 +2112,9 @@
>>   #
>>   # @speed:  #optional the maximum speed, in bytes per second
>>   #
>> +# @granularity: #optional the granularity to be used for the operation, in
>> +#               bytes; has to be a power of two and at least 512 (since 2.1)
>> +#
> At least you documented here that an explicit '0' is rejected, even
> though it might be nicer to allow it for the sake of requesting the
> default even if the default later changes in size.
>
> Overall, though, I'm liking this series.
>
diff mbox

Patch

diff --git a/block/commit.c b/block/commit.c
index acec4ac..3758af7 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -37,6 +37,7 @@  typedef struct CommitBlockJob {
     BlockdevOnError on_error;
     int base_flags;
     int orig_overlay_flags;
+    int64_t granularity;
 } CommitBlockJob;
 
 static int coroutine_fn commit_populate(BlockDriverState *bs,
@@ -93,7 +94,7 @@  static void coroutine_fn commit_run(void *opaque)
     }
 
     end = s->common.len >> BDRV_SECTOR_BITS;
-    buf = qemu_blockalign(top, COMMIT_BUFFER_SIZE);
+    buf = qemu_blockalign(top, s->granularity);
 
     for (sector_num = 0; sector_num < end; sector_num += n) {
         uint64_t delay_ns = 0;
@@ -109,7 +110,7 @@  wait:
         }
         /* Copy if allocated above the base */
         ret = bdrv_is_allocated_above(top, base, sector_num,
-                                      COMMIT_BUFFER_SIZE / BDRV_SECTOR_SIZE,
+                                      s->granularity / BDRV_SECTOR_SIZE,
                                       &n);
         copy = (ret == 1);
         trace_commit_one_iteration(s, sector_num, n, ret);
@@ -180,7 +181,7 @@  static const BlockJobDriver commit_job_driver = {
 };
 
 void commit_start(BlockDriverState *bs, BlockDriverState *base,
-                  BlockDriverState *top, int64_t speed,
+                  BlockDriverState *top, int64_t speed, int64_t granularity,
                   BlockdevOnError on_error, BlockDriverCompletionFunc *cb,
                   void *opaque, Error **errp)
 {
@@ -214,6 +215,13 @@  void commit_start(BlockDriverState *bs, BlockDriverState *base,
     orig_base_flags    = bdrv_get_flags(base);
     orig_overlay_flags = bdrv_get_flags(overlay_bs);
 
+    if (!granularity) {
+        granularity = COMMIT_BUFFER_SIZE;
+    }
+
+    assert(granularity >= BDRV_SECTOR_SIZE);
+    assert(!(granularity & (granularity - 1)));
+
     /* convert base & overlay_bs to r/w, if necessary */
     if (!(orig_base_flags & BDRV_O_RDWR)) {
         reopen_queue = bdrv_reopen_queue(reopen_queue, base,
@@ -244,6 +252,8 @@  void commit_start(BlockDriverState *bs, BlockDriverState *base,
     s->base_flags          = orig_base_flags;
     s->orig_overlay_flags  = orig_overlay_flags;
 
+    s->granularity = granularity;
+
     s->on_error = on_error;
     s->common.co = qemu_coroutine_create(commit_run);
 
diff --git a/block/mirror.c b/block/mirror.c
index 0ef41f9..5b1ebb2 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -632,7 +632,7 @@  void mirror_start(BlockDriverState *bs, BlockDriverState *target,
 }
 
 void commit_active_start(BlockDriverState *bs, BlockDriverState *base,
-                         int64_t speed,
+                         int64_t speed, int64_t granularity,
                          BlockdevOnError on_error,
                          BlockDriverCompletionFunc *cb,
                          void *opaque, Error **errp)
@@ -674,7 +674,7 @@  void commit_active_start(BlockDriverState *bs, BlockDriverState *base,
     }
 
     bdrv_ref(base);
-    mirror_start_job(bs, base, speed, 0, 0,
+    mirror_start_job(bs, base, speed, granularity, 0,
                      on_error, on_error, cb, opaque, &local_err,
                      &commit_active_job_driver, false, base);
     if (error_is_set(&local_err)) {
diff --git a/blockdev.c b/blockdev.c
index c3422a1..0be4601 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1865,8 +1865,8 @@  void qmp_block_stream(const char *device, bool has_base,
 
 void qmp_block_commit(const char *device,
                       bool has_base, const char *base, const char *top,
-                      bool has_speed, int64_t speed,
-                      Error **errp)
+                      bool has_speed, int64_t speed, bool has_granularity,
+                      int64_t granularity, Error **errp)
 {
     BlockDriverState *bs;
     BlockDriverState *base_bs, *top_bs;
@@ -1876,6 +1876,15 @@  void qmp_block_commit(const char *device,
      */
     BlockdevOnError on_error = BLOCKDEV_ON_ERROR_REPORT;
 
+    granularity = has_granularity ? granularity : 0;
+
+    if (has_granularity &&
+        (granularity < BDRV_SECTOR_SIZE || (granularity & (granularity - 1))))
+    {
+        error_set(errp, QERR_INVALID_PARAMETER, "granularity");
+        return;
+    }
+
     /* drain all i/o before commits */
     bdrv_drain_all();
 
@@ -1911,11 +1920,11 @@  void qmp_block_commit(const char *device,
     }
 
     if (top_bs == bs) {
-        commit_active_start(bs, base_bs, speed, on_error, block_job_cb,
-                            bs, &local_err);
+        commit_active_start(bs, base_bs, speed, granularity, on_error,
+                            block_job_cb, bs, &local_err);
     } else {
-        commit_start(bs, base_bs, top_bs, speed, on_error, block_job_cb, bs,
-                    &local_err);
+        commit_start(bs, base_bs, top_bs, speed, granularity, on_error,
+                     block_job_cb, bs, &local_err);
     }
     if (local_err != NULL) {
         error_propagate(errp, local_err);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index cd5bc73..2e4b470 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -426,6 +426,7 @@  void stream_start(BlockDriverState *bs, BlockDriverState *base,
  * @top: Top block device to be committed.
  * @base: Block device that will be written into, and become the new top.
  * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
+ * @granularity: The granularity, in bytes, or 0 for a default value.
  * @on_error: The action to take upon error.
  * @cb: Completion function for the job.
  * @opaque: Opaque pointer value passed to @cb.
@@ -433,7 +434,7 @@  void stream_start(BlockDriverState *bs, BlockDriverState *base,
  *
  */
 void commit_start(BlockDriverState *bs, BlockDriverState *base,
-                 BlockDriverState *top, int64_t speed,
+                 BlockDriverState *top, int64_t speed, int64_t granularity,
                  BlockdevOnError on_error, BlockDriverCompletionFunc *cb,
                  void *opaque, Error **errp);
 /**
@@ -441,6 +442,7 @@  void commit_start(BlockDriverState *bs, BlockDriverState *base,
  * @bs: Active block device to be committed.
  * @base: Block device that will be written into, and become the new top.
  * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
+ * @granularity: The granularity, in bytes, or 0 for cluster size.
  * @on_error: The action to take upon error.
  * @cb: Completion function for the job.
  * @opaque: Opaque pointer value passed to @cb.
@@ -448,7 +450,7 @@  void commit_start(BlockDriverState *bs, BlockDriverState *base,
  *
  */
 void commit_active_start(BlockDriverState *bs, BlockDriverState *base,
-                         int64_t speed,
+                         int64_t speed, int64_t granularity,
                          BlockdevOnError on_error,
                          BlockDriverCompletionFunc *cb,
                          void *opaque, Error **errp);
diff --git a/qapi-schema.json b/qapi-schema.json
index 391356f..de07a4c 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2112,6 +2112,9 @@ 
 #
 # @speed:  #optional the maximum speed, in bytes per second
 #
+# @granularity: #optional the granularity to be used for the operation, in
+#               bytes; has to be a power of two and at least 512 (since 2.1)
+#
 # Returns: Nothing on success
 #          If commit or stream is already active on this device, DeviceInUse
 #          If @device does not exist, DeviceNotFound
@@ -2124,7 +2127,7 @@ 
 ##
 { 'command': 'block-commit',
   'data': { 'device': 'str', '*base': 'str', 'top': 'str',
-            '*speed': 'int' } }
+            '*speed': 'int', '*granularity': 'int' } }
 
 ##
 # @drive-backup