Patchwork [12/30] QAPI: add command for live block commit, 'block-commit'

login
register
mail settings
Submitter Kevin Wolf
Date Sept. 28, 2012, 5:56 p.m.
Message ID <1348855033-17174-13-git-send-email-kwolf@redhat.com>
Download mbox | patch
Permalink /patch/187886/
State New
Headers show

Comments

Kevin Wolf - Sept. 28, 2012, 5:56 p.m.
From: Jeff Cody <jcody@redhat.com>

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 (mandatory for now; see
        note, below)

speed:  maximum speed, in bytes/sec

Note: Eventually this command will support merging down the active layer,
      but that code is not yet complete.  If the active layer is passed
      in as top, then an error will be returned.  Once merging down the
      active layer is supported, the 'top' argument may become optional,
      and default to the active layer.

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>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 QMP/qmp-events.txt |    6 +++-
 blockdev.c         |   58 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 qapi-schema.json   |   34 ++++++++++++++++++++++++++++++
 qmp-commands.hx    |    6 +++++
 4 files changed, 102 insertions(+), 2 deletions(-)
Eric Blake - Oct. 5, 2012, 5:29 p.m.
On 09/28/2012 11:56 AM, Kevin Wolf wrote:
> From: Jeff Cody <jcody@redhat.com>
> 
> 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 (mandatory for now; see
>         note, below)

We will need a followup patch, for this to work on chains with relative
backing file names.


> +    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, base ? base : "NULL");
> +        return;
> +    }
> +
> +    /* default top_bs is the active layer */
> +    top_bs = bs;
> +
> +    if (top) {
> +        if (strcmp(bs->filename, top) != 0) {
> +            top_bs = bdrv_find_backing_image(bs, top);
> +        }
> +    }

Right now, if I have 'base' <- 'snap1'(base) <- 'snap2'(snap1) <-
'active'(snap2), then base_bs and top_bs will be found, but later on in
commit_start, we have:

> 
> +    /* top and base may be valid, but let's make sure that base is reachable
> +     * from top */
> +    if (bdrv_find_backing_image(top, base->filename) != base) {
> +        error_setg(errp,
> +                   "Base (%s) is not reachable from top (%s)",
> +                   base->filename, top->filename);
> +        return;
> +    }

which uses the absolute file name base->filename and fails to find base,
making it impossible to commit into a base file that was referenced via
relative name in the chain.  I think that bdrv_find_backing_image needs
to canonicalize any relative name passed in on input relative to the
directory of the bs where it is starting the search, and then search for
absolute name matches rather than the current approach of searching for
exact string matches (even if the exact string is a relative name).

Also, consider this case of mixed relative and absolute backing files in
the chain:

/dir1/base <- /dir1/file1(base) <- /dir2/base(/dir1/file1) <-
/dir2/file2(base)

