From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:36741) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T9jCN-0007aQ-1d for qemu-devel@nongnu.org; Thu, 06 Sep 2012 17:01:16 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1T9jCL-0001qv-Jc for qemu-devel@nongnu.org; Thu, 06 Sep 2012 17:01:14 -0400 Received: from mx1.redhat.com ([209.132.183.28]:4243) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T9jCL-0001qe-B8 for qemu-devel@nongnu.org; Thu, 06 Sep 2012 17:01:13 -0400 Message-ID: <50490EFD.50107@redhat.com> Date: Thu, 06 Sep 2012 17:00:45 -0400 From: Jeff Cody MIME-Version: 1.0 References: <5048B358.1080003@redhat.com> In-Reply-To: <5048B358.1080003@redhat.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RFC v2 PATCH 6/6] QAPI: add command for live block commit, 'block-commit' Reply-To: jcody@redhat.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: supriyak@linux.vnet.ibm.com, pbonzini@redhat.com, eblake@redhat.com, qemu-devel@nongnu.org, stefanha@gmail.com On 09/06/2012 10:29 AM, Kevin Wolf wrote: > Am 30.08.2012 20:47, schrieb Jeff Cody: >> The command for live block commit is added, which has the following >> arguments: >> >> device: the block device to perform the commit on (mandatory) >> base: the base image to commit into; optional (if not specified, >> it is the underlying original image) >> top: the top image of the commit - all data from inside top down >> to base will be committed into base. optional (if not specified, >> it is the active image) - see note below >> speed: maximum speed, in bytes/sec >> >> note: eventually this will support merging down the active layer, >> but that code is not yet complete. If the active layer is passed >> in currently as top, or top is left to the default, then the error >> QERR_TOP_NOT_FOUND will be returned. >> >> The is done as a block job, so upon completion a BLOCK_JOB_COMPLETED will >> be emitted. >> >> Signed-off-by: Jeff Cody >> --- >> blockdev.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> qapi-schema.json | 30 ++++++++++++++++++++ >> qmp-commands.hx | 6 ++++ >> 3 files changed, 119 insertions(+) >> >> diff --git a/blockdev.c b/blockdev.c >> index 68d65fb..e0d6ca0 100644 >> --- a/blockdev.c >> +++ b/blockdev.c >> @@ -827,6 +827,89 @@ exit: >> return; >> } >> >> +void qmp_block_commit(const char *device, >> + bool has_base, const char *base, >> + bool has_top, const char *top, >> + bool has_speed, int64_t speed, >> + Error **errp) >> +{ >> + BlockDriverState *bs; >> + BlockDriverState *base_bs, *top_bs, *child_bs; >> + Error *local_err = NULL; >> + int orig_base_flags, orig_top_flags; >> + BlockReopenQueue *reopen_queue = NULL; >> + /* This will be part of the QMP command, if/when the >> + * BlockdevOnError change for blkmirror makes it in >> + */ >> + BlockErrorAction on_error = BLOCK_ERR_REPORT; >> + >> + /* drain all i/o before commits */ >> + bdrv_drain_all(); >> + >> + bs = bdrv_find(device); >> + if (!bs) { >> + error_set(errp, QERR_DEVICE_NOT_FOUND, device); >> + return; >> + } >> + if (base && has_base) { >> + base_bs = bdrv_find_backing_image(bs, base); >> + } else { >> + base_bs = bdrv_find_base(bs); >> + } >> + >> + if (base_bs == NULL) { >> + error_set(errp, QERR_BASE_NOT_FOUND, "NULL"); > > Shouldn't it be base rather than "NULL"? > Yes >> + return; >> + } >> + >> + if (top && has_top) { >> + /* if we want to allow the active layer, >> + * use 'bdrv_find_image()' here */ >> + top_bs = bdrv_find_backing_image(bs, top); >> + if (top_bs == NULL) { >> + error_set(errp, QERR_TOP_NOT_FOUND, top); >> + return; >> + } >> + } else { >> + /* we will eventually default to the top layer,i.e. top_bs = bs */ >> + error_set(errp, QERR_TOP_NOT_FOUND, top); >> + return; >> + } >> + >> + child_bs = bdrv_find_child(bs, top_bs); >> + >> + orig_base_flags = bdrv_get_flags(base_bs); /* what we are writing into */ >> + orig_top_flags = bdrv_get_flags(child_bs); /* to change the backing file */ >> + >> + /* convert base_bs to r/w, if necessary */ >> + if (!(orig_base_flags & BDRV_O_RDWR)) { >> + reopen_queue = bdrv_reopen_queue(reopen_queue, base_bs, >> + orig_base_flags | BDRV_O_RDWR); >> + } >> + if (!(orig_top_flags & BDRV_O_RDWR)) { >> + reopen_queue = bdrv_reopen_queue(reopen_queue, base_bs, >> + orig_base_flags | BDRV_O_RDWR); > > I think you mean child_bs and orig_top_flags. (Why isn't it called > orig_child_flags?) > Yes, thanks - and (per your comments on the previous patch), I'll move the reopen bits into commit_start(). >> + } >> + if (reopen_queue) { >> + bdrv_reopen_multiple(reopen_queue, &local_err); >> + if (local_err != NULL) { >> + error_propagate(errp, local_err); >> + return; >> + } >> + } >> + >> + commit_start(bs, base_bs, top_bs, speed, on_error, >> + block_job_cb, bs, orig_base_flags, orig_top_flags, >> + &local_err); >> + if (local_err != NULL) { >> + error_propagate(errp, local_err); >> + return; >> + } >> + /* Grab a reference so hotplug does not delete the BlockDriverState from >> + * underneath us. >> + */ >> + drive_get_ref(drive_get_by_blockdev(bs)); >> +} >> >> static void eject_device(BlockDriverState *bs, int force, Error **errp) >> { >> diff --git a/qapi-schema.json b/qapi-schema.json >> index bd8ad74..45feda6 100644 >> --- a/qapi-schema.json >> +++ b/qapi-schema.json >> @@ -1401,6 +1401,36 @@ >> 'returns': 'str' } >> >> ## >> +# @block-commit >> +# >> +# Live commit of data from child image nodes into parent nodes - i.e., >> +# writes data between 'top' and 'base' into 'base'. >> +# >> +# @device: the name of the device >> +# >> +# @base: #optional The parent image of the device to write data into. >> +# If not specified, this is the original parent image. > > "The file name of the parent image", actually. > > Which I'm afraid means that this isn't an API for the long term, but > there's little we can do about it now... > >> +# >> +# @top: #optional The child image, above which data will not be committed >> +# down. If not specified, this is the active layer. > > Again, the file name. > > Kevin >