[Qemu-devel] [PATCH 02/12] block/backup: Add mirror sync mode 'bitmap'

[John Snow]

We don't need or want a new sync mode for simple differences in
semantics.  Create a new mode simply named "BITMAP" that is designed to
make use of the new Bitmap Sync Mode field.

Because the only bitmap mode is 'conditional', this adds no new
functionality to the backup job (yet). The old incremental backup mode
is maintained as a syntactic sugar for sync=bitmap, mode=conditional.

Add all of the plumbing necessary to support this new instruction.

Signed-off-by: John Snow <address@hidden>
---
 qapi/block-core.json      | 30 ++++++++++++++++++++++--------
 include/block/block_int.h |  6 +++++-
 block/backup.c            | 35 ++++++++++++++++++++++++++++-------
 block/mirror.c            |  6 ++++--
 block/replication.c       |  2 +-
 blockdev.c                |  8 ++++++--
 6 files changed, 66 insertions(+), 21 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index caf28a71a0..6d05ad8f47 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1127,12 +1127,15 @@
 #
 # @none: only copy data written from now on
 #
-# @incremental: only copy data described by the dirty bitmap. Since: 2.4
+# @incremental: only copy data described by the dirty bitmap. (since: 2.4)
+#
+# @bitmap: only copy data described by the dirty bitmap. (since: 4.1)
+#          Behavior on completion is determined by the BitmapSyncMode.
 #
 # Since: 1.3
 ##
 { 'enum': 'MirrorSyncMode',
-  'data': ['top', 'full', 'none', 'incremental'] }
+  'data': ['top', 'full', 'none', 'incremental', 'bitmap'] }
 
 ##
 # @BitmapSyncMode:
@@ -1352,10 +1355,14 @@
 #
 # @speed: the maximum speed, in bytes per second
 #
-# @bitmap: the name of dirty bitmap if sync is "incremental".
-#          Must be present if sync is "incremental", must NOT be present
+# @bitmap: the name of dirty bitmap if sync is "bitmap".
+#          Must be present if sync is "bitmap", must NOT be present
 #          otherwise. (Since 2.4)
 #
+# @bitmap-mode: Specifies the type of data the bitmap should contain after
+#               the operation concludes. Must be present if sync is "bitmap".
+#               Must NOT be present otherwise. (Since 4.1)
+#
 # @compress: true to compress data, if the target format supports it.
 #            (default: false) (since 2.8)
 #
@@ -1390,7 +1397,8 @@
   'data': { '*job-id': 'str', 'device': 'str', 'target': 'str',
             '*format': 'str', 'sync': 'MirrorSyncMode',
             '*mode': 'NewImageMode', '*speed': 'int',
-            '*bitmap': 'str', '*compress': 'bool',
+            '*bitmap': 'str', '*bitmap-mode': 'BitmapSyncMode',
+            '*compress': 'bool',
             '*on-source-error': 'BlockdevOnError',
             '*on-target-error': 'BlockdevOnError',
             '*auto-finalize': 'bool', '*auto-dismiss': 'bool' } }
@@ -1412,10 +1420,14 @@
 # @speed: the maximum speed, in bytes per second. The default is 0,
 #         for unlimited.
 #
-# @bitmap: the name of dirty bitmap if sync is "incremental".
-#          Must be present if sync is "incremental", must NOT be present
+# @bitmap: the name of dirty bitmap if sync is "bitmap".
+#          Must be present if sync is "bitmap", must NOT be present
 #          otherwise. (Since 3.1)
 #
+# @bitmap-mode: Specifies the type of data the bitmap should contain after
+#               the operation concludes. Must be present if sync is "bitmap".
+#               Must NOT be present otherwise. (Since 4.1)
+#
 # @compress: true to compress data, if the target format supports it.
 #            (default: false) (since 2.8)
 #
@@ -1449,7 +1461,9 @@
 { 'struct': 'BlockdevBackup',
   'data': { '*job-id': 'str', 'device': 'str', 'target': 'str',
             'sync': 'MirrorSyncMode', '*speed': 'int',
-            '*bitmap': 'str', '*compress': 'bool',
+            '*bitmap': 'str',
+            '*bitmap-mode': 'BitmapSyncMode',
+            '*compress': 'bool',
             '*on-source-error': 'BlockdevOnError',
             '*on-target-error': 'BlockdevOnError',
             '*auto-finalize': 'bool', '*auto-dismiss': 'bool' } }
