Re: [Qemu-devel] [PATCH v11 09/13] qmp: Add support of "dirty-bitmap" sync mode for drive-backup

[Fam Zheng <famz@redhat.com>]

On Tue, 01/13 12:50, John Snow wrote:
> 
> 
> On 01/13/2015 04:37 AM, Fam Zheng wrote:
> >On Mon, 01/12 11:31, 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 <famz@redhat.com>
> >>Signed-off-by: John Snow <jsnow@redhat.com>
> >>---
> >>  block.c                   |   5 ++
> >>  block/backup.c            | 120 ++++++++++++++++++++++++++++++++++++++--------
> >>  block/mirror.c            |   4 ++
> >>  blockdev.c                |  14 +++++-
> >>  hmp.c                     |   3 +-
> >>  include/block/block.h     |   1 +
> >>  include/block/block_int.h |   2 +
> >>  qapi/block-core.json      |  11 +++--
> >>  qmp-commands.hx           |   7 +--
> >>  9 files changed, 137 insertions(+), 30 deletions(-)
> >>
> >>diff --git a/block.c b/block.c
> >>index 3f33b9d..5eb6788 100644
> >>--- a/block.c
> >>+++ b/block.c
> >>@@ -5633,6 +5633,11 @@ static void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector,
> >>      }
> >>  }
> >>
> >>+void bdrv_set_dirty_iter(HBitmapIter *hbi, int64_t offset)
> >>+{
> >>+    hbitmap_iter_init(hbi, hbi->hb, offset);
> >
> >What's the reason for this indirection? Can caller just use hbitmap_iter_init?
> >
> 
> Essentially it is just a rename of hbitmap_iter_init to make its usage here
> clear, that we are manually advancing the pointer. How we accomplish that
> (hbitmap_iter_init) is an implementation detail.
> 
> Yes, I can just call this directly from block/backup, but at the time I was
> less sure of how I would advance the pointer, so I created a wrapper where I
> could change details as needed, if I needed to.

OK, either is fine for me.

> 
> >>+}
> >>+
> >>  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 792e655..0626a3e 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,31 @@ 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 void coroutine_fn backup_run(void *opaque)
> >>  {
> >>      BackupBlockJob *job = opaque;
> >>@@ -254,13 +281,13 @@ static void coroutine_fn backup_run(void *opaque)
> >>      };
> >>      int64_t start, end;
> >>      int ret = 0;
> >>+    bool error_is_read;
> >>
> >>      QLIST_INIT(&job->inflight_reqs);
> >>      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 +305,45 @@ 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) {
> >>+        /* Dirty Bitmap sync has a slightly different iteration method */
> >>+        HBitmapIter hbi;
> >>+        int64_t sector;
> >>+        int64_t cluster;
> >>+        bool polyrhythmic;
> >>+
> >>+        bdrv_dirty_iter_init(bs, job->sync_bitmap, &hbi);
> >>+        /* Does the granularity happen to match our backup cluster size? */
> >>+        polyrhythmic = (bdrv_dirty_bitmap_granularity(bs, job->sync_bitmap) !=
> >>+                        BACKUP_CLUSTER_SIZE);
> >>+
> >>+        /* Find the next dirty /sector/ and copy that /cluster/ */
> >>+        while ((sector = hbitmap_iter_next(&hbi)) != -1) {
> >>+            if (yield_and_check(job)) {
> >>+                goto leave;
> >>+            }
> >>+            cluster = sector / BACKUP_SECTORS_PER_CLUSTER;
> >>+
> >>+            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) {
> >>+                    goto leave;
> >>+                }
> >>+            } while (ret < 0);
> >>+
> >>+            /* Advance (or rewind) our iterator if we need to. */
> >>+            if (polyrhythmic) {
> >>+                bdrv_set_dirty_iter(&hbi,
> >>+                                    (cluster + 1) * BACKUP_SECTORS_PER_CLUSTER);
> >>+            }
> >>+        }
> >>      } 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;
> >>              }
> >>
> >>@@ -351,12 +395,23 @@ static void coroutine_fn backup_run(void *opaque)
> >>          }
> >>      }
> >>
> >>+leave:
> >>      notifier_with_return_remove(&before_write);
> >>
> >>      /* wait until pending backup_do_cow() calls have completed */
> >>      qemu_co_rwlock_wrlock(&job->flush_rwlock);
> >>      qemu_co_rwlock_unlock(&job->flush_rwlock);
> >>
> >>+    if (job->sync_bitmap) {
> >>+        if (ret < 0) {
> >>+            /* Merge the successor back into the parent, delete nothing. */
> >>+            assert(bdrv_reclaim_dirty_bitmap(bs, job->sync_bitmap, NULL));
> >
> >Why can't reclaim fail here? If we canassert, please move the expression out
> >because it has a side effect.
> >
> 
> It shouldn't fail; if it does, something went very wrong. The only chance
> this has to fail is if the sync bitmap has no successor, but we explicitly
> installed one (and explicitly check that it succeeded) before going into the
> block backup.
> 
> I am not sure what other meaningful recovery could happen in this case,
> though we *could* just report an error and continue on, acknowledging that
> the BdrvDirtyBitmap is now compromised and of questionable validity.
> 
> Does anyone have an error handling preference here?

I don't, as long as you move it out from assert:

int r = bdrv_reclaim_dirty_bitmap(bs, job->sync_bitmap, NULL);
assert(r);

Fam