| Message ID | 1396961442-24046-4-git-send-email-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
Am 08.04.2014 um 14:50 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. Leaves us with the HMP commit command that uses the old bdrv_commit() function. I wonder if we can get rid of it by letting the HMP command stop the VM, do a live commit, and then restart the VM. > 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. In fact, I think since qcow2 has discard support it would actually be possible to write a sensible implementation of bdrv_make_empty(). That's a separate feature, though, and can go in a different patch series. > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > block/Makefile.objs | 2 +- > qemu-img.c | 70 ++++++++++++++++++++++++++++++++++++++--------------- > 2 files changed, 52 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..e86911f 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) > +{ > +} Why don't we need to check the return value? Kevin
On 04/08/2014 09:14 AM, Kevin Wolf wrote: > Am 08.04.2014 um 14:50 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. > > Leaves us with the HMP commit command that uses the old bdrv_commit() > function. I wonder if we can get rid of it by letting the HMP command > stop the VM, do a live commit, and then restart the VM. Not a bad idea, actually. Of course, as additional patches.
On 08.04.2014 17:14, Kevin Wolf wrote: > Am 08.04.2014 um 14:50 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. > Leaves us with the HMP commit command that uses the old bdrv_commit() > function. I wonder if we can get rid of it by letting the HMP command > stop the VM, do a live commit, and then restart the VM. > >> 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. > In fact, I think since qcow2 has discard support it would actually be > possible to write a sensible implementation of bdrv_make_empty(). That's > a separate feature, though, and can go in a different patch series. > >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> block/Makefile.objs | 2 +- >> qemu-img.c | 70 ++++++++++++++++++++++++++++++++++++++--------------- >> 2 files changed, 52 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..e86911f 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) >> +{ >> +} > Why don't we need to check the return value? I didn't check it, because it was not called – which apparently didn't sound strange to me at all. I checked and the much more interesting fact is that I assumed block_job_complete() would actually complete the block job without any further need for aio_poll(); but it doesn't. I'll fix both things (calling aio_poll() until the CB is called and checking the return value). Max > Kevin
Am 10.04.2014 um 16:32 hat Max Reitz geschrieben: > On 08.04.2014 17:14, Kevin Wolf wrote: > >Am 08.04.2014 um 14:50 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. > >Leaves us with the HMP commit command that uses the old bdrv_commit() > >function. I wonder if we can get rid of it by letting the HMP command > >stop the VM, do a live commit, and then restart the VM. > > > >>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. > >In fact, I think since qcow2 has discard support it would actually be > >possible to write a sensible implementation of bdrv_make_empty(). That's > >a separate feature, though, and can go in a different patch series. > > > >>Signed-off-by: Max Reitz <mreitz@redhat.com> > >>--- > >> block/Makefile.objs | 2 +- > >> qemu-img.c | 70 ++++++++++++++++++++++++++++++++++++++--------------- > >> 2 files changed, 52 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..e86911f 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) > >>+{ > >>+} > >Why don't we need to check the return value? > > I didn't check it, because it was not called – which apparently > didn't sound strange to me at all. I checked and the much more > interesting fact is that I assumed block_job_complete() would > actually complete the block job without any further need for > aio_poll(); but it doesn't. I'll fix both things (calling aio_poll() > until the CB is called and checking the return value). There is a block_job_cancel_sync(). Should we add a new block_job_complete_sync() instead of adding the polling logic to qemu-img? Kevin
On 11.04.2014 14:54, Kevin Wolf wrote: > Am 10.04.2014 um 16:32 hat Max Reitz geschrieben: >> On 08.04.2014 17:14, Kevin Wolf wrote: >>> Am 08.04.2014 um 14:50 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. >>> Leaves us with the HMP commit command that uses the old bdrv_commit() >>> function. I wonder if we can get rid of it by letting the HMP command >>> stop the VM, do a live commit, and then restart the VM. >>> >>>> 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. >>> In fact, I think since qcow2 has discard support it would actually be >>> possible to write a sensible implementation of bdrv_make_empty(). That's >>> a separate feature, though, and can go in a different patch series. >>> >>>> Signed-off-by: Max Reitz <mreitz@redhat.com> >>>> --- >>>> block/Makefile.objs | 2 +- >>>> qemu-img.c | 70 ++++++++++++++++++++++++++++++++++++++--------------- >>>> 2 files changed, 52 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..e86911f 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) >>>> +{ >>>> +} >>> Why don't we need to check the return value? >> I didn't check it, because it was not called – which apparently >> didn't sound strange to me at all. I checked and the much more >> interesting fact is that I assumed block_job_complete() would >> actually complete the block job without any further need for >> aio_poll(); but it doesn't. I'll fix both things (calling aio_poll() >> until the CB is called and checking the return value). > There is a block_job_cancel_sync(). Should we add a new > block_job_complete_sync() instead of adding the polling logic to > qemu-img? I don't see the immediate need for it, as we still need the polling logic while running the block job. However, if there is a block_job_cancel_sync() (as you said), it would make sense to introduce the same for "complete" and reuse it here. I'll send a v4, then. Max
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..e86911f 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; @@ -728,29 +754,35 @@ 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; + + /* This is different from QMP, which by default uses the deepest file in the + * backing chain (i.e., the very base); however, the traditional behavior of + * qemu-img commit is using the immediate backing file. */ + base_bs = bs->backing_hd; + 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 (local_err) { + goto done; + } + + run_block_job(bs->job, &local_err); + +done: bdrv_unref(bs); - if (ret) { + + if (local_err) { + qerror_report_err(local_err); + error_free(local_err); return 1; } + + qprintf(quiet, "Image committed.\n"); return 0; }
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 | 70 ++++++++++++++++++++++++++++++++++++++--------------- 2 files changed, 52 insertions(+), 20 deletions(-)