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: John Snow
Subject: Re: [Qemu-devel] [PATCH v5 10/21] qmp: Add support of "dirty-bitmap" sync mode for drive-backup
Date: Fri, 17 Apr 2015 12:21:09 -0400
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0



On 04/17/2015 09:17 AM, Max Reitz wrote:
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.


But this way we get to put your name in the source code.

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


Yes, I should be mindful of the case where we might have to copy many clusters per dirty bit. I don't think we lose anything by inserting it at the top of the do{}while(), but we will potentially exit the loop quicker on cancellation cases.

+            } 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...


The inner for() is going to advance cluster as an index, plus an extra before it exits the loop. Either I manually do cluster-- or just subtract one from last_cluster... Either way I have to fiddle with the index.

I could also use a separate index and only advance cluster once we make it into the loop, but that looks awkward too, so I choice my poison.

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

Max


[email_truncate()]



reply via email to

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