diff --git a/include/block/block_int.h b/include/block/block_int.h
index d6415b53c1..89370c1b9b 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -1132,7 +1132,9 @@ void mirror_start(const char *job_id, BlockDriverState 
*bs,
  * @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_INCREMENTAL.
+ * @sync_bitmap: The dirty bitmap if sync_mode is 'bitmap' or 'incremental'
+ * @has_bitmap_mode: true if @bitmap_sync carries a meaningful value.
+ * @bitmap_mode: The bitmap synchronization policy to use.
  * @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.
  * @creation_flags: Flags that control the behavior of the Job lifetime.
@@ -1148,6 +1150,8 @@ BlockJob *backup_job_create(const char *job_id, 
BlockDriverState *bs,
                             BlockDriverState *target, int64_t speed,
                             MirrorSyncMode sync_mode,
                             BdrvDirtyBitmap *sync_bitmap,
+                            bool has_bitmap_mode,
+                            BitmapSyncMode bitmap_mode,
                             bool compress,
                             BlockdevOnError on_source_error,
                             BlockdevOnError on_target_error,
diff --git a/block/backup.c b/block/backup.c
index 715e1d3be8..c4f83d4ef7 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -38,9 +38,9 @@ typedef struct CowRequest {
 typedef struct BackupBlockJob {
     BlockJob common;
     BlockBackend *target;
-    /* bitmap for sync=incremental */
     BdrvDirtyBitmap *sync_bitmap;
     MirrorSyncMode sync_mode;
+    BitmapSyncMode bitmap_mode;
     BlockdevOnError on_source_error;
     BlockdevOnError on_target_error;
     CoRwlock flush_rwlock;
@@ -452,7 +452,7 @@ static int coroutine_fn backup_run(Job *job, Error **errp)
 
     job_progress_set_remaining(job, s->len);
 
-    if (s->sync_mode == MIRROR_SYNC_MODE_INCREMENTAL) {
+    if (s->sync_mode == MIRROR_SYNC_MODE_BITMAP) {
         backup_incremental_init_copy_bitmap(s);
     } else {
         hbitmap_set(s->copy_bitmap, 0, s->len);
@@ -536,6 +536,7 @@ static int64_t 
backup_calculate_cluster_size(BlockDriverState *target,
 BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
                   BlockDriverState *target, int64_t speed,
                   MirrorSyncMode sync_mode, BdrvDirtyBitmap *sync_bitmap,
+                  bool has_bitmap_mode, BitmapSyncMode bitmap_mode,
                   bool compress,
                   BlockdevOnError on_source_error,
                   BlockdevOnError on_target_error,
@@ -548,6 +549,7 @@ BlockJob *backup_job_create(const char *job_id, 
BlockDriverState *bs,
     int ret;
     int64_t cluster_size;
     HBitmap *copy_bitmap = NULL;
+    MirrorSyncMode effective_mode = sync_mode;
 
     assert(bs);
     assert(target);
@@ -584,9 +586,28 @@ BlockJob *backup_job_create(const char *job_id, 
BlockDriverState *bs,
     }
 
     if (sync_mode == MIRROR_SYNC_MODE_INCREMENTAL) {
+        if (has_bitmap_mode &&
+            bitmap_mode != BITMAP_SYNC_MODE_CONDITIONAL) {
+            error_setg(errp, "Bitmap sync mode must be 'conditional' "
+                       "when using sync mode '%s'",
+                       MirrorSyncMode_str(sync_mode));
+            return NULL;
+        }
+        has_bitmap_mode = true;
+        bitmap_mode = BITMAP_SYNC_MODE_CONDITIONAL;
+        effective_mode = MIRROR_SYNC_MODE_BITMAP;
+    }
+
+    if (effective_mode == MIRROR_SYNC_MODE_BITMAP) {
         if (!sync_bitmap) {
             error_setg(errp, "must provide a valid bitmap name for "
-                             "\"incremental\" sync mode");
+                       "'%s' sync mode", MirrorSyncMode_str(sync_mode));
+            return NULL;
+        }
+
+        if (!has_bitmap_mode) {
+            error_setg(errp, "must provide a valid bitmap mode for "
+                       "'%s' sync mode", MirrorSyncMode_str(sync_mode));
             return NULL;
         }
 
@@ -596,8 +617,8 @@ BlockJob *backup_job_create(const char *job_id, 
BlockDriverState *bs,
         }
     } else if (sync_bitmap) {
         error_setg(errp,
-                   "a sync_bitmap was provided to backup_run, "
-                   "but received an incompatible sync_mode (%s)",
+                   "a bitmap was given to backup_job_create, "
+                   "but it received an incompatible sync_mode (%s)",
                    MirrorSyncMode_str(sync_mode));
         return NULL;
     }
@@ -639,8 +660,8 @@ BlockJob *backup_job_create(const char *job_id, 
BlockDriverState *bs,
     job->on_source_error = on_source_error;
     job->on_target_error = on_target_error;
     job->sync_mode = sync_mode;
-    job->sync_bitmap = sync_mode == MIRROR_SYNC_MODE_INCREMENTAL ?
-                       sync_bitmap : NULL;
+    job->sync_bitmap = sync_bitmap;
+    job->bitmap_mode = bitmap_mode;
     job->compress = compress;
 
     /* Detect image-fleecing (and similar) schemes */
diff --git a/block/mirror.c b/block/mirror.c
index d17be4cdbc..42b3d9acd0 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1717,8 +1717,10 @@ void mirror_start(const char *job_id, BlockDriverState 
*bs,
     bool is_none_mode;
     BlockDriverState *base;
 
-    if (mode == MIRROR_SYNC_MODE_INCREMENTAL) {
-        error_setg(errp, "Sync mode 'incremental' not supported");
+    if ((mode == MIRROR_SYNC_MODE_INCREMENTAL) ||
+        (mode == MIRROR_SYNC_MODE_BITMAP)) {
+        error_setg(errp, "Sync mode '%s' not supported",
+                   MirrorSyncMode_str(mode));
         return;
     }
     is_none_mode = mode == MIRROR_SYNC_MODE_NONE;
diff --git a/block/replication.c b/block/replication.c
index b41bc507c0..a62aaeb879 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -543,7 +543,7 @@ static void replication_start(ReplicationState *rs, 
ReplicationMode mode,
 
         s->backup_job = backup_job_create(
                                 NULL, s->secondary_disk->bs, 
s->hidden_disk->bs,
-                                0, MIRROR_SYNC_MODE_NONE, NULL, false,
+                                0, MIRROR_SYNC_MODE_NONE, NULL, false, 0, 
false,
                                 BLOCKDEV_ON_ERROR_REPORT,
                                 BLOCKDEV_ON_ERROR_REPORT, JOB_INTERNAL,
                                 backup_job_completed, bs, NULL, &local_err);
diff --git a/blockdev.c b/blockdev.c
index 5d6a13dea9..7abbd6bbf2 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3572,7 +3572,9 @@ static BlockJob *do_drive_backup(DriveBackup *backup, 
JobTxn *txn,
     }
 
     job = backup_job_create(backup->job_id, bs, target_bs, backup->speed,
-                            backup->sync, bmap, backup->compress,
+                            backup->sync, bmap,
+                            backup->has_bitmap_mode, backup->bitmap_mode,
+                            backup->compress,
                             backup->on_source_error, backup->on_target_error,
                             job_flags, NULL, NULL, txn, &local_err);
     if (local_err != NULL) {
@@ -3677,7 +3679,9 @@ BlockJob *do_blockdev_backup(BlockdevBackup *backup, 
JobTxn *txn,
         job_flags |= JOB_MANUAL_DISMISS;
     }
     job = backup_job_create(backup->job_id, bs, target_bs, backup->speed,
-                            backup->sync, bmap, backup->compress,
+                            backup->sync, bmap,
+                            backup->has_bitmap_mode, backup->bitmap_mode,
+                            backup->compress,
                             backup->on_source_error, backup->on_target_error,
                             job_flags, NULL, NULL, txn, &local_err);
     if (local_err != NULL) {
-- 
2.21.0