[Qemu-devel] [PATCH 0/4] qemu-img: Implement commit like QMP

[Qemu-devel] [PATCH 0/4] qemu-img: Implement commit like QMP

[Max Reitz]

qemu-img should use QMP commands whenever possible in order to ensure
feature completeness of both online and offline image operations. For
the "commit" command, this is relatively easy, so implement it first
(in the hope that indeed others will follow).

As qemu-img does not have access to QMP (due to QMP being intertwined
with basically everything in qemu), we cannot directly use QMP, but at
least use the functions the corresponding QMP commands are using (which
would be "block-commit", in this case).


Max Reitz (4):
  block-commit: Expose granularity
  block-commit: speed is an optional parameter
  qemu-img: Implement commit like QMP
  qemu-img: Enable progress output for commit

 block/Makefile.objs       |   2 +-
 block/commit.c            |  16 ++++++--
 block/mirror.c            |   4 +-
 blockdev.c                |  22 +++++++---
 include/block/block_int.h |   6 ++-
 qapi-schema.json          |   5 ++-
 qemu-img-cmds.hx          |   4 +-
 qemu-img.c                | 100 ++++++++++++++++++++++++++++++++++++----------
 qemu-img.texi             |   2 +-
 9 files changed, 122 insertions(+), 39 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH 1/4] block-commit: Expose granularity

[Max Reitz]

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(-)

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
-- 
1.9.1

[Qemu-devel] [PATCH 2/4] block-commit: speed is an optional parameter

[Max Reitz]

