qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v9 3/4] block: Add replaces argument to drive-mi


From: Max Reitz
Subject: Re: [Qemu-devel] [PATCH v9 3/4] block: Add replaces argument to drive-mirror
Date: Sat, 14 Jun 2014 01:59:32 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.6.0

On 11.06.2014 15:24, Benoît Canet wrote:
drive-mirror will bdrv_swap the new BDS named node-name with the one
pointed by replaces when the mirroring is finished.

Signed-off-by: Benoit Canet <address@hidden>
---
  block.c                   | 17 ++++++++++++++
  block/mirror.c            | 60 +++++++++++++++++++++++++++++++++++++----------
  blockdev.c                | 30 +++++++++++++++++++++++-
  hmp.c                     |  2 +-
  include/block/block.h     |  4 ++++
  include/block/block_int.h |  4 +++-
  qapi/block-core.json      |  6 ++++-
  qmp-commands.hx           |  4 +++-
  8 files changed, 109 insertions(+), 18 deletions(-)

diff --git a/block.c b/block.c
index 17f763d..318f1e6 100644
--- a/block.c
+++ b/block.c
@@ -5795,3 +5795,20 @@ bool bdrv_is_first_non_filter(BlockDriverState 
*candidate)
return false;
  }
+
+BlockDriverState *check_to_replace_node(const char *node_name, Error **errp)
+{
+    BlockDriverState *to_replace_bs = bdrv_find_node(node_name);
+    if (!to_replace_bs) {
+        error_setg(errp, "Node name '%s' not found",
+                   node_name);
+        return NULL;
+    }
+
+    if (bdrv_op_is_blocked(to_replace_bs, BLOCK_OP_TYPE_REPLACE, errp)) {
+        return NULL;
+    }
+
+    return to_replace_bs;
+}
+
diff --git a/block/mirror.c b/block/mirror.c
index 94c8661..b00365f 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -32,6 +32,12 @@ typedef struct MirrorBlockJob {
      RateLimit limit;
      BlockDriverState *target;
      BlockDriverState *base;
+    /* The name of the graph node to replace */
+    char *replaces;
+    /* The block BDS to replace */

Maybe s/block //

+    BlockDriverState *to_replace;
+    /* Used to block operations on the drive-mirror-replace target */
+    Error *replace_blocker;
      bool is_none_mode;
      BlockdevOnError on_source_error, on_target_error;
      bool synced;
@@ -490,10 +496,14 @@ immediate_exit:
      bdrv_release_dirty_bitmap(bs, s->dirty_bitmap);
      bdrv_iostatus_disable(s->target);
      if (s->should_complete && ret == 0) {
-        if (bdrv_get_flags(s->target) != bdrv_get_flags(s->common.bs)) {
-            bdrv_reopen(s->target, bdrv_get_flags(s->common.bs), NULL);
+        BlockDriverState *to_replace = s->common.bs;
+        if (s->to_replace) {
+            to_replace = s->to_replace;
          }
-        bdrv_swap(s->target, s->common.bs);
+        if (bdrv_get_flags(s->target) != bdrv_get_flags(to_replace)) {
+            bdrv_reopen(s->target, bdrv_get_flags(to_replace), NULL);
+        }
+        bdrv_swap(s->target, to_replace);
          if (s->common.driver->job_type == BLOCK_JOB_TYPE_COMMIT) {
              /* drop the bs loop chain formed by the swap: break the loop then
               * trigger the unref from the top one */
@@ -502,6 +512,12 @@ immediate_exit:
              bdrv_unref(p);
          }
      }
+    if (s->to_replace) {
+        bdrv_op_unblock_all(s->to_replace, s->replace_blocker);
+        error_free(s->replace_blocker);
+        bdrv_unref(s->to_replace);
+        g_free(s->replaces);

If we get here without mirror_complete() having been invoked (which is possible, as far as I know), s->to_replace will still be NULL while s->replaces may be not. In that case, s->replaces will be leaked.

+    }
      bdrv_unref(s->target);
      block_job_completed(&s->common, ret);
  }
@@ -540,6 +556,20 @@ static void mirror_complete(BlockJob *job, Error **errp)
          return;
      }
+ /* check the target bs is not block and block all operations on it */

s/block/blocked/

+    if (s->replaces) {
+        s->to_replace = check_to_replace_node(s->replaces, errp);
+
+        if (!s->to_replace) {
+            return;
+        }
+
+        error_setg(&s->replace_blocker,
+                   "block device is in use by block-job-complete");
+        bdrv_op_block_all(s->to_replace, s->replace_blocker);
+        bdrv_ref(s->to_replace);
+    }
+
      s->should_complete = true;
      block_job_resume(job);
  }
@@ -562,14 +592,15 @@ static const BlockJobDriver commit_active_job_driver = {
  };
static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
-                            int64_t speed, int64_t granularity,
-                            int64_t buf_size,
-                            BlockdevOnError on_source_error,
-                            BlockdevOnError on_target_error,
-                            BlockDriverCompletionFunc *cb,
-                            void *opaque, Error **errp,
-                            const BlockJobDriver *driver,
-                            bool is_none_mode, BlockDriverState *base)
+                             const char *replaces,
+                             int64_t speed, int64_t granularity,
+                             int64_t buf_size,
+                             BlockdevOnError on_source_error,
+                             BlockdevOnError on_target_error,
+                             BlockDriverCompletionFunc *cb,
+                             void *opaque, Error **errp,
+                             const BlockJobDriver *driver,
+                             bool is_none_mode, BlockDriverState *base)
  {
      MirrorBlockJob *s;
@@ -600,6 +631,7 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
          return;
      }
