From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39455) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XhHcX-0004Y9-2L for qemu-devel@nongnu.org; Thu, 23 Oct 2014 08:36:07 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XhHcQ-00020m-Sj for qemu-devel@nongnu.org; Thu, 23 Oct 2014 08:36:01 -0400 Received: from mx1.redhat.com ([209.132.183.28]:60572) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XhHcQ-00020g-IK for qemu-devel@nongnu.org; Thu, 23 Oct 2014 08:35:54 -0400 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s9NCZrRb027389 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Thu, 23 Oct 2014 08:35:53 -0400 Message-ID: <5448F627.2080409@redhat.com> Date: Thu, 23 Oct 2014 14:35:51 +0200 From: Max Reitz MIME-Version: 1.0 References: <1413982283-10186-1-git-send-email-mreitz@redhat.com> <1413982283-10186-9-git-send-email-mreitz@redhat.com> <20141023115916.GK3522@noname.redhat.com> In-Reply-To: <20141023115916.GK3522@noname.redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v13 08/14] qemu-img: Implement commit like QMP List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: qemu-devel@nongnu.org, Stefan Hajnoczi On 2014-10-23 at 13:59, Kevin Wolf wrote: > Am 22.10.2014 um 14:51 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. >> >> Signed-off-by: Max Reitz >> --- >> block/Makefile.objs | 3 +- >> qemu-img.c | 82 ++++++++++++++++++++++++++++++++++++++++------------- >> 2 files changed, 64 insertions(+), 21 deletions(-) >> >> diff --git a/block/Makefile.objs b/block/Makefile.objs >> index 27911b6..04b0e43 100644 >> --- a/block/Makefile.objs >> +++ b/block/Makefile.objs >> @@ -9,7 +9,7 @@ block-obj-y += block-backend.o 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 += null.o >> +block-obj-y += null.o mirror.o >> >> block-obj-y += nbd.o nbd-client.o sheepdog.o >> block-obj-$(CONFIG_LIBISCSI) += iscsi.o >> @@ -23,7 +23,6 @@ block-obj-y += accounting.o >> >> 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 09e7e72..f1f2857 100644 >> --- a/qemu-img.c >> +++ b/qemu-img.c >> @@ -31,6 +31,7 @@ >> #include "sysemu/sysemu.h" >> #include "sysemu/block-backend.h" >> #include "block/block_int.h" >> +#include "block/blockjob.h" >> #include "block/qapi.h" >> #include >> >> @@ -715,13 +716,47 @@ fail: >> return ret; >> } >> >> +typedef struct CommonBlockJobCBInfo { >> + BlockDriverState *bs; >> + Error **errp; >> +} CommonBlockJobCBInfo; >> + >> +static void common_block_job_cb(void *opaque, int ret) >> +{ >> + CommonBlockJobCBInfo *cbi = opaque; >> + >> + if (ret < 0) { >> + error_setg_errno(cbi->errp, -ret, "Block job failed"); >> + } >> + >> + /* Drop this block job's reference */ >> + bdrv_unref(cbi->bs); >> +} >> + >> +static void run_block_job(BlockJob *job, Error **errp) >> +{ >> + AioContext *aio_context = bdrv_get_aio_context(job->bs); >> + >> + do { >> + aio_poll(aio_context, true); >> + >> + if (!job->busy && !job->ready) { >> + block_job_resume(job); >> + } > I wasn't quite sure what this is for. With BLOCKDEV_ON_ERROR_REPORT, why > would the job ever be paused? I remember you telling that I puzzled this code together until it worked. At one point in time, it didn't work without this. It works now. I'm going to trust you. > While trying out what would happen with failing requests (both for > checking this and what error message I would get), my image got > corrupted (as I told you on IRC): > > $ qemu-img create -f qcow2 -b /tmp/backing.qcow2 /tmp/test.qcow2 64M > $ qemu-img commit blkdebug::/tmp/test.qcow2 > qemu-img: Could not empty blkdebug::/tmp/test.qcow2: Operation not supported > $ qemu-io -c 'write 0 1M' /tmp/test.qcow2 > qcow2: Marking image as corrupt: Preventing invalid write on metadata > (overlaps with active L1 table); further corruption events will be > suppressed > write failed: Input/output error As discussed on IRC, this is due to qcow2 marking every image clean when it's closed. Setting bs->drv to NULL solves this issue. >> + } while (!job->ready); >> + >> + block_job_complete_sync(job, errp); >> +} >> + >> static int img_commit(int argc, char **argv) >> { >> int c, ret, flags; >> const char *filename, *fmt, *cache; >> BlockBackend *blk; >> - BlockDriverState *bs; >> + BlockDriverState *bs, *base_bs; >> bool quiet = false; >> + Error *local_err = NULL; >> + CommonBlockJobCBInfo cbi; >> >> fmt = NULL; >> cache = BDRV_DEFAULT_CACHE; >> @@ -764,29 +799,38 @@ static int img_commit(int argc, char **argv) >> } >> bs = blk_bs(blk); >> >> - 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"); > $ qemu-img create -f qcow2 /tmp/test.qcow2 64M > Formatting '/tmp/test.qcow2', fmt=qcow2 size=67108864 encryption=off cluster_size=65536 lazy_refcounts=off > $ qemu-img commit /tmp/test.qcow2 > qemu-img: Base 'NULL' not found > > Do we expect any user to understand what we want to tell them? This > should clearly be something along the lines of error_setg(&local_err, > "Image doesn't have a backing file"). (Before this patch, it said > "Image is already committed", which isn't great either, but not as bad > as the new message) Yes, will fix. Max