As speed is an optional parameter for the QMP block-commit command, it
should be set to 0 if not given (as it is undefined if has_speed is
false), that is, the speed should not be limited.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 blockdev.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/blockdev.c b/blockdev.c
index 0be4601..337c11f 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1876,6 +1876,7 @@ void qmp_block_commit(const char *device,
      */
     BlockdevOnError on_error = BLOCKDEV_ON_ERROR_REPORT;
 
+    speed       = speed           ? speed       : 0;
     granularity = has_granularity ? granularity : 0;
 
     if (has_granularity &&
-- 
1.9.1

[Qemu-devel] [PATCH 3/4] qemu-img: Implement commit like QMP

[Max Reitz]

qemu-img should use QMP commands whenever possible in order to ensure
feature completeness of both online and offline image operations. As
qemu-img itself has no access to QMP (since this would basically require
just everything being linked into qemu-img), imitate QMP's
implementation of block-commit by using commit_active_start() and then
waiting for the block job to finish.

This new implementation does not empty the snapshot image, as opposed to
the old implementation using bdrv_commit(). However, as QMP's
block-commit apparently never did this and as qcow2 (which is probably
qemu's standard image format) does not even implement the required
function (bdrv_make_empty()), it does not seem necessary.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/Makefile.objs |  2 +-
 qemu-img.c          | 68 ++++++++++++++++++++++++++++++++++++++---------------
 2 files changed, 50 insertions(+), 20 deletions(-)

diff --git a/block/Makefile.objs b/block/Makefile.objs
index fd88c03..2c37e80 100644
--- a/block/Makefile.objs
+++ b/block/Makefile.objs
@@ -9,6 +9,7 @@ block-obj-y += snapshot.o qapi.o
 block-obj-$(CONFIG_WIN32) += raw-win32.o win32-aio.o
 block-obj-$(CONFIG_POSIX) += raw-posix.o
 block-obj-$(CONFIG_LINUX_AIO) += linux-aio.o
+block-obj-y += mirror.o
 
 ifeq ($(CONFIG_POSIX),y)
 block-obj-y += nbd.o nbd-client.o sheepdog.o
@@ -22,7 +23,6 @@ endif
 
 common-obj-y += stream.o
 common-obj-y += commit.o
-common-obj-y += mirror.o
 common-obj-y += backup.o
 
 iscsi.o-cflags     := $(LIBISCSI_CFLAGS)
diff --git a/qemu-img.c b/qemu-img.c
index 8455994..22ce01d 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -30,6 +30,7 @@
 #include "qemu/osdep.h"
 #include "sysemu/sysemu.h"
 #include "block/block_int.h"
+#include "block/blockjob.h"
 #include "block/qapi.h"
 #include <getopt.h>
 
@@ -682,12 +683,37 @@ fail:
     return ret;
 }
 
+static void dummy_block_job_cb(void *opaque, int ret)
+{
+}
+
+static void run_block_job(BlockJob *job, Error **errp)
+{
+    BlockJobInfo *info;
+
+    do {
+        aio_poll(qemu_get_aio_context(), true);
+
+        info = block_job_query(job);
+
+        if (!info->busy && info->offset < info->len) {
+            block_job_resume(job);
+        }
+    } while (info->offset < info->len);
+
+    block_job_complete(job, errp);
+}
+
+/* Same as in block.c */
+#define COMMIT_BUF_SECTORS 2048
+
 static int img_commit(int argc, char **argv)
 {
     int c, ret, flags;
     const char *filename, *fmt, *cache;
-    BlockDriverState *bs;
+    BlockDriverState *bs, *base_bs;
     bool quiet = false;
+    Error *local_err = NULL;
 
     fmt = NULL;
     cache = BDRV_DEFAULT_CACHE;
@@ -712,6 +738,7 @@ static int img_commit(int argc, char **argv)
             break;
         }
     }
+
     if (optind != argc - 1) {
         help();
     }
@@ -728,29 +755,32 @@ static int img_commit(int argc, char **argv)
     if (!bs) {
         return 1;
     }
-    ret = bdrv_commit(bs);
-    switch(ret) {
-    case 0:
-        qprintf(quiet, "Image committed.\n");
-        break;
-    case -ENOENT:
-        error_report("No disk inserted");
-        break;
-    case -EACCES:
-        error_report("Image is read-only");
-        break;
-    case -ENOTSUP:
-        error_report("Image is already committed");
-        break;
-    default:
-        error_report("Error while committing image");
-        break;
+
+    base_bs = bdrv_find_base(bs);
+    if (!base_bs) {
+        error_set(&local_err, QERR_BASE_NOT_FOUND, "NULL");
+        goto done;
+    }
+
+    commit_active_start(bs, base_bs, 0, COMMIT_BUF_SECTORS << BDRV_SECTOR_BITS,
+                        BLOCKDEV_ON_ERROR_REPORT, dummy_block_job_cb, bs,
+                        &local_err);
+    if (error_is_set(&local_err)) {
+        goto done;
     }
 
+    run_block_job(bs->job, &local_err);
+
+done:
     bdrv_unref(bs);
-    if (ret) {
+
+    if (error_is_set(&local_err)) {
+        qerror_report_err(local_err);
+        error_free(local_err);
         return 1;
     }
+
+    qprintf(quiet, "Image committed.\n");
     return 0;
 }
 
-- 
1.9.1

[Qemu-devel] [PATCH 4/4] qemu-img: Enable progress output for commit

[Max Reitz]

Implement progress output for the commit command by querying the
progress of the block job.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 qemu-img-cmds.hx |  4 ++--
 qemu-img.c       | 32 ++++++++++++++++++++++++++++++--
 qemu-img.texi    |  2 +-
 3 files changed, 33 insertions(+), 5 deletions(-)

diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index d029609..8bc55cd 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -22,9 +22,9 @@ STEXI
 ETEXI
 
 DEF("commit", img_commit,
-    "commit [-q] [-f fmt] [-t cache] filename")
+    "commit [-q] [-f fmt] [-t cache] [-p] filename")
 STEXI
-@item commit [-q] [-f @var{fmt}] [-t @var{cache}] @var{filename}
+@item commit [-q] [-f @var{fmt}] [-t @var{cache}] [-p] @var{filename}
 ETEXI
 
 DEF("compare", img_compare,
diff --git a/qemu-img.c b/qemu-img.c
index 22ce01d..84119df 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -690,12 +690,27 @@ static void dummy_block_job_cb(void *opaque, int ret)
 static void run_block_job(BlockJob *job, Error **errp)
 {
     BlockJobInfo *info;
+    uint64_t mod_offset = 0;
 
     do {
         aio_poll(qemu_get_aio_context(), true);
 
         info = block_job_query(job);
 
+        if (info->offset) {
+            if (!mod_offset) {
+                /* Some block jobs (at least "commit") will only work on a
+                 * subset of the image file and therefore basically skip many
+                 * sectors at the start (processing them apparently
+                 * instantaneously). These sectors should be ignored when
+                 * calculating the progress. */
+                mod_offset = info->offset;
+            }
+
+            qemu_progress_print((float)(info->offset - mod_offset) /
+                                (info->len - mod_offset) * 100.f, 0);
+        }
+
         if (!info->busy && info->offset < info->len) {
             block_job_resume(job);
         }
@@ -712,13 +727,13 @@ static int img_commit(int argc, char **argv)
     int c, ret, flags;
     const char *filename, *fmt, *cache;
     BlockDriverState *bs, *base_bs;
-    bool quiet = false;
+    bool progress = false, quiet = false;
     Error *local_err = NULL;
 
     fmt = NULL;
     cache = BDRV_DEFAULT_CACHE;
     for(;;) {
-        c = getopt(argc, argv, "f:ht:q");
+        c = getopt(argc, argv, "f:ht:qp");
         if (c == -1) {
             break;
         }
@@ -733,12 +748,20 @@ static int img_commit(int argc, char **argv)
         case 't':
             cache = optarg;
             break;
+        case 'p':
+            progress = true;
+            break;
         case 'q':
             quiet = true;
             break;
         }
     }
 
+    /* Progress is not shown in Quiet mode */
+    if (quiet) {
+        progress = false;
+    }
+
     if (optind != argc - 1) {
         help();
     }
@@ -756,6 +779,9 @@ static int img_commit(int argc, char **argv)
         return 1;
     }
 
+    qemu_progress_init(progress, 1.f);
+    qemu_progress_print(0.f, 100);
+
     base_bs = bdrv_find_base(bs);
     if (!base_bs) {
         error_set(&local_err, QERR_BASE_NOT_FOUND, "NULL");
@@ -772,6 +798,8 @@ static int img_commit(int argc, char **argv)
     run_block_job(bs->job, &local_err);
 
 done:
+    qemu_progress_end();
+
     bdrv_unref(bs);
 
     if (error_is_set(&local_err)) {
diff --git a/qemu-img.texi b/qemu-img.texi
index f84590e..1a9c08f 100644
--- a/qemu-img.texi
+++ b/qemu-img.texi
@@ -140,7 +140,7 @@ this case. @var{backing_file} will never be modified unless you use the
 The size can also be specified using the @var{size} option with @code{-o},
 it doesn't need to be specified separately in this case.
 
-@item commit [-f @var{fmt}] [-t @var{cache}] @var{filename}
+@item commit [-q] [-f @var{fmt}] [-t @var{cache}] [-p] @var{filename}
 
 Commit the changes recorded in @var{filename} in its base image or backing file.
 If the backing file is smaller than the snapshot, then the backing file will be
-- 
1.9.1

Re: [Qemu-devel] [PATCH 2/4] block-commit: speed is an optional parameter

[Eric Blake]

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

On 04/07/2014 11:29 AM, Max Reitz wrote:
> As speed is an optional parameter for the QMP block-commit command, it
> should be set to 0 if not given (as it is undefined if has_speed is
> false), that is, the speed should not be limited.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  blockdev.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 0be4601..337c11f 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1876,6 +1876,7 @@ void qmp_block_commit(const char *device,
>       */
>      BlockdevOnError on_error = BLOCKDEV_ON_ERROR_REPORT;
>  
> +    speed       = speed           ? speed       : 0;

Oops, branching based on the contents of an undefined variable.  You
meant has_speed in the second of the three uses.

-- 
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] [PATCH 2/4] block-commit: speed is an optional parameter

[Max Reitz]

On 07.04.2014 20:55, Eric Blake wrote:
> On 04/07/2014 11:29 AM, Max Reitz wrote:
>> As speed is an optional parameter for the QMP block-commit command, it
>> should be set to 0 if not given (as it is undefined if has_speed is
>> false), that is, the speed should not be limited.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   blockdev.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/blockdev.c b/blockdev.c
>> index 0be4601..337c11f 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -1876,6 +1876,7 @@ void qmp_block_commit(const char *device,
>>        */
>>       BlockdevOnError on_error = BLOCKDEV_ON_ERROR_REPORT;
>>   
>> +    speed       = speed           ? speed       : 0;
> Oops, branching based on the contents of an undefined variable.  You
> meant has_speed in the second of the three uses.

Right – this way, it makes little sense but as a test for compiler 
optimizations. ;-)

Max

Re: [Qemu-devel] [PATCH 1/4] block-commit: Expose granularity

[Eric Blake]

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

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.

-- 
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] [PATCH 3/4] qemu-img: Implement commit like QMP

[Eric Blake]

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

On 04/07/2014 11:29 AM, Max Reitz wrote:
> qemu-img should use QMP commands whenever possible in order to ensure
> feature completeness of both online and offline image operations. As
> qemu-img itself has no access to QMP (since this would basically require
> just everything being linked into qemu-img), imitate QMP's
> implementation of block-commit by using commit_active_start() and then
> waiting for the block job to finish.
> 
> This new implementation does not empty the snapshot image, as opposed to
> the old implementation using bdrv_commit(). However, as QMP's
> block-commit apparently never did this and as qcow2 (which is probably
> qemu's standard image format) does not even implement the required
> function (bdrv_make_empty()), it does not seem necessary.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/Makefile.objs |  2 +-
>  qemu-img.c          | 68 ++++++++++++++++++++++++++++++++++++++---------------
>  2 files changed, 50 insertions(+), 20 deletions(-)
> 

> @@ -728,29 +755,32 @@ static int img_commit(int argc, char **argv)
>      if (!bs) {
>          return 1;
>      }

> +
> +    base_bs = bdrv_find_base(bs);
> +    if (!base_bs) {
> +        error_set(&local_err, QERR_BASE_NOT_FOUND, "NULL");
> +        goto done;
> +    }

Is it worth adding an optional '-b base' image to allow qemu-img to
commit across multiple images?  That is, QMP can shorten from 'a <- b <-
c' all the way to 'a'; but qemu-img has to be called twice (once to 'a
<- b' and second to 'a').  Separate commit, of course.

> +
> +    commit_active_start(bs, base_bs, 0, COMMIT_BUF_SECTORS << BDRV_SECTOR_BITS,
> +                        BLOCKDEV_ON_ERROR_REPORT, dummy_block_job_cb, bs,
> +                        &local_err);
> +    if (error_is_set(&local_err)) {

No new uses of error_is_set if we can help it.  This can be 'if
(local_err)'.

> +        goto done;
>      }
>  
> +    run_block_job(bs->job, &local_err);
> +
> +done:
>      bdrv_unref(bs);
> -    if (ret) {
> +
> +    if (error_is_set(&local_err)) {

and again.

-- 
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] [PATCH 1/4] block-commit: Expose granularity

[Max Reitz]

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.
>

Re: [Qemu-devel] [PATCH 3/4] qemu-img: Implement commit like QMP

[Max Reitz]

On 07.04.2014 21:10, Eric Blake wrote:
> On 04/07/2014 11:29 AM, Max Reitz wrote:
>> qemu-img should use QMP commands whenever possible in order to ensure
>> feature completeness of both online and offline image operations. As
>> qemu-img itself has no access to QMP (since this would basically require
>> just everything being linked into qemu-img), imitate QMP's
>> implementation of block-commit by using commit_active_start() and then
>> waiting for the block job to finish.
>>
>> This new implementation does not empty the snapshot image, as opposed to
>> the old implementation using bdrv_commit(). However, as QMP's
>> block-commit apparently never did this and as qcow2 (which is probably
>> qemu's standard image format) does not even implement the required
>> function (bdrv_make_empty()), it does not seem necessary.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   block/Makefile.objs |  2 +-
>>   qemu-img.c          | 68 ++++++++++++++++++++++++++++++++++++++---------------
>>   2 files changed, 50 insertions(+), 20 deletions(-)
>>
>> @@ -728,29 +755,32 @@ static int img_commit(int argc, char **argv)
>>       if (!bs) {
>>           return 1;
>>       }
>> +
>> +    base_bs = bdrv_find_base(bs);
>> +    if (!base_bs) {
>> +        error_set(&local_err, QERR_BASE_NOT_FOUND, "NULL");
>> +        goto done;
>> +    }
> Is it worth adding an optional '-b base' image to allow qemu-img to
> commit across multiple images?  That is, QMP can shorten from 'a <- b <-
> c' all the way to 'a'; but qemu-img has to be called twice (once to 'a
> <- b' and second to 'a').  Separate commit, of course.

Sounds interesting, I'll have a look.

>> +
>> +    commit_active_start(bs, base_bs, 0, COMMIT_BUF_SECTORS << BDRV_SECTOR_BITS,
>> +                        BLOCKDEV_ON_ERROR_REPORT, dummy_block_job_cb, bs,
>> +                        &local_err);
>> +    if (error_is_set(&local_err)) {
> No new uses of error_is_set if we can help it.  This can be 'if
> (local_err)'.

Okay, seems like I missed something.

Max

>> +        goto done;
>>       }
>>   
>> +    run_block_job(bs->job, &local_err);
>> +
>> +done:
>>       bdrv_unref(bs);
>> -    if (ret) {
>> +
>> +    if (error_is_set(&local_err)) {
> and again.
>

Re: [Qemu-devel] [PATCH 4/4] qemu-img: Enable progress output for commit

[Eric Blake]

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

On 04/07/2014 11:30 AM, Max Reitz wrote:
> Implement progress output for the commit command by querying the
> progress of the block job.

Yay!  I've been asking for this for a while.

> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  qemu-img-cmds.hx |  4 ++--
>  qemu-img.c       | 32 ++++++++++++++++++++++++++++++--
>  qemu-img.texi    |  2 +-
>  3 files changed, 33 insertions(+), 5 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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] [PATCH 3/4] qemu-img: Implement commit like QMP

[Markus Armbruster]

Max Reitz <mreitz@redhat.com> writes:

> On 07.04.2014 21:10, Eric Blake wrote:
>> On 04/07/2014 11:29 AM, Max Reitz wrote:
>>> qemu-img should use QMP commands whenever possible in order to ensure
>>> feature completeness of both online and offline image operations. As
>>> qemu-img itself has no access to QMP (since this would basically require
>>> just everything being linked into qemu-img), imitate QMP's
>>> implementation of block-commit by using commit_active_start() and then
>>> waiting for the block job to finish.
>>>
>>> This new implementation does not empty the snapshot image, as opposed to
>>> the old implementation using bdrv_commit(). However, as QMP's
>>> block-commit apparently never did this and as qcow2 (which is probably
>>> qemu's standard image format) does not even implement the required
>>> function (bdrv_make_empty()), it does not seem necessary.
>>>
>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>> ---
>>>   block/Makefile.objs |  2 +-
>>>   qemu-img.c          | 68 ++++++++++++++++++++++++++++++++++++++---------------
>>>   2 files changed, 50 insertions(+), 20 deletions(-)
>>>
>>> @@ -728,29 +755,32 @@ static int img_commit(int argc, char **argv)
>>>       if (!bs) {
>>>           return 1;
>>>       }
>>> +
>>> +    base_bs = bdrv_find_base(bs);
>>> +    if (!base_bs) {
>>> +        error_set(&local_err, QERR_BASE_NOT_FOUND, "NULL");
>>> +        goto done;
>>> +    }
>> Is it worth adding an optional '-b base' image to allow qemu-img to
>> commit across multiple images?  That is, QMP can shorten from 'a <- b <-
>> c' all the way to 'a'; but qemu-img has to be called twice (once to 'a
>> <- b' and second to 'a').  Separate commit, of course.
>
> Sounds interesting, I'll have a look.
>
>>> +
>>> + commit_active_start(bs, base_bs, 0, COMMIT_BUF_SECTORS <<
>>> BDRV_SECTOR_BITS,
>>> +                        BLOCKDEV_ON_ERROR_REPORT, dummy_block_job_cb, bs,
>>> +                        &local_err);
>>> +    if (error_is_set(&local_err)) {
>> No new uses of error_is_set if we can help it.  This can be 'if
>> (local_err)'.
>
> Okay, seems like I missed something.

Yes :)

commit 84d18f065fb041a1c0d78d20320d740ae0673c8a
Author: Markus Armbruster <armbru@redhat.com>
Date:   Thu Jan 30 15:07:28 2014 +0100

    Use error_is_set() only when necessary
    
    error_is_set(&var) is the same as var != NULL, but it takes
    whole-program analysis to figure that out.  Unnecessarily hard for
    optimizers, static checkers, and human readers.  Dumb it down to
    obvious.
    
    Gets rid of several dozen Coverity false positives.
    
    Note that the obvious form is already used in many places.

[...]

Re: [Qemu-devel] [PATCH 3/4] qemu-img: Implement commit like QMP

[Max Reitz]

On 08.04.2014 08:49, Markus Armbruster wrote:
> Max Reitz <mreitz@redhat.com> writes:
>
>> On 07.04.2014 21:10, Eric Blake wrote:
>>> On 04/07/2014 11:29 AM, Max Reitz wrote:
>>>> qemu-img should use QMP commands whenever possible in order to ensure
>>>> feature completeness of both online and offline image operations. As
>>>> qemu-img itself has no access to QMP (since this would basically require
>>>> just everything being linked into qemu-img), imitate QMP's
>>>> implementation of block-commit by using commit_active_start() and then
>>>> waiting for the block job to finish.
>>>>
>>>> This new implementation does not empty the snapshot image, as opposed to
>>>> the old implementation using bdrv_commit(). However, as QMP's
>>>> block-commit apparently never did this and as qcow2 (which is probably
>>>> qemu's standard image format) does not even implement the required
>>>> function (bdrv_make_empty()), it does not seem necessary.
>>>>
>>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>>> ---
>>>>    block/Makefile.objs |  2 +-
>>>>    qemu-img.c          | 68 ++++++++++++++++++++++++++++++++++++++---------------
>>>>    2 files changed, 50 insertions(+), 20 deletions(-)
>>>>
>>>> @@ -728,29 +755,32 @@ static int img_commit(int argc, char **argv)
>>>>        if (!bs) {
>>>>            return 1;
>>>>        }
>>>> +
>>>> +    base_bs = bdrv_find_base(bs);
>>>> +    if (!base_bs) {
>>>> +        error_set(&local_err, QERR_BASE_NOT_FOUND, "NULL");
>>>> +        goto done;
>>>> +    }
>>> Is it worth adding an optional '-b base' image to allow qemu-img to
>>> commit across multiple images?  That is, QMP can shorten from 'a <- b <-
>>> c' all the way to 'a'; but qemu-img has to be called twice (once to 'a
>>> <- b' and second to 'a').  Separate commit, of course.
>> Sounds interesting, I'll have a look.
>>
>>>> +
>>>> + commit_active_start(bs, base_bs, 0, COMMIT_BUF_SECTORS <<
>>>> BDRV_SECTOR_BITS,
>>>> +                        BLOCKDEV_ON_ERROR_REPORT, dummy_block_job_cb, bs,
>>>> +                        &local_err);
>>>> +    if (error_is_set(&local_err)) {
>>> No new uses of error_is_set if we can help it.  This can be 'if
>>> (local_err)'.
>> Okay, seems like I missed something.
> Yes :)
>
> commit 84d18f065fb041a1c0d78d20320d740ae0673c8a
> Author: Markus Armbruster <armbru@redhat.com>
> Date:   Thu Jan 30 15:07:28 2014 +0100
>
>      Use error_is_set() only when necessary
>      
>      error_is_set(&var) is the same as var != NULL, but it takes
>      whole-program analysis to figure that out.  Unnecessarily hard for
>      optimizers, static checkers, and human readers.  Dumb it down to
>      obvious.
>      
>      Gets rid of several dozen Coverity false positives.
>      
>      Note that the obvious form is already used in many places.
>
> [...]

Thank you for the reference. Seems like I did not miss it, but worse, I 
forgot about it.

Max