qemu-devel
[Top][All Lists]
Advanced

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

Re: [Qemu-devel] [PATCH v5 10/21] qmp: Add support of "dirty-bitmap" syn


From: Max Reitz
Subject: Re: [Qemu-devel] [PATCH v5 10/21] qmp: Add support of "dirty-bitmap" sync mode for drive-backup
Date: Fri, 17 Apr 2015 15:17:40 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0

On 09.04.2015 00:19, John Snow wrote:
For "dirty-bitmap" sync mode, the block job will iterate through the
given dirty bitmap to decide if a sector needs backup (backup all the
dirty clusters and skip clean ones), just as allocation conditions of
"top" sync mode.

Signed-off-by: Fam Zheng <address@hidden>
Signed-off-by: John Snow <address@hidden>
---
  block.c                   |   9 +++
  block/backup.c            | 156 +++++++++++++++++++++++++++++++++++++++-------
  block/mirror.c            |   4 ++
  blockdev.c                |  18 +++++-
  hmp.c                     |   3 +-
  include/block/block.h     |   1 +
  include/block/block_int.h |   2 +
  qapi/block-core.json      |  13 ++--
  qmp-commands.hx           |   7 ++-
  9 files changed, 180 insertions(+), 33 deletions(-)

diff --git a/block.c b/block.c
index 9d30379..2367311 100644
--- a/block.c
+++ b/block.c
@@ -5717,6 +5717,15 @@ static void bdrv_reset_dirty(BlockDriverState *bs, 
int64_t cur_sector,
      }
  }