+ s->replaces = g_strdup(replaces);
      s->on_source_error = on_source_error;
      s->on_target_error = on_target_error;
      s->target = target;
@@ -621,6 +653,7 @@ static void mirror_start_job(BlockDriverState *bs, 
BlockDriverState *target,
  }
void mirror_start(BlockDriverState *bs, BlockDriverState *target,
+                  const char *replaces,
                    int64_t speed, int64_t granularity, int64_t buf_size,
                    MirrorSyncMode mode, BlockdevOnError on_source_error,
                    BlockdevOnError on_target_error,
@@ -632,7 +665,8 @@ void mirror_start(BlockDriverState *bs, BlockDriverState 
*target,
is_none_mode = mode == MIRROR_SYNC_MODE_NONE;
      base = mode == MIRROR_SYNC_MODE_TOP ? bs->backing_hd : NULL;
-    mirror_start_job(bs, target, speed, granularity, buf_size,
+    mirror_start_job(bs, target, replaces,
+                     speed, granularity, buf_size,
                       on_source_error, on_target_error, cb, opaque, errp,
                       &mirror_job_driver, is_none_mode, base);
  }
@@ -680,7 +714,7 @@ void commit_active_start(BlockDriverState *bs, 
BlockDriverState *base,
      }
bdrv_ref(base);
-    mirror_start_job(bs, base, speed, 0, 0,
+    mirror_start_job(bs, base, NULL, speed, 0, 0,
                       on_error, on_error, cb, opaque, &local_err,
                       &commit_active_job_driver, false, base);
      if (local_err) {
diff --git a/blockdev.c b/blockdev.c
index 06b14f2..237a548 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2107,6 +2107,7 @@ BlockDeviceInfoList *qmp_query_named_block_nodes(Error 
**errp)
  void qmp_drive_mirror(const char *device, const char *target,
                        bool has_format, const char *format,
                        bool has_node_name, const char *node_name,
+                      bool has_replaces, const char *replaces,
                        enum MirrorSyncMode sync,
                        bool has_mode, enum NewImageMode mode,
                        bool has_speed, int64_t speed,
@@ -2194,6 +2195,28 @@ void qmp_drive_mirror(const char *device, const char 
*target,
          return;
      }
+ if (has_replaces) {
+        BlockDriverState *to_replace_bs;
+
+        if (!has_node_name) {
+            error_setg(errp, "a node-name must be provided when replacing a"
+                             " named node of the graph");
+            return;
+        }
+
+        to_replace_bs = check_to_replace_node(replaces, errp);
+
+        if (!to_replace_bs) {
+            return;
+        }
+
+        if (size != bdrv_getlength(to_replace_bs)) {
+            error_setg(errp, "cannot replace image with a mirror image of "
+                             "different size");
+            return;
+        }
+    }
+
      if ((sync == MIRROR_SYNC_MODE_FULL || !source)
          && mode != NEW_IMAGE_MODE_EXISTING)
      {
@@ -2238,7 +2261,12 @@ void qmp_drive_mirror(const char *device, const char 
*target,
          return;
      }
- mirror_start(bs, target_bs, speed, granularity, buf_size, sync,
+    /* pass the node name to replace to mirror start since it's loose coupling
+     * and will allow to check whether the node still exist at mirror 
completion
+     */
+    mirror_start(bs, target_bs,
+                 has_replaces ? replaces : NULL,
+                 speed, granularity, buf_size, sync,
                   on_source_error, on_target_error,
                   block_job_cb, bs, &local_err);
      if (local_err != NULL) {
diff --git a/hmp.c b/hmp.c
index ef0d583..9a4b8da 100644
--- a/hmp.c
+++ b/hmp.c
@@ -930,7 +930,7 @@ void hmp_drive_mirror(Monitor *mon, const QDict *qdict)
      }
qmp_drive_mirror(device, filename, !!format, format,
-                     false, NULL,
+                     false, NULL, false, NULL,
                       full ? MIRROR_SYNC_MODE_FULL : MIRROR_SYNC_MODE_TOP,
                       true, mode, false, 0, false, 0, false, 0,
                       false, 0, false, 0, &err);
diff --git a/include/block/block.h b/include/block/block.h
index 7d86e29..b0ed701 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -179,6 +179,7 @@ typedef enum BlockOpType {
      BLOCK_OP_TYPE_MIRROR,
      BLOCK_OP_TYPE_RESIZE,
      BLOCK_OP_TYPE_STREAM,
+    BLOCK_OP_TYPE_REPLACE,
      BLOCK_OP_TYPE_MAX,
  } BlockOpType;
@@ -319,6 +320,9 @@ bool bdrv_recurse_is_first_non_filter(BlockDriverState *bs,
                                        BlockDriverState *candidate);
  bool bdrv_is_first_non_filter(BlockDriverState *candidate);
+/* check if a named node can be replaced when doing drive-mirror */
+BlockDriverState *check_to_replace_node(const char *node_name, Error **errp);
+
  /* async block I/O */
  typedef void BlockDriverDirtyHandler(BlockDriverState *bs, int64_t sector,
                                       int sector_num);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 8d58334..fdf3ed2 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -86,7 +86,6 @@ struct BlockDriver {
       */
      bool (*bdrv_recurse_is_first_non_filter)(BlockDriverState *bs,
                                               BlockDriverState *candidate);
-
      int (*bdrv_probe)(const uint8_t *buf, int buf_size, const char *filename);
      int (*bdrv_probe_device)(const char *filename);

This hunk doesn't seem intended...?


Other than these, the patch looks fine to me. If you fix the leakage of s->replaces (or explain why mirror_complete() is always invoked before immediate_exit):

Reviewed-by: Max Reitz <address@hidden>



reply via email to

[Prev in Thread] Current Thread [Next in Thread]