The relative name 'base' appears twice in the chain, so you either have
to declare it ambiguous, or else declare that relative names are
canonicalized relative to the starting point (such that
bdrv_find_backing_image(/dir1/file1, "base") gives a different result
than bdrv_find_backing_image(/dir2/file2, "base").  Which means, if I
request block-commit "top":"/dir1/file1", "base":"base", am I requesting
a commit into /dir1/base (good) or into /dir2/base (swapped arguments)?

Fortunately, it looks like if I have 'base' <- 'snap1'(/path/to/base) <-
'snap2'(/path/to/snap1) <- 'active'(/path/to/snap2), then things work
for committing snap2 into snap1, when I specify absolute file names for
top and base.  But here, it would be convenient if I could pass names
relative to the location of active and have them canonicalized into
absolute, so the same fix for relative chains will make absolute chains
easier to use.

Sounds like we need more tests to cover these scenarios.
Eric Blake - Oct. 5, 2012, 6:05 p.m.
On 10/05/2012 11:29 AM, Eric Blake wrote:

> 
> which uses the absolute file name base->filename and fails to find base,
> making it impossible to commit into a base file that was referenced via
> relative name in the chain.  I think that bdrv_find_backing_image needs
> to canonicalize any relative name passed in on input relative to the
> directory of the bs where it is starting the search, and then search for
> absolute name matches rather than the current approach of searching for
> exact string matches (even if the exact string is a relative name).
> 
> Also, consider this case of mixed relative and absolute backing files in
> the chain:
> 
> /dir1/base <- /dir1/file1(base) <- /dir2/base(/dir1/file1) <-
> /dir2/file2(base)
> 
> The relative name 'base' appears twice in the chain, so you either have
> to declare it ambiguous, or else declare that relative names are
> canonicalized relative to the starting point (such that
> bdrv_find_backing_image(/dir1/file1, "base") gives a different result
> than bdrv_find_backing_image(/dir2/file2, "base").  Which means, if I
> request block-commit "top":"/dir1/file1", "base":"base", am I requesting
> a commit into /dir1/base (good) or into /dir2/base (swapped arguments)?

Related question:

/dir/base <- /dir/snap1(base) <- /dir/active(/dir/snap1)

If I commit snap1 into base, should /dir/active encode the backing file
as '/dir/base' (keep things absolute, since /dir/active had already been
absolute) or as 'base' (relative, since the part of the chain previously
referring to base was using relative).  And if we store relative names,
we need to make sure that the relative name stored in /dir/active
correctly points to the correct file name, even if the ultimate base
file lives in a different directory (similar problems exist with
relative chains during block pull).

I'm wondering if this means 'block-stream' and 'block-commit' both need
to add @mode:#optional arguments, similar to Paolo's 'drive-mirror'
command [1], so that the user can request 'absolute-paths' vs.
'relative-paths' as the mode for which of two file names will be written
as the backing file at the end of the operation.

[1] https://lists.gnu.org/archive/html/qemu-devel/2012-09/msg04592.html
Paolo Bonzini - Oct. 8, 2012, 2:37 p.m.
Il 05/10/2012 20:05, Eric Blake ha scritto:
> 
> If I commit snap1 into base, should /dir/active encode the backing file
> as '/dir/base' (keep things absolute, since /dir/active had already been
> absolute) or as 'base' (relative, since the part of the chain previously
> referring to base was using relative).  And if we store relative names,
> we need to make sure that the relative name stored in /dir/active
> correctly points to the correct file name, even if the ultimate base
> file lives in a different directory (similar problems exist with
> relative chains during block pull).
> 
> I'm wondering if this means 'block-stream' and 'block-commit' both need
> to add @mode:#optional arguments, similar to Paolo's 'drive-mirror'
> command [1], so that the user can request 'absolute-paths' vs.
> 'relative-paths' as the mode for which of two file names will be written
> as the backing file at the end of the operation.

Yes, everything that calls bdrv_change_backing_file should have such a
@mode argument.  Good point!

Paolo

> [1] https://lists.gnu.org/archive/html/qemu-devel/2012-09/msg04592.html
Eric Blake - Oct. 11, 2012, 3:42 p.m.
On 09/28/2012 11:56 AM, Kevin Wolf wrote:
> From: Jeff Cody <jcody@redhat.com>
> 
> The command for live block commit is added, which has the following
> arguments:
> 

> +    /* default top_bs is the active layer */
> +    top_bs = bs;
> +
> +    if (top) {
> +        if (strcmp(bs->filename, top) != 0) {
> +            top_bs = bdrv_find_backing_image(bs, top);
> +        }
> +    }

In light of Jeff's followup patches to fix bdrv_find_backing_image when
dealing with relative file names, I see another bug here (but impact is
limited to only a poor-quality error message, claiming that 'top was not
found' instead of the intended 'commit of an active top is not
supported').  That is, strcmp() on user-supplied 'top' is not guaranteed
to succeed if the user provided a different spelling for the same file
as was actually recorded in bs->filename, and since you have resorted to
realpath() before strcmp() in bdrv_find_backing_image to cope with
alternat user spellings, I think you also need to use realpath() before
comparison here (probably a new helper function bdrv_is_equal() or some
such name, that compares a user-supplied name with a bs).  Saving this
until a followup patch as part of your phase 2 series to add active
image commit support is fine by me.

Patch

diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt
index 2878058..4491020 100644
--- a/QMP/qmp-events.txt
+++ b/QMP/qmp-events.txt
@@ -50,7 +50,8 @@  Emitted when a block job has been cancelled.
 
 Data:
 
-- "type":     Job type ("stream" for image streaming, json-string)
+- "type":     Job type (json-string; "stream" for image streaming
+                                     "commit" for block commit)
 - "device":   Device name (json-string)
 - "len":      Maximum progress value (json-int)
 - "offset":   Current progress value (json-int)
@@ -73,7 +74,8 @@  Emitted when a block job has completed.
 
 Data:
 
-- "type":     Job type ("stream" for image streaming, json-string)
+- "type":     Job type (json-string; "stream" for image streaming
+                                     "commit" for block commit)
 - "device":   Device name (json-string)
 - "len":      Maximum progress value (json-int)
 - "offset":   Current progress value (json-int)
diff --git a/blockdev.c b/blockdev.c
index f910ac5..cea22e4 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1131,6 +1131,64 @@  void qmp_block_stream(const char *device, bool has_base,
     trace_qmp_block_stream(bs, bs->job);
 }
 
+void qmp_block_commit(const char *device,
+                      bool has_base, const char *base, const char *top,
+                      bool has_speed, int64_t speed,
+                      Error **errp)
+{
+    BlockDriverState *bs;
+    BlockDriverState *base_bs, *top_bs;
+    Error *local_err = 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, base ? base : "NULL");
+        return;
+    }
+
+    /* default top_bs is the active layer */
+    top_bs = bs;
+
+    if (top) {
+        if (strcmp(bs->filename, top) != 0) {
+            top_bs = bdrv_find_backing_image(bs, top);
+        }
+    }
+
+    if (top_bs == NULL) {
+        error_setg(errp, "Top image file %s not found", top ? top : "NULL");
+        return;
+    }
+
+    commit_start(bs, base_bs, top_bs, speed, on_error, block_job_cb, bs,
+                &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 BlockJob *find_block_job(const char *device)
 {
     BlockDriverState *bs;
diff --git a/qapi-schema.json b/qapi-schema.json
index 14e4419..5816545 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1468,6 +1468,40 @@ 
   'returns': 'str' }
 
 ##
+# @block-commit
+#
+# Live commit of data from overlay image nodes into backing nodes - i.e.,
+# writes data between 'top' and 'base' into 'base'.
+#
+# @device:  the name of the device
+#
+# @base:   #optional The file name of the backing image to write data into.
+#                    If not specified, this is the deepest backing image
+#
+# @top:              The file name of the backing image within the image chain,
+#                    which contains the topmost data to be committed down.
+#                    Note, the active layer as 'top' is currently unsupported.
+#
+#                    If top == base, that is an error.
+#
+#
+# @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 or @top is invalid, a generic error is returned
+#          If @top is the active layer, or omitted, a generic error is returned
+#          If @speed is invalid, InvalidParameter
+#
+# Since: 1.3
+#
+##
+{ '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 6e21ddb..a55a3f5 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -792,6 +792,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,