diff mbox

[RFC,v2,6/6] QAPI: add command for live block commit, 'block-commit'

Message ID dc69616e2722b0819fa40fd13f8c959838521391.1346351079.git.jcody@redhat.com
State New
Headers show

Commit Message

Jeff Cody Aug. 30, 2012, 6:47 p.m. UTC 
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 <jcody@redhat.com>
---
 blockdev.c       | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 qapi-schema.json | 30 ++++++++++++++++++++
 qmp-commands.hx  |  6 ++++
 3 files changed, 119 insertions(+)

Comments

Eric Blake Aug. 30, 2012, 11:06 p.m. UTC | #1 
On 08/30/2012 11:47 AM, Jeff Cody wrote:
> 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.

Likewise, the job can be canceled, and it is possible to track progress
of the job or change the speed on the fly, using existing block job
commands.

Will the BLOCK_JOB_COMPLETED event have a new category listing that it
was a commit job instead of a stream job that completed?  That is, I
think this patch is incomplete, and that you also need to modify
QMP/qmp-events.txt to modify the 'type' field of
BLOCK_JOB_{CANCELLED,COMPLETED} to distinguish this new sub-type of event.

>  
>  ##
> +# @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.
> +#
> +# @top:    #optional The child image, above which data will not be committed
> +#                    down.  If not specified, this is the active layer.
> +#
> +# @speed:  #optional the maximum speed, in bytes per second
> +#
> +# Returns: Nothing on success
> +#          If commit or stream is already active on this device, DeviceInUse
> +#          If @device does not exist, DeviceNotFound
> +#          If image commit is not supported by this device, NotSupported
> +#          If @base does not exist, BaseNotFound
> +#          If @top does not exist, TopNotFound
> +#          If @speed is invalid, InvalidParameter
> +#
> +# Since: 1.2

1.3
Jeff Cody Aug. 31, 2012, 2:42 p.m. UTC | #2 
On 08/30/2012 07:06 PM, Eric Blake wrote:
> On 08/30/2012 11:47 AM, Jeff Cody wrote:
>> 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.
> 
> Likewise, the job can be canceled, and it is possible to track progress
> of the job or change the speed on the fly, using existing block job
> commands.

Correct.

> 
> Will the BLOCK_JOB_COMPLETED event have a new category listing that it
> was a commit job instead of a stream job that completed?  That is, I
> think this patch is incomplete, and that you also need to modify
> QMP/qmp-events.txt to modify the 'type' field of
> BLOCK_JOB_{CANCELLED,COMPLETED} to distinguish this new sub-type of event.
>

You are right, I should add the sub-type info into QMP/qmp-events.txt; the
type is 'commit'.

Here is what the BLOCK_JOB_COMPLETED event looks like for commit:

{"timestamp": {"seconds": 1346423249, "microseconds": 551295},
  "event": "BLOCK_JOB_COMPLETED",
  "data": {"device": "virtio0", "len": 6442450944, "offset": 6442450944,
            "speed": 140732751976032, "type": "commit"}}


>>  
>>  ##
>> +# @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.
>> +#
>> +# @top:    #optional The child image, above which data will not be committed
>> +#                    down.  If not specified, this is the active layer.
>> +#
>> +# @speed:  #optional the maximum speed, in bytes per second
>> +#
>> +# Returns: Nothing on success
>> +#          If commit or stream is already active on this device, DeviceInUse
>> +#          If @device does not exist, DeviceNotFound
>> +#          If image commit is not supported by this device, NotSupported
>> +#          If @base does not exist, BaseNotFound
>> +#          If @top does not exist, TopNotFound
>> +#          If @speed is invalid, InvalidParameter
>> +#
>> +# Since: 1.2
> 
> 1.3
>
Kevin Wolf Sept. 6, 2012, 2:29 p.m. UTC | #3 
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 <jcody@redhat.com>
> ---
>  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"?

> +        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?)

> +    }
> +    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
Jeff Cody Sept. 6, 2012, 9 p.m. UTC | #4 
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 <jcody@redhat.com>
>> ---
>>  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
>
diff mbox

Patch

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");
+        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);
+    }
+    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.
+#
+# @top:    #optional The child image, above which data will not be committed
+#                    down.  If not specified, this is the active layer.
+#
+# @speed:  #optional the maximum speed, in bytes per second
+#
+# Returns: Nothing on success
+#          If commit or stream is already active on this device, DeviceInUse
+#          If @device does not exist, DeviceNotFound
+#          If image commit is not supported by this device, NotSupported
+#          If @base does not exist, BaseNotFound
+#          If @top does not exist, TopNotFound
+#          If @speed is invalid, InvalidParameter
+#
+# Since: 1.2
+#
+##
+{ 'command': 'block-commit',
+  'data': { 'device': 'str', '*base': 'str', '*top': 'str',
+            '*speed': 'int' } }
+
 # @migrate_cancel
 #
 # Cancel the current executing migration process.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 3745a21..9292877 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -767,6 +767,12 @@  EQMP
     },
 
     {
+        .name       = "block-commit",
+        .args_type  = "device:B,base:s?,top:s?,speed:o?",
+        .mhandler.cmd_new = qmp_marshal_input_block_commit,
+    },
+
+    {
         .name       = "block-job-set-speed",
         .args_type  = "device:B,speed:o",
         .mhandler.cmd_new = qmp_marshal_input_block_job_set_speed,