+/**
+ * Advance an HBitmapIter to an arbitrary offset.
+ */
+void bdrv_set_dirty_iter(HBitmapIter *hbi, int64_t offset)
+{
+    assert(hbi->hb);
+    hbitmap_iter_init(hbi, hbi->hb, offset);
+}
+
  int64_t bdrv_get_dirty_count(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
  {
      return hbitmap_count(bitmap->bitmap);
diff --git a/block/backup.c b/block/backup.c
index 1c535b1..8513917 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -37,6 +37,8 @@ typedef struct CowRequest {
  typedef struct BackupBlockJob {
      BlockJob common;
      BlockDriverState *target;
+    /* bitmap for sync=dirty-bitmap */
+    BdrvDirtyBitmap *sync_bitmap;
      MirrorSyncMode sync_mode;
      RateLimit limit;
      BlockdevOnError on_source_error;
@@ -242,6 +244,92 @@ static void backup_complete(BlockJob *job, void *opaque)
      g_free(data);
  }
+static bool coroutine_fn yield_and_check(BackupBlockJob *job)
+{
+    if (block_job_is_cancelled(&job->common)) {
+        return true;
+    }
+
+    /* we need to yield so that qemu_aio_flush() returns.
+     * (without, VM does not reboot)
+     */
+    if (job->common.speed) {
+        uint64_t delay_ns = ratelimit_calculate_delay(&job->limit,
+                                                      job->sectors_read);
+        job->sectors_read = 0;
+        block_job_sleep_ns(&job->common, QEMU_CLOCK_REALTIME, delay_ns);
+    } else {
+        block_job_sleep_ns(&job->common, QEMU_CLOCK_REALTIME, 0);
+    }
+
+    if (block_job_is_cancelled(&job->common)) {
+        return true;
+    }
+
+    return false;
+}
+
+static int coroutine_fn backup_run_incremental(BackupBlockJob *job)
+{
+    bool error_is_read;
+    int ret = 0;
+    int clusters_per_iter;
+    uint32_t granularity;
+    int64_t sector;
+    int64_t cluster;
+    int64_t end;
+    int64_t last_cluster = -1;
+    BlockDriverState *bs = job->common.bs;
+    HBitmapIter hbi;
+
+    granularity = bdrv_dirty_bitmap_granularity(job->sync_bitmap);
+    clusters_per_iter = MAX((granularity / BACKUP_CLUSTER_SIZE), 1);

DIV_ROUND_UP(granularity, BACKUP_CLUSTER_SIZE) would've worked, too (instead of the MAX()), but since both are powers of two, this is equivalent.

+    bdrv_dirty_iter_init(bs, job->sync_bitmap, &hbi);
+
+    /* Find the next dirty sector(s) */
+    while ((sector = hbitmap_iter_next(&hbi)) != -1) {
+        cluster = sector / BACKUP_SECTORS_PER_CLUSTER;
+
+        /* Fake progress updates for any clusters we skipped */
+        if (cluster != last_cluster + 1) {
+            job->common.offset += ((cluster - last_cluster - 1) *
+                                   BACKUP_CLUSTER_SIZE);
+        }
+
+        for (end = cluster + clusters_per_iter; cluster < end; cluster++) {
+            if (yield_and_check(job)) {
+                return ret;
+            }
+
+            do {
+                ret = backup_do_cow(bs, cluster * BACKUP_SECTORS_PER_CLUSTER,
+                                    BACKUP_SECTORS_PER_CLUSTER, 
&error_is_read);
+                if ((ret < 0) &&
+                    backup_error_action(job, error_is_read, -ret) ==
+                    BLOCK_ERROR_ACTION_REPORT) {
+                    return ret;
+                }

Now that I'm reading this code again... The other backup implementation handles retries differently; it redoes the whole loop, with the effective difference being that it calls yield_and_check() between every retry. Would it make sense to move the yield_and_check() call into this loop?

+            } while (ret < 0);
+        }
+
+        /* If the bitmap granularity is smaller than the backup granularity,
+         * we need to advance the iterator pointer to the next cluster. */
+        if (granularity < BACKUP_CLUSTER_SIZE) {

Actually, whenever BACKUP_CLUSTER_SIZE isn't a factor of granularity. Both are powers of two, though, so that's the case iff granularity < BACKUP_CLUSTER_SIZE. (thus, the condition is correct)

+            bdrv_set_dirty_iter(&hbi, cluster * BACKUP_SECTORS_PER_CLUSTER);
+        }
+
+        last_cluster = cluster - 1;

A bit awkward, but hey...

So, what's preventing me from giving an R-b is whether or not yield_and_check() should be moved.

Max

+    }
+
+    /* Play some final catchup with the progress meter */
+    end = DIV_ROUND_UP(job->common.len, BACKUP_CLUSTER_SIZE);
+    if (last_cluster + 1 < end) {
+        job->common.offset += ((end - last_cluster - 1) * BACKUP_CLUSTER_SIZE);
+    }
+
+    return ret;
+}
+
  static void coroutine_fn backup_run(void *opaque)
  {
      BackupBlockJob *job = opaque;
@@ -259,8 +347,7 @@ static void coroutine_fn backup_run(void *opaque)
      qemu_co_rwlock_init(&job->flush_rwlock);
start = 0;
-    end = DIV_ROUND_UP(job->common.len / BDRV_SECTOR_SIZE,
-                       BACKUP_SECTORS_PER_CLUSTER);
+    end = DIV_ROUND_UP(job->common.len, BACKUP_CLUSTER_SIZE);
job->bitmap = hbitmap_alloc(end, 0); @@ -278,28 +365,13 @@ static void coroutine_fn backup_run(void *opaque)
              qemu_coroutine_yield();
              job->common.busy = true;
          }
+    } else if (job->sync_mode == MIRROR_SYNC_MODE_DIRTY_BITMAP) {
+        ret = backup_run_incremental(job);
      } else {
          /* Both FULL and TOP SYNC_MODE's require copying.. */
          for (; start < end; start++) {
              bool error_is_read;
-
-            if (block_job_is_cancelled(&job->common)) {
-                break;
-            }
-
-            /* we need to yield so that qemu_aio_flush() returns.
-             * (without, VM does not reboot)
-             */
-            if (job->common.speed) {
-                uint64_t delay_ns = ratelimit_calculate_delay(
-                        &job->limit, job->sectors_read);
-                job->sectors_read = 0;
-                block_job_sleep_ns(&job->common, QEMU_CLOCK_REALTIME, 
delay_ns);
-            } else {
-                block_job_sleep_ns(&job->common, QEMU_CLOCK_REALTIME, 0);
-            }
-
-            if (block_job_is_cancelled(&job->common)) {
+            if (yield_and_check(job)) {
                  break;
              }
@@ -357,6 +429,18 @@ static void coroutine_fn backup_run(void *opaque)
      qemu_co_rwlock_wrlock(&job->flush_rwlock);
      qemu_co_rwlock_unlock(&job->flush_rwlock);
+ if (job->sync_bitmap) {
+        BdrvDirtyBitmap *bm;
+        if (ret < 0) {
+            /* Merge the successor back into the parent, delete nothing. */
+            bm = bdrv_reclaim_dirty_bitmap(bs, job->sync_bitmap, NULL);
+            assert(bm);
+        } else {
+            /* Everything is fine, delete this bitmap and install the backup. 
*/
+            bm = bdrv_dirty_bitmap_abdicate(bs, job->sync_bitmap, NULL);
+            assert(bm);
+        }
+    }
      hbitmap_free(job->bitmap);
bdrv_iostatus_disable(target);
@@ -369,6 +453,7 @@ static void coroutine_fn backup_run(void *opaque)
void backup_start(BlockDriverState *bs, BlockDriverState *target,
                    int64_t speed, MirrorSyncMode sync_mode,
+                  BdrvDirtyBitmap *sync_bitmap,
                    BlockdevOnError on_source_error,
                    BlockdevOnError on_target_error,
                    BlockCompletionFunc *cb, void *opaque,
@@ -412,17 +497,36 @@ void backup_start(BlockDriverState *bs, BlockDriverState 
*target,
          return;
      }
+ if (sync_mode == MIRROR_SYNC_MODE_DIRTY_BITMAP) {
+        if (!sync_bitmap) {
+            error_setg(errp, "must provide a valid bitmap name for "
+                             "\"dirty-bitmap\" sync mode");
+            return;
+        }
+
+        /* Create a new bitmap, and freeze/disable this one. */
+        if (bdrv_dirty_bitmap_create_successor(bs, sync_bitmap, errp) < 0) {
+            return;
+        }
+    } else if (sync_bitmap) {
+        error_setg(errp,
+                   "a sync_bitmap was provided to backup_run, "
+                   "but received an incompatible sync_mode (%s)",
+                   MirrorSyncMode_lookup[sync_mode]);
+        return;
+    }
+
      len = bdrv_getlength(bs);
      if (len < 0) {
          error_setg_errno(errp, -len, "unable to get length for '%s'",
                           bdrv_get_device_name(bs));
-        return;
+        goto error;
      }
BackupBlockJob *job = block_job_create(&backup_job_driver, bs, speed,
                                             cb, opaque, errp);
      if (!job) {
-        return;
+        goto error;
      }
bdrv_op_block_all(target, job->common.blocker);
@@ -431,7 +535,15 @@ void backup_start(BlockDriverState *bs, BlockDriverState 
*target,
      job->on_target_error = on_target_error;
      job->target = target;
      job->sync_mode = sync_mode;
+    job->sync_bitmap = sync_mode == MIRROR_SYNC_MODE_DIRTY_BITMAP ?
+                       sync_bitmap : NULL;
      job->common.len = len;
      job->common.co = qemu_coroutine_create(backup_run);
      qemu_coroutine_enter(job->common.co, job);
+    return;
+
+ error:
+    if (sync_bitmap) {
+        bdrv_reclaim_dirty_bitmap(bs, sync_bitmap, NULL);
+    }
  }
diff --git a/block/mirror.c b/block/mirror.c
index 1cb700e..f89eccf 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -718,6 +718,10 @@ void mirror_start(BlockDriverState *bs, BlockDriverState 
*target,
      bool is_none_mode;
      BlockDriverState *base;
+ if (mode == MIRROR_SYNC_MODE_DIRTY_BITMAP) {
+        error_setg(errp, "Sync mode 'dirty-bitmap' not supported");
+        return;
+    }
      is_none_mode = mode == MIRROR_SYNC_MODE_NONE;
      base = mode == MIRROR_SYNC_MODE_TOP ? bs->backing_hd : NULL;
      mirror_start_job(bs, target, replaces,
diff --git a/blockdev.c b/blockdev.c
index 373d19a..90ba5b6 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1584,6 +1584,7 @@ static void drive_backup_prepare(BlkTransactionState 
*common, Error **errp)
                       backup->sync,
                       backup->has_mode, backup->mode,
                       backup->has_speed, backup->speed,
+                     backup->has_bitmap, backup->bitmap,
                       backup->has_on_source_error, backup->on_source_error,
                       backup->has_on_target_error, backup->on_target_error,
                       &local_err);
@@ -2394,6 +2395,7 @@ void qmp_drive_backup(const char *device, const char 
*target,
                        enum MirrorSyncMode sync,
                        bool has_mode, enum NewImageMode mode,
                        bool has_speed, int64_t speed,
+                      bool has_bitmap, const char *bitmap,
                        bool has_on_source_error, BlockdevOnError 
on_source_error,
                        bool has_on_target_error, BlockdevOnError 
on_target_error,
                        Error **errp)
@@ -2402,6 +2404,7 @@ void qmp_drive_backup(const char *device, const char 
*target,
      BlockDriverState *bs;
      BlockDriverState *target_bs;
      BlockDriverState *source = NULL;
+    BdrvDirtyBitmap *bmap = NULL;
      AioContext *aio_context;
      BlockDriver *drv = NULL;
      Error *local_err = NULL;
@@ -2501,7 +2504,16 @@ void qmp_drive_backup(const char *device, const char 
*target,
bdrv_set_aio_context(target_bs, aio_context); - backup_start(bs, target_bs, speed, sync, on_source_error, on_target_error,
+    if (has_bitmap) {
+        bmap = bdrv_find_dirty_bitmap(bs, bitmap);
+        if (!bmap) {
+            error_setg(errp, "Bitmap '%s' could not be found", bitmap);
+            goto out;
+        }
+    }
+
+    backup_start(bs, target_bs, speed, sync, bmap,
+                 on_source_error, on_target_error,
                   block_job_cb, bs, &local_err);
      if (local_err != NULL) {
          bdrv_unref(target_bs);
@@ -2562,8 +2574,8 @@ void qmp_blockdev_backup(const char *device, const char 
*target,
bdrv_ref(target_bs);
      bdrv_set_aio_context(target_bs, aio_context);
-    backup_start(bs, target_bs, speed, sync, on_source_error, on_target_error,
-                 block_job_cb, bs, &local_err);
+    backup_start(bs, target_bs, speed, sync, NULL, on_source_error,
+                 on_target_error, block_job_cb, bs, &local_err);
      if (local_err != NULL) {
          bdrv_unref(target_bs);
          error_propagate(errp, local_err);
diff --git a/hmp.c b/hmp.c
index f31ae27..d85d913 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1061,7 +1061,8 @@ void hmp_drive_backup(Monitor *mon, const QDict *qdict)
qmp_drive_backup(device, filename, !!format, format,
                       full ? MIRROR_SYNC_MODE_FULL : MIRROR_SYNC_MODE_TOP,
-                     true, mode, false, 0, false, 0, false, 0, &err);
+                     true, mode, false, 0, false, NULL,
+                     false, 0, false, 0, &err);
      hmp_handle_error(mon, &err);
  }
diff --git a/include/block/block.h b/include/block/block.h
index b02c670..80ac2cc 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -480,6 +480,7 @@ void bdrv_reset_dirty_bitmap(BlockDriverState *bs, 
BdrvDirtyBitmap *bitmap,
                               int64_t cur_sector, int nr_sectors);
  void bdrv_dirty_iter_init(BlockDriverState *bs,
                            BdrvDirtyBitmap *bitmap, struct HBitmapIter *hbi);
+void bdrv_set_dirty_iter(struct HBitmapIter *hbi, int64_t offset);
  int64_t bdrv_get_dirty_count(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
void bdrv_enable_copy_on_read(BlockDriverState *bs);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index fb9e100..e0d5561 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -602,6 +602,7 @@ void mirror_start(BlockDriverState *bs, BlockDriverState 
*target,
   * @target: Block device to write to.
   * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
   * @sync_mode: What parts of the disk image should be copied to the 
destination.
+ * @sync_bitmap: The dirty bitmap if sync_mode is 
MIRROR_SYNC_MODE_DIRTY_BITMAP.
   * @on_source_error: The action to take upon error reading from the source.
   * @on_target_error: The action to take upon error writing to the target.
   * @cb: Completion function for the job.
@@ -612,6 +613,7 @@ void mirror_start(BlockDriverState *bs, BlockDriverState 
*target,
   */
  void backup_start(BlockDriverState *bs, BlockDriverState *target,
                    int64_t speed, MirrorSyncMode sync_mode,
+                  BdrvDirtyBitmap *sync_bitmap,
                    BlockdevOnError on_source_error,
                    BlockdevOnError on_target_error,
                    BlockCompletionFunc *cb, void *opaque,
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 710be68..7f629e2 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -512,10 +512,12 @@
  #
  # @none: only copy data written from now on
  #
+# @dirty-bitmap: only copy data described by the dirty bitmap. Since: 2.4
+#
  # Since: 1.3
  ##
  { 'enum': 'MirrorSyncMode',
-  'data': ['top', 'full', 'none'] }
+  'data': ['top', 'full', 'none', 'dirty-bitmap'] }
##
  # @BlockJobType:
@@ -690,14 +692,17 @@
  #          probe if @mode is 'existing', else the format of the source
  #
  # @sync: what parts of the disk image should be copied to the destination
-#        (all the disk, only the sectors allocated in the topmost image, or
-#        only new I/O).
+#        (all the disk, only the sectors allocated in the topmost image, from a
+#        dirty bitmap, or only new I/O).
  #
  # @mode: #optional whether and how QEMU should create a new image, default is
  #        'absolute-paths'.
  #
  # @speed: #optional the maximum speed, in bytes per second
  #
+# @bitmap: #optional the name of dirty bitmap if sync is "dirty-bitmap"
+#          (Since 2.4)
+#
  # @on-source-error: #optional the action to take on an error on the source,
  #                   default 'report'.  'stop' and 'enospc' can only be used
  #                   if the block device supports io-status (see BlockInfo).
@@ -715,7 +720,7 @@
  { 'type': 'DriveBackup',
    'data': { 'device': 'str', 'target': 'str', '*format': 'str',
              'sync': 'MirrorSyncMode', '*mode': 'NewImageMode',
-            '*speed': 'int',
+            '*speed': 'int', '*bitmap': 'str',
              '*on-source-error': 'BlockdevOnError',
              '*on-target-error': 'BlockdevOnError' } }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index b6bd455..da750f0 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1074,7 +1074,7 @@ EQMP
      {
          .name       = "drive-backup",
          .args_type  = "sync:s,device:B,target:s,speed:i?,mode:s?,format:s?,"
-                      "on-source-error:s?,on-target-error:s?",
+                      "bitmap:s?,on-source-error:s?,on-target-error:s?",
          .mhandler.cmd_new = qmp_marshal_input_drive_backup,
      },
@@ -1101,8 +1101,9 @@ Arguments:
              (json-string, optional)
  - "sync": what parts of the disk image should be copied to the destination;
    possibilities include "full" for all the disk, "top" for only the sectors
-  allocated in the topmost image, or "none" to only replicate new I/O
-  (MirrorSyncMode).
+  allocated in the topmost image, "dirty-bitmap" for only the dirty sectors in
+  the bitmap, or "none" to only replicate new I/O (MirrorSyncMode).
+- "bitmap": dirty bitmap name for sync==dirty-bitmap
  - "mode": whether and how QEMU should create a new image
            (NewImageMode, optional, default 'absolute-paths')
  - "speed": the maximum speed, in bytes per second (json-int, optional)




reply via email to

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