|
From: | Max Reitz |
Subject: | Re: [Qemu-devel] [PATCH v4 3/6] qemu-img: Implement commit like QMP |
Date: | Thu, 17 Apr 2014 00:03:58 +0200 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.4.0 |
On 16.04.2014 16:40, Kevin Wolf wrote:
Am 12.04.2014 um 20:57 hat Max Reitz geschrieben: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 <address@hidden> --- block/Makefile.objs | 2 +- qemu-img.c | 86 +++++++++++++++++++++++++++++++++++++++++------------ 2 files changed, 68 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.oifeq ($(CONFIG_POSIX),y)block-obj-y += nbd.o nbd-client.o sheepdog.o @@ -22,7 +23,6 @@ endifcommon-obj-y += stream.ocommon-obj-y += commit.o -common-obj-y += mirror.o common-obj-y += backup.oiscsi.o-cflags := $(LIBISCSI_CFLAGS)diff --git a/qemu-img.c b/qemu-img.c index 8455994..9fe6384 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,49 @@ fail:return ret; }+struct CommonBlockJobCBInfo {+ Error **errp; + bool done;Looks unused (set, but never read).
Right, it's an artifact from an earlier version and in-between work.
+}; + +static void common_block_job_cb(void *opaque, int ret) +{ + struct CommonBlockJobCBInfo *cbi = opaque; + + if (ret < 0) { + error_setg_errno(cbi->errp, -ret, "Block job failed"); + }In practice, I guess this will give us rather bad error messages. Perhaps we need to replace 'int ret' with 'Error *errp' for block job callbacks in a followup.
Probably, yes.
+ + cbi->done = true; +} + +static void run_block_job(BlockJob *job, struct CommonBlockJobCBInfo *cbi) +{ + BlockJobInfo *info; + + do { + qemu_aio_wait(); + + info = block_job_query(job);Where does info get freed?
That, indeed, is a good question. I'll fix it, thanks.
+ + if (!info->busy && info->offset < info->len) { + block_job_resume(job); + } + } while (info->offset < info->len); + + block_job_complete_sync(job, cbi->errp); +} + +/* Same as in block.c */ +#define COMMIT_BUF_SECTORS 2048[...]+ commit_active_start(bs, base_bs, 0, COMMIT_BUF_SECTORS << BDRV_SECTOR_BITS, + BLOCKDEV_ON_ERROR_REPORT, common_block_job_cb, &cbi, + &local_err);Though bdrv_commit() uses it for a different purpose: There it's the buffer size that is used for committing. A single request can never be larger than this value, but depending on bdrv_is_allocated() it can be smaller. So the granularity for the decision whether to copy data is still the granularity of bdrv_is_allocated(), i.e. one cluster. For the mirror block job, the decision is taken on the granularity that you specify. This should be the same as for bdrv_commit(), i.e. the default that you get when you specify 0. mirror_start_job() also has a buf_size parameter, which is however not exposed by commit_active_start(). This is where COMMIT_BUF_SECTORS would be right.
Hm, interesting. I remembered trying this and it being pretty slow (test 20 and some other test images). However, I can't reproduce it any longer. I guess that means I can drop patch 1 of this series, too.
Max
[Prev in Thread] | Current Thread | [Next in Thread] |