[Qemu-devel] [PATCH 00/12] bitmaps: introduce 'bitmap' sync mode

[Qemu-devel] [PATCH 00/12] bitmaps: introduce 'bitmap' sync mode

[John Snow]

This series adds a new "BITMAP" sync mode that is meant to replace the
existing "INCREMENTAL" sync mode.

This mode can be changed by issuing any of three bitmap sync modes;
passed as arguments to the job.

The three bitmap sync modes are:
- CONDITIONAL: This is an alias for the old incremental mode. The bitmap is
               conditionally synchronized based on the return code of the job
               upon completion.
- NEVER: This is, effectively, the differential backup mode. It never clears
         the bitmap, as the name suggests.
- ALWAYS: Here is the new, exciting thing. The bitmap is always synchronized,
          even on failure. On success, this is identical to incremental, but
          on failure it clears only the bits that were copied successfully.
          This can be used to "resume" incremental backups from later points
          in times.

I wrote this series by accident on my way to implement incremental mode
for mirror, but this happened first -- the problem is that Mirror mode
uses its existing modes in a very particular way; and this was the best
way to add bitmap support into the mirror job properly.

In this series:
patches 1-2: Adds CONDITIONAL
patch 3: Adds NEVER
Patches 4-7: Adds ALWAYS.
Patches 8-11: Adds iotest 257
Patch 12: Minor permission loosening for RO bitmaps.

Next up:
 - Add these modes to Mirror. (Done*, but need tests.)
 - Allow the use of bitmaps and bitmap sync modes with non-BITMAP modes;
   This will allow for resumable/re-tryable full backups.

John Snow (12):
  qapi: add BitmapSyncMode enum
  block/backup: Add mirror sync mode 'bitmap'
  block/backup: add 'never' policy to bitmap sync mode
  hbitmap: Fix merge when b is empty, and result is not an alias of a
  hbitmap: enable merging across granularities
  block/dirty-bitmap: add bdrv_dirty_bitmap_claim
  block/backup: add 'always' bitmap sync policy
  iotests: add testing shim for script-style python tests
  iotests: teach run_job to cancel pending jobs
  iotests: teach FilePath to produce multiple paths
  iotests: add test 257 for bitmap-mode backups
  block/backup: loosen restriction on readonly bitmaps

 qapi/block-core.json          |   51 +-
 include/block/block_int.h     |    7 +-
 include/qemu/hbitmap.h        |    8 +
 block/backup.c                |   52 +-
 block/dirty-bitmap.c          |   14 +
 block/mirror.c                |    6 +-
 block/replication.c           |    2 +-
 blockdev.c                    |   12 +-
 util/hbitmap.c                |   36 +-
 tests/qemu-iotests/257        |  412 ++++++
 tests/qemu-iotests/257.out    | 2199 +++++++++++++++++++++++++++++++++
 tests/qemu-iotests/group      |    1 +
 tests/qemu-iotests/iotests.py |   83 +-
 13 files changed, 2830 insertions(+), 53 deletions(-)
 create mode 100755 tests/qemu-iotests/257
 create mode 100644 tests/qemu-iotests/257.out

-- 
2.21.0

[Qemu-devel] [PATCH 01/12] qapi: add BitmapSyncMode enum

[John Snow]

Depending on what a user is trying to accomplish, there might be a few
bitmap cleanup actions that occur when an operation is finished that
could be useful.

I am proposing three:
- NEVER: The bitmap is never synchronized against what was copied.
- ALWAYS: The bitmap is always synchronized, even on failures.
- CONDITIONAL: The bitmap is synchronized only on success.

The existing incremental backup modes use 'conditional' semantics,
so add just that one for right now.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 qapi/block-core.json | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 0d43d4f37c..caf28a71a0 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1134,6 +1134,20 @@
 { 'enum': 'MirrorSyncMode',
   'data': ['top', 'full', 'none', 'incremental'] }
 
+##
+# @BitmapSyncMode:
+#
+# An enumeration of possible behaviors for the synchronization of a bitmap
+# when used for data copy operations.
+#
+# @conditional: The bitmap is only synchronized when the operation is successul.
+#               This is useful for Incremental semantics.
+#
+# Since: 4.1
+##
+{ 'enum': 'BitmapSyncMode',
+  'data': ['conditional'] }
+
 ##
 # @MirrorCopyMode:
 #
-- 
2.21.0

Re: [Qemu-devel] [PATCH 01/12] qapi: add BitmapSyncMode enum

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 1867 bytes --]

On 20.06.19 03:03, John Snow wrote:
> Depending on what a user is trying to accomplish, there might be a few
> bitmap cleanup actions that occur when an operation is finished that
> could be useful.
> 
> I am proposing three:
> - NEVER: The bitmap is never synchronized against what was copied.
> - ALWAYS: The bitmap is always synchronized, even on failures.
> - CONDITIONAL: The bitmap is synchronized only on success.
> 
> The existing incremental backup modes use 'conditional' semantics,
> so add just that one for right now.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  qapi/block-core.json | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 0d43d4f37c..caf28a71a0 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1134,6 +1134,20 @@
>  { 'enum': 'MirrorSyncMode',
>    'data': ['top', 'full', 'none', 'incremental'] }
>  
> +##
> +# @BitmapSyncMode:
> +#
> +# An enumeration of possible behaviors for the synchronization of a bitmap
> +# when used for data copy operations.
> +#
> +# @conditional: The bitmap is only synchronized when the operation is successul.

*successful

> +#               This is useful for Incremental semantics.

Hm, well.  All bitmap modes are for incremental semantics, in some way
or another.  (“conditional” and “always” just automatically create
point-in-time snapshots, in a sense, where “never” requires the user to
manually do so.)

So maybe something more concrete would be better?  Like “This allows
incremental use from one successful operation to the next, and
restarting any operation on failure”?

Max

> +#
> +# Since: 4.1
> +##
> +{ 'enum': 'BitmapSyncMode',
> +  'data': ['conditional'] }
> +
>  ##
>  # @MirrorCopyMode:
>  #
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[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 <jsnow@redhat.com>
---
 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

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

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 7971 bytes --]

On 20.06.19 03:03, John Snow wrote:
> 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 <jsnow@redhat.com>
> ---
>  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)

Why not deprecate this in the process and note that this is equal to
sync=bitmap, bitmap-mode=conditional?

(I don’t think there is a rule that forces us to actually remove
deprecated stuff after two releases if it doesn’t hurt to keep it.)

> +#
> +# @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)

Er, well, now this is too fast of a deprecation. :-)  It must still also
be present if sync is “incremental”.

>  #
> +# @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)

Do we have any rule that qemu must enforce “must not”s? :-)

(No, I don’t think so.  I think it’s very reasonable that you accept
bitmap-mode=conditional for sync=incremental.)

>  # @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)

Same as above.

> +# @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.

Hmm...  If you moved the conversion of incremental/- =>
bitmap/conditional into blockdev.c, you could get rid of this parameter
because it would be equal to (sync_bitmap != NULL).

(It itches me to get rid of this parameter because there is no other
has* parameter for this function yet.)

> + * @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

[...]

> @@ -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;
> +    }
> +

I also just don’t quite feel like this is the correct place to put this.
 It’s a deprecated interface, so it should be translated in the
interface code, i.e. in blockdev.c.

(Sure, this gives you a central place for the translation, but you can
just as well add a function to the same effect to blockdev.c.)

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[John Snow]

On 6/20/19 11:00 AM, Max Reitz wrote:
> On 20.06.19 03:03, John Snow wrote:
>> 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 <jsnow@redhat.com>
>> ---
>>  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)
> 
> Why not deprecate this in the process and note that this is equal to
> sync=bitmap, bitmap-mode=conditional?
> 
> (I don’t think there is a rule that forces us to actually remove
> deprecated stuff after two releases if it doesn’t hurt to keep it.)
> 

Mostly I thought it would be fine to keep as sugar. In your replies so
far I gather that "incremental" and "differential" don't mean specific
backup paradigms to you, so maybe these seem like worthless words.

It was my general understanding that in terms of backup
paradigms/methodologies that "incremental" and "differential" mean very
specific things.

Incremental: Each backup contains only the delta from the last
incremental backup.
Differential: Each backup contains the delta from the last FULL backup.

You can search "incremental vs differential backup" on your search
engine of choice and find many relevant results. I took a Networking/IT
vocational degree in 2007 and these terms were taught in textbooks then.

So I will resist quite strongly changing them, and for this reason, felt
that it was strictly a good thing to keep incremental as sugar, because
I thought that people would know what it meant.

(More than "conditional", anyway, which is jargon I made up.)

>> +#
>> +# @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)
> 
> Er, well, now this is too fast of a deprecation. :-)  It must still also
> be present if sync is “incremental”.
> 

OK; I will try to phrase it better. This is reflecting too much the
implementation -- I think I was trying to communicate that incremental
was just sugar for "bitmap", so I was trusting that was understood here.

...But, depending on the order in which you read the docs, this could be
confusing, so I guess I will change that.

>>  #
>> +# @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)
> 
> Do we have any rule that qemu must enforce “must not”s? :-)
> 
> (No, I don’t think so.  I think it’s very reasonable that you accept
> bitmap-mode=conditional for sync=incremental.)
> 

Right, I left this a secret wiggle room. If you specify the correct
bitmap sync mode for the incremental sugar, it will actually let it
slide. If you specify the wrong one, it will error out.

However, I think this is perfectly correct advice from the API: Please
use this mode with sync=bitmap and do not use it otherwise.

Would you like me to change it to be more technically correct and
document the little affordance I made?

>>  # @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)
> 
> Same as above.
> 

OK

>> +# @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.
> 
> Hmm...  If you moved the conversion of incremental/- =>
> bitmap/conditional into blockdev.c, you could get rid of this parameter
> because it would be equal to (sync_bitmap != NULL).
> 
> (It itches me to get rid of this parameter because there is no other
> has* parameter for this function yet.)
> 

Yeah, it annoyed me too, and I believe later you do correctly guess why
I did it -- it's so that the sugar conversion occurs all in one place
where the logic was easiest to condense.

I ran into the issue that there's no way to define a QAPI enum that has
a "default"/"unset" state without also allowing that value to be entered
by the user explicitly; so there was no way to pass along an "unset
enum" down this far.

So... (thought continued below)

>> + * @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
> 
> [...]
> 
>> @@ -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;
>> +    }
>> +
> 
> I also just don’t quite feel like this is the correct place to put this.
>  It’s a deprecated interface, so it should be translated in the
> interface code, i.e. in blockdev.c.
> 
> (Sure, this gives you a central place for the translation, but you can
> just as well add a function to the same effect to blockdev.c.)
> 
> Max
> 

... I can toy around with your idea of making a helper that can be
called in blockdev and see if I like it.

Thank you for taking a look!

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

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 4380 bytes --]

On 20.06.19 18:01, John Snow wrote:
> 
> 
> On 6/20/19 11:00 AM, Max Reitz wrote:
>> On 20.06.19 03:03, John Snow wrote:
>>> 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 <jsnow@redhat.com>
>>> ---
>>>  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)
>>
>> Why not deprecate this in the process and note that this is equal to
>> sync=bitmap, bitmap-mode=conditional?
>>
>> (I don’t think there is a rule that forces us to actually remove
>> deprecated stuff after two releases if it doesn’t hurt to keep it.)
>>
> 
> Mostly I thought it would be fine to keep as sugar. In your replies so
> far I gather that "incremental" and "differential" don't mean specific
> backup paradigms to you, so maybe these seem like worthless words.
> 
> It was my general understanding that in terms of backup
> paradigms/methodologies that "incremental" and "differential" mean very
> specific things.
> 
> Incremental: Each backup contains only the delta from the last
> incremental backup.
> Differential: Each backup contains the delta from the last FULL backup.
> 
> You can search "incremental vs differential backup" on your search
> engine of choice and find many relevant results. I took a Networking/IT
> vocational degree in 2007 and these terms were taught in textbooks then.
> 
> So I will resist quite strongly changing them, and for this reason, felt
> that it was strictly a good thing to keep incremental as sugar, because
> I thought that people would know what it meant.

:C

OK.  I’m happy as long as it’s all explained somewhere (i.e.
bitmaps.rst).  Personally, I’d also like a pointer to that documentation
here.  (Sure, people should just look there if they don’t understand
something about bitmaps anyway, but I can’t see it hurting to just put a
pointer here anyway.)

> (More than "conditional", anyway, which is jargon I made up.)

But you make it up in this series, which is great for me, because that
means I get the definition (from the cover letter) without having to
look it up. O:-)

[...]

>>>  #
>>> +# @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)
>>
>> Do we have any rule that qemu must enforce “must not”s? :-)
>>
>> (No, I don’t think so.  I think it’s very reasonable that you accept
>> bitmap-mode=conditional for sync=incremental.)
>>
> 
> Right, I left this a secret wiggle room. If you specify the correct
> bitmap sync mode for the incremental sugar, it will actually let it
> slide. If you specify the wrong one, it will error out.
> 
> However, I think this is perfectly correct advice from the API: Please
> use this mode with sync=bitmap and do not use it otherwise.
> 
> Would you like me to change it to be more technically correct and
> document the little affordance I made?

It’s probably better not to.  Better forbid as much as we can so that we
can break compatibility to users that happened to use it still “because
it works”.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Vladimir Sementsov-Ogievskiy]

20.06.2019 4:03, John Snow wrote:
> 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.

I don't follow, why you don't want to just add bitmap-mode optional parameter
for incremental mode?

For this all looks similar to just two separate things:
1. add bitmap-mode parameter
2. rename incremental to bitmap

Why do we need [2.] ?
If we do only [1.], we'll avoid creating two similar modes, syntax sugar, a bit
of mess as it seems to me..

Hmm, about differential backups, as I understood, we call 'differential' an incremental
backup, but which considers difference not from latest incremental backup but from some
in the past.. Is it incorrect?

-- 
Best regards,
Vladimir

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

[John Snow]

On 6/21/19 7:29 AM, Vladimir Sementsov-Ogievskiy wrote:
> 20.06.2019 4:03, John Snow wrote:
>> 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.
> 
> I don't follow, why you don't want to just add bitmap-mode optional parameter
> for incremental mode?
> 

Vocabulary reasons, see below.

> For this all looks similar to just two separate things:
> 1. add bitmap-mode parameter
> 2. rename incremental to bitmap

This is exactly correct!

> 
> Why do we need [2.] ?
> If we do only [1.], we'll avoid creating two similar modes, syntax sugar, a bit
> of mess as it seems to me..
> 
> Hmm, about differential backups, as I understood, we call 'differential' an incremental
> backup, but which considers difference not from latest incremental backup but from some
> in the past.. Is it incorrect?
> 

The reason is because I have been treating "INCREMENTAL" as meaning
something very specific -- I gather from you and Max that you don't
consider this term to mean something specific.

So, by other prominent backup vendors, they use these terms in this way:

INCREMENTAL: This backup contains a delta from the last INCREMENTAL
backup made. In effect, this creates a chain of backups that must be
squashed together to recover data, but uses less info on copy.

DIFFERENTIAL: This backup contains a delta from the last FULL backup
made. In effect, each differential backup only requires a base image and
a single differential. This usually wastes more data during the backup
process, but makes restoration processes easier.


I *always* use these terms in these *exact* ways; you can see that the
bitmap behavior we use is exactly what MIRROR_SYNC_MODE_INCREMENTAL
does. Even when we are using bitmap manipulation techniques to get it to
do something else, the block job itself is engineered to think that it
is producing an "Incremental" backup.


In the early days of this feature, Fam actually proposed something like
what I am proposing here:

a BITMAP sync mode with an on_complete parameter for the backup job that
would either roll the bitmap forward or not (like my "conditional",
"never") based on the success of the job.

We removed that because at the time we wanted to target a simpler
feature. As part of that removal, I renamed the mode "INCREMENTAL" under
the premise that if we ever wanted to add a "DIFFERENTIAL" mode like
what Fam's original design allowed for, we could add
MIRROR_SYNC_MODE_DIFFERENTIAL and that would differentiate the two
modes. This rename was done with the specific knowledge and intent that
the mode was named after the exact specific backup paradigm it was
enabling. Otherwise, I would have left it "BITMAP" back then.

I've had patches in my branch to add a DIFFERENTIAL mode ever since
then! However, since we added bitmap merging, you'll notice that we
actually CAN do "Differential" backups by playing around with the
bitmaps ourselves, which has largely stopped me from wanting to
introduce the new mode.

You'll recall that recently Xie Changlong sent patches to add
"incremental" support to mirror, but what they ACTUALLY implemented was
"Differential" mode -- they didn't clear the bitmap afterwards. I
actually responded as such on-list -- that if we implement a
"Differential" mode that their patches would have been appropriate for
that mode.

As a result of that discussion, I went to add a "Differential" mode to
mirror, but in the process realized that it's much easier to make the
bitmap sync behavior its own parameter.

However, because the new parameters no longer mean the backup is
"Incremental" by that definition, I decided to rename the mode "BITMAP"
again to be *less specific* and, perhaps now ironically, avoid confusion.

Even given this confusion ... I actually still think that we should NOT
use "Incremental" to mean something generic, and I will continue to
enforce the idea that "Incremental" should mean a series of
non-overlapping time-sliced deltas.

--js

[Qemu-devel] [PATCH 03/12] block/backup: add 'never' policy to bitmap sync mode

[John Snow]

This adds a "never" policy for bitmap synchronization. Regardless of if
the job succeeds or fails, we never update the bitmap. This can be used
to perform differential backups, or simply to avoid the job modifying a
bitmap.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 qapi/block-core.json | 6 +++++-
 block/backup.c       | 5 +++--
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 6d05ad8f47..0332dcaabc 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1146,10 +1146,14 @@
 # @conditional: The bitmap is only synchronized when the operation is successul.
 #               This is useful for Incremental semantics.
 #
+# @never: The bitmap is never synchronized with the operation, and is
+#         treated solely as a manifest of blocks to copy.
+#         This is useful for Differential semantics.
+#
 # Since: 4.1
 ##
 { 'enum': 'BitmapSyncMode',
-  'data': ['conditional'] }
+  'data': ['conditional', 'never'] }
 
 ##
 # @MirrorCopyMode:
diff --git a/block/backup.c b/block/backup.c
index c4f83d4ef7..627f724b68 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -265,8 +265,9 @@ static void backup_cleanup_sync_bitmap(BackupBlockJob *job, int ret)
     BdrvDirtyBitmap *bm;
     BlockDriverState *bs = blk_bs(job->common.blk);
 
-    if (ret < 0) {
-        /* Merge the successor back into the parent, delete nothing. */
+    if (ret < 0 || job->bitmap_mode == BITMAP_SYNC_MODE_NEVER) {
+        /* Failure, or we don't want to synchronize the bitmap.
+         * Merge the successor back into the parent, delete nothing. */
         bm = bdrv_reclaim_dirty_bitmap(bs, job->sync_bitmap, NULL);
         assert(bm);
     } else {
-- 
2.21.0

Re: [Qemu-devel] [PATCH 03/12] block/backup: add 'never' policy to bitmap sync mode

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 2183 bytes --]

On 20.06.19 03:03, John Snow wrote:
> This adds a "never" policy for bitmap synchronization. Regardless of if
> the job succeeds or fails, we never update the bitmap. This can be used
> to perform differential backups, or simply to avoid the job modifying a
> bitmap.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  qapi/block-core.json | 6 +++++-
>  block/backup.c       | 5 +++--
>  2 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 6d05ad8f47..0332dcaabc 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1146,10 +1146,14 @@
>  # @conditional: The bitmap is only synchronized when the operation is successul.
>  #               This is useful for Incremental semantics.
>  #
> +# @never: The bitmap is never synchronized with the operation, and is
> +#         treated solely as a manifest of blocks to copy.
> +#         This is useful for Differential semantics.
> +#

Again, this is too buzzword-y for my taste.  I don’t find it as bad
because there is not much to explain about this mode, and you do explain
it above, but still.

Like, I (me myself) read this and after the first sentence I think I’ve
understood what this is.  Then I read “for Differential semantics” and
I’m confused.  After a couple of seconds, I realize what you mean
because I’ve described in my response to patch 1.

One reason it leaves the buzzword-y taste is because “differential” is
never explained anywhere.  bitmaps.rst makes two mentions of it, but it
too just assumes I know what you mean.  Also, incremental backups are
just a certain kind of differential backups.

So you need to explain “differential” somewhere and how it differs from
“incremental” in this regard.  Why not here?

“This is useful when you wish to repeatedly perform operations in
reference to a constant synchronization point (when the bitmap was
created).”

Or something.

Max

>  # Since: 4.1
>  ##
>  { 'enum': 'BitmapSyncMode',
> -  'data': ['conditional'] }
> +  'data': ['conditional', 'never'] }
>  
>  ##
>  # @MirrorCopyMode:


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 03/12] block/backup: add 'never' policy to bitmap sync mode

[John Snow]

On 6/20/19 11:25 AM, Max Reitz wrote:
> On 20.06.19 03:03, John Snow wrote:
>> This adds a "never" policy for bitmap synchronization. Regardless of if
>> the job succeeds or fails, we never update the bitmap. This can be used
>> to perform differential backups, or simply to avoid the job modifying a
>> bitmap.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  qapi/block-core.json | 6 +++++-
>>  block/backup.c       | 5 +++--
>>  2 files changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index 6d05ad8f47..0332dcaabc 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -1146,10 +1146,14 @@
>>  # @conditional: The bitmap is only synchronized when the operation is successul.
>>  #               This is useful for Incremental semantics.
>>  #
>> +# @never: The bitmap is never synchronized with the operation, and is
>> +#         treated solely as a manifest of blocks to copy.
>> +#         This is useful for Differential semantics.
>> +#
> 
> Again, this is too buzzword-y for my taste.  I don’t find it as bad
> because there is not much to explain about this mode, and you do explain
> it above, but still.
> 

Explained in my response to patch 2, I disagree.

> Like, I (me myself) read this and after the first sentence I think I’ve
> understood what this is.  Then I read “for Differential semantics” and
> I’m confused.  After a couple of seconds, I realize what you mean
> because I’ve described in my response to patch 1.
> 
> One reason it leaves the buzzword-y taste is because “differential” is
> never explained anywhere.  bitmaps.rst makes two mentions of it, but it
> too just assumes I know what you mean.  Also, incremental backups are
> just a certain kind of differential backups.
> 

This, however, is a real shortcoming of the doc. You'll notice I didn't
propose a doc update in this patchset, because secretly it's an RFC and
I did expect a v2+.

> So you need to explain “differential” somewhere and how it differs from
> “incremental” in this regard.  Why not here?
> 

Too broad of a concept to explain down in qapi comment strings, or I'd
have to explain it everywhere. bitmaps.rst is the correct place.

> “This is useful when you wish to repeatedly perform operations in
> reference to a constant synchronization point (when the bitmap was
> created).”
> 
> Or something.
> 
> Max
> 
>>  # Since: 4.1
>>  ##
>>  { 'enum': 'BitmapSyncMode',
>> -  'data': ['conditional'] }
>> +  'data': ['conditional', 'never'] }
>>  
>>  ##
>>  # @MirrorCopyMode:
> 

[Qemu-devel] [PATCH 04/12] hbitmap: Fix merge when b is empty, and result is not an alias of a

[John Snow]

Nobody calls the function like this currently, but we neither prohibit
or cope with this behavior. I decided to make the function cope with it.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 util/hbitmap.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/util/hbitmap.c b/util/hbitmap.c
index 7905212a8b..45d1725daf 100644
--- a/util/hbitmap.c
+++ b/util/hbitmap.c
@@ -781,8 +781,9 @@ bool hbitmap_can_merge(const HBitmap *a, const HBitmap *b)
 }
 
 /**
- * Given HBitmaps A and B, let A := A (BITOR) B.
- * Bitmap B will not be modified.
+ * Given HBitmaps A and B, let R := A (BITOR) B.
+ * Bitmaps A and B will not be modified,
+ *     except when bitmap R is an alias of A or B.
  *
  * @return true if the merge was successful,
  *         false if it was not attempted.
@@ -797,7 +798,9 @@ bool hbitmap_merge(const HBitmap *a, const HBitmap *b, HBitmap *result)
     }
     assert(hbitmap_can_merge(b, result));
 
-    if (hbitmap_count(b) == 0) {
+    if ((!hbitmap_count(a) && result == b) ||
+        (!hbitmap_count(b) && result == a) ||
+        (!hbitmap_count(a) && !hbitmap_count(b))) {
         return true;
     }
 
-- 
2.21.0

Re: [Qemu-devel] [PATCH 04/12] hbitmap: Fix merge when b is empty, and result is not an alias of a

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 1565 bytes --]

On 20.06.19 03:03, John Snow wrote:
> Nobody calls the function like this currently, but we neither prohibit
> or cope with this behavior. I decided to make the function cope with it.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  util/hbitmap.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/util/hbitmap.c b/util/hbitmap.c
> index 7905212a8b..45d1725daf 100644
> --- a/util/hbitmap.c
> +++ b/util/hbitmap.c
> @@ -781,8 +781,9 @@ bool hbitmap_can_merge(const HBitmap *a, const HBitmap *b)
>  }
>  
>  /**
> - * Given HBitmaps A and B, let A := A (BITOR) B.
> - * Bitmap B will not be modified.
> + * Given HBitmaps A and B, let R := A (BITOR) B.
> + * Bitmaps A and B will not be modified,
> + *     except when bitmap R is an alias of A or B.
>   *
>   * @return true if the merge was successful,
>   *         false if it was not attempted.
> @@ -797,7 +798,9 @@ bool hbitmap_merge(const HBitmap *a, const HBitmap *b, HBitmap *result)
>      }
>      assert(hbitmap_can_merge(b, result));
>  
> -    if (hbitmap_count(b) == 0) {
> +    if ((!hbitmap_count(a) && result == b) ||
> +        (!hbitmap_count(b) && result == a) ||
> +        (!hbitmap_count(a) && !hbitmap_count(b))) {
>          return true;
>      }

The rest of this function completely overwrites the @result bitmap.
Therefor, @result does not need to be cleared when calling this function.

Therfore, hbitmap_merge(hbitmap_alloc(), hbitmap_alloc(), output) should
actually clear @output, I think.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 04/12] hbitmap: Fix merge when b is empty, and result is not an alias of a

[John Snow]

On 6/20/19 11:39 AM, Max Reitz wrote:
> On 20.06.19 03:03, John Snow wrote:
>> Nobody calls the function like this currently, but we neither prohibit
>> or cope with this behavior. I decided to make the function cope with it.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  util/hbitmap.c | 9 ++++++---
>>  1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/util/hbitmap.c b/util/hbitmap.c
>> index 7905212a8b..45d1725daf 100644
>> --- a/util/hbitmap.c
>> +++ b/util/hbitmap.c
>> @@ -781,8 +781,9 @@ bool hbitmap_can_merge(const HBitmap *a, const HBitmap *b)
>>  }
>>  
>>  /**
>> - * Given HBitmaps A and B, let A := A (BITOR) B.
>> - * Bitmap B will not be modified.
>> + * Given HBitmaps A and B, let R := A (BITOR) B.
>> + * Bitmaps A and B will not be modified,
>> + *     except when bitmap R is an alias of A or B.
>>   *
>>   * @return true if the merge was successful,
>>   *         false if it was not attempted.
>> @@ -797,7 +798,9 @@ bool hbitmap_merge(const HBitmap *a, const HBitmap *b, HBitmap *result)
>>      }
>>      assert(hbitmap_can_merge(b, result));
>>  
>> -    if (hbitmap_count(b) == 0) {
>> +    if ((!hbitmap_count(a) && result == b) ||
>> +        (!hbitmap_count(b) && result == a) ||
>> +        (!hbitmap_count(a) && !hbitmap_count(b))) {
>>          return true;
>>      }
> 
> The rest of this function completely overwrites the @result bitmap.
> Therefor, @result does not need to be cleared when calling this function.
> 
> Therfore, hbitmap_merge(hbitmap_alloc(), hbitmap_alloc(), output) should
> actually clear @output, I think.
> 
> Max
> 

Ah, wellp, you're right. That'd be the second problem with this function.

It used to be strictly A = A | B, but we changed it -- very incompletely
-- to R = A | B; which explains these two bugs.

Thanks.

[Qemu-devel] [PATCH 05/12] hbitmap: enable merging across granularities

[John Snow]

Signed-off-by: John Snow <jsnow@redhat.com>
---
 util/hbitmap.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/util/hbitmap.c b/util/hbitmap.c
index 45d1725daf..0d6724b7bc 100644
--- a/util/hbitmap.c
+++ b/util/hbitmap.c
@@ -777,7 +777,17 @@ void hbitmap_truncate(HBitmap *hb, uint64_t size)
 
 bool hbitmap_can_merge(const HBitmap *a, const HBitmap *b)
 {
-    return (a->size == b->size) && (a->granularity == b->granularity);
+    return (a->size == b->size);
+}
+
+static void hbitmap_sparse_merge(HBitmap *dst, const HBitmap *src)
+{
+    uint64_t offset = 0;
+    uint64_t count = src->orig_size;
+
+    while (hbitmap_next_dirty_area(src, &offset, &count)) {
+        hbitmap_set(dst, offset, count);
+    }
 }
 
 /**
@@ -804,6 +814,16 @@ bool hbitmap_merge(const HBitmap *a, const HBitmap *b, HBitmap *result)
         return true;
     }
 
+    if (a->size != b->size) {
+        if (a != result) {
+            hbitmap_sparse_merge(result, a);
+        }
+        if (b != result) {
+            hbitmap_sparse_merge(result, b);
+        }
+        return true;
+    }
+
     /* This merge is O(size), as BITS_PER_LONG and HBITMAP_LEVELS are constant.
      * It may be possible to improve running times for sparsely populated maps
      * by using hbitmap_iter_next, but this is suboptimal for dense maps.
-- 
2.21.0

Re: [Qemu-devel] [PATCH 05/12] hbitmap: enable merging across granularities

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 1701 bytes --]

On 20.06.19 03:03, John Snow wrote:
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  util/hbitmap.c | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/util/hbitmap.c b/util/hbitmap.c
> index 45d1725daf..0d6724b7bc 100644
> --- a/util/hbitmap.c
> +++ b/util/hbitmap.c
> @@ -777,7 +777,17 @@ void hbitmap_truncate(HBitmap *hb, uint64_t size)
>  
>  bool hbitmap_can_merge(const HBitmap *a, const HBitmap *b)
>  {
> -    return (a->size == b->size) && (a->granularity == b->granularity);
> +    return (a->size == b->size);
> +}
> +
> +static void hbitmap_sparse_merge(HBitmap *dst, const HBitmap *src)
> +{
> +    uint64_t offset = 0;
> +    uint64_t count = src->orig_size;
> +
> +    while (hbitmap_next_dirty_area(src, &offset, &count)) {
> +        hbitmap_set(dst, offset, count);
> +    }
>  }
>  
>  /**
> @@ -804,6 +814,16 @@ bool hbitmap_merge(const HBitmap *a, const HBitmap *b, HBitmap *result)
>          return true;
>      }
>  
> +    if (a->size != b->size) {

Don’t you mean s/size/granularity/?

Right now, this is dead code, which leads me to asking for a test.
(Well, no, I would’ve asked anyway.)

Max

> +        if (a != result) {
> +            hbitmap_sparse_merge(result, a);
> +        }
> +        if (b != result) {
> +            hbitmap_sparse_merge(result, b);
> +        }
> +        return true;
> +    }
> +
>      /* This merge is O(size), as BITS_PER_LONG and HBITMAP_LEVELS are constant.
>       * It may be possible to improve running times for sparsely populated maps
>       * by using hbitmap_iter_next, but this is suboptimal for dense maps.
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 05/12] hbitmap: enable merging across granularities

[John Snow]

On 6/20/19 11:47 AM, Max Reitz wrote:
> On 20.06.19 03:03, John Snow wrote:
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  util/hbitmap.c | 22 +++++++++++++++++++++-
>>  1 file changed, 21 insertions(+), 1 deletion(-)
>>
>> diff --git a/util/hbitmap.c b/util/hbitmap.c
>> index 45d1725daf..0d6724b7bc 100644
>> --- a/util/hbitmap.c
>> +++ b/util/hbitmap.c
>> @@ -777,7 +777,17 @@ void hbitmap_truncate(HBitmap *hb, uint64_t size)
>>  
>>  bool hbitmap_can_merge(const HBitmap *a, const HBitmap *b)
>>  {
>> -    return (a->size == b->size) && (a->granularity == b->granularity);
>> +    return (a->size == b->size);
>> +}
>> +
>> +static void hbitmap_sparse_merge(HBitmap *dst, const HBitmap *src)
>> +{
>> +    uint64_t offset = 0;
>> +    uint64_t count = src->orig_size;
>> +
>> +    while (hbitmap_next_dirty_area(src, &offset, &count)) {
>> +        hbitmap_set(dst, offset, count);
>> +    }
>>  }
>>  
>>  /**
>> @@ -804,6 +814,16 @@ bool hbitmap_merge(const HBitmap *a, const HBitmap *b, HBitmap *result)
>>          return true;
>>      }
>>  
>> +    if (a->size != b->size) {
> 
> Don’t you mean s/size/granularity/?
> 
> Right now, this is dead code, which leads me to asking for a test.
> (Well, no, I would’ve asked anyway.)
> 
> Max
> 

Ah, crud. Caught red-handed. Yes and Yes.

As to your later question: Can we use this for backup initialization?
Also yes; but it might be the case that we want the copy bitmap to
become a full-fledged "bdrv dirty bitmap" instead of an hbitmap, which
will actually make this easier and probably eliminate the need for the
"_take" or "_claim" function I added, too.

Re: [Qemu-devel] [PATCH 05/12] hbitmap: enable merging across granularities

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 351 bytes --]

On 20.06.19 03:03, John Snow wrote:
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  util/hbitmap.c | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)

I wonder whether this could be used in
backup_incremental_init_copy_bitmap().  The sync_bitmap must have the
same length as the source BDS, right?

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 05/12] hbitmap: enable merging across granularities

[Vladimir Sementsov-Ogievskiy]

20.06.2019 19:47, Max Reitz wrote:
> On 20.06.19 03:03, John Snow wrote:
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   util/hbitmap.c | 22 +++++++++++++++++++++-
>>   1 file changed, 21 insertions(+), 1 deletion(-)
> 
> I wonder whether this could be used in
> backup_incremental_init_copy_bitmap().  The sync_bitmap must have the
> same length as the source BDS, right?
> 

A little problem here is that in backup code we merge BdrvDirtyBitmap into
HBitmap, so hbitmap API can't be directly used here, and to avoid code
duplication we need to create a bit strange interface of merging exactly
BdrvDirtyBitmap into HBitmap.. But, anyway it's better than code duplication.

Yes, they should have the same length.

But we may allow at least merging smaller bitmap into larger, why not?

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 05/12] hbitmap: enable merging across granularities

[Vladimir Sementsov-Ogievskiy]

20.06.2019 4:03, John Snow wrote:
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   util/hbitmap.c | 22 +++++++++++++++++++++-
>   1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/util/hbitmap.c b/util/hbitmap.c
> index 45d1725daf..0d6724b7bc 100644
> --- a/util/hbitmap.c
> +++ b/util/hbitmap.c
> @@ -777,7 +777,17 @@ void hbitmap_truncate(HBitmap *hb, uint64_t size)
>   
>   bool hbitmap_can_merge(const HBitmap *a, const HBitmap *b)
>   {
> -    return (a->size == b->size) && (a->granularity == b->granularity);
> +    return (a->size == b->size);
> +}
> +
> +static void hbitmap_sparse_merge(HBitmap *dst, const HBitmap *src)
> +{
> +    uint64_t offset = 0;
> +    uint64_t count = src->orig_size;
> +
> +    while (hbitmap_next_dirty_area(src, &offset, &count)) {
> +        hbitmap_set(dst, offset, count);

This will not work, as hbitmap_next_dirty_area will return the same answer all the time I think.
You need to update offset and count in a loop, like it is done in backup_incremental_init_copy_bitmap.


> +    }
>   }
>   
>   /**
> @@ -804,6 +814,16 @@ bool hbitmap_merge(const HBitmap *a, const HBitmap *b, HBitmap *result)
>           return true;
>       }
>   
> +    if (a->size != b->size) {
> +        if (a != result) {
> +            hbitmap_sparse_merge(result, a);
> +        }
> +        if (b != result) {
> +            hbitmap_sparse_merge(result, b);
> +        }
> +        return true;
> +    }
> +
>       /* This merge is O(size), as BITS_PER_LONG and HBITMAP_LEVELS are constant.
>        * It may be possible to improve running times for sparsely populated maps
>        * by using hbitmap_iter_next, but this is suboptimal for dense maps.
> 


-- 
Best regards,
Vladimir

[Qemu-devel] [PATCH 06/12] block/dirty-bitmap: add bdrv_dirty_bitmap_claim

[John Snow]

This function can claim an hbitmap to replace its own current hbitmap.
In the case that the granularities do not match, it will use
hbitmap_merge to copy the bit data instead.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 include/block/block_int.h |  1 +
 include/qemu/hbitmap.h    |  8 ++++++++
 block/dirty-bitmap.c      | 14 ++++++++++++++
 util/hbitmap.c            |  5 +++++
 4 files changed, 28 insertions(+)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index 89370c1b9b..7348ea8e78 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -1240,6 +1240,7 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t offset, int64_t bytes);
 
 void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out);
 void bdrv_restore_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap *backup);
+void bdrv_dirty_bitmap_claim(BdrvDirtyBitmap *bitmap, HBitmap **hbitmap);
 
 void bdrv_inc_in_flight(BlockDriverState *bs);
 void bdrv_dec_in_flight(BlockDriverState *bs);
diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
index 4afbe6292e..c74519042a 100644
--- a/include/qemu/hbitmap.h
+++ b/include/qemu/hbitmap.h
@@ -82,6 +82,14 @@ void hbitmap_truncate(HBitmap *hb, uint64_t size);
  */
 bool hbitmap_merge(const HBitmap *a, const HBitmap *b, HBitmap *result);
 
+/**
+ * hbitmap_same_conf:
+ *
+ * Compares the configuration for HBitmaps A and B.
+ * Return true if they are identical, false otherwise.
+ */
+bool hbitmap_same_conf(const HBitmap *a, const HBitmap *b);
+
 /**
  * hbitmap_can_merge:
  *
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 95a9c2a5d8..15c857e445 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -632,6 +632,20 @@ void bdrv_restore_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap *backup)
     hbitmap_free(tmp);
 }
 
+/* claim ownership of an hbitmap */
+void bdrv_dirty_bitmap_claim(BdrvDirtyBitmap *bitmap, HBitmap **hbitmap)
+{
+    if (hbitmap_same_conf(bitmap->bitmap, *hbitmap)) {
+        bdrv_restore_dirty_bitmap(bitmap, *hbitmap);
+    } else {
+        assert(hbitmap_can_merge(bitmap->bitmap, *hbitmap));
+        bdrv_clear_dirty_bitmap(bitmap, NULL);
+        hbitmap_merge(bitmap->bitmap, *hbitmap, bitmap->bitmap);
+        hbitmap_free(*hbitmap);
+    }
+    *hbitmap = NULL;
+}
+
 uint64_t bdrv_dirty_bitmap_serialization_size(const BdrvDirtyBitmap *bitmap,
                                               uint64_t offset, uint64_t bytes)
 {
diff --git a/util/hbitmap.c b/util/hbitmap.c
index 0d6724b7bc..a2abd425b5 100644
--- a/util/hbitmap.c
+++ b/util/hbitmap.c
@@ -775,6 +775,11 @@ void hbitmap_truncate(HBitmap *hb, uint64_t size)
     }
 }
 
+bool hbitmap_same_conf(const HBitmap *a, const HBitmap *b)
+{
+    return (a->size == b->size) && (a->granularity == b->granularity);
+}
+
 bool hbitmap_can_merge(const HBitmap *a, const HBitmap *b)
 {
     return (a->size == b->size);
-- 
2.21.0

Re: [Qemu-devel] [PATCH 06/12] block/dirty-bitmap: add bdrv_dirty_bitmap_claim

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 1068 bytes --]

On 20.06.19 03:03, John Snow wrote:
> This function can claim an hbitmap to replace its own current hbitmap.
> In the case that the granularities do not match, it will use
> hbitmap_merge to copy the bit data instead.

I really do not like this name because to me it implies a relationship
to bdrv_reclaim_dirty_bitmap().  Maybe just bdrv_dirty_bitmap_take()?
Or, if you want to get more fancy, bdrv_dirty_dirty_bitmap_steal().
(Because references are taken or stolen.)

The latter might fit in nicely with the abdication theme.  We’d just
need to rename bdrv_reclaim_dirty_bitmap() to
bdrv_dirty_bitmap_backstab(), and it’d be perfect.

(On another note: bdrv_restore_dirty_bitmap() vs.
bdrv_dirty_bitmap_restore() – really? :-/)

> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  include/block/block_int.h |  1 +
>  include/qemu/hbitmap.h    |  8 ++++++++
>  block/dirty-bitmap.c      | 14 ++++++++++++++
>  util/hbitmap.c            |  5 +++++
>  4 files changed, 28 insertions(+)

The implementation looks good to me.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 06/12] block/dirty-bitmap: add bdrv_dirty_bitmap_claim

[John Snow]

On 6/20/19 12:02 PM, Max Reitz wrote:
> On 20.06.19 03:03, John Snow wrote:
>> This function can claim an hbitmap to replace its own current hbitmap.
>> In the case that the granularities do not match, it will use
>> hbitmap_merge to copy the bit data instead.
> 
> I really do not like this name because to me it implies a relationship
> to bdrv_reclaim_dirty_bitmap().  Maybe just bdrv_dirty_bitmap_take()?
> Or, if you want to get more fancy, bdrv_dirty_dirty_bitmap_steal().
> (Because references are taken or stolen.)
> 

take or steal is good. I just wanted to highlight that it truly takes
ownership. The double-pointer and erasing the caller's reference for
emphasis of this idea.

> The latter might fit in nicely with the abdication theme.  We’d just
> need to rename bdrv_reclaim_dirty_bitmap() to
> bdrv_dirty_bitmap_backstab(), and it’d be perfect.
> 

Don't tempt me; I do like my silly function names. You are lucky I don't
call

> (On another note: bdrv_restore_dirty_bitmap() vs.
> bdrv_dirty_bitmap_restore() – really? :-/)
> 

I have done a terrible job at enforcing any kind of consistency here,
and it gets me all the time too. I had a big series that re-arranged and
re-named a ton of stuff just to make things a little more nicer, but I
let it bitrot because I didn't want to deal with the bike-shedding.

I do agree I am in desperate need of a spring cleaning in here.

One thing that does upset me quite often is that the canonical name for
the structure is "bdrv dirty bitmap", which makes the function names all
quite long.

>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  include/block/block_int.h |  1 +
>>  include/qemu/hbitmap.h    |  8 ++++++++
>>  block/dirty-bitmap.c      | 14 ++++++++++++++
>>  util/hbitmap.c            |  5 +++++
>>  4 files changed, 28 insertions(+)
> 
> The implementation looks good to me.
> 
> Max
> 

Re: [Qemu-devel] [PATCH 06/12] block/dirty-bitmap: add bdrv_dirty_bitmap_claim

[Vladimir Sementsov-Ogievskiy]

20.06.2019 19:36, John Snow wrote:
> 
> 
> On 6/20/19 12:02 PM, Max Reitz wrote:
>> On 20.06.19 03:03, John Snow wrote:
>>> This function can claim an hbitmap to replace its own current hbitmap.
>>> In the case that the granularities do not match, it will use
>>> hbitmap_merge to copy the bit data instead.
>>
>> I really do not like this name because to me it implies a relationship
>> to bdrv_reclaim_dirty_bitmap().  Maybe just bdrv_dirty_bitmap_take()?
>> Or, if you want to get more fancy, bdrv_dirty_dirty_bitmap_steal().
>> (Because references are taken or stolen.)
>>
> 
> take or steal is good. I just wanted to highlight that it truly takes
> ownership. The double-pointer and erasing the caller's reference for
> emphasis of this idea.

Didn't you consider bdrv_dirty_bitmap_set_hbitmap? Hmm, but your function
actually eats pointer, so 'set' is not enough.. bdrv_dirty_bitmap_eat_hbitmap?

And to be serious: it is the point where we definitely should drop HBitmap:meta
field, as it in bad relation with parent hbitmap stealing.

> 
>> The latter might fit in nicely with the abdication theme.  We’d just
>> need to rename bdrv_reclaim_dirty_bitmap() to
>> bdrv_dirty_bitmap_backstab(), and it’d be perfect.
>>
> 
> Don't tempt me; I do like my silly function names. You are lucky I don't
> call
> 
>> (On another note: bdrv_restore_dirty_bitmap() vs.
>> bdrv_dirty_bitmap_restore() – really? :-/)
>>
> 
> I have done a terrible job at enforcing any kind of consistency here,
> and it gets me all the time too. I had a big series that re-arranged and
> re-named a ton of stuff just to make things a little more nicer, but I
> let it bitrot because I didn't want to deal with the bike-shedding.
> 
> I do agree I am in desperate need of a spring cleaning in here.
> 
> One thing that does upset me quite often is that the canonical name for
> the structure is "bdrv dirty bitmap", which makes the function names all
> quite long.
> 
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>>   include/block/block_int.h |  1 +
>>>   include/qemu/hbitmap.h    |  8 ++++++++
>>>   block/dirty-bitmap.c      | 14 ++++++++++++++
>>>   util/hbitmap.c            |  5 +++++
>>>   4 files changed, 28 insertions(+)
>>
>> The implementation looks good to me.
>>
>> Max
>>
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 06/12] block/dirty-bitmap: add bdrv_dirty_bitmap_claim

[John Snow]

On 6/21/19 7:58 AM, Vladimir Sementsov-Ogievskiy wrote:
> 20.06.2019 19:36, John Snow wrote:
>>
>>
>> On 6/20/19 12:02 PM, Max Reitz wrote:
>>> On 20.06.19 03:03, John Snow wrote:
>>>> This function can claim an hbitmap to replace its own current hbitmap.
>>>> In the case that the granularities do not match, it will use
>>>> hbitmap_merge to copy the bit data instead.
>>>
>>> I really do not like this name because to me it implies a relationship
>>> to bdrv_reclaim_dirty_bitmap().  Maybe just bdrv_dirty_bitmap_take()?
>>> Or, if you want to get more fancy, bdrv_dirty_dirty_bitmap_steal().
>>> (Because references are taken or stolen.)
>>>
>>
>> take or steal is good. I just wanted to highlight that it truly takes
>> ownership. The double-pointer and erasing the caller's reference for
>> emphasis of this idea.
> 
> Didn't you consider bdrv_dirty_bitmap_set_hbitmap? Hmm, but your function
> actually eats pointer, so 'set' is not enough.. bdrv_dirty_bitmap_eat_hbitmap?
> 

:)

> And to be serious: it is the point where we definitely should drop HBitmap:meta
> field, as it in bad relation with parent hbitmap stealing.
> 

You're right, I didn't consider how this would interact with that. Allow
me the time to re-audit how this feature works, there's clearly a lot of
problems with what I've proposed for cross-granularity merging.

>>
>>> The latter might fit in nicely with the abdication theme.  We’d just
>>> need to rename bdrv_reclaim_dirty_bitmap() to
>>> bdrv_dirty_bitmap_backstab(), and it’d be perfect.
>>>
>>
>> Don't tempt me; I do like my silly function names. You are lucky I don't
>> call
>>
>>> (On another note: bdrv_restore_dirty_bitmap() vs.
>>> bdrv_dirty_bitmap_restore() – really? :-/)
>>>
>>
>> I have done a terrible job at enforcing any kind of consistency here,
>> and it gets me all the time too. I had a big series that re-arranged and
>> re-named a ton of stuff just to make things a little more nicer, but I
>> let it bitrot because I didn't want to deal with the bike-shedding.
>>
>> I do agree I am in desperate need of a spring cleaning in here.
>>
>> One thing that does upset me quite often is that the canonical name for
>> the structure is "bdrv dirty bitmap", which makes the function names all
>> quite long.
>>
>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>> ---
>>>>   include/block/block_int.h |  1 +
>>>>   include/qemu/hbitmap.h    |  8 ++++++++
>>>>   block/dirty-bitmap.c      | 14 ++++++++++++++
>>>>   util/hbitmap.c            |  5 +++++
>>>>   4 files changed, 28 insertions(+)
>>>
>>> The implementation looks good to me.
>>>
>>> Max
>>>
>>
> 
> 

[Qemu-devel] [PATCH 07/12] block/backup: add 'always' bitmap sync policy

[John Snow]

This adds an "always" policy for bitmap synchronization. Regardless of if
the job succeeds or fails, the bitmap is *always* synchronized. This means
that for backups that fail part-way through, the bitmap retains a record of
which sectors need to be copied out to accomplish a new backup using the
old, partial result.

In effect, this allows us to "resume" a failed backup; however the new backup
will be from the new point in time, so it isn't a "resume" as much as it is
an "incremental retry." This can be useful in the case of extremely large
backups that fail considerably through the operation and we'd like to not waste
the work that was already performed.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 qapi/block-core.json |  5 ++++-
 block/backup.c       | 10 ++++++----
 2 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 0332dcaabc..58d267f1f5 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1143,6 +1143,9 @@
 # An enumeration of possible behaviors for the synchronization of a bitmap
 # when used for data copy operations.
 #
+# @always: The bitmap is always synchronized with remaining blocks to copy,
+#          whether or not the operation has completed successfully or not.
+#
 # @conditional: The bitmap is only synchronized when the operation is successul.
 #               This is useful for Incremental semantics.
 #
@@ -1153,7 +1156,7 @@
 # Since: 4.1
 ##
 { 'enum': 'BitmapSyncMode',
-  'data': ['conditional', 'never'] }
+  'data': ['always', 'conditional', 'never'] }
 
 ##
 # @MirrorCopyMode:
diff --git a/block/backup.c b/block/backup.c
index 627f724b68..beb2078696 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -266,15 +266,17 @@ static void backup_cleanup_sync_bitmap(BackupBlockJob *job, int ret)
     BlockDriverState *bs = blk_bs(job->common.blk);
 
     if (ret < 0 || job->bitmap_mode == BITMAP_SYNC_MODE_NEVER) {
-        /* Failure, or we don't want to synchronize the bitmap.
-         * Merge the successor back into the parent, delete nothing. */
+        /* Failure, or we don't want to synchronize the bitmap. */
+        if (job->bitmap_mode == BITMAP_SYNC_MODE_ALWAYS) {
+            bdrv_dirty_bitmap_claim(job->sync_bitmap, &job->copy_bitmap);
+        }
+        /* Merge the successor back into the parent. */
         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);
     }
+    assert(bm);
 }
 
 static void backup_commit(Job *job)
-- 
2.21.0

Re: [Qemu-devel] [PATCH 07/12] block/backup: add 'always' bitmap sync policy

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 4484 bytes --]

On 20.06.19 03:03, John Snow wrote:
> This adds an "always" policy for bitmap synchronization. Regardless of if
> the job succeeds or fails, the bitmap is *always* synchronized. This means
> that for backups that fail part-way through, the bitmap retains a record of
> which sectors need to be copied out to accomplish a new backup using the
> old, partial result.
> 
> In effect, this allows us to "resume" a failed backup; however the new backup
> will be from the new point in time, so it isn't a "resume" as much as it is
> an "incremental retry." This can be useful in the case of extremely large
> backups that fail considerably through the operation and we'd like to not waste
> the work that was already performed.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  qapi/block-core.json |  5 ++++-
>  block/backup.c       | 10 ++++++----
>  2 files changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 0332dcaabc..58d267f1f5 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1143,6 +1143,9 @@
>  # An enumeration of possible behaviors for the synchronization of a bitmap
>  # when used for data copy operations.
>  #
> +# @always: The bitmap is always synchronized with remaining blocks to copy,
> +#          whether or not the operation has completed successfully or not.
> +#
>  # @conditional: The bitmap is only synchronized when the operation is successul.
>  #               This is useful for Incremental semantics.
>  #
> @@ -1153,7 +1156,7 @@
>  # Since: 4.1
>  ##
>  { 'enum': 'BitmapSyncMode',
> -  'data': ['conditional', 'never'] }
> +  'data': ['always', 'conditional', 'never'] }
>  
>  ##
>  # @MirrorCopyMode:
> diff --git a/block/backup.c b/block/backup.c
> index 627f724b68..beb2078696 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -266,15 +266,17 @@ static void backup_cleanup_sync_bitmap(BackupBlockJob *job, int ret)
>      BlockDriverState *bs = blk_bs(job->common.blk);
>  
>      if (ret < 0 || job->bitmap_mode == BITMAP_SYNC_MODE_NEVER) {
> -        /* Failure, or we don't want to synchronize the bitmap.
> -         * Merge the successor back into the parent, delete nothing. */
> +        /* Failure, or we don't want to synchronize the bitmap. */
> +        if (job->bitmap_mode == BITMAP_SYNC_MODE_ALWAYS) {
> +            bdrv_dirty_bitmap_claim(job->sync_bitmap, &job->copy_bitmap);

Hmm...  OK, bitmaps in backup always confuse me, so bear with me, please.

(Hi, I’m a time traveler from the end of this section and I can tell you
that everything is fine.  I was just confused.  I’ll still keep this
here, because it was so much work.)

The copy_bitmap is copied from the sync_bitmap at the beginning, so the
sync_bitmap can continue to be dirtied, but that won’t affect the job.
In normal incremental mode, this means that the sync point is always at
the beginning of the job.  (Well, naturally, because that’s how backup
is supposed to go.)

But then replacing the sync_bitmap with the copy_bitmap here means that
all of these dirtyings that happened during the job are lost.  Hmm, but
that doesn’t matter, does it?  Because whenever something was dirtied in
sync_bitmap, the corresponding area must have been copied to the backup
due to the job.

Ah, yes, it would actually be wrong to keep the new dirty bits, because
in this mode, sync_bitmap should (on failure) reflect what is left to
copy to make the backup complete.  Copying these newly dirtied sectors
would be wrong.  (Yes, I know you wrote that in the documentation of
@always.  I just tried to get a different perspective.)

Yes, yes, and copy_bitmap is always set whenever a CBW to the target
fails before the source can be updated.  Good, good.


Hi, I’m the time traveler from above.  I also left the section here so I
can give one of my trademark “Ramble, ramble,

Reviewed-by: Max Reitz <mreitz@redhat.com>

”

> +        }
> +        /* Merge the successor back into the parent. */
>          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);
>      }
> +    assert(bm);
>  }
>  
>  static void backup_commit(Job *job)
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 07/12] block/backup: add 'always' bitmap sync policy

[John Snow]

On 6/20/19 1:00 PM, Max Reitz wrote:
> On 20.06.19 03:03, John Snow wrote:
>> This adds an "always" policy for bitmap synchronization. Regardless of if
>> the job succeeds or fails, the bitmap is *always* synchronized. This means
>> that for backups that fail part-way through, the bitmap retains a record of
>> which sectors need to be copied out to accomplish a new backup using the
>> old, partial result.
>>
>> In effect, this allows us to "resume" a failed backup; however the new backup
>> will be from the new point in time, so it isn't a "resume" as much as it is
>> an "incremental retry." This can be useful in the case of extremely large
>> backups that fail considerably through the operation and we'd like to not waste
>> the work that was already performed.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  qapi/block-core.json |  5 ++++-
>>  block/backup.c       | 10 ++++++----
>>  2 files changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index 0332dcaabc..58d267f1f5 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -1143,6 +1143,9 @@
>>  # An enumeration of possible behaviors for the synchronization of a bitmap
>>  # when used for data copy operations.
>>  #
>> +# @always: The bitmap is always synchronized with remaining blocks to copy,
>> +#          whether or not the operation has completed successfully or not.
>> +#
>>  # @conditional: The bitmap is only synchronized when the operation is successul.
>>  #               This is useful for Incremental semantics.
>>  #
>> @@ -1153,7 +1156,7 @@
>>  # Since: 4.1
>>  ##
>>  { 'enum': 'BitmapSyncMode',
>> -  'data': ['conditional', 'never'] }
>> +  'data': ['always', 'conditional', 'never'] }
>>  
>>  ##
>>  # @MirrorCopyMode:
>> diff --git a/block/backup.c b/block/backup.c
>> index 627f724b68..beb2078696 100644
>> --- a/block/backup.c
>> +++ b/block/backup.c
>> @@ -266,15 +266,17 @@ static void backup_cleanup_sync_bitmap(BackupBlockJob *job, int ret)
>>      BlockDriverState *bs = blk_bs(job->common.blk);
>>  
>>      if (ret < 0 || job->bitmap_mode == BITMAP_SYNC_MODE_NEVER) {
>> -        /* Failure, or we don't want to synchronize the bitmap.
>> -         * Merge the successor back into the parent, delete nothing. */
>> +        /* Failure, or we don't want to synchronize the bitmap. */
>> +        if (job->bitmap_mode == BITMAP_SYNC_MODE_ALWAYS) {
>> +            bdrv_dirty_bitmap_claim(job->sync_bitmap, &job->copy_bitmap);
> 
> Hmm...  OK, bitmaps in backup always confuse me, so bear with me, please.
> 

I realize this is an extremely dense section that actually covers a
*lot* of pathways.

> (Hi, I’m a time traveler from the end of this section and I can tell you
> that everything is fine.  I was just confused.  I’ll still keep this
> here, because it was so much work.)
> 
> The copy_bitmap is copied from the sync_bitmap at the beginning, so the
> sync_bitmap can continue to be dirtied, but that won’t affect the job.
> In normal incremental mode, this means that the sync point is always at
> the beginning of the job.  (Well, naturally, because that’s how backup
> is supposed to go.)
> 

sync_bitmap: This is used as an initial manifest for which sectors to
copy out. It is the user-provided bitmap. We actually *never* edit this
bitmap in the body of the job.

copy_bitmap: This is the manifest for which blocks remain to be copied
out. We clear bits in this as we go, because we use it as our loop
condition.

So what you say is actually only half-true: the sync_bitmap actually
remains static during the duration of the job, and it has an anonymous
child that accrues new writes. This is a holdover from before we had a
copy_bitmap, and we used to use a sync_bitmap directly as our loop
condition.

(This could be simplified upstream at present; but after this patch it
cannot be for reasons explained below. We do wish to maintain three
distinct sets of bits:
1. The bits at the start of the operation,
2. The bits accrued during the operation, and
3. The bits that remain to be, or were not, copied during the operation.)

So there's actually three bitmaps:

- sync_bitmap: actually just static and read-only
- sync_bitmap's anonymous child: accrues new writes.
- copy_bitmap: loop conditional.

> But then replacing the sync_bitmap with the copy_bitmap here means that
> all of these dirtyings that happened during the job are lost.  Hmm, but
> that doesn’t matter, does it?  Because whenever something was dirtied in
> sync_bitmap, the corresponding area must have been copied to the backup
> due to the job.
> 

The new dirty bits were accrued very secretly in the anonymous child.
The new dirty bits are merged in via the reclaim() function.

So, what happens is:

- Sync_bitmap gets the bit pattern of copy_bitmap (one way or another)
- Sync_bitmap reclaims (merges with) its anonymous child.

> Ah, yes, it would actually be wrong to keep the new dirty bits, because
> in this mode, sync_bitmap should (on failure) reflect what is left to
> copy to make the backup complete.  Copying these newly dirtied sectors
> would be wrong.  (Yes, I know you wrote that in the documentation of
> @always.  I just tried to get a different perspective.)
> 
> Yes, yes, and copy_bitmap is always set whenever a CBW to the target
> fails before the source can be updated.  Good, good.
> 

You might have slightly the wrong idea; it's important to keep track of
what was dirtied during the operation because that data is important for
the next bitmap backup.

The merging of "sectors left to copy" (in the case of a failed backup)
and "sectors dirtied since we started the operation" forms the actual
minimal set needed to re-write to this target to achieve a new
functioning point in time. This is what you get with the "always" mode
in a failure case.

In a success case, it just so happens that "sectors left to copy" is the
empty set.

It's like an incremental on top of the incremental.

Consider this:

We have a 4TB drive and we have dirtied 3TB of it since our full backup.
We copy out 2TB as part of a new incremental backup before suffering
some kind of failure.

Today, you'd need to start a new incremental backup that copies that
entire 3TB *plus* whatever was dirtied since the job failed.

With this mode, you'd only need to copy the remaining 1TB + whatever was
dirtied since.

So, what this logic is really doing is:

If we failed, OR if we want the "never" sync policy:

Merge the anonymous child (bits written during op) back into sync_bitmap
(bits we were instructed to copy), leaving us as if we have never
started this operation.

If, however, we failed and we have the "always" sync policy, we destroy
the sync_bitmap (bits we were instructed to copy) and replace it with
the copy_bitmap (bits remaining to copy). Then, we merge that with the
anonymous child (bits written during op).

Or, in success cases (when sync policy is not never), we simply delete
the sync_bitmap (bits we were instructed to copy) and replace it with
its anonymous child (bits written during op).

> 
> Hi, I’m the time traveler from above.  I also left the section here so I
> can give one of my trademark “Ramble, ramble,
> 
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> 
> ”
> 
>> +        }
>> +        /* Merge the successor back into the parent. */
>>          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);
>>      }
>> +    assert(bm);
>>  }
>>  
>>  static void backup_commit(Job *job)
>>
> 
> 

-- 
—js

Re: [Qemu-devel] [PATCH 07/12] block/backup: add 'always' bitmap sync policy

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 8346 bytes --]

On 20.06.19 20:44, John Snow wrote:
> 
> 
> On 6/20/19 1:00 PM, Max Reitz wrote:
>> On 20.06.19 03:03, John Snow wrote:
>>> This adds an "always" policy for bitmap synchronization. Regardless of if
>>> the job succeeds or fails, the bitmap is *always* synchronized. This means
>>> that for backups that fail part-way through, the bitmap retains a record of
>>> which sectors need to be copied out to accomplish a new backup using the
>>> old, partial result.
>>>
>>> In effect, this allows us to "resume" a failed backup; however the new backup
>>> will be from the new point in time, so it isn't a "resume" as much as it is
>>> an "incremental retry." This can be useful in the case of extremely large
>>> backups that fail considerably through the operation and we'd like to not waste
>>> the work that was already performed.
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>>  qapi/block-core.json |  5 ++++-
>>>  block/backup.c       | 10 ++++++----
>>>  2 files changed, 10 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>> index 0332dcaabc..58d267f1f5 100644
>>> --- a/qapi/block-core.json
>>> +++ b/qapi/block-core.json
>>> @@ -1143,6 +1143,9 @@
>>>  # An enumeration of possible behaviors for the synchronization of a bitmap
>>>  # when used for data copy operations.
>>>  #
>>> +# @always: The bitmap is always synchronized with remaining blocks to copy,
>>> +#          whether or not the operation has completed successfully or not.
>>> +#
>>>  # @conditional: The bitmap is only synchronized when the operation is successul.
>>>  #               This is useful for Incremental semantics.
>>>  #
>>> @@ -1153,7 +1156,7 @@
>>>  # Since: 4.1
>>>  ##
>>>  { 'enum': 'BitmapSyncMode',
>>> -  'data': ['conditional', 'never'] }
>>> +  'data': ['always', 'conditional', 'never'] }
>>>  
>>>  ##
>>>  # @MirrorCopyMode:
>>> diff --git a/block/backup.c b/block/backup.c
>>> index 627f724b68..beb2078696 100644
>>> --- a/block/backup.c
>>> +++ b/block/backup.c
>>> @@ -266,15 +266,17 @@ static void backup_cleanup_sync_bitmap(BackupBlockJob *job, int ret)
>>>      BlockDriverState *bs = blk_bs(job->common.blk);
>>>  
>>>      if (ret < 0 || job->bitmap_mode == BITMAP_SYNC_MODE_NEVER) {
>>> -        /* Failure, or we don't want to synchronize the bitmap.
>>> -         * Merge the successor back into the parent, delete nothing. */
>>> +        /* Failure, or we don't want to synchronize the bitmap. */
>>> +        if (job->bitmap_mode == BITMAP_SYNC_MODE_ALWAYS) {
>>> +            bdrv_dirty_bitmap_claim(job->sync_bitmap, &job->copy_bitmap);
>>
>> Hmm...  OK, bitmaps in backup always confuse me, so bear with me, please.
>>
> 
> I realize this is an extremely dense section that actually covers a
> *lot* of pathways.
> 
>> (Hi, I’m a time traveler from the end of this section and I can tell you
>> that everything is fine.  I was just confused.  I’ll still keep this
>> here, because it was so much work.)
>>
>> The copy_bitmap is copied from the sync_bitmap at the beginning, so the
>> sync_bitmap can continue to be dirtied, but that won’t affect the job.
>> In normal incremental mode, this means that the sync point is always at
>> the beginning of the job.  (Well, naturally, because that’s how backup
>> is supposed to go.)
>>
> 
> sync_bitmap: This is used as an initial manifest for which sectors to
> copy out. It is the user-provided bitmap. We actually *never* edit this
> bitmap in the body of the job.
> 
> copy_bitmap: This is the manifest for which blocks remain to be copied
> out. We clear bits in this as we go, because we use it as our loop
> condition.
> 
> So what you say is actually only half-true: the sync_bitmap actually
> remains static during the duration of the job, and it has an anonymous
> child that accrues new writes. This is a holdover from before we had a
> copy_bitmap, and we used to use a sync_bitmap directly as our loop
> condition.
> 
> (This could be simplified upstream at present; but after this patch it
> cannot be for reasons explained below. We do wish to maintain three
> distinct sets of bits:
> 1. The bits at the start of the operation,
> 2. The bits accrued during the operation, and
> 3. The bits that remain to be, or were not, copied during the operation.)
> 
> So there's actually three bitmaps:
> 
> - sync_bitmap: actually just static and read-only
> - sync_bitmap's anonymous child: accrues new writes.

Ah, right...  Thanks for writing that up.

> - copy_bitmap: loop conditional.
> 
>> But then replacing the sync_bitmap with the copy_bitmap here means that
>> all of these dirtyings that happened during the job are lost.  Hmm, but
>> that doesn’t matter, does it?  Because whenever something was dirtied in
>> sync_bitmap, the corresponding area must have been copied to the backup
>> due to the job.
>>
> 
> The new dirty bits were accrued very secretly in the anonymous child.
> The new dirty bits are merged in via the reclaim() function.
> 
> So, what happens is:
> 
> - Sync_bitmap gets the bit pattern of copy_bitmap (one way or another)
> - Sync_bitmap reclaims (merges with) its anonymous child.
> 
>> Ah, yes, it would actually be wrong to keep the new dirty bits, because
>> in this mode, sync_bitmap should (on failure) reflect what is left to
>> copy to make the backup complete.  Copying these newly dirtied sectors
>> would be wrong.  (Yes, I know you wrote that in the documentation of
>> @always.  I just tried to get a different perspective.)
>>
>> Yes, yes, and copy_bitmap is always set whenever a CBW to the target
>> fails before the source can be updated.  Good, good.
>>
> 
> You might have slightly the wrong idea; it's important to keep track of
> what was dirtied during the operation because that data is important for
> the next bitmap backup.
> 
> The merging of "sectors left to copy" (in the case of a failed backup)
> and "sectors dirtied since we started the operation" forms the actual
> minimal set needed to re-write to this target to achieve a new
> functioning point in time. This is what you get with the "always" mode
> in a failure case.
> 
> In a success case, it just so happens that "sectors left to copy" is the
> empty set.
> 
> It's like an incremental on top of the incremental.
> 
> Consider this:
> 
> We have a 4TB drive and we have dirtied 3TB of it since our full backup.
> We copy out 2TB as part of a new incremental backup before suffering
> some kind of failure.
> 
> Today, you'd need to start a new incremental backup that copies that
> entire 3TB *plus* whatever was dirtied since the job failed.
> 
> With this mode, you'd only need to copy the remaining 1TB + whatever was
> dirtied since.
> 
> So, what this logic is really doing is:
> 
> If we failed, OR if we want the "never" sync policy:
> 
> Merge the anonymous child (bits written during op) back into sync_bitmap
> (bits we were instructed to copy), leaving us as if we have never
> started this operation.
> 
> If, however, we failed and we have the "always" sync policy, we destroy
> the sync_bitmap (bits we were instructed to copy) and replace it with
> the copy_bitmap (bits remaining to copy). Then, we merge that with the
> anonymous child (bits written during op).

Oh, so that’s the way it works.  I thought “always” meant that you can
repeat the backup.  But it just means you keep your partial backup and
pretend it’s a full incremental one.

Now that I think about it again...  Yeah, you can’t repeat a backup at a
later point, of course.  If data is gone in the meantime, it’s gone.

So, uh, I was wrong that it’s all good, because it would have been
wrong?  But thankfully I was just wrong myself, and so it is all good
after all?  My confusion with bitmaps as lifted, now I’m just confused
with myself.

I revoke my R-b and give a new one:

Reviewed-by: Max Reitz <mreitz@redhat.com>

Or something like that.

Again, thanks a lot for clarifying.

Max

> Or, in success cases (when sync policy is not never), we simply delete
> the sync_bitmap (bits we were instructed to copy) and replace it with
> its anonymous child (bits written during op).


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 07/12] block/backup: add 'always' bitmap sync policy

[Vladimir Sementsov-Ogievskiy]

20.06.2019 4:03, John Snow wrote:
> This adds an "always" policy for bitmap synchronization. Regardless of if
> the job succeeds or fails, the bitmap is *always* synchronized. This means
> that for backups that fail part-way through, the bitmap retains a record of
> which sectors need to be copied out to accomplish a new backup using the
> old, partial result.
> 
> In effect, this allows us to "resume" a failed backup; however the new backup
> will be from the new point in time, so it isn't a "resume" as much as it is
> an "incremental retry." This can be useful in the case of extremely large
> backups that fail considerably through the operation and we'd like to not waste
> the work that was already performed.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   qapi/block-core.json |  5 ++++-
>   block/backup.c       | 10 ++++++----
>   2 files changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 0332dcaabc..58d267f1f5 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1143,6 +1143,9 @@
>   # An enumeration of possible behaviors for the synchronization of a bitmap
>   # when used for data copy operations.
>   #
> +# @always: The bitmap is always synchronized with remaining blocks to copy,
> +#          whether or not the operation has completed successfully or not.

Hmm, now I think that 'always' sounds a bit like 'really always' i.e. during backup
too, which is confusing.. But I don't have better suggestion.

> +#
>   # @conditional: The bitmap is only synchronized when the operation is successul.
>   #               This is useful for Incremental semantics.
>   #
> @@ -1153,7 +1156,7 @@
>   # Since: 4.1
>   ##
>   { 'enum': 'BitmapSyncMode',
> -  'data': ['conditional', 'never'] }
> +  'data': ['always', 'conditional', 'never'] }
>   
>   ##
>   # @MirrorCopyMode:
> diff --git a/block/backup.c b/block/backup.c
> index 627f724b68..beb2078696 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -266,15 +266,17 @@ static void backup_cleanup_sync_bitmap(BackupBlockJob *job, int ret)
>       BlockDriverState *bs = blk_bs(job->common.blk);
>   
>       if (ret < 0 || job->bitmap_mode == BITMAP_SYNC_MODE_NEVER) {
> -        /* Failure, or we don't want to synchronize the bitmap.
> -         * Merge the successor back into the parent, delete nothing. */
> +        /* Failure, or we don't want to synchronize the bitmap. */
> +        if (job->bitmap_mode == BITMAP_SYNC_MODE_ALWAYS) {
> +            bdrv_dirty_bitmap_claim(job->sync_bitmap, &job->copy_bitmap);
> +        }
> +        /* Merge the successor back into the parent. */
>           bm = bdrv_reclaim_dirty_bitmap(bs, job->sync_bitmap, NULL);

Hmm good, it should work. It's a lot more tricky, than just
"synchronized with remaining blocks to copy", but I'm not sure the we need more details in
spec.

What we have in backup? So, from one hand we have an incremental backup, and a bitmap, counting from it.
On the other hand it's not normal incremental backup, as it don't correspond to any valid state of vm disk,
and it may be used only as a backing in a chain of further successful incremental backup, yes?

And then I think: with this mode we can not stop on first error, but ignore it, just leaving dirty bit for
resulting bitmap.. We have BLOCKDEV_ON_ERROR_IGNORE, which may be used to achieve it, but seems it don't
work as expected, as in backup_loop() we retry operation if ret < 0 and  action != BLOCK_ERROR_ACTION_REPORT.

And another thought: can user take a decision of discarding (like CONDITIONAL) or saving in backing chain (like
ALWAYS) failed backup result _after_ backup job complete? For example, for small resulting backup it may be
better to discard it and for large - to save.
Will it work if we start job with ALWAYS mode and autocomplete = false, then on fail we can look at job progress,
and if it is small we cancel job, otherwise call complete? Or stop, block-job-complete will not work with failure
scenarios? Then we have to set BLOCKDEV_ON_ERROR_IGNORE, and on first error event decide, cancel or not? But we
can only cancel or continue..

Hmm. Cancel. So on cancel and abort you synchronize bitmap too? Seems in bad relation with what cancel should do,
and in transactions in general...


> -        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);
>       }
> +    assert(bm);
>   }
>   
>   static void backup_commit(Job *job)
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 07/12] block/backup: add 'always' bitmap sync policy

[Vladimir Sementsov-Ogievskiy]

21.06.2019 15:57, Vladimir Sementsov-Ogievskiy wrote:
> 20.06.2019 4:03, John Snow wrote:
>> This adds an "always" policy for bitmap synchronization. Regardless of if
>> the job succeeds or fails, the bitmap is *always* synchronized. This means
>> that for backups that fail part-way through, the bitmap retains a record of
>> which sectors need to be copied out to accomplish a new backup using the
>> old, partial result.
>>
>> In effect, this allows us to "resume" a failed backup; however the new backup
>> will be from the new point in time, so it isn't a "resume" as much as it is
>> an "incremental retry." This can be useful in the case of extremely large
>> backups that fail considerably through the operation and we'd like to not waste
>> the work that was already performed.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   qapi/block-core.json |  5 ++++-
>>   block/backup.c       | 10 ++++++----
>>   2 files changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index 0332dcaabc..58d267f1f5 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -1143,6 +1143,9 @@
>>   # An enumeration of possible behaviors for the synchronization of a bitmap
>>   # when used for data copy operations.
>>   #
>> +# @always: The bitmap is always synchronized with remaining blocks to copy,
>> +#          whether or not the operation has completed successfully or not.
> 
> Hmm, now I think that 'always' sounds a bit like 'really always' i.e. during backup
> too, which is confusing.. But I don't have better suggestion.
> 
>> +#
>>   # @conditional: The bitmap is only synchronized when the operation is successul.
>>   #               This is useful for Incremental semantics.
>>   #
>> @@ -1153,7 +1156,7 @@
>>   # Since: 4.1
>>   ##
>>   { 'enum': 'BitmapSyncMode',
>> -  'data': ['conditional', 'never'] }
>> +  'data': ['always', 'conditional', 'never'] }
>>   ##
>>   # @MirrorCopyMode:
>> diff --git a/block/backup.c b/block/backup.c
>> index 627f724b68..beb2078696 100644
>> --- a/block/backup.c
>> +++ b/block/backup.c
>> @@ -266,15 +266,17 @@ static void backup_cleanup_sync_bitmap(BackupBlockJob *job, int ret)
>>       BlockDriverState *bs = blk_bs(job->common.blk);
>>       if (ret < 0 || job->bitmap_mode == BITMAP_SYNC_MODE_NEVER) {
>> -        /* Failure, or we don't want to synchronize the bitmap.
>> -         * Merge the successor back into the parent, delete nothing. */
>> +        /* Failure, or we don't want to synchronize the bitmap. */
>> +        if (job->bitmap_mode == BITMAP_SYNC_MODE_ALWAYS) {
>> +            bdrv_dirty_bitmap_claim(job->sync_bitmap, &job->copy_bitmap);
>> +        }
>> +        /* Merge the successor back into the parent. */
>>           bm = bdrv_reclaim_dirty_bitmap(bs, job->sync_bitmap, NULL);
> 
> Hmm good, it should work. It's a lot more tricky, than just
> "synchronized with remaining blocks to copy", but I'm not sure the we need more details in
> spec.
> 
> What we have in backup? So, from one hand we have an incremental backup, and a bitmap, counting from it.
> On the other hand it's not normal incremental backup, as it don't correspond to any valid state of vm disk,
> and it may be used only as a backing in a chain of further successful incremental backup, yes?
> 
> And then I think: with this mode we can not stop on first error, but ignore it, just leaving dirty bit for
> resulting bitmap.. We have BLOCKDEV_ON_ERROR_IGNORE, which may be used to achieve it, but seems it don't
> work as expected, as in backup_loop() we retry operation if ret < 0 and  action != BLOCK_ERROR_ACTION_REPORT.
> 
> And another thought: can user take a decision of discarding (like CONDITIONAL) or saving in backing chain (like
> ALWAYS) failed backup result _after_ backup job complete? For example, for small resulting backup it may be
> better to discard it and for large - to save.
> Will it work if we start job with ALWAYS mode and autocomplete = false, then on fail we can look at job progress,
> and if it is small we cancel job, otherwise call complete? Or stop, block-job-complete will not work with failure
> scenarios? Then we have to set BLOCKDEV_ON_ERROR_IGNORE, and on first error event decide, cancel or not? But we
> can only cancel or continue..
> 
> Hmm. Cancel. So on cancel and abort you synchronize bitmap too? Seems in bad relation with what cancel should do,
> and in transactions in general...

I mean grouped transaction mode, how should it work with this?

> 
> 
>> -        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);
>>       }
>> +    assert(bm);
>>   }
>>   static void backup_commit(Job *job)
>>
> 
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 07/12] block/backup: add 'always' bitmap sync policy

[Vladimir Sementsov-Ogievskiy]

21.06.2019 15:59, Vladimir Sementsov-Ogievskiy wrote:
> 21.06.2019 15:57, Vladimir Sementsov-Ogievskiy wrote:
>> 20.06.2019 4:03, John Snow wrote:
>>> This adds an "always" policy for bitmap synchronization. Regardless of if
>>> the job succeeds or fails, the bitmap is *always* synchronized. This means
>>> that for backups that fail part-way through, the bitmap retains a record of
>>> which sectors need to be copied out to accomplish a new backup using the
>>> old, partial result.
>>>
>>> In effect, this allows us to "resume" a failed backup; however the new backup
>>> will be from the new point in time, so it isn't a "resume" as much as it is
>>> an "incremental retry." This can be useful in the case of extremely large
>>> backups that fail considerably through the operation and we'd like to not waste
>>> the work that was already performed.
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>>   qapi/block-core.json |  5 ++++-
>>>   block/backup.c       | 10 ++++++----
>>>   2 files changed, 10 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>> index 0332dcaabc..58d267f1f5 100644
>>> --- a/qapi/block-core.json
>>> +++ b/qapi/block-core.json
>>> @@ -1143,6 +1143,9 @@
>>>   # An enumeration of possible behaviors for the synchronization of a bitmap
>>>   # when used for data copy operations.
>>>   #
>>> +# @always: The bitmap is always synchronized with remaining blocks to copy,
>>> +#          whether or not the operation has completed successfully or not.
>>
>> Hmm, now I think that 'always' sounds a bit like 'really always' i.e. during backup
>> too, which is confusing.. But I don't have better suggestion.
>>
>>> +#
>>>   # @conditional: The bitmap is only synchronized when the operation is successul.
>>>   #               This is useful for Incremental semantics.
>>>   #
>>> @@ -1153,7 +1156,7 @@
>>>   # Since: 4.1
>>>   ##
>>>   { 'enum': 'BitmapSyncMode',
>>> -  'data': ['conditional', 'never'] }
>>> +  'data': ['always', 'conditional', 'never'] }
>>>   ##
>>>   # @MirrorCopyMode:
>>> diff --git a/block/backup.c b/block/backup.c
>>> index 627f724b68..beb2078696 100644
>>> --- a/block/backup.c
>>> +++ b/block/backup.c
>>> @@ -266,15 +266,17 @@ static void backup_cleanup_sync_bitmap(BackupBlockJob *job, int ret)
>>>       BlockDriverState *bs = blk_bs(job->common.blk);
>>>       if (ret < 0 || job->bitmap_mode == BITMAP_SYNC_MODE_NEVER) {
>>> -        /* Failure, or we don't want to synchronize the bitmap.
>>> -         * Merge the successor back into the parent, delete nothing. */
>>> +        /* Failure, or we don't want to synchronize the bitmap. */
>>> +        if (job->bitmap_mode == BITMAP_SYNC_MODE_ALWAYS) {
>>> +            bdrv_dirty_bitmap_claim(job->sync_bitmap, &job->copy_bitmap);
>>> +        }
>>> +        /* Merge the successor back into the parent. */
>>>           bm = bdrv_reclaim_dirty_bitmap(bs, job->sync_bitmap, NULL);
>>
>> Hmm good, it should work. It's a lot more tricky, than just
>> "synchronized with remaining blocks to copy", but I'm not sure the we need more details in
>> spec.
>>
>> What we have in backup? So, from one hand we have an incremental backup, and a bitmap, counting from it.
>> On the other hand it's not normal incremental backup, as it don't correspond to any valid state of vm disk,
>> and it may be used only as a backing in a chain of further successful incremental backup, yes?
>>
>> And then I think: with this mode we can not stop on first error, but ignore it, just leaving dirty bit for
>> resulting bitmap.. We have BLOCKDEV_ON_ERROR_IGNORE, which may be used to achieve it, but seems it don't
>> work as expected, as in backup_loop() we retry operation if ret < 0 and  action != BLOCK_ERROR_ACTION_REPORT.
>>
>> And another thought: can user take a decision of discarding (like CONDITIONAL) or saving in backing chain (like
>> ALWAYS) failed backup result _after_ backup job complete? For example, for small resulting backup it may be
>> better to discard it and for large - to save.
>> Will it work if we start job with ALWAYS mode and autocomplete = false, then on fail we can look at job progress,
>> and if it is small we cancel job, otherwise call complete? Or stop, block-job-complete will not work with failure
>> scenarios? Then we have to set BLOCKDEV_ON_ERROR_IGNORE, and on first error event decide, cancel or not? But we
>> can only cancel or continue..
>>
>> Hmm. Cancel. So on cancel and abort you synchronize bitmap too? Seems in bad relation with what cancel should do,
>> and in transactions in general...
> 
> I mean grouped transaction mode, how should it work with this?

Actual the problem is that you want to implement partial success, and block jobs api and transactions api are not prepared
for such thing

> 
>>
>>
>>> -        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);
>>>       }
>>> +    assert(bm);
>>>   }
>>>   static void backup_commit(Job *job)
>>>
>>
>>
> 
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 07/12] block/backup: add 'always' bitmap sync policy

[Vladimir Sementsov-Ogievskiy]

21.06.2019 16:08, Vladimir Sementsov-Ogievskiy wrote:
> 21.06.2019 15:59, Vladimir Sementsov-Ogievskiy wrote:
>> 21.06.2019 15:57, Vladimir Sementsov-Ogievskiy wrote:
>>> 20.06.2019 4:03, John Snow wrote:
>>>> This adds an "always" policy for bitmap synchronization. Regardless of if
>>>> the job succeeds or fails, the bitmap is *always* synchronized. This means
>>>> that for backups that fail part-way through, the bitmap retains a record of
>>>> which sectors need to be copied out to accomplish a new backup using the
>>>> old, partial result.
>>>>
>>>> In effect, this allows us to "resume" a failed backup; however the new backup
>>>> will be from the new point in time, so it isn't a "resume" as much as it is
>>>> an "incremental retry." This can be useful in the case of extremely large
>>>> backups that fail considerably through the operation and we'd like to not waste
>>>> the work that was already performed.
>>>>
>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>> ---
>>>>   qapi/block-core.json |  5 ++++-
>>>>   block/backup.c       | 10 ++++++----
>>>>   2 files changed, 10 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>>> index 0332dcaabc..58d267f1f5 100644
>>>> --- a/qapi/block-core.json
>>>> +++ b/qapi/block-core.json
>>>> @@ -1143,6 +1143,9 @@
>>>>   # An enumeration of possible behaviors for the synchronization of a bitmap
>>>>   # when used for data copy operations.
>>>>   #
>>>> +# @always: The bitmap is always synchronized with remaining blocks to copy,
>>>> +#          whether or not the operation has completed successfully or not.
>>>
>>> Hmm, now I think that 'always' sounds a bit like 'really always' i.e. during backup
>>> too, which is confusing.. But I don't have better suggestion.
>>>
>>>> +#
>>>>   # @conditional: The bitmap is only synchronized when the operation is successul.
>>>>   #               This is useful for Incremental semantics.
>>>>   #
>>>> @@ -1153,7 +1156,7 @@
>>>>   # Since: 4.1
>>>>   ##
>>>>   { 'enum': 'BitmapSyncMode',
>>>> -  'data': ['conditional', 'never'] }
>>>> +  'data': ['always', 'conditional', 'never'] }
>>>>   ##
>>>>   # @MirrorCopyMode:
>>>> diff --git a/block/backup.c b/block/backup.c
>>>> index 627f724b68..beb2078696 100644
>>>> --- a/block/backup.c
>>>> +++ b/block/backup.c
>>>> @@ -266,15 +266,17 @@ static void backup_cleanup_sync_bitmap(BackupBlockJob *job, int ret)
>>>>       BlockDriverState *bs = blk_bs(job->common.blk);
>>>>       if (ret < 0 || job->bitmap_mode == BITMAP_SYNC_MODE_NEVER) {
>>>> -        /* Failure, or we don't want to synchronize the bitmap.
>>>> -         * Merge the successor back into the parent, delete nothing. */
>>>> +        /* Failure, or we don't want to synchronize the bitmap. */
>>>> +        if (job->bitmap_mode == BITMAP_SYNC_MODE_ALWAYS) {
>>>> +            bdrv_dirty_bitmap_claim(job->sync_bitmap, &job->copy_bitmap);
>>>> +        }
>>>> +        /* Merge the successor back into the parent. */
>>>>           bm = bdrv_reclaim_dirty_bitmap(bs, job->sync_bitmap, NULL);
>>>
>>> Hmm good, it should work. It's a lot more tricky, than just
>>> "synchronized with remaining blocks to copy", but I'm not sure the we need more details in
>>> spec.
>>>
>>> What we have in backup? So, from one hand we have an incremental backup, and a bitmap, counting from it.
>>> On the other hand it's not normal incremental backup, as it don't correspond to any valid state of vm disk,
>>> and it may be used only as a backing in a chain of further successful incremental backup, yes?
>>>
>>> And then I think: with this mode we can not stop on first error, but ignore it, just leaving dirty bit for
>>> resulting bitmap.. We have BLOCKDEV_ON_ERROR_IGNORE, which may be used to achieve it, but seems it don't
>>> work as expected, as in backup_loop() we retry operation if ret < 0 and  action != BLOCK_ERROR_ACTION_REPORT.
>>>
>>> And another thought: can user take a decision of discarding (like CONDITIONAL) or saving in backing chain (like
>>> ALWAYS) failed backup result _after_ backup job complete? For example, for small resulting backup it may be
>>> better to discard it and for large - to save.
>>> Will it work if we start job with ALWAYS mode and autocomplete = false, then on fail we can look at job progress,
>>> and if it is small we cancel job, otherwise call complete? Or stop, block-job-complete will not work with failure
>>> scenarios? Then we have to set BLOCKDEV_ON_ERROR_IGNORE, and on first error event decide, cancel or not? But we
>>> can only cancel or continue..
>>>
>>> Hmm. Cancel. So on cancel and abort you synchronize bitmap too? Seems in bad relation with what cancel should do,
>>> and in transactions in general...
>>
>> I mean grouped transaction mode, how should it work with this?
> 
> Actual the problem is that you want to implement partial success, and block jobs api and transactions api are not prepared
> for such thing


Should it be OK if we just:

1. restrict using ALWAYS together with grouped transaction mode, so we don't need to deal with other job failures.
2. don't claim but only reclaim on cancel even in ALWAYS mode, to make cancel roll-back all things

?


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 07/12] block/backup: add 'always' bitmap sync policy

[John Snow]

On 6/21/19 9:44 AM, Vladimir Sementsov-Ogievskiy wrote:
> 21.06.2019 16:08, Vladimir Sementsov-Ogievskiy wrote:
>> 21.06.2019 15:59, Vladimir Sementsov-Ogievskiy wrote:
>>> 21.06.2019 15:57, Vladimir Sementsov-Ogievskiy wrote:

^ Home Run!

I'm going to reply to all four of these mails at once below, I'm sorry
for the words but I want to make sure I am being clear in my intent.

>>>> 20.06.2019 4:03, John Snow wrote:
>>>>> This adds an "always" policy for bitmap synchronization. Regardless of if
>>>>> the job succeeds or fails, the bitmap is *always* synchronized. This means
>>>>> that for backups that fail part-way through, the bitmap retains a record of
>>>>> which sectors need to be copied out to accomplish a new backup using the
>>>>> old, partial result.
>>>>>
>>>>> In effect, this allows us to "resume" a failed backup; however the new backup
>>>>> will be from the new point in time, so it isn't a "resume" as much as it is
>>>>> an "incremental retry." This can be useful in the case of extremely large
>>>>> backups that fail considerably through the operation and we'd like to not waste
>>>>> the work that was already performed.
>>>>>
>>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>>> ---
>>>>>   qapi/block-core.json |  5 ++++-
>>>>>   block/backup.c       | 10 ++++++----
>>>>>   2 files changed, 10 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>>>> index 0332dcaabc..58d267f1f5 100644
>>>>> --- a/qapi/block-core.json
>>>>> +++ b/qapi/block-core.json
>>>>> @@ -1143,6 +1143,9 @@
>>>>>   # An enumeration of possible behaviors for the synchronization of a bitmap
>>>>>   # when used for data copy operations.
>>>>>   #
>>>>> +# @always: The bitmap is always synchronized with remaining blocks to copy,
>>>>> +#          whether or not the operation has completed successfully or not.
>>>>
>>>> Hmm, now I think that 'always' sounds a bit like 'really always' i.e. during backup
>>>> too, which is confusing.. But I don't have better suggestion.
>>>>

I could probably clarify to say "at the conclusion of the operation",
but we should also keep in mind that bitmaps tied to an operation can't
be used during that timeframe anyway.

>>>>> +#
>>>>>   # @conditional: The bitmap is only synchronized when the operation is successul.
>>>>>   #               This is useful for Incremental semantics.
>>>>>   #
>>>>> @@ -1153,7 +1156,7 @@
>>>>>   # Since: 4.1
>>>>>   ##
>>>>>   { 'enum': 'BitmapSyncMode',
>>>>> -  'data': ['conditional', 'never'] }
>>>>> +  'data': ['always', 'conditional', 'never'] }
>>>>>   ##
>>>>>   # @MirrorCopyMode:
>>>>> diff --git a/block/backup.c b/block/backup.c
>>>>> index 627f724b68..beb2078696 100644
>>>>> --- a/block/backup.c
>>>>> +++ b/block/backup.c
>>>>> @@ -266,15 +266,17 @@ static void backup_cleanup_sync_bitmap(BackupBlockJob *job, int ret)
>>>>>       BlockDriverState *bs = blk_bs(job->common.blk);
>>>>>       if (ret < 0 || job->bitmap_mode == BITMAP_SYNC_MODE_NEVER) {
>>>>> -        /* Failure, or we don't want to synchronize the bitmap.
>>>>> -         * Merge the successor back into the parent, delete nothing. */
>>>>> +        /* Failure, or we don't want to synchronize the bitmap. */
>>>>> +        if (job->bitmap_mode == BITMAP_SYNC_MODE_ALWAYS) {
>>>>> +            bdrv_dirty_bitmap_claim(job->sync_bitmap, &job->copy_bitmap);
>>>>> +        }
>>>>> +        /* Merge the successor back into the parent. */
>>>>>           bm = bdrv_reclaim_dirty_bitmap(bs, job->sync_bitmap, NULL);
>>>>
>>>> Hmm good, it should work. It's a lot more tricky, than just
>>>> "synchronized with remaining blocks to copy", but I'm not sure the we need more details in
>>>> spec.
>>>>

Right, it's complicated because backups involve two points in time; the
start and finish of the operation. The actual technical truth of what
happens is hard to phrase succinctly.

It was difficult to phrase for even the normal Incremental/conditional
mode that we have.

I can't help but feel I need to write a blog post that has some good
diagrams that can be used to explain the concept clearly.

>>>> What we have in backup? So, from one hand we have an incremental backup, and a bitmap, counting from it.
>>>> On the other hand it's not normal incremental backup, as it don't correspond to any valid state of vm disk,
>>>> and it may be used only as a backing in a chain of further successful incremental backup, yes?
>>>>

You can also continue writing directly into it, which is likely the
smarter choice because it saves you the trouble of doing an intermediate
block commit later, and then you don't keep any image files that are
"meaningless" by themselves.

However, yes, iotest 257 uses them as backing images.

>>>> And then I think: with this mode we can not stop on first error, but ignore it, just leaving dirty bit for
>>>> resulting bitmap.. We have BLOCKDEV_ON_ERROR_IGNORE, which may be used to achieve it, but seems it don't
>>>> work as expected, as in backup_loop() we retry operation if ret < 0 and  action != BLOCK_ERROR_ACTION_REPORT.
>>>>

This strikes me as a good idea, but I wonder: if we retry already for
'ignore', it seems likely that transient network errors likely recover
on their own as a result already. Are there cases where we really want
the job to move forward, because we expect certain sectors will never
copy correctly, like reading from unreliable media? Are those cases ones
we expect to be able to fix later?

(Actually, what happens if we ignore errors and we get stuck on a
sector? How many times do we retry this before we give up and admit that
it's actually an error we can't ignore?)

The use cases aren't clear to me right away, but it's worth looking into
because it sounds like it could be useful. I think that should not be
part of this series, however.

>>>> And another thought: can user take a decision of discarding (like CONDITIONAL) or saving in backing chain (like
>>>> ALWAYS) failed backup result _after_ backup job complete? For example, for small resulting backup it may be
>>>> better to discard it and for large - to save.

That seems complicated, because you need to keep the bitmap split into
its component subsets (original, copy manifest, and writes since start)
all the way until AFTER the job, which means more bitmap management
commands that need to be issued after the job is done.

Which means the job would move the bitmap into yet another new state
where it is "busy" and cannot be used, but is awaiting some kind of
rollover command from the user.

However, you could also just use our 'merge' support to make a copy of
the bitmap before you begin and use the 'always' sync mode, then if you
decide it's not worth restarting after the fact, you can just delete the
copy.

>>>> Will it work if we start job with ALWAYS mode and autocomplete = false, then on fail we can look at job progress,
>>>> and if it is small we cancel job, otherwise call complete? Or stop, block-job-complete will not work with failure
>>>> scenarios? Then we have to set BLOCKDEV_ON_ERROR_IGNORE, and on first error event decide, cancel or not? But we
>>>> can only cancel or continue..
>>>>
>>>> Hmm. Cancel. So on cancel and abort you synchronize bitmap too? Seems in bad relation with what cancel should do,
>>>> and in transactions in general...
>>>
>>> I mean grouped transaction mode, how should it work with this?
>>
>> Actual the problem is that you want to implement partial success, and block jobs api and transactions api are not prepared
>> for such thing

I wouldn't call it partial success, but rather a "failure with detailed
error log" -- but I concede I am playing games with terminology.

The operation failed, but the bitmap can be considered a record of
exactly which bitmap regions didn't succeed in being copied.

You're right, though; the regions that got cleared could be considered a
record of partial success; but I think I might resist the idea of
wanting to formalize that in a new API. I think it's easier to
conceptualize it as a recoverable failure, and the bitmap behaves as the
resume/recovery data.

> 
> 
> Should it be OK if we just:
> 
> 1. restrict using ALWAYS together with grouped transaction mode, so we don't need to deal with other job failures.
> 2. don't claim but only reclaim on cancel even in ALWAYS mode, to make cancel roll-back all things
> 
> ?
> 
> 

> "grouped transaction mode, how should it work with this?"

With or without the grouped completion mode, it does the same thing: it
ALWAYS synchronizes!

Yes, that means that:

1. In the case of user cancellation, it still saves partial work.
2. In the case of an induced cancellation from a peer job, it saves
partial work.

I think this behavior is correct because grouped completion mode does
not actually guarantee that jobs that are already running clean up
completely as if they were never launched; that is, we cannot undo the
fact that we DID copy data out to a target. Therefore, because we
factually DID perform some work, this mode simply preserves a record of
what DID occur, in the case that the client prefers to salvage partial
work instead of restarting from scratch.

Just because we launched this as part of a transaction, in other words,
does not seem like a good case for negating the intention of the user to
be able to resume from failures if they occur.

I realize this seems like a strange usage of a transaction -- because
state from the transaction can escape a failed transaction -- but real
database systems in practice do allow the ability to do partial unwinds
to minimize the amount of work that needs to be redone. I don't think
this is too surprising -- and it happens ONLY when the user specifically
requests it, so I believe this is safe behavior.

I do agree that it *probably* doesn't make sense to use these together
-- it will work just fine, but why would you be okay with one type of
partial completion when you're not ok with another? I don't understand
why you'd ask for it, but I think it will do exactly what you ask for.
It's actually less code to just let it work like this.


> "So on cancel and abort you synchronize bitmap too?"

I will concede that this means that if you ask for a bitmap backup with
the 'always' policy and, for whatever reason change your mind about
this, there's no way to "cancel" the job in a manner that does not edit
the bitmap at this point.

I do agree that this seems to go against the wishes of the user, because
we have different "kinds" of cancellations:

A) Cancellations that actually represent failures in transactions
B) Cancellations that represent genuine user intention

It might be nice to allow the user to say "No wait, please don't edit
that bitmap, I made a mistake!"

In these cases, how about a slight overloading of the "forced" user
cancellation? For mirror, a "forced" cancel means "Please don't try to
sync the mirror, just exit immediately." whereas an unforced cancel
means "Please complete!"

For backup, it could mean something similar:

force: "Please exit immediately, don't even sync the bitmap!"
!force: "Exit, but proceed with normal cleanup."

This would mean that in the grouped completion failure case, we actually
still sync the bitmap, but if the user sends a forced-cancel, they can
actually abort the entire transaction.

Actually, that needs just a minor edit:

job.c:746:

job_cancel_async(other_job, false);

should become:

job_cancel_async(other_job, job->force_cancel)


And then the bitmap cleanup code can check for this condition to avoid
synchronizing the bitmap in these cases.

Re: [Qemu-devel] [PATCH 07/12] block/backup: add 'always' bitmap sync policy

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 2035 bytes --]

On 21.06.19 22:58, John Snow wrote:
> 
> 
> On 6/21/19 9:44 AM, Vladimir Sementsov-Ogievskiy wrote:

[...]

Just chiming in on this:

>> "So on cancel and abort you synchronize bitmap too?"
> 
> I will concede that this means that if you ask for a bitmap backup with
> the 'always' policy and, for whatever reason change your mind about
> this, there's no way to "cancel" the job in a manner that does not edit
> the bitmap at this point.
> 
> I do agree that this seems to go against the wishes of the user, because
> we have different "kinds" of cancellations:
> 
> A) Cancellations that actually represent failures in transactions
> B) Cancellations that represent genuine user intention
> 
> It might be nice to allow the user to say "No wait, please don't edit
> that bitmap, I made a mistake!"

So that “always” doesn’t mean “always”?  To me, that seems like not so
good an idea.

If the user uses always, they have to live with that.  I had to live
with calling “rm” on the wrong file before.  Life’s tough.

In all seriousness: “Always” is not something a user would use, is it?
It’s something for management tools.  Why would they cancel because
“They made a mistake”?

Second, what’s the worst thing that may come out of such a mistake?
Having to perform a full backup?  If so, that doesn’t seem so bad to me.
 It certainly doesn’t seem so bad to make an unrelated mechanic have an
influence on whether “always” means “always”.

Also, this cancel idea would only work for jobs where the bitmap mode
does not come into play until the job is done, i.e. backup.  I suppose
if we want to have bitmap modes other than 'always' for mirror, that too
would have to make a copy of the user-supplied bitmap, so there the
bitmap mode would make a difference only at the end of the job, too, but
who knows.

And if it only makes a difference at the end of the job, you might as
well just add a way to change a running job’s bitmap-mode.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 07/12] block/backup: add 'always' bitmap sync policy

[John Snow]

On 6/21/19 5:48 PM, Max Reitz wrote:
> On 21.06.19 22:58, John Snow wrote:
>>
>>
>> On 6/21/19 9:44 AM, Vladimir Sementsov-Ogievskiy wrote:
> 
> [...]
> 
> Just chiming in on this:
> 
>>> "So on cancel and abort you synchronize bitmap too?"
>>
>> I will concede that this means that if you ask for a bitmap backup with
>> the 'always' policy and, for whatever reason change your mind about
>> this, there's no way to "cancel" the job in a manner that does not edit
>> the bitmap at this point.
>>
>> I do agree that this seems to go against the wishes of the user, because
>> we have different "kinds" of cancellations:
>>
>> A) Cancellations that actually represent failures in transactions
>> B) Cancellations that represent genuine user intention
>>
>> It might be nice to allow the user to say "No wait, please don't edit
>> that bitmap, I made a mistake!"
> 
> So that “always” doesn’t mean “always”?  To me, that seems like not so
> good an idea.
> 
> If the user uses always, they have to live with that.  I had to live
> with calling “rm” on the wrong file before.  Life’s tough.
> 

I actually agree, but I was making a concession in the ONE conceivable
case where you would theoretically want to abort "always".

> In all seriousness: “Always” is not something a user would use, is it?
> It’s something for management tools.  Why would they cancel because
> “They made a mistake”?
> 

A user might use it -- it's an attractive mode. It's basically
Incremental with retry ability. It is designed for use by a management
utility though, yes.

> Second, what’s the worst thing that may come out of such a mistake?
> Having to perform a full backup?  If so, that doesn’t seem so bad to me.
>  It certainly doesn’t seem so bad to make an unrelated mechanic have an
> influence on whether “always” means “always”.
> 

No, if you "accidentally" issue always (and you change your mind for
whatever reason), the correct way to fix this is:

(1) If the job completes successfully, nothing. Everything is situation
normal. This behaves exactly like "incremental" mode.

(2) If the job fails so hard you don't succeed in writing data anywhere
at all, nothing. Everything is fine. This behaves exactly like a failure
in "incremental" mode. The only way to reliably tell if this happened is
if job never even succeeded in creating a target for you, or your target
is still verifiably empty. (Even so: good practice would be to never
delete a target if you used 'always' mode.)

(3) If the job fails after writing SOME data, you simply issue another
mode=bitmap policy=always against the same target. (Presumably after
fixing your network or clearing room on the target storage.)

The worst mistake you can make is this:

- Issue sync=bitmap policy=always
- Cancel the job because it's taking too long, and you are impatient
- Forget that you used "always", delete the incomplete backup target

Oops! That had data that QEMU was counting on having written out
already. Your bitmap is now garbage.

You fix this with a full backup, yes.

> Also, this cancel idea would only work for jobs where the bitmap mode
> does not come into play until the job is done, i.e. backup.  I suppose
> if we want to have bitmap modes other than 'always' for mirror, that too
> would have to make a copy of the user-supplied bitmap, so there the
> bitmap mode would make a difference only at the end of the job, too, but
> who knows.
> 

Reasonable point; at the moment I modeled bitmap support for mirror to
only do synchronization at the end of the job as well. In this case,
"soft cancels" are modeled (upstream, today) as ret == 0, so those won't
count as failures at all.

(And, actually, force cancels will count as real failures. So maybe it
IS best not to overload this already hacky semantic we have on cancel.)

> And if it only makes a difference at the end of the job, you might as
> well just add a way to change a running job’s bitmap-mode.
> 

This is prescient. I have wanted a "completion-mode" for mirror that you
can change during its runtime (and to deprecate cancel as a way to
"complete" the job) for a very long time.

It's just that the QAPI for it always seems ugly so I shy away from it.

> Max
> 

So, I will say this:

1) I think the implementation of "always" is perfectly correct, in
single, transaction, or grouped-completion transaction modes.

2) Some of these combinations don't make much practical sense, but it's
more work to disallow them, and past experience reminds me that it's not
my job to save the user from themselves at the primitive level.

3) Adding nicer features like "I want a different completion mode since
i started this job" don't exist for any other mode or any other job
right now, and I don't think I will add them to this series.

[Qemu-devel] [PATCH 08/12] iotests: add testing shim for script-style python tests

[John Snow]

Because the new-style python tests don't use the iotests.main() test
launcher, we don't turn on the debugger logging for these scripts
when invoked via ./check -d.

Refactor the launcher shim into new and old style shims so that they
share environmental configuration.

Two cleanup notes: debug was not actually used as a global, and there
was no reason to create a class in an inner scope just to achieve
default variables; we can simply create an instance of the runner with
the values we want instead.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/iotests.py | 40 +++++++++++++++++++++++------------
 1 file changed, 26 insertions(+), 14 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 3ecef5bc90..fcad957d63 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -61,7 +61,6 @@ cachemode = os.environ.get('CACHEMODE')
 qemu_default_machine = os.environ.get('QEMU_DEFAULT_MACHINE')
 
 socket_scm_helper = os.environ.get('SOCKET_SCM_HELPER', 'socket_scm_helper')
-debug = False
 
 luks_default_secret_object = 'secret,id=keysec0,data=' + \
                              os.environ.get('IMGKEYSECRET', '')
@@ -834,11 +833,22 @@ def skip_if_unsupported(required_formats=[], read_only=False):
         return func_wrapper
     return skip_test_decorator
 
-def main(supported_fmts=[], supported_oses=['linux'], supported_cache_modes=[],
-         unsupported_fmts=[]):
-    '''Run tests'''
+def execute_unittest(output, verbosity, debug):
+    runner = unittest.TextTestRunner(stream=output, descriptions=True,
+                                     verbosity=verbosity)
+    try:
+        # unittest.main() will use sys.exit(); so expect a SystemExit
+        # exception
+        unittest.main(testRunner=runner)
+    finally:
+        if not debug:
+            sys.stderr.write(re.sub(r'Ran (\d+) tests? in [\d.]+s',
+                                    r'Ran \1 tests', output.getvalue()))
 
-    global debug
+def execute_test(test_function=None,
+                 supported_fmts=[], supported_oses=['linux'],
+                 supported_cache_modes=[], unsupported_fmts=[]):
+    """Run either unittest or script-style tests."""
 
     # We are using TEST_DIR and QEMU_DEFAULT_MACHINE as proxies to
     # indicate that we're not being run via "check". There may be
@@ -870,13 +880,15 @@ def main(supported_fmts=[], supported_oses=['linux'], supported_cache_modes=[],
 
     logging.basicConfig(level=(logging.DEBUG if debug else logging.WARN))
 
-    class MyTestRunner(unittest.TextTestRunner):
-        def __init__(self, stream=output, descriptions=True, verbosity=verbosity):
-            unittest.TextTestRunner.__init__(self, stream, descriptions, verbosity)
+    if not test_function:
+        execute_unittest(output, verbosity, debug)
+    else:
+        test_function()
 
-    # unittest.main() will use sys.exit() so expect a SystemExit exception
-    try:
-        unittest.main(testRunner=MyTestRunner)
-    finally:
-        if not debug:
-            sys.stderr.write(re.sub(r'Ran (\d+) tests? in [\d.]+s', r'Ran \1 tests', output.getvalue()))
+def script_main(test_function, *args, **kwargs):
+    """Run script-style tests outside of the unittest framework"""
+    execute_test(test_function, *args, **kwargs)
+
+def main(*args, **kwargs):
+    """Run tests using the unittest framework"""
+    execute_test(None, *args, **kwargs)
-- 
2.21.0

Re: [Qemu-devel] [PATCH 08/12] iotests: add testing shim for script-style python tests

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 951 bytes --]

On 20.06.19 03:03, John Snow wrote:
> Because the new-style python tests don't use the iotests.main() test
> launcher, we don't turn on the debugger logging for these scripts
> when invoked via ./check -d.
> 
> Refactor the launcher shim into new and old style shims so that they
> share environmental configuration.
> 
> Two cleanup notes: debug was not actually used as a global, and there
> was no reason to create a class in an inner scope just to achieve
> default variables; we can simply create an instance of the runner with
> the values we want instead.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  tests/qemu-iotests/iotests.py | 40 +++++++++++++++++++++++------------
>  1 file changed, 26 insertions(+), 14 deletions(-)

I don’t quite get how script_main() works (yes, both my Pythonfu and my
Googlefu are that bad), but it works and looks good, so have a

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 08/12] iotests: add testing shim for script-style python tests

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 1200 bytes --]

On 20.06.19 19:09, Max Reitz wrote:
> On 20.06.19 03:03, John Snow wrote:
>> Because the new-style python tests don't use the iotests.main() test
>> launcher, we don't turn on the debugger logging for these scripts
>> when invoked via ./check -d.
>>
>> Refactor the launcher shim into new and old style shims so that they
>> share environmental configuration.
>>
>> Two cleanup notes: debug was not actually used as a global, and there
>> was no reason to create a class in an inner scope just to achieve
>> default variables; we can simply create an instance of the runner with
>> the values we want instead.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  tests/qemu-iotests/iotests.py | 40 +++++++++++++++++++++++------------
>>  1 file changed, 26 insertions(+), 14 deletions(-)
> 
> I don’t quite get how script_main() works (yes, both my Pythonfu and my
> Googlefu are that bad), but it works and looks good, so have a

Oh, it doesn’t work (well, not automagically).  I just assumed seeing
the log output means it’s working.  Seeing that the test needs to call
iotests.script_main() explicitly does clear up my confusion.

All OK with me.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 08/12] iotests: add testing shim for script-style python tests

[John Snow]

On 6/20/19 1:26 PM, Max Reitz wrote:
> On 20.06.19 19:09, Max Reitz wrote:
>> On 20.06.19 03:03, John Snow wrote:
>>> Because the new-style python tests don't use the iotests.main() test
>>> launcher, we don't turn on the debugger logging for these scripts
>>> when invoked via ./check -d.
>>>
>>> Refactor the launcher shim into new and old style shims so that they
>>> share environmental configuration.
>>>
>>> Two cleanup notes: debug was not actually used as a global, and there
>>> was no reason to create a class in an inner scope just to achieve
>>> default variables; we can simply create an instance of the runner with
>>> the values we want instead.
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>>  tests/qemu-iotests/iotests.py | 40 +++++++++++++++++++++++------------
>>>  1 file changed, 26 insertions(+), 14 deletions(-)
>>
>> I don’t quite get how script_main() works (yes, both my Pythonfu and my
>> Googlefu are that bad), but it works and looks good, so have a
> 
> Oh, it doesn’t work (well, not automagically).  I just assumed seeing
> the log output means it’s working.  Seeing that the test needs to call
> iotests.script_main() explicitly does clear up my confusion.
> 
> All OK with me.
> 
> Max
> 

Yes. I should convert the others to opt-in to the new format so that
copy-paste in the future will get us the right paradigm.

Tests just need to be refactored to have a single point of entry so it
can be passed as a closure to the test runner.

If this seems like a good change I will do that as a follow-up series
with only the churn.

--js

Re: [Qemu-devel] [PATCH 08/12] iotests: add testing shim for script-style python tests

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 1897 bytes --]

On 20.06.19 20:47, John Snow wrote:
> 
> 
> On 6/20/19 1:26 PM, Max Reitz wrote:
>> On 20.06.19 19:09, Max Reitz wrote:
>>> On 20.06.19 03:03, John Snow wrote:
>>>> Because the new-style python tests don't use the iotests.main() test
>>>> launcher, we don't turn on the debugger logging for these scripts
>>>> when invoked via ./check -d.
>>>>
>>>> Refactor the launcher shim into new and old style shims so that they
>>>> share environmental configuration.
>>>>
>>>> Two cleanup notes: debug was not actually used as a global, and there
>>>> was no reason to create a class in an inner scope just to achieve
>>>> default variables; we can simply create an instance of the runner with
>>>> the values we want instead.
>>>>
>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>> ---
>>>>  tests/qemu-iotests/iotests.py | 40 +++++++++++++++++++++++------------
>>>>  1 file changed, 26 insertions(+), 14 deletions(-)
>>>
>>> I don’t quite get how script_main() works (yes, both my Pythonfu and my
>>> Googlefu are that bad), but it works and looks good, so have a
>>
>> Oh, it doesn’t work (well, not automagically).  I just assumed seeing
>> the log output means it’s working.  Seeing that the test needs to call
>> iotests.script_main() explicitly does clear up my confusion.
>>
>> All OK with me.
>>
>> Max
>>
> 
> Yes. I should convert the others to opt-in to the new format so that
> copy-paste in the future will get us the right paradigm.
> 
> Tests just need to be refactored to have a single point of entry so it
> can be passed as a closure to the test runner.
> 
> If this seems like a good change I will do that as a follow-up series
> with only the churn.

It does seem good to me.  Not even because of the test runner, but maybe
even more so because it seems like better style to split the tests into
one function per case.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[Qemu-devel] [PATCH 09/12] iotests: teach run_job to cancel pending jobs

[John Snow]

run_job can cancel pending jobs to simulate failure. This lets us use
the pending callback to issue test commands while the job is open, but
then still have the job fail in the end.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/iotests.py | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index fcad957d63..c544659ecb 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -541,7 +541,22 @@ class VM(qtest.QEMUQtestMachine):
 
     # Returns None on success, and an error string on failure
     def run_job(self, job, auto_finalize=True, auto_dismiss=False,
-                pre_finalize=None, wait=60.0):
+                pre_finalize=None, cancel=False, wait=60.0):
+        """
+        run_job moves a job from creation through to dismissal.
+
+        :param job: String. ID of recently-launched job
+        :param auto_finalize: Bool. True if the job was launched with
+                              auto_finalize. Defaults to True.
+        :param auto_dismiss: Bool. True if the job was launched with
+                             auto_dismiss=True. Defaults to False.
+        :param pre_finalize: Callback. A callable that takes no arguments to be
+                             invoked prior to issuing job-finalize, if any.
+        :param cancel: Bool. When true, cancels the job after the pre_finalize
+                       callback.
+        :param wait: Float. Timeout value specifying how long to wait for any
+                     event, in seconds. Defaults to 60.0.
+        """
         match_device = {'data': {'device': job}}
         match_id = {'data': {'id': job}}
         events = [
@@ -568,7 +583,10 @@ class VM(qtest.QEMUQtestMachine):
             elif status == 'pending' and not auto_finalize:
                 if pre_finalize:
                     pre_finalize()
-                self.qmp_log('job-finalize', id=job)
+                if cancel:
+                    self.qmp_log('job-cancel', id=job)
+                else:
+                    self.qmp_log('job-finalize', id=job)
             elif status == 'concluded' and not auto_dismiss:
                 self.qmp_log('job-dismiss', id=job)
             elif status == 'null':
-- 
2.21.0

Re: [Qemu-devel] [PATCH 09/12] iotests: teach run_job to cancel pending jobs

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 449 bytes --]

On 20.06.19 03:03, John Snow wrote:
> run_job can cancel pending jobs to simulate failure. This lets us use
> the pending callback to issue test commands while the job is open, but
> then still have the job fail in the end.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  tests/qemu-iotests/iotests.py | 22 ++++++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[Qemu-devel] [PATCH 10/12] iotests: teach FilePath to produce multiple paths

[John Snow]

Use "FilePaths" instead of "FilePath" to request multiple files be
cleaned up after we leave that object's scope.

This is not crucial; but it saves a little typing.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/iotests.py | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index c544659ecb..b938fa9719 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -359,7 +359,7 @@ class Timeout:
         raise Exception(self.errmsg)
 
 
-class FilePath(object):
+class FilePaths(object):
     '''An auto-generated filename that cleans itself up.
 
     Use this context manager to generate filenames and ensure that the file
@@ -369,20 +369,29 @@ class FilePath(object):
             qemu_img('create', img_path, '1G')
         # migration_sock_path is automatically deleted
     '''
-    def __init__(self, name):
-        filename = '{0}-{1}'.format(os.getpid(), name)
-        self.path = os.path.join(test_dir, filename)
+    def __init__(self, names):
+        self.paths = []
+        for name in names:
+            filename = '{0}-{1}'.format(os.getpid(), name)
+            self.paths.append(os.path.join(test_dir, filename))
 
     def __enter__(self):
-        return self.path
+        return self.paths
 
     def __exit__(self, exc_type, exc_val, exc_tb):
         try:
-            os.remove(self.path)
+            for path in self.paths:
+                os.remove(path)
         except OSError:
             pass
         return False
 
+class FilePath(FilePaths):
+    def __init__(self, name):
+        super(FilePath, self).__init__([name])
+
+    def __enter__(self):
+        return self.paths[0]
 
 def file_path_remover():
     for path in reversed(file_path_remover.paths):
-- 
2.21.0

Re: [Qemu-devel] [PATCH 10/12] iotests: teach FilePath to produce multiple paths

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 555 bytes --]

On 20.06.19 03:03, John Snow wrote:
> Use "FilePaths" instead of "FilePath" to request multiple files be
> cleaned up after we leave that object's scope.
> 
> This is not crucial; but it saves a little typing.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  tests/qemu-iotests/iotests.py | 21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)

The example in the comment for FilePaths looks stale now, but it looked
just as stale before.  (“TestFilePath”)

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[Qemu-devel] [PATCH 11/12] iotests: add test 257 for bitmap-mode backups

[John Snow]

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/257     |  412 +++++++
 tests/qemu-iotests/257.out | 2199 ++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/group   |    1 +
 3 files changed, 2612 insertions(+)
 create mode 100755 tests/qemu-iotests/257
 create mode 100644 tests/qemu-iotests/257.out

diff --git a/tests/qemu-iotests/257 b/tests/qemu-iotests/257
new file mode 100755
index 0000000000..5f7f388504
--- /dev/null
+++ b/tests/qemu-iotests/257
@@ -0,0 +1,412 @@
+#!/usr/bin/env python
+#
+# Test bitmap-sync backups (incremental, differential, and partials)
+#
+# Copyright (c) 2019 John Snow for Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+# owner=jsnow@redhat.com
+
+from collections import namedtuple
+import math
+import os
+
+import iotests
+from iotests import log, qemu_img
+
+SIZE = 64 * 1024 * 1024
+GRANULARITY = 64 * 1024
+
+Pattern = namedtuple('Pattern', ['byte', 'offset', 'size'])
+def mkpattern(byte, offset, size=GRANULARITY):
+    """Constructor for Pattern() with default size"""
+    return Pattern(byte, offset, size)
+
+class PatternGroup:
+    """Grouping of Pattern objects. Initialize with an iterable of Patterns."""
+    def __init__(self, patterns):
+        self.patterns = patterns
+
+    def bits(self, granularity):
+        """Calculate the unique bits dirtied by this pattern grouping"""
+        res = set()
+        for pattern in self.patterns:
+            lower = math.floor(pattern.offset / granularity)
+            upper = math.floor((pattern.offset + pattern.size - 1) / granularity)
+            res = res | set(range(lower, upper + 1))
+        return res
+
+GROUPS = [
+    PatternGroup([
+        # Batch 0: 4 clusters
+        mkpattern('0x49', 0x0000000),
+        mkpattern('0x6c', 0x0100000),   # 1M
+        mkpattern('0x6f', 0x2000000),   # 32M
+        mkpattern('0x76', 0x3ff0000)]), # 64M - 64K
+    PatternGroup([
+        # Batch 1: 6 clusters (3 new)
+        mkpattern('0x65', 0x0000000),   # Full overwrite
+        mkpattern('0x77', 0x00f8000),   # Partial-left (1M-32K)
+        mkpattern('0x72', 0x2008000),   # Partial-right (32M+32K)
+        mkpattern('0x69', 0x3fe0000)]), # Adjacent-left (64M - 128K)
+    PatternGroup([
+        # Batch 2: 7 clusters (3 new)
+        mkpattern('0x74', 0x0010000),   # Adjacent-right
+        mkpattern('0x69', 0x00e8000),   # Partial-left  (1M-96K)
+        mkpattern('0x6e', 0x2018000),   # Partial-right (32M+96K)
+        mkpattern('0x67', 0x3fe0000,
+                  2*GRANULARITY)]),     # Overwrite [(64M-128K)-64M)
+    PatternGroup([
+        # Batch 3: 8 clusters (5 new)
+        # Carefully chosen such that nothing re-dirties the one cluster
+        # that copies out successfully before failure in Group #1.
+        mkpattern('0xaa', 0x0010000,
+                  3*GRANULARITY),       # Overwrite and 2x Adjacent-right
+        mkpattern('0xbb', 0x00d8000),   # Partial-left (1M-160K)
+        mkpattern('0xcc', 0x2028000),   # Partial-right (32M+160K)
+        mkpattern('0xdd', 0x3fc0000)]), # New; leaving a gap to the right
+    ]
+
+class Drive:
+    """Represents, vaguely, a drive attached to a VM.
+    Includes format, graph, and device information."""
+
+    def __init__(self, path, vm=None):
+        self.path = path
+        self.vm = vm
+        self.fmt = None
+        self.size = None
+        self.node = None
+        self.device = None
+
+    @property
+    def name(self):
+        return self.node or self.device
+
+    def img_create(self, fmt, size):
+        self.fmt = fmt
+        self.size = size
+        iotests.qemu_img_create('-f', self.fmt, self.path, str(self.size))
+
+    def create_target(self, name, fmt, size):
+        basename = os.path.basename(self.path)
+        file_node_name = "file_{}".format(basename)
+        vm = self.vm
+
+        log(vm.command('blockdev-create', job_id='bdc-file-job',
+                       options={
+                           'driver': 'file',
+                           'filename': self.path,
+                           'size': 0,
+                       }))
+        vm.run_job('bdc-file-job')
+        log(vm.command('blockdev-add', driver='file',
+                       node_name=file_node_name, filename=self.path))
+
+        log(vm.command('blockdev-create', job_id='bdc-fmt-job',
+                       options={
+                           'driver': fmt,
+                           'file': file_node_name,
+                           'size': size,
+                       }))
+        vm.run_job('bdc-fmt-job')
+        log(vm.command('blockdev-add', driver=fmt,
+                       node_name=name,
+                       file=file_node_name))
+        self.fmt = fmt
+        self.size = size
+        self.node = name
+
+def query_bitmaps(vm):
+    res = vm.qmp("query-block")
+    return {"bitmaps": {device['device'] or device['qdev']:
+                        device.get('dirty-bitmaps', []) for
+                        device in res['return']}}
+
+def get_bitmap(bitmaps, drivename, name):
+    for bitmap in bitmaps['bitmaps'][drivename]:
+        if bitmap.get('name', '') == name:
+            return bitmap
+    return None
+
+def reference_backup(drive, n, filepath):
+    log("--- Reference Backup #{:d} ---\n".format(n))
+    target_id = "ref_target_{:d}".format(n)
+    job_id = "ref_backup_{:d}".format(n)
+    target_drive = Drive(filepath, vm=drive.vm)
+
+    target_drive.create_target(target_id, drive.fmt, drive.size)
+    drive.vm.qmp_log("blockdev-backup",
+                     job_id=job_id, device=drive.name,
+                     target=target_id, sync="full")
+    drive.vm.run_job(job_id, auto_dismiss=True)
+    log('')
+
+def bitmap_backup(drive, n, filepath, bitmap, bitmap_mode):
+    log("--- Bitmap Backup #{:d} ---\n".format(n))
+    target_id = "bitmap_target_{:d}".format(n)
+    job_id = "bitmap_backup_{:d}".format(n)
+    target_drive = Drive(filepath, vm=drive.vm)
+
+    target_drive.create_target(target_id, drive.fmt, drive.size)
+    drive.vm.qmp_log("blockdev-backup", job_id=job_id, device=drive.name,
+                     target=target_id, sync="bitmap",
+                     bitmap_mode=bitmap_mode,
+                     bitmap=bitmap,
+                     auto_finalize=False)
+    return job_id
+
+def perform_writes(drive, n):
+    log("--- Write #{:d} ---\n".format(n))
+    for pattern in GROUPS[n].patterns:
+        cmd = "write -P{:s} 0x{:07x} 0x{:x}".format(
+            pattern.byte,
+            pattern.offset,
+            pattern.size)
+        log(cmd)
+        log(drive.vm.hmp_qemu_io(drive.name, cmd))
+    bitmaps = query_bitmaps(drive.vm)
+    log(bitmaps, indent=2)
+    log('')
+    return bitmaps
+
+def calculate_bits(groups=None):
+    """Calculate how many bits we expect to see dirtied."""
+    if groups:
+        bits = set.union(*(GROUPS[group].bits(GRANULARITY) for group in groups))
+        return len(bits)
+    return 0
+
+def bitmap_comparison(bitmap, groups=None, want=0):
+    """
+    Print a nice human-readable message checking that this bitmap has as
+    many bits set as we expect it to.
+    """
+    log("= Checking Bitmap {:s} =".format(bitmap.get('name', '(anonymous)')))
+
+    if groups:
+        want = calculate_bits(groups)
+    have = int(bitmap['count'] / bitmap['granularity'])
+
+    log("expecting {:d} dirty sectors; have {:d}. {:s}".format(
+        want, have, "OK!" if want == have else "ERROR!"))
+    log('')
+
+def compare_images(image, reference, baseimg=None, expected_match=True):
+    """
+    Print a nice human-readable message comparing these images.
+    """
+    expected_ret = 0 if expected_match else 1
+    if baseimg:
+        assert qemu_img("rebase", "-u", "-b", baseimg, image) == 0
+    ret = qemu_img("compare", image, reference)
+    log('qemu_img compare "{:s}" "{:s}" ==> {:s}, {:s}'.format(
+        image, reference,
+        "Identical" if ret == 0 else "Mismatch",
+        "OK!" if ret == expected_ret else "ERROR!"),
+        filters=[iotests.filter_testfiles])
+
+def test_bitmap_sync(bsync_mode, failure=None):
+    """
+    Test bitmap backup routines.
+
+    :param bsync_mode: Is the Bitmap Sync mode, and can be any of:
+        - conditional: This is the "incremental" style mode. Bitmaps are
+                       synchronized to what was copied out only on success.
+                       (Partial images must be discarded.)
+        - never:       This is the "differential" style mode.
+                       Bitmaps are never synchronized.
+        - always:      This is a "best effort" style mode.
+                       Bitmaps are always synchronized, regardless of failure.
+                       (Partial images must be kept.)
+
+    :param failure: Is the (optional) failure mode, and can be any of:
+        - None:         No failure. Test the normative path. Default.
+        - simulated:    Cancel the job right before it completes.
+                        This also tests writes "during" the job.
+        - intermediate: This tests a job that fails mid-process and produces
+                        an incomplete backup. Testing limitations prevent
+                        testing competing writes.
+    """
+    with iotests.FilePaths(['img', 'bsync1', 'bsync2',
+                            'fbackup0', 'fbackup1', 'fbackup2']) as \
+                            (img_path, bsync1, bsync2,
+                             fbackup0, fbackup1, fbackup2), \
+         iotests.VM() as vm:
+
+        mode = "Bitmap Sync Mode {:s}".format(bsync_mode)
+        preposition = "with" if failure else "without"
+        cond = "{:s} {:s}".format(preposition,
+                                  "{:s} failure".format(failure) if failure
+                                  else "failure")
+        log("\n=== {:s} {:s} ===\n".format(mode, cond))
+
+        log('--- Preparing image & VM ---\n')
+        drive0 = Drive(img_path, vm=vm)
+        drive0.img_create(iotests.imgfmt, SIZE)
+        vm.add_device('virtio-scsi-pci,id=scsi0')
+        vm.launch()
+
+        file_config = {
+            'driver': 'file',
+            'filename': drive0.path
+        }
+
+        if failure == 'intermediate':
+            file_config = {
+                'driver': 'blkdebug',
+                'image': file_config,
+                'set-state': [{
+                    'event': 'flush_to_disk',
+                    'state': 1,
+                    'new_state': 2
+                }, {
+                    'event': 'read_aio',
+                    'state': 2,
+                    'new_state': 3
+                }],
+                'inject-error': [{
+                    'event': 'read_aio',
+                    'errno': 5,
+                    'state': 3,
+                    'immediately': False,
+                    'once': True
+                }]
+            }
+
+        vm.qmp_log('blockdev-add',
+                   filters=[iotests.filter_qmp_testfiles],
+                   node_name="drive0",
+                   driver=drive0.fmt,
+                   file=file_config)
+        drive0.node = 'drive0'
+        drive0.device = 'device0'
+        # Use share-rw to allow writes directly to the node;
+        # The anonymous block-backend for this configuration prevents us
+        # from using HMP's qemu-io commands to address the device.
+        vm.qmp_log("device_add", id=drive0.device,
+                   drive=drive0.name, driver="scsi-hd",
+                   share_rw=True)
+        log('')
+
+        # 0 - Writes and Reference Backup
+        perform_writes(drive0, 0)
+        reference_backup(drive0, 0, fbackup0)
+        log('--- Add Bitmap ---\n')
+        vm.qmp_log("block-dirty-bitmap-add", node=drive0.name,
+                   name="bitmap0", granularity=GRANULARITY)
+        log('')
+
+        # 1 - Writes and Reference Backup
+        bitmaps = perform_writes(drive0, 1)
+        dirty_groups = {1}
+        bitmap = get_bitmap(bitmaps, drive0.device, 'bitmap0')
+        bitmap_comparison(bitmap, groups=dirty_groups)
+        reference_backup(drive0, 1, fbackup1)
+
+        # 1 - Bitmap Backup (Optional induced failure)
+        if failure == 'intermediate':
+            # Activate blkdebug induced failure for second-to-next read
+            log(vm.hmp_qemu_io(drive0.name, 'flush'))
+            log('')
+        job = bitmap_backup(drive0, 1, bsync1, "bitmap0", bsync_mode)
+
+        def _callback():
+            """Issue writes while the job is open to test bitmap divergence."""
+            # Note: when `failure` is 'intermediate', this isn't called.
+            log('')
+            bitmaps = perform_writes(drive0, 2)
+            # Named bitmap (static, should be unchanged)
+            bitmap_comparison(get_bitmap(bitmaps, drive0.device, 'bitmap0'),
+                              groups=dirty_groups)
+            # Anonymous bitmap (dynamic, shows new writes)
+            bitmap_comparison(get_bitmap(bitmaps, drive0.device, ''),
+                              groups={2})
+            dirty_groups.add(2)
+
+        vm.run_job(job, auto_dismiss=True, auto_finalize=False,
+                   pre_finalize=_callback,
+                   cancel=failure == 'simulated')
+        bitmaps = query_bitmaps(vm)
+        bitmap = get_bitmap(bitmaps, drive0.device, 'bitmap0')
+        log(bitmaps, indent=2)
+        log('')
+
+        if ((bsync_mode == 'conditional' and not failure) or
+                (bsync_mode == 'always' and failure != 'intermediate')):
+            dirty_groups.remove(1)
+
+        if bsync_mode == 'always' and failure == 'intermediate':
+            # We manage to copy one sector (one bit) before the error.
+            bitmap_comparison(bitmap,
+                              want=calculate_bits(groups=dirty_groups) - 1)
+        else:
+            bitmap_comparison(bitmap, groups=dirty_groups)
+
+        # 2 - Writes and Reference Backup
+        bitmaps = perform_writes(drive0, 3)
+        dirty_groups.add(3)
+        bitmap = get_bitmap(bitmaps, drive0.device, 'bitmap0')
+        if bsync_mode == 'always' and failure == 'intermediate':
+            # We're one bit short, still.
+            bitmap_comparison(bitmap,
+                              want=calculate_bits(groups=dirty_groups) - 1)
+        else:
+            bitmap_comparison(bitmap, groups=dirty_groups)
+        reference_backup(drive0, 2, fbackup2)
+
+        # 2 - Bitmap Backup (In failure modes, this is a recovery.)
+        job = bitmap_backup(drive0, 2, bsync2, "bitmap0", bsync_mode)
+        vm.run_job(job, auto_dismiss=True, auto_finalize=False)
+        bitmaps = query_bitmaps(vm)
+        bitmap = get_bitmap(bitmaps, drive0.device, 'bitmap0')
+        log(bitmaps, indent=2)
+        log('')
+        bitmap_comparison(bitmap, groups={}
+                          if bsync_mode != 'never'
+                          else dirty_groups)
+
+        log('--- Cleanup ---\n')
+        vm.qmp_log("block-dirty-bitmap-remove",
+                   node=drive0.name, name="bitmap0")
+        log(query_bitmaps(vm), indent=2)
+        vm.shutdown()
+        log('')
+
+        log('--- Verification ---\n')
+        # 'simulated' failures will actually all pass here because we canceled
+        # while "pending". This is actually undefined behavior,
+        # don't rely on this to be true!
+        compare_images(bsync1, fbackup1, baseimg=fbackup0,
+                       expected_match=failure != 'intermediate')
+        if not failure or bsync_mode == 'always':
+            # Always keep the last backup on success or when using 'always'
+            base = bsync1
+        else:
+            base = fbackup0
+        compare_images(bsync2, fbackup2, baseimg=base)
+        compare_images(img_path, fbackup2)
+        log('')
+
+def main():
+    for bsync_mode in ("never", "conditional", "always"):
+        for failure in ("simulated", None):
+            test_bitmap_sync(bsync_mode, failure)
+
+    for bsync_mode in ("never", "conditional", "always"):
+        test_bitmap_sync(bsync_mode, "intermediate")
+
+if __name__ == '__main__':
+    iotests.script_main(main, supported_fmts=['qcow2'])
diff --git a/tests/qemu-iotests/257.out b/tests/qemu-iotests/257.out
new file mode 100644
index 0000000000..1ba615c990
--- /dev/null
+++ b/tests/qemu-iotests/257.out
@@ -0,0 +1,2199 @@
+
+=== Bitmap Sync Mode never with simulated failure ===
+
+--- Preparing image & VM ---
+
+{"execute": "blockdev-add", "arguments": {"driver": "qcow2", "file": {"driver": "file", "filename": "TEST_DIR/PID-img"}, "node-name": "drive0"}}
+{"return": {}}
+{"execute": "device_add", "arguments": {"drive": "drive0", "driver": "scsi-hd", "id": "device0", "share-rw": true}}
+{"return": {}}
+
+--- Write #0 ---
+
+write -P0x49 0x0000000 0x10000
+{"return": ""}
+write -P0x6c 0x0100000 0x10000
+{"return": ""}
+write -P0x6f 0x2000000 0x10000
+{"return": ""}
+write -P0x76 0x3ff0000 0x10000
+{"return": ""}
+{
+  "bitmaps": {
+    "device0": []
+  }
+}
+
+--- Reference Backup #0 ---
+
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-file-job"}}
+{"return": {}}
+{}
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-fmt-job"}}
+{"return": {}}
+{}
+{"execute": "blockdev-backup", "arguments": {"device": "drive0", "job-id": "ref_backup_0", "sync": "full", "target": "ref_target_0"}}
+{"return": {}}
+{"data": {"device": "ref_backup_0", "len": 67108864, "offset": 67108864, "speed": 0, "type": "backup"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+
+--- Add Bitmap ---
+
+{"execute": "block-dirty-bitmap-add", "arguments": {"granularity": 65536, "name": "bitmap0", "node": "drive0"}}
+{"return": {}}
+
+--- Write #1 ---
+
+write -P0x65 0x0000000 0x10000
+{"return": ""}
+write -P0x77 0x00f8000 0x10000
+{"return": ""}
+write -P0x72 0x2008000 0x10000
+{"return": ""}
+write -P0x69 0x3fe0000 0x10000
+{"return": ""}
+{
+  "bitmaps": {
+    "device0": [
+      {
+        "busy": false,
+        "count": 393216,
+        "granularity": 65536,
+        "name": "bitmap0",
+        "persistent": false,
+        "recording": true,
+        "status": "active"
+      }
+    ]
+  }
+}
+
+= Checking Bitmap bitmap0 =
+expecting 6 dirty sectors; have 6. OK!
+
+--- Reference Backup #1 ---
+
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-file-job"}}
+{"return": {}}
+{}
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-fmt-job"}}
+{"return": {}}
+{}
+{"execute": "blockdev-backup", "arguments": {"device": "drive0", "job-id": "ref_backup_1", "sync": "full", "target": "ref_target_1"}}
+{"return": {}}
+{"data": {"device": "ref_backup_1", "len": 67108864, "offset": 67108864, "speed": 0, "type": "backup"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+
+--- Bitmap Backup #1 ---
+
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-file-job"}}
+{"return": {}}
+{}
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-fmt-job"}}
+{"return": {}}
+{}
+{"execute": "blockdev-backup", "arguments": {"auto-finalize": false, "bitmap": "bitmap0", "bitmap-mode": "never", "device": "drive0", "job-id": "bitmap_backup_1", "sync": "bitmap", "target": "bitmap_target_1"}}
+{"return": {}}
+
+--- Write #2 ---
+
+write -P0x74 0x0010000 0x10000
+{"return": ""}
+write -P0x69 0x00e8000 0x10000
+{"return": ""}
+write -P0x6e 0x2018000 0x10000
+{"return": ""}
+write -P0x67 0x3fe0000 0x20000
+{"return": ""}
+{
+  "bitmaps": {
+    "device0": [
+      {
+        "busy": false,
+        "count": 458752,
+        "granularity": 65536,
+        "persistent": false,
+        "recording": true,
+        "status": "active"
+      },
+      {
+        "busy": true,
+        "count": 393216,
+        "granularity": 65536,
+        "name": "bitmap0",
+        "persistent": false,
+        "recording": true,
+        "status": "frozen"
+      }
+    ]
+  }
+}
+
+= Checking Bitmap bitmap0 =
+expecting 6 dirty sectors; have 6. OK!
+
+= Checking Bitmap (anonymous) =
+expecting 7 dirty sectors; have 7. OK!
+
+{"execute": "job-cancel", "arguments": {"id": "bitmap_backup_1"}}
+{"return": {}}
+{"data": {"id": "bitmap_backup_1", "type": "backup"}, "event": "BLOCK_JOB_PENDING", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"data": {"device": "bitmap_backup_1", "len": 67108864, "offset": 67108864, "speed": 0, "type": "backup"}, "event": "BLOCK_JOB_CANCELLED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{
+  "bitmaps": {
+    "device0": [
+      {
+        "busy": false,
+        "count": 655360,
+        "granularity": 65536,
+        "name": "bitmap0",
+        "persistent": false,
+        "recording": true,
+        "status": "active"
+      }
+    ]
+  }
+}
+
+= Checking Bitmap bitmap0 =
+expecting 10 dirty sectors; have 10. OK!
+
+--- Write #3 ---
+
+write -P0xaa 0x0010000 0x30000
+{"return": ""}
+write -P0xbb 0x00d8000 0x10000
+{"return": ""}
+write -P0xcc 0x2028000 0x10000
+{"return": ""}
+write -P0xdd 0x3fc0000 0x10000
+{"return": ""}
+{
+  "bitmaps": {
+    "device0": [
+      {
+        "busy": false,
+        "count": 983040,
+        "granularity": 65536,
+        "name": "bitmap0",
+        "persistent": false,
+        "recording": true,
+        "status": "active"
+      }
+    ]
+  }
+}
+
+= Checking Bitmap bitmap0 =
+expecting 15 dirty sectors; have 15. OK!
+
+--- Reference Backup #2 ---
+
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-file-job"}}
+{"return": {}}
+{}
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-fmt-job"}}
+{"return": {}}
+{}
+{"execute": "blockdev-backup", "arguments": {"device": "drive0", "job-id": "ref_backup_2", "sync": "full", "target": "ref_target_2"}}
+{"return": {}}
+{"data": {"device": "ref_backup_2", "len": 67108864, "offset": 67108864, "speed": 0, "type": "backup"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+
+--- Bitmap Backup #2 ---
+
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-file-job"}}
+{"return": {}}
+{}
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-fmt-job"}}
+{"return": {}}
+{}
+{"execute": "blockdev-backup", "arguments": {"auto-finalize": false, "bitmap": "bitmap0", "bitmap-mode": "never", "device": "drive0", "job-id": "bitmap_backup_2", "sync": "bitmap", "target": "bitmap_target_2"}}
+{"return": {}}
+{"execute": "job-finalize", "arguments": {"id": "bitmap_backup_2"}}
+{"return": {}}
+{"data": {"id": "bitmap_backup_2", "type": "backup"}, "event": "BLOCK_JOB_PENDING", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"data": {"device": "bitmap_backup_2", "len": 67108864, "offset": 67108864, "speed": 0, "type": "backup"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{
+  "bitmaps": {
+    "device0": [
+      {
+        "busy": false,
+        "count": 983040,
+        "granularity": 65536,
+        "name": "bitmap0",
+        "persistent": false,
+        "recording": true,
+        "status": "active"
+      }
+    ]
+  }
+}
+
+= Checking Bitmap bitmap0 =
+expecting 15 dirty sectors; have 15. OK!
+
+--- Cleanup ---
+
+{"execute": "block-dirty-bitmap-remove", "arguments": {"name": "bitmap0", "node": "drive0"}}
+{"return": {}}
+{
+  "bitmaps": {
+    "device0": []
+  }
+}
+
+--- Verification ---
+
+qemu_img compare "TEST_DIR/PID-bsync1" "TEST_DIR/PID-fbackup1" ==> Identical, OK!
+qemu_img compare "TEST_DIR/PID-bsync2" "TEST_DIR/PID-fbackup2" ==> Identical, OK!
+qemu_img compare "TEST_DIR/PID-img" "TEST_DIR/PID-fbackup2" ==> Identical, OK!
+
+
+=== Bitmap Sync Mode never without failure ===
+
+--- Preparing image & VM ---
+
+{"execute": "blockdev-add", "arguments": {"driver": "qcow2", "file": {"driver": "file", "filename": "TEST_DIR/PID-img"}, "node-name": "drive0"}}
+{"return": {}}
+{"execute": "device_add", "arguments": {"drive": "drive0", "driver": "scsi-hd", "id": "device0", "share-rw": true}}
+{"return": {}}
+
+--- Write #0 ---
+
+write -P0x49 0x0000000 0x10000
+{"return": ""}
+write -P0x6c 0x0100000 0x10000
+{"return": ""}
+write -P0x6f 0x2000000 0x10000
+{"return": ""}
+write -P0x76 0x3ff0000 0x10000
+{"return": ""}
+{
+  "bitmaps": {
+    "device0": []
+  }
+}
+
+--- Reference Backup #0 ---
+
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-file-job"}}
+{"return": {}}
+{}
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-fmt-job"}}
+{"return": {}}
+{}
+{"execute": "blockdev-backup", "arguments": {"device": "drive0", "job-id": "ref_backup_0", "sync": "full", "target": "ref_target_0"}}
+{"return": {}}
+{"data": {"device": "ref_backup_0", "len": 67108864, "offset": 67108864, "speed": 0, "type": "backup"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+
+--- Add Bitmap ---
+
+{"execute": "block-dirty-bitmap-add", "arguments": {"granularity": 65536, "name": "bitmap0", "node": "drive0"}}
+{"return": {}}
+
+--- Write #1 ---
+
+write -P0x65 0x0000000 0x10000
+{"return": ""}
+write -P0x77 0x00f8000 0x10000
+{"return": ""}
+write -P0x72 0x2008000 0x10000
+{"return": ""}
+write -P0x69 0x3fe0000 0x10000
+{"return": ""}
+{
+  "bitmaps": {
+    "device0": [
+      {
+        "busy": false,
+        "count": 393216,
+        "granularity": 65536,
+        "name": "bitmap0",
+        "persistent": false,
+        "recording": true,
+        "status": "active"
+      }
+    ]
+  }
+}
+
+= Checking Bitmap bitmap0 =
+expecting 6 dirty sectors; have 6. OK!
+
+--- Reference Backup #1 ---
+
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-file-job"}}
+{"return": {}}
+{}
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-fmt-job"}}
+{"return": {}}
+{}
+{"execute": "blockdev-backup", "arguments": {"device": "drive0", "job-id": "ref_backup_1", "sync": "full", "target": "ref_target_1"}}
+{"return": {}}
+{"data": {"device": "ref_backup_1", "len": 67108864, "offset": 67108864, "speed": 0, "type": "backup"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+
+--- Bitmap Backup #1 ---
+
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-file-job"}}
+{"return": {}}
+{}
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-fmt-job"}}
+{"return": {}}
+{}
+{"execute": "blockdev-backup", "arguments": {"auto-finalize": false, "bitmap": "bitmap0", "bitmap-mode": "never", "device": "drive0", "job-id": "bitmap_backup_1", "sync": "bitmap", "target": "bitmap_target_1"}}
+{"return": {}}
+
+--- Write #2 ---
+
+write -P0x74 0x0010000 0x10000
+{"return": ""}
+write -P0x69 0x00e8000 0x10000
+{"return": ""}
+write -P0x6e 0x2018000 0x10000
+{"return": ""}
+write -P0x67 0x3fe0000 0x20000
+{"return": ""}
+{
+  "bitmaps": {
+    "device0": [
+      {
+        "busy": false,
+        "count": 458752,
+        "granularity": 65536,
+        "persistent": false,
+        "recording": true,
+        "status": "active"
+      },
+      {
+        "busy": true,
+        "count": 393216,
+        "granularity": 65536,
+        "name": "bitmap0",
+        "persistent": false,
+        "recording": true,
+        "status": "frozen"
+      }
+    ]
+  }
+}
+
+= Checking Bitmap bitmap0 =
+expecting 6 dirty sectors; have 6. OK!
+
+= Checking Bitmap (anonymous) =
+expecting 7 dirty sectors; have 7. OK!
+
+{"execute": "job-finalize", "arguments": {"id": "bitmap_backup_1"}}
+{"return": {}}
+{"data": {"id": "bitmap_backup_1", "type": "backup"}, "event": "BLOCK_JOB_PENDING", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"data": {"device": "bitmap_backup_1", "len": 67108864, "offset": 67108864, "speed": 0, "type": "backup"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{
+  "bitmaps": {
+    "device0": [
+      {
+        "busy": false,
+        "count": 655360,
+        "granularity": 65536,
+        "name": "bitmap0",
+        "persistent": false,
+        "recording": true,
+        "status": "active"
+      }
+    ]
+  }
+}
+
+= Checking Bitmap bitmap0 =
+expecting 10 dirty sectors; have 10. OK!
+
+--- Write #3 ---
+
+write -P0xaa 0x0010000 0x30000
+{"return": ""}
+write -P0xbb 0x00d8000 0x10000
+{"return": ""}
+write -P0xcc 0x2028000 0x10000
+{"return": ""}
+write -P0xdd 0x3fc0000 0x10000
+{"return": ""}
+{
+  "bitmaps": {
+    "device0": [
+      {
+        "busy": false,
+        "count": 983040,
+        "granularity": 65536,
+        "name": "bitmap0",
+        "persistent": false,
+        "recording": true,
+        "status": "active"
+      }
+    ]
+  }
+}
+
+= Checking Bitmap bitmap0 =
+expecting 15 dirty sectors; have 15. OK!
+
+--- Reference Backup #2 ---
+
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-file-job"}}
+{"return": {}}
+{}
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-fmt-job"}}
+{"return": {}}
+{}
+{"execute": "blockdev-backup", "arguments": {"device": "drive0", "job-id": "ref_backup_2", "sync": "full", "target": "ref_target_2"}}
+{"return": {}}
+{"data": {"device": "ref_backup_2", "len": 67108864, "offset": 67108864, "speed": 0, "type": "backup"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+
+--- Bitmap Backup #2 ---
+
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-file-job"}}
+{"return": {}}
+{}
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-fmt-job"}}
+{"return": {}}
+{}
+{"execute": "blockdev-backup", "arguments": {"auto-finalize": false, "bitmap": "bitmap0", "bitmap-mode": "never", "device": "drive0", "job-id": "bitmap_backup_2", "sync": "bitmap", "target": "bitmap_target_2"}}
+{"return": {}}
+{"execute": "job-finalize", "arguments": {"id": "bitmap_backup_2"}}
+{"return": {}}
+{"data": {"id": "bitmap_backup_2", "type": "backup"}, "event": "BLOCK_JOB_PENDING", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"data": {"device": "bitmap_backup_2", "len": 67108864, "offset": 67108864, "speed": 0, "type": "backup"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{
+  "bitmaps": {
+    "device0": [
+      {
+        "busy": false,
+        "count": 983040,
+        "granularity": 65536,
+        "name": "bitmap0",
+        "persistent": false,
+        "recording": true,
+        "status": "active"
+      }
+    ]
+  }
+}
+
+= Checking Bitmap bitmap0 =
+expecting 15 dirty sectors; have 15. OK!
+
+--- Cleanup ---
+
+{"execute": "block-dirty-bitmap-remove", "arguments": {"name": "bitmap0", "node": "drive0"}}
+{"return": {}}
+{
+  "bitmaps": {
+    "device0": []
+  }
+}
+
+--- Verification ---
+
+qemu_img compare "TEST_DIR/PID-bsync1" "TEST_DIR/PID-fbackup1" ==> Identical, OK!
+qemu_img compare "TEST_DIR/PID-bsync2" "TEST_DIR/PID-fbackup2" ==> Identical, OK!
+qemu_img compare "TEST_DIR/PID-img" "TEST_DIR/PID-fbackup2" ==> Identical, OK!
+
+
+=== Bitmap Sync Mode conditional with simulated failure ===
+
+--- Preparing image & VM ---
+
+{"execute": "blockdev-add", "arguments": {"driver": "qcow2", "file": {"driver": "file", "filename": "TEST_DIR/PID-img"}, "node-name": "drive0"}}
+{"return": {}}
+{"execute": "device_add", "arguments": {"drive": "drive0", "driver": "scsi-hd", "id": "device0", "share-rw": true}}
+{"return": {}}
+
+--- Write #0 ---
+
+write -P0x49 0x0000000 0x10000
+{"return": ""}
+write -P0x6c 0x0100000 0x10000
+{"return": ""}
+write -P0x6f 0x2000000 0x10000
+{"return": ""}
+write -P0x76 0x3ff0000 0x10000
+{"return": ""}
+{
+  "bitmaps": {
+    "device0": []
+  }
+}
+
+--- Reference Backup #0 ---
+
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-file-job"}}
+{"return": {}}
+{}
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-fmt-job"}}
+{"return": {}}
+{}
+{"execute": "blockdev-backup", "arguments": {"device": "drive0", "job-id": "ref_backup_0", "sync": "full", "target": "ref_target_0"}}
+{"return": {}}
+{"data": {"device": "ref_backup_0", "len": 67108864, "offset": 67108864, "speed": 0, "type": "backup"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+
+--- Add Bitmap ---
+
+{"execute": "block-dirty-bitmap-add", "arguments": {"granularity": 65536, "name": "bitmap0", "node": "drive0"}}
+{"return": {}}
+
+--- Write #1 ---
+
+write -P0x65 0x0000000 0x10000
+{"return": ""}
+write -P0x77 0x00f8000 0x10000
+{"return": ""}
+write -P0x72 0x2008000 0x10000
+{"return": ""}
+write -P0x69 0x3fe0000 0x10000
+{"return": ""}
+{
+  "bitmaps": {
+    "device0": [
+      {
+        "busy": false,
+        "count": 393216,
+        "granularity": 65536,
+        "name": "bitmap0",
+        "persistent": false,
+        "recording": true,
+        "status": "active"
+      }
+    ]
+  }
+}
+
+= Checking Bitmap bitmap0 =
+expecting 6 dirty sectors; have 6. OK!
+
+--- Reference Backup #1 ---
+
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-file-job"}}
+{"return": {}}
+{}
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-fmt-job"}}
+{"return": {}}
+{}
+{"execute": "blockdev-backup", "arguments": {"device": "drive0", "job-id": "ref_backup_1", "sync": "full", "target": "ref_target_1"}}
+{"return": {}}
+{"data": {"device": "ref_backup_1", "len": 67108864, "offset": 67108864, "speed": 0, "type": "backup"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+
+--- Bitmap Backup #1 ---
+
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-file-job"}}
+{"return": {}}
+{}
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-fmt-job"}}
+{"return": {}}
+{}
+{"execute": "blockdev-backup", "arguments": {"auto-finalize": false, "bitmap": "bitmap0", "bitmap-mode": "conditional", "device": "drive0", "job-id": "bitmap_backup_1", "sync": "bitmap", "target": "bitmap_target_1"}}
+{"return": {}}
+
+--- Write #2 ---
+
+write -P0x74 0x0010000 0x10000
+{"return": ""}
+write -P0x69 0x00e8000 0x10000
+{"return": ""}
+write -P0x6e 0x2018000 0x10000
+{"return": ""}
+write -P0x67 0x3fe0000 0x20000
+{"return": ""}
+{
+  "bitmaps": {
+    "device0": [
+      {
+        "busy": false,
+        "count": 458752,
+        "granularity": 65536,
+        "persistent": false,
+        "recording": true,
+        "status": "active"
+      },
+      {
+        "busy": true,
+        "count": 393216,
+        "granularity": 65536,
+        "name": "bitmap0",
+        "persistent": false,
+        "recording": true,
+        "status": "frozen"
+      }
+    ]
+  }
+}
+
+= Checking Bitmap bitmap0 =
+expecting 6 dirty sectors; have 6. OK!
+
+= Checking Bitmap (anonymous) =
+expecting 7 dirty sectors; have 7. OK!
+
+{"execute": "job-cancel", "arguments": {"id": "bitmap_backup_1"}}
+{"return": {}}
+{"data": {"id": "bitmap_backup_1", "type": "backup"}, "event": "BLOCK_JOB_PENDING", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"data": {"device": "bitmap_backup_1", "len": 67108864, "offset": 67108864, "speed": 0, "type": "backup"}, "event": "BLOCK_JOB_CANCELLED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{
+  "bitmaps": {
+    "device0": [
+      {
+        "busy": false,
+        "count": 655360,
+        "granularity": 65536,
+        "name": "bitmap0",
+        "persistent": false,
+        "recording": true,
+        "status": "active"
+      }
+    ]
+  }
+}
+
+= Checking Bitmap bitmap0 =
+expecting 10 dirty sectors; have 10. OK!
+
+--- Write #3 ---
+
+write -P0xaa 0x0010000 0x30000
+{"return": ""}
+write -P0xbb 0x00d8000 0x10000
+{"return": ""}
+write -P0xcc 0x2028000 0x10000
+{"return": ""}
+write -P0xdd 0x3fc0000 0x10000
+{"return": ""}
+{
+  "bitmaps": {
+    "device0": [
+      {
+        "busy": false,
+        "count": 983040,
+        "granularity": 65536,
+        "name": "bitmap0",
+        "persistent": false,
+        "recording": true,
+        "status": "active"
+      }
+    ]
+  }
+}
+
+= Checking Bitmap bitmap0 =
+expecting 15 dirty sectors; have 15. OK!
+
+--- Reference Backup #2 ---
+
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-file-job"}}
+{"return": {}}
+{}
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-fmt-job"}}
+{"return": {}}
+{}
+{"execute": "blockdev-backup", "arguments": {"device": "drive0", "job-id": "ref_backup_2", "sync": "full", "target": "ref_target_2"}}
+{"return": {}}
+{"data": {"device": "ref_backup_2", "len": 67108864, "offset": 67108864, "speed": 0, "type": "backup"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+
+--- Bitmap Backup #2 ---
+
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-file-job"}}
+{"return": {}}
+{}
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-fmt-job"}}
+{"return": {}}
+{}
+{"execute": "blockdev-backup", "arguments": {"auto-finalize": false, "bitmap": "bitmap0", "bitmap-mode": "conditional", "device": "drive0", "job-id": "bitmap_backup_2", "sync": "bitmap", "target": "bitmap_target_2"}}
+{"return": {}}
+{"execute": "job-finalize", "arguments": {"id": "bitmap_backup_2"}}
+{"return": {}}
+{"data": {"id": "bitmap_backup_2", "type": "backup"}, "event": "BLOCK_JOB_PENDING", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"data": {"device": "bitmap_backup_2", "len": 67108864, "offset": 67108864, "speed": 0, "type": "backup"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{
+  "bitmaps": {
+    "device0": [
+      {
+        "busy": false,
+        "count": 0,
+        "granularity": 65536,
+        "name": "bitmap0",
+        "persistent": false,
+        "recording": true,
+        "status": "active"
+      }
+    ]
+  }
+}
+
+= Checking Bitmap bitmap0 =
+expecting 0 dirty sectors; have 0. OK!
+
+--- Cleanup ---
+
+{"execute": "block-dirty-bitmap-remove", "arguments": {"name": "bitmap0", "node": "drive0"}}
+{"return": {}}
+{
+  "bitmaps": {
+    "device0": []
+  }
+}
+
+--- Verification ---
+
+qemu_img compare "TEST_DIR/PID-bsync1" "TEST_DIR/PID-fbackup1" ==> Identical, OK!
+qemu_img compare "TEST_DIR/PID-bsync2" "TEST_DIR/PID-fbackup2" ==> Identical, OK!
+qemu_img compare "TEST_DIR/PID-img" "TEST_DIR/PID-fbackup2" ==> Identical, OK!
+
+
+=== Bitmap Sync Mode conditional without failure ===
+
+--- Preparing image & VM ---
+
+{"execute": "blockdev-add", "arguments": {"driver": "qcow2", "file": {"driver": "file", "filename": "TEST_DIR/PID-img"}, "node-name": "drive0"}}
+{"return": {}}
+{"execute": "device_add", "arguments": {"drive": "drive0", "driver": "scsi-hd", "id": "device0", "share-rw": true}}
+{"return": {}}
+
+--- Write #0 ---
+
+write -P0x49 0x0000000 0x10000
+{"return": ""}
+write -P0x6c 0x0100000 0x10000
+{"return": ""}
+write -P0x6f 0x2000000 0x10000
+{"return": ""}
+write -P0x76 0x3ff0000 0x10000
+{"return": ""}
+{
+  "bitmaps": {
+    "device0": []
+  }
+}
+
+--- Reference Backup #0 ---
+
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-file-job"}}
+{"return": {}}
+{}
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-fmt-job"}}
+{"return": {}}
+{}
+{"execute": "blockdev-backup", "arguments": {"device": "drive0", "job-id": "ref_backup_0", "sync": "full", "target": "ref_target_0"}}
+{"return": {}}
+{"data": {"device": "ref_backup_0", "len": 67108864, "offset": 67108864, "speed": 0, "type": "backup"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+
+--- Add Bitmap ---
+
+{"execute": "block-dirty-bitmap-add", "arguments": {"granularity": 65536, "name": "bitmap0", "node": "drive0"}}
+{"return": {}}
+
+--- Write #1 ---
+
+write -P0x65 0x0000000 0x10000
+{"return": ""}
+write -P0x77 0x00f8000 0x10000
+{"return": ""}
+write -P0x72 0x2008000 0x10000
+{"return": ""}
+write -P0x69 0x3fe0000 0x10000
+{"return": ""}
+{
+  "bitmaps": {
+    "device0": [
+      {
+        "busy": false,
+        "count": 393216,
+        "granularity": 65536,
+        "name": "bitmap0",
+        "persistent": false,
+        "recording": true,
+        "status": "active"
+      }
+    ]
+  }
+}
+
+= Checking Bitmap bitmap0 =
+expecting 6 dirty sectors; have 6. OK!
+
+--- Reference Backup #1 ---
+
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-file-job"}}
+{"return": {}}
+{}
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-fmt-job"}}
+{"return": {}}
+{}
+{"execute": "blockdev-backup", "arguments": {"device": "drive0", "job-id": "ref_backup_1", "sync": "full", "target": "ref_target_1"}}
+{"return": {}}
+{"data": {"device": "ref_backup_1", "len": 67108864, "offset": 67108864, "speed": 0, "type": "backup"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+
+--- Bitmap Backup #1 ---
+
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-file-job"}}
+{"return": {}}
+{}
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-fmt-job"}}
+{"return": {}}
+{}
+{"execute": "blockdev-backup", "arguments": {"auto-finalize": false, "bitmap": "bitmap0", "bitmap-mode": "conditional", "device": "drive0", "job-id": "bitmap_backup_1", "sync": "bitmap", "target": "bitmap_target_1"}}
+{"return": {}}
+
+--- Write #2 ---
+
+write -P0x74 0x0010000 0x10000
+{"return": ""}
+write -P0x69 0x00e8000 0x10000
+{"return": ""}
+write -P0x6e 0x2018000 0x10000
+{"return": ""}
+write -P0x67 0x3fe0000 0x20000
+{"return": ""}
+{
+  "bitmaps": {
+    "device0": [
+      {
+        "busy": false,
+        "count": 458752,
+        "granularity": 65536,
+        "persistent": false,
+        "recording": true,
+        "status": "active"
+      },
+      {
+        "busy": true,
+        "count": 393216,
+        "granularity": 65536,
+        "name": "bitmap0",
+        "persistent": false,
+        "recording": true,
+        "status": "frozen"
+      }
+    ]
+  }
+}
+
+= Checking Bitmap bitmap0 =
+expecting 6 dirty sectors; have 6. OK!
+
+= Checking Bitmap (anonymous) =
+expecting 7 dirty sectors; have 7. OK!
+
+{"execute": "job-finalize", "arguments": {"id": "bitmap_backup_1"}}
+{"return": {}}
+{"data": {"id": "bitmap_backup_1", "type": "backup"}, "event": "BLOCK_JOB_PENDING", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"data": {"device": "bitmap_backup_1", "len": 67108864, "offset": 67108864, "speed": 0, "type": "backup"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{
+  "bitmaps": {
+    "device0": [
+      {
+        "busy": false,
+        "count": 458752,
+        "granularity": 65536,
+        "name": "bitmap0",
+        "persistent": false,
+        "recording": true,
+        "status": "active"
+      }
+    ]
+  }
+}
+
+= Checking Bitmap bitmap0 =
+expecting 7 dirty sectors; have 7. OK!
+
+--- Write #3 ---
+
+write -P0xaa 0x0010000 0x30000
+{"return": ""}
+write -P0xbb 0x00d8000 0x10000
+{"return": ""}
+write -P0xcc 0x2028000 0x10000
+{"return": ""}
+write -P0xdd 0x3fc0000 0x10000
+{"return": ""}
+{
+  "bitmaps": {
+    "device0": [
+      {
+        "busy": false,
+        "count": 786432,
+        "granularity": 65536,
+        "name": "bitmap0",
+        "persistent": false,
+        "recording": true,
+        "status": "active"
+      }
+    ]
+  }
+}
+
+= Checking Bitmap bitmap0 =
+expecting 12 dirty sectors; have 12. OK!
+
+--- Reference Backup #2 ---
+
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-file-job"}}
+{"return": {}}
+{}
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-fmt-job"}}
+{"return": {}}
+{}
+{"execute": "blockdev-backup", "arguments": {"device": "drive0", "job-id": "ref_backup_2", "sync": "full", "target": "ref_target_2"}}
+{"return": {}}
+{"data": {"device": "ref_backup_2", "len": 67108864, "offset": 67108864, "speed": 0, "type": "backup"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+
+--- Bitmap Backup #2 ---
+
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-file-job"}}
+{"return": {}}
+{}
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-fmt-job"}}
+{"return": {}}
+{}
+{"execute": "blockdev-backup", "arguments": {"auto-finalize": false, "bitmap": "bitmap0", "bitmap-mode": "conditional", "device": "drive0", "job-id": "bitmap_backup_2", "sync": "bitmap", "target": "bitmap_target_2"}}
+{"return": {}}
+{"execute": "job-finalize", "arguments": {"id": "bitmap_backup_2"}}
+{"return": {}}
+{"data": {"id": "bitmap_backup_2", "type": "backup"}, "event": "BLOCK_JOB_PENDING", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"data": {"device": "bitmap_backup_2", "len": 67108864, "offset": 67108864, "speed": 0, "type": "backup"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{
+  "bitmaps": {
+    "device0": [
+      {
+        "busy": false,
+        "count": 0,
+        "granularity": 65536,
+        "name": "bitmap0",
+        "persistent": false,
+        "recording": true,
+        "status": "active"
+      }
+    ]
+  }
+}
+
+= Checking Bitmap bitmap0 =
+expecting 0 dirty sectors; have 0. OK!
+
+--- Cleanup ---
+
+{"execute": "block-dirty-bitmap-remove", "arguments": {"name": "bitmap0", "node": "drive0"}}
+{"return": {}}
+{
+  "bitmaps": {
+    "device0": []
+  }
+}
+
+--- Verification ---
+
+qemu_img compare "TEST_DIR/PID-bsync1" "TEST_DIR/PID-fbackup1" ==> Identical, OK!
+qemu_img compare "TEST_DIR/PID-bsync2" "TEST_DIR/PID-fbackup2" ==> Identical, OK!
+qemu_img compare "TEST_DIR/PID-img" "TEST_DIR/PID-fbackup2" ==> Identical, OK!
+
+
+=== Bitmap Sync Mode always with simulated failure ===
+
+--- Preparing image & VM ---
+
+{"execute": "blockdev-add", "arguments": {"driver": "qcow2", "file": {"driver": "file", "filename": "TEST_DIR/PID-img"}, "node-name": "drive0"}}
+{"return": {}}
+{"execute": "device_add", "arguments": {"drive": "drive0", "driver": "scsi-hd", "id": "device0", "share-rw": true}}
+{"return": {}}
+
+--- Write #0 ---
+
+write -P0x49 0x0000000 0x10000
+{"return": ""}
+write -P0x6c 0x0100000 0x10000
+{"return": ""}
+write -P0x6f 0x2000000 0x10000
+{"return": ""}
+write -P0x76 0x3ff0000 0x10000
+{"return": ""}
+{
+  "bitmaps": {
+    "device0": []
+  }
+}
+
+--- Reference Backup #0 ---
+
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-file-job"}}
+{"return": {}}
+{}
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-fmt-job"}}
+{"return": {}}
+{}
+{"execute": "blockdev-backup", "arguments": {"device": "drive0", "job-id": "ref_backup_0", "sync": "full", "target": "ref_target_0"}}
+{"return": {}}
+{"data": {"device": "ref_backup_0", "len": 67108864, "offset": 67108864, "speed": 0, "type": "backup"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+
+--- Add Bitmap ---
+
+{"execute": "block-dirty-bitmap-add", "arguments": {"granularity": 65536, "name": "bitmap0", "node": "drive0"}}
+{"return": {}}
+
+--- Write #1 ---
+
+write -P0x65 0x0000000 0x10000
+{"return": ""}
+write -P0x77 0x00f8000 0x10000
+{"return": ""}
+write -P0x72 0x2008000 0x10000
+{"return": ""}
+write -P0x69 0x3fe0000 0x10000
+{"return": ""}
+{
+  "bitmaps": {
+    "device0": [
+      {
+        "busy": false,
+        "count": 393216,
+        "granularity": 65536,
+        "name": "bitmap0",
+        "persistent": false,
+        "recording": true,
+        "status": "active"
+      }
+    ]
+  }
+}
+
+= Checking Bitmap bitmap0 =
+expecting 6 dirty sectors; have 6. OK!
+
+--- Reference Backup #1 ---
+
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-file-job"}}
+{"return": {}}
+{}
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-fmt-job"}}
+{"return": {}}
+{}
+{"execute": "blockdev-backup", "arguments": {"device": "drive0", "job-id": "ref_backup_1", "sync": "full", "target": "ref_target_1"}}
+{"return": {}}
+{"data": {"device": "ref_backup_1", "len": 67108864, "offset": 67108864, "speed": 0, "type": "backup"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+
+--- Bitmap Backup #1 ---
+
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-file-job"}}
+{"return": {}}
+{}
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-fmt-job"}}
+{"return": {}}
+{}
+{"execute": "blockdev-backup", "arguments": {"auto-finalize": false, "bitmap": "bitmap0", "bitmap-mode": "always", "device": "drive0", "job-id": "bitmap_backup_1", "sync": "bitmap", "target": "bitmap_target_1"}}
+{"return": {}}
+
+--- Write #2 ---
+
+write -P0x74 0x0010000 0x10000
+{"return": ""}
+write -P0x69 0x00e8000 0x10000
+{"return": ""}
+write -P0x6e 0x2018000 0x10000
+{"return": ""}
+write -P0x67 0x3fe0000 0x20000
+{"return": ""}
+{
+  "bitmaps": {
+    "device0": [
+      {
+        "busy": false,
+        "count": 458752,
+        "granularity": 65536,
+        "persistent": false,
+        "recording": true,
+        "status": "active"
+      },
+      {
+        "busy": true,
+        "count": 393216,
+        "granularity": 65536,
+        "name": "bitmap0",
+        "persistent": false,
+        "recording": true,
+        "status": "frozen"
+      }
+    ]
+  }
+}
+
+= Checking Bitmap bitmap0 =
+expecting 6 dirty sectors; have 6. OK!
+
+= Checking Bitmap (anonymous) =
+expecting 7 dirty sectors; have 7. OK!
+
+{"execute": "job-cancel", "arguments": {"id": "bitmap_backup_1"}}
+{"return": {}}
+{"data": {"id": "bitmap_backup_1", "type": "backup"}, "event": "BLOCK_JOB_PENDING", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"data": {"device": "bitmap_backup_1", "len": 67108864, "offset": 67108864, "speed": 0, "type": "backup"}, "event": "BLOCK_JOB_CANCELLED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{
+  "bitmaps": {
+    "device0": [
+      {
+        "busy": false,
+        "count": 458752,
+        "granularity": 65536,
+        "name": "bitmap0",
+        "persistent": false,
+        "recording": true,
+        "status": "active"
+      }
+    ]
+  }
+}
+
+= Checking Bitmap bitmap0 =
+expecting 7 dirty sectors; have 7. OK!
+
+--- Write #3 ---
+
+write -P0xaa 0x0010000 0x30000
+{"return": ""}
+write -P0xbb 0x00d8000 0x10000
+{"return": ""}
+write -P0xcc 0x2028000 0x10000
+{"return": ""}
+write -P0xdd 0x3fc0000 0x10000
+{"return": ""}
+{
+  "bitmaps": {
+    "device0": [
+      {
+        "busy": false,
+        "count": 786432,
+        "granularity": 65536,
+        "name": "bitmap0",
+        "persistent": false,
+        "recording": true,
+        "status": "active"
+      }
+    ]
+  }
+}
+
+= Checking Bitmap bitmap0 =
+expecting 12 dirty sectors; have 12. OK!
+
+--- Reference Backup #2 ---
+
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-file-job"}}
+{"return": {}}
+{}
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-fmt-job"}}
+{"return": {}}
+{}
+{"execute": "blockdev-backup", "arguments": {"device": "drive0", "job-id": "ref_backup_2", "sync": "full", "target": "ref_target_2"}}
+{"return": {}}
+{"data": {"device": "ref_backup_2", "len": 67108864, "offset": 67108864, "speed": 0, "type": "backup"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+
+--- Bitmap Backup #2 ---
+
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-file-job"}}
+{"return": {}}
+{}
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-fmt-job"}}
+{"return": {}}
+{}
+{"execute": "blockdev-backup", "arguments": {"auto-finalize": false, "bitmap": "bitmap0", "bitmap-mode": "always", "device": "drive0", "job-id": "bitmap_backup_2", "sync": "bitmap", "target": "bitmap_target_2"}}
+{"return": {}}
+{"execute": "job-finalize", "arguments": {"id": "bitmap_backup_2"}}
+{"return": {}}
+{"data": {"id": "bitmap_backup_2", "type": "backup"}, "event": "BLOCK_JOB_PENDING", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"data": {"device": "bitmap_backup_2", "len": 67108864, "offset": 67108864, "speed": 0, "type": "backup"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{
+  "bitmaps": {
+    "device0": [
+      {
+        "busy": false,
+        "count": 0,
+        "granularity": 65536,
+        "name": "bitmap0",
+        "persistent": false,
+        "recording": true,
+        "status": "active"
+      }
+    ]
+  }
+}
+
+= Checking Bitmap bitmap0 =
+expecting 0 dirty sectors; have 0. OK!
+
+--- Cleanup ---
+
+{"execute": "block-dirty-bitmap-remove", "arguments": {"name": "bitmap0", "node": "drive0"}}
+{"return": {}}
+{
+  "bitmaps": {
+    "device0": []
+  }
+}
+
+--- Verification ---
+
+qemu_img compare "TEST_DIR/PID-bsync1" "TEST_DIR/PID-fbackup1" ==> Identical, OK!
+qemu_img compare "TEST_DIR/PID-bsync2" "TEST_DIR/PID-fbackup2" ==> Identical, OK!
+qemu_img compare "TEST_DIR/PID-img" "TEST_DIR/PID-fbackup2" ==> Identical, OK!
+
+
+=== Bitmap Sync Mode always without failure ===
+
+--- Preparing image & VM ---
+
+{"execute": "blockdev-add", "arguments": {"driver": "qcow2", "file": {"driver": "file", "filename": "TEST_DIR/PID-img"}, "node-name": "drive0"}}
+{"return": {}}
+{"execute": "device_add", "arguments": {"drive": "drive0", "driver": "scsi-hd", "id": "device0", "share-rw": true}}
+{"return": {}}
+
+--- Write #0 ---
+
+write -P0x49 0x0000000 0x10000
+{"return": ""}
+write -P0x6c 0x0100000 0x10000
+{"return": ""}
+write -P0x6f 0x2000000 0x10000
+{"return": ""}
+write -P0x76 0x3ff0000 0x10000
+{"return": ""}
+{
+  "bitmaps": {
+    "device0": []
+  }
+}
+
+--- Reference Backup #0 ---
+
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-file-job"}}
+{"return": {}}
+{}
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-fmt-job"}}
+{"return": {}}
+{}
+{"execute": "blockdev-backup", "arguments": {"device": "drive0", "job-id": "ref_backup_0", "sync": "full", "target": "ref_target_0"}}
+{"return": {}}
+{"data": {"device": "ref_backup_0", "len": 67108864, "offset": 67108864, "speed": 0, "type": "backup"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+
+--- Add Bitmap ---
+
+{"execute": "block-dirty-bitmap-add", "arguments": {"granularity": 65536, "name": "bitmap0", "node": "drive0"}}
+{"return": {}}
+
+--- Write #1 ---
+
+write -P0x65 0x0000000 0x10000
+{"return": ""}
+write -P0x77 0x00f8000 0x10000
+{"return": ""}
+write -P0x72 0x2008000 0x10000
+{"return": ""}
+write -P0x69 0x3fe0000 0x10000
+{"return": ""}
+{
+  "bitmaps": {
+    "device0": [
+      {
+        "busy": false,
+        "count": 393216,
+        "granularity": 65536,
+        "name": "bitmap0",
+        "persistent": false,
+        "recording": true,
+        "status": "active"
+      }
+    ]
+  }
+}
+
+= Checking Bitmap bitmap0 =
+expecting 6 dirty sectors; have 6. OK!
+
+--- Reference Backup #1 ---
+
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-file-job"}}
+{"return": {}}
+{}
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-fmt-job"}}
+{"return": {}}
+{}
+{"execute": "blockdev-backup", "arguments": {"device": "drive0", "job-id": "ref_backup_1", "sync": "full", "target": "ref_target_1"}}
+{"return": {}}
+{"data": {"device": "ref_backup_1", "len": 67108864, "offset": 67108864, "speed": 0, "type": "backup"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+
+--- Bitmap Backup #1 ---
+
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-file-job"}}
+{"return": {}}
+{}
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-fmt-job"}}
+{"return": {}}
+{}
+{"execute": "blockdev-backup", "arguments": {"auto-finalize": false, "bitmap": "bitmap0", "bitmap-mode": "always", "device": "drive0", "job-id": "bitmap_backup_1", "sync": "bitmap", "target": "bitmap_target_1"}}
+{"return": {}}
+
+--- Write #2 ---
+
+write -P0x74 0x0010000 0x10000
+{"return": ""}
+write -P0x69 0x00e8000 0x10000
+{"return": ""}
+write -P0x6e 0x2018000 0x10000
+{"return": ""}
+write -P0x67 0x3fe0000 0x20000
+{"return": ""}
+{
+  "bitmaps": {
+    "device0": [
+      {
+        "busy": false,
+        "count": 458752,
+        "granularity": 65536,
+        "persistent": false,
+        "recording": true,
+        "status": "active"
+      },
+      {
+        "busy": true,
+        "count": 393216,
+        "granularity": 65536,
+        "name": "bitmap0",
+        "persistent": false,
+        "recording": true,
+        "status": "frozen"
+      }
+    ]
+  }
+}
+
+= Checking Bitmap bitmap0 =
+expecting 6 dirty sectors; have 6. OK!
+
+= Checking Bitmap (anonymous) =
+expecting 7 dirty sectors; have 7. OK!
+
+{"execute": "job-finalize", "arguments": {"id": "bitmap_backup_1"}}
+{"return": {}}
+{"data": {"id": "bitmap_backup_1", "type": "backup"}, "event": "BLOCK_JOB_PENDING", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"data": {"device": "bitmap_backup_1", "len": 67108864, "offset": 67108864, "speed": 0, "type": "backup"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{
+  "bitmaps": {
+    "device0": [
+      {
+        "busy": false,
+        "count": 458752,
+        "granularity": 65536,
+        "name": "bitmap0",
+        "persistent": false,
+        "recording": true,
+        "status": "active"
+      }
+    ]
+  }
+}
+
+= Checking Bitmap bitmap0 =
+expecting 7 dirty sectors; have 7. OK!
+
+--- Write #3 ---
+
+write -P0xaa 0x0010000 0x30000
+{"return": ""}
+write -P0xbb 0x00d8000 0x10000
+{"return": ""}
+write -P0xcc 0x2028000 0x10000
+{"return": ""}
+write -P0xdd 0x3fc0000 0x10000
+{"return": ""}
+{
+  "bitmaps": {
+    "device0": [
+      {
+        "busy": false,
+        "count": 786432,
+        "granularity": 65536,
+        "name": "bitmap0",
+        "persistent": false,
+        "recording": true,
+        "status": "active"
+      }
+    ]
+  }
+}
+
+= Checking Bitmap bitmap0 =
+expecting 12 dirty sectors; have 12. OK!
+
+--- Reference Backup #2 ---
+
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-file-job"}}
+{"return": {}}
+{}
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-fmt-job"}}
+{"return": {}}
+{}
+{"execute": "blockdev-backup", "arguments": {"device": "drive0", "job-id": "ref_backup_2", "sync": "full", "target": "ref_target_2"}}
+{"return": {}}
+{"data": {"device": "ref_backup_2", "len": 67108864, "offset": 67108864, "speed": 0, "type": "backup"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+
+--- Bitmap Backup #2 ---
+
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-file-job"}}
+{"return": {}}
+{}
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-fmt-job"}}
+{"return": {}}
+{}
+{"execute": "blockdev-backup", "arguments": {"auto-finalize": false, "bitmap": "bitmap0", "bitmap-mode": "always", "device": "drive0", "job-id": "bitmap_backup_2", "sync": "bitmap", "target": "bitmap_target_2"}}
+{"return": {}}
+{"execute": "job-finalize", "arguments": {"id": "bitmap_backup_2"}}
+{"return": {}}
+{"data": {"id": "bitmap_backup_2", "type": "backup"}, "event": "BLOCK_JOB_PENDING", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"data": {"device": "bitmap_backup_2", "len": 67108864, "offset": 67108864, "speed": 0, "type": "backup"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{
+  "bitmaps": {
+    "device0": [
+      {
+        "busy": false,
+        "count": 0,
+        "granularity": 65536,
+        "name": "bitmap0",
+        "persistent": false,
+        "recording": true,
+        "status": "active"
+      }
+    ]
+  }
+}
+
+= Checking Bitmap bitmap0 =
+expecting 0 dirty sectors; have 0. OK!
+
+--- Cleanup ---
+
+{"execute": "block-dirty-bitmap-remove", "arguments": {"name": "bitmap0", "node": "drive0"}}
+{"return": {}}
+{
+  "bitmaps": {
+    "device0": []
+  }
+}
+
+--- Verification ---
+
+qemu_img compare "TEST_DIR/PID-bsync1" "TEST_DIR/PID-fbackup1" ==> Identical, OK!
+qemu_img compare "TEST_DIR/PID-bsync2" "TEST_DIR/PID-fbackup2" ==> Identical, OK!
+qemu_img compare "TEST_DIR/PID-img" "TEST_DIR/PID-fbackup2" ==> Identical, OK!
+
+
+=== Bitmap Sync Mode never with intermediate failure ===
+
+--- Preparing image & VM ---
+
+{"execute": "blockdev-add", "arguments": {"driver": "qcow2", "file": {"driver": "blkdebug", "image": {"driver": "file", "filename": "TEST_DIR/PID-img"}, "inject-error": [{"errno": 5, "event": "read_aio", "immediately": false, "once": true, "state": 3}], "set-state": [{"event": "flush_to_disk", "new-state": 2, "state": 1}, {"event": "read_aio", "new-state": 3, "state": 2}]}, "node-name": "drive0"}}
+{"return": {}}
+{"execute": "device_add", "arguments": {"drive": "drive0", "driver": "scsi-hd", "id": "device0", "share-rw": true}}
+{"return": {}}
+
+--- Write #0 ---
+
+write -P0x49 0x0000000 0x10000
+{"return": ""}
+write -P0x6c 0x0100000 0x10000
+{"return": ""}
+write -P0x6f 0x2000000 0x10000
+{"return": ""}
+write -P0x76 0x3ff0000 0x10000
+{"return": ""}
+{
+  "bitmaps": {
+    "device0": []
+  }
+}
+
+--- Reference Backup #0 ---
+
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-file-job"}}
+{"return": {}}
+{}
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-fmt-job"}}
+{"return": {}}
+{}
+{"execute": "blockdev-backup", "arguments": {"device": "drive0", "job-id": "ref_backup_0", "sync": "full", "target": "ref_target_0"}}
+{"return": {}}
+{"data": {"device": "ref_backup_0", "len": 67108864, "offset": 67108864, "speed": 0, "type": "backup"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+
+--- Add Bitmap ---
+
+{"execute": "block-dirty-bitmap-add", "arguments": {"granularity": 65536, "name": "bitmap0", "node": "drive0"}}
+{"return": {}}
+
+--- Write #1 ---
+
+write -P0x65 0x0000000 0x10000
+{"return": ""}
+write -P0x77 0x00f8000 0x10000
+{"return": ""}
+write -P0x72 0x2008000 0x10000
+{"return": ""}
+write -P0x69 0x3fe0000 0x10000
+{"return": ""}
+{
+  "bitmaps": {
+    "device0": [
+      {
+        "busy": false,
+        "count": 393216,
+        "granularity": 65536,
+        "name": "bitmap0",
+        "persistent": false,
+        "recording": true,
+        "status": "active"
+      }
+    ]
+  }
+}
+
+= Checking Bitmap bitmap0 =
+expecting 6 dirty sectors; have 6. OK!
+
+--- Reference Backup #1 ---
+
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-file-job"}}
+{"return": {}}
+{}
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-fmt-job"}}
+{"return": {}}
+{}
+{"execute": "blockdev-backup", "arguments": {"device": "drive0", "job-id": "ref_backup_1", "sync": "full", "target": "ref_target_1"}}
+{"return": {}}
+{"data": {"device": "ref_backup_1", "len": 67108864, "offset": 67108864, "speed": 0, "type": "backup"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+
+{"return": ""}
+
+--- Bitmap Backup #1 ---
+
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-file-job"}}
+{"return": {}}
+{}
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-fmt-job"}}
+{"return": {}}
+{}
+{"execute": "blockdev-backup", "arguments": {"auto-finalize": false, "bitmap": "bitmap0", "bitmap-mode": "never", "device": "drive0", "job-id": "bitmap_backup_1", "sync": "bitmap", "target": "bitmap_target_1"}}
+{"return": {}}
+{"data": {"action": "report", "device": "bitmap_backup_1", "operation": "read"}, "event": "BLOCK_JOB_ERROR", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"data": {"device": "bitmap_backup_1", "error": "Input/output error", "len": 67108864, "offset": 66781184, "speed": 0, "type": "backup"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{
+  "bitmaps": {
+    "device0": [
+      {
+        "busy": false,
+        "count": 393216,
+        "granularity": 65536,
+        "name": "bitmap0",
+        "persistent": false,
+        "recording": true,
+        "status": "active"
+      }
+    ]
+  }
+}
+
+= Checking Bitmap bitmap0 =
+expecting 6 dirty sectors; have 6. OK!
+
+--- Write #3 ---
+
+write -P0xaa 0x0010000 0x30000
+{"return": ""}
+write -P0xbb 0x00d8000 0x10000
+{"return": ""}
+write -P0xcc 0x2028000 0x10000
+{"return": ""}
+write -P0xdd 0x3fc0000 0x10000
+{"return": ""}
+{
+  "bitmaps": {
+    "device0": [
+      {
+        "busy": false,
+        "count": 917504,
+        "granularity": 65536,
+        "name": "bitmap0",
+        "persistent": false,
+        "recording": true,
+        "status": "active"
+      }
+    ]
+  }
+}
+
+= Checking Bitmap bitmap0 =
+expecting 14 dirty sectors; have 14. OK!
+
+--- Reference Backup #2 ---
+
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-file-job"}}
+{"return": {}}
+{}
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-fmt-job"}}
+{"return": {}}
+{}
+{"execute": "blockdev-backup", "arguments": {"device": "drive0", "job-id": "ref_backup_2", "sync": "full", "target": "ref_target_2"}}
+{"return": {}}
+{"data": {"device": "ref_backup_2", "len": 67108864, "offset": 67108864, "speed": 0, "type": "backup"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+
+--- Bitmap Backup #2 ---
+
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-file-job"}}
+{"return": {}}
+{}
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-fmt-job"}}
+{"return": {}}
+{}
+{"execute": "blockdev-backup", "arguments": {"auto-finalize": false, "bitmap": "bitmap0", "bitmap-mode": "never", "device": "drive0", "job-id": "bitmap_backup_2", "sync": "bitmap", "target": "bitmap_target_2"}}
+{"return": {}}
+{"execute": "job-finalize", "arguments": {"id": "bitmap_backup_2"}}
+{"return": {}}
+{"data": {"id": "bitmap_backup_2", "type": "backup"}, "event": "BLOCK_JOB_PENDING", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"data": {"device": "bitmap_backup_2", "len": 67108864, "offset": 67108864, "speed": 0, "type": "backup"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{
+  "bitmaps": {
+    "device0": [
+      {
+        "busy": false,
+        "count": 917504,
+        "granularity": 65536,
+        "name": "bitmap0",
+        "persistent": false,
+        "recording": true,
+        "status": "active"
+      }
+    ]
+  }
+}
+
+= Checking Bitmap bitmap0 =
+expecting 14 dirty sectors; have 14. OK!
+
+--- Cleanup ---
+
+{"execute": "block-dirty-bitmap-remove", "arguments": {"name": "bitmap0", "node": "drive0"}}
+{"return": {}}
+{
+  "bitmaps": {
+    "device0": []
+  }
+}
+
+--- Verification ---
+
+qemu_img compare "TEST_DIR/PID-bsync1" "TEST_DIR/PID-fbackup1" ==> Mismatch, OK!
+qemu_img compare "TEST_DIR/PID-bsync2" "TEST_DIR/PID-fbackup2" ==> Identical, OK!
+qemu_img compare "TEST_DIR/PID-img" "TEST_DIR/PID-fbackup2" ==> Identical, OK!
+
+
+=== Bitmap Sync Mode conditional with intermediate failure ===
+
+--- Preparing image & VM ---
+
+{"execute": "blockdev-add", "arguments": {"driver": "qcow2", "file": {"driver": "blkdebug", "image": {"driver": "file", "filename": "TEST_DIR/PID-img"}, "inject-error": [{"errno": 5, "event": "read_aio", "immediately": false, "once": true, "state": 3}], "set-state": [{"event": "flush_to_disk", "new-state": 2, "state": 1}, {"event": "read_aio", "new-state": 3, "state": 2}]}, "node-name": "drive0"}}
+{"return": {}}
+{"execute": "device_add", "arguments": {"drive": "drive0", "driver": "scsi-hd", "id": "device0", "share-rw": true}}
+{"return": {}}
+
+--- Write #0 ---
+
+write -P0x49 0x0000000 0x10000
+{"return": ""}
+write -P0x6c 0x0100000 0x10000
+{"return": ""}
+write -P0x6f 0x2000000 0x10000
+{"return": ""}
+write -P0x76 0x3ff0000 0x10000
+{"return": ""}
+{
+  "bitmaps": {
+    "device0": []
+  }
+}
+
+--- Reference Backup #0 ---
+
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-file-job"}}
+{"return": {}}
+{}
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-fmt-job"}}
+{"return": {}}
+{}
+{"execute": "blockdev-backup", "arguments": {"device": "drive0", "job-id": "ref_backup_0", "sync": "full", "target": "ref_target_0"}}
+{"return": {}}
+{"data": {"device": "ref_backup_0", "len": 67108864, "offset": 67108864, "speed": 0, "type": "backup"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+
+--- Add Bitmap ---
+
+{"execute": "block-dirty-bitmap-add", "arguments": {"granularity": 65536, "name": "bitmap0", "node": "drive0"}}
+{"return": {}}
+
+--- Write #1 ---
+
+write -P0x65 0x0000000 0x10000
+{"return": ""}
+write -P0x77 0x00f8000 0x10000
+{"return": ""}
+write -P0x72 0x2008000 0x10000
+{"return": ""}
+write -P0x69 0x3fe0000 0x10000
+{"return": ""}
+{
+  "bitmaps": {
+    "device0": [
+      {
+        "busy": false,
+        "count": 393216,
+        "granularity": 65536,
+        "name": "bitmap0",
+        "persistent": false,
+        "recording": true,
+        "status": "active"
+      }
+    ]
+  }
+}
+
+= Checking Bitmap bitmap0 =
+expecting 6 dirty sectors; have 6. OK!
+
+--- Reference Backup #1 ---
+
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-file-job"}}
+{"return": {}}
+{}
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-fmt-job"}}
+{"return": {}}
+{}
+{"execute": "blockdev-backup", "arguments": {"device": "drive0", "job-id": "ref_backup_1", "sync": "full", "target": "ref_target_1"}}
+{"return": {}}
+{"data": {"device": "ref_backup_1", "len": 67108864, "offset": 67108864, "speed": 0, "type": "backup"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+
+{"return": ""}
+
+--- Bitmap Backup #1 ---
+
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-file-job"}}
+{"return": {}}
+{}
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-fmt-job"}}
+{"return": {}}
+{}
+{"execute": "blockdev-backup", "arguments": {"auto-finalize": false, "bitmap": "bitmap0", "bitmap-mode": "conditional", "device": "drive0", "job-id": "bitmap_backup_1", "sync": "bitmap", "target": "bitmap_target_1"}}
+{"return": {}}
+{"data": {"action": "report", "device": "bitmap_backup_1", "operation": "read"}, "event": "BLOCK_JOB_ERROR", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"data": {"device": "bitmap_backup_1", "error": "Input/output error", "len": 67108864, "offset": 66781184, "speed": 0, "type": "backup"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{
+  "bitmaps": {
+    "device0": [
+      {
+        "busy": false,
+        "count": 393216,
+        "granularity": 65536,
+        "name": "bitmap0",
+        "persistent": false,
+        "recording": true,
+        "status": "active"
+      }
+    ]
+  }
+}
+
+= Checking Bitmap bitmap0 =
+expecting 6 dirty sectors; have 6. OK!
+
+--- Write #3 ---
+
+write -P0xaa 0x0010000 0x30000
+{"return": ""}
+write -P0xbb 0x00d8000 0x10000
+{"return": ""}
+write -P0xcc 0x2028000 0x10000
+{"return": ""}
+write -P0xdd 0x3fc0000 0x10000
+{"return": ""}
+{
+  "bitmaps": {
+    "device0": [
+      {
+        "busy": false,
+        "count": 917504,
+        "granularity": 65536,
+        "name": "bitmap0",
+        "persistent": false,
+        "recording": true,
+        "status": "active"
+      }
+    ]
+  }
+}
+
+= Checking Bitmap bitmap0 =
+expecting 14 dirty sectors; have 14. OK!
+
+--- Reference Backup #2 ---
+
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-file-job"}}
+{"return": {}}
+{}
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-fmt-job"}}
+{"return": {}}
+{}
+{"execute": "blockdev-backup", "arguments": {"device": "drive0", "job-id": "ref_backup_2", "sync": "full", "target": "ref_target_2"}}
+{"return": {}}
+{"data": {"device": "ref_backup_2", "len": 67108864, "offset": 67108864, "speed": 0, "type": "backup"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+
+--- Bitmap Backup #2 ---
+
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-file-job"}}
+{"return": {}}
+{}
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-fmt-job"}}
+{"return": {}}
+{}
+{"execute": "blockdev-backup", "arguments": {"auto-finalize": false, "bitmap": "bitmap0", "bitmap-mode": "conditional", "device": "drive0", "job-id": "bitmap_backup_2", "sync": "bitmap", "target": "bitmap_target_2"}}
+{"return": {}}
+{"execute": "job-finalize", "arguments": {"id": "bitmap_backup_2"}}
+{"return": {}}
+{"data": {"id": "bitmap_backup_2", "type": "backup"}, "event": "BLOCK_JOB_PENDING", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"data": {"device": "bitmap_backup_2", "len": 67108864, "offset": 67108864, "speed": 0, "type": "backup"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{
+  "bitmaps": {
+    "device0": [
+      {
+        "busy": false,
+        "count": 0,
+        "granularity": 65536,
+        "name": "bitmap0",
+        "persistent": false,
+        "recording": true,
+        "status": "active"
+      }
+    ]
+  }
+}
+
+= Checking Bitmap bitmap0 =
+expecting 0 dirty sectors; have 0. OK!
+
+--- Cleanup ---
+
+{"execute": "block-dirty-bitmap-remove", "arguments": {"name": "bitmap0", "node": "drive0"}}
+{"return": {}}
+{
+  "bitmaps": {
+    "device0": []
+  }
+}
+
+--- Verification ---
+
+qemu_img compare "TEST_DIR/PID-bsync1" "TEST_DIR/PID-fbackup1" ==> Mismatch, OK!
+qemu_img compare "TEST_DIR/PID-bsync2" "TEST_DIR/PID-fbackup2" ==> Identical, OK!
+qemu_img compare "TEST_DIR/PID-img" "TEST_DIR/PID-fbackup2" ==> Identical, OK!
+
+
+=== Bitmap Sync Mode always with intermediate failure ===
+
+--- Preparing image & VM ---
+
+{"execute": "blockdev-add", "arguments": {"driver": "qcow2", "file": {"driver": "blkdebug", "image": {"driver": "file", "filename": "TEST_DIR/PID-img"}, "inject-error": [{"errno": 5, "event": "read_aio", "immediately": false, "once": true, "state": 3}], "set-state": [{"event": "flush_to_disk", "new-state": 2, "state": 1}, {"event": "read_aio", "new-state": 3, "state": 2}]}, "node-name": "drive0"}}
+{"return": {}}
+{"execute": "device_add", "arguments": {"drive": "drive0", "driver": "scsi-hd", "id": "device0", "share-rw": true}}
+{"return": {}}
+
+--- Write #0 ---
+
+write -P0x49 0x0000000 0x10000
+{"return": ""}
+write -P0x6c 0x0100000 0x10000
+{"return": ""}
+write -P0x6f 0x2000000 0x10000
+{"return": ""}
+write -P0x76 0x3ff0000 0x10000
+{"return": ""}
+{
+  "bitmaps": {
+    "device0": []
+  }
+}
+
+--- Reference Backup #0 ---
+
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-file-job"}}
+{"return": {}}
+{}
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-fmt-job"}}
+{"return": {}}
+{}
+{"execute": "blockdev-backup", "arguments": {"device": "drive0", "job-id": "ref_backup_0", "sync": "full", "target": "ref_target_0"}}
+{"return": {}}
+{"data": {"device": "ref_backup_0", "len": 67108864, "offset": 67108864, "speed": 0, "type": "backup"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+
+--- Add Bitmap ---
+
+{"execute": "block-dirty-bitmap-add", "arguments": {"granularity": 65536, "name": "bitmap0", "node": "drive0"}}
+{"return": {}}
+
+--- Write #1 ---
+
+write -P0x65 0x0000000 0x10000
+{"return": ""}
+write -P0x77 0x00f8000 0x10000
+{"return": ""}
+write -P0x72 0x2008000 0x10000
+{"return": ""}
+write -P0x69 0x3fe0000 0x10000
+{"return": ""}
+{
+  "bitmaps": {
+    "device0": [
+      {
+        "busy": false,
+        "count": 393216,
+        "granularity": 65536,
+        "name": "bitmap0",
+        "persistent": false,
+        "recording": true,
+        "status": "active"
+      }
+    ]
+  }
+}
+
+= Checking Bitmap bitmap0 =
+expecting 6 dirty sectors; have 6. OK!
+
+--- Reference Backup #1 ---
+
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-file-job"}}
+{"return": {}}
+{}
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-fmt-job"}}
+{"return": {}}
+{}
+{"execute": "blockdev-backup", "arguments": {"device": "drive0", "job-id": "ref_backup_1", "sync": "full", "target": "ref_target_1"}}
+{"return": {}}
+{"data": {"device": "ref_backup_1", "len": 67108864, "offset": 67108864, "speed": 0, "type": "backup"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+
+{"return": ""}
+
+--- Bitmap Backup #1 ---
+
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-file-job"}}
+{"return": {}}
+{}
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-fmt-job"}}
+{"return": {}}
+{}
+{"execute": "blockdev-backup", "arguments": {"auto-finalize": false, "bitmap": "bitmap0", "bitmap-mode": "always", "device": "drive0", "job-id": "bitmap_backup_1", "sync": "bitmap", "target": "bitmap_target_1"}}
+{"return": {}}
+{"data": {"action": "report", "device": "bitmap_backup_1", "operation": "read"}, "event": "BLOCK_JOB_ERROR", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"data": {"device": "bitmap_backup_1", "error": "Input/output error", "len": 67108864, "offset": 66781184, "speed": 0, "type": "backup"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{
+  "bitmaps": {
+    "device0": [
+      {
+        "busy": false,
+        "count": 327680,
+        "granularity": 65536,
+        "name": "bitmap0",
+        "persistent": false,
+        "recording": true,
+        "status": "active"
+      }
+    ]
+  }
+}
+
+= Checking Bitmap bitmap0 =
+expecting 5 dirty sectors; have 5. OK!
+
+--- Write #3 ---
+
+write -P0xaa 0x0010000 0x30000
+{"return": ""}
+write -P0xbb 0x00d8000 0x10000
+{"return": ""}
+write -P0xcc 0x2028000 0x10000
+{"return": ""}
+write -P0xdd 0x3fc0000 0x10000
+{"return": ""}
+{
+  "bitmaps": {
+    "device0": [
+      {
+        "busy": false,
+        "count": 851968,
+        "granularity": 65536,
+        "name": "bitmap0",
+        "persistent": false,
+        "recording": true,
+        "status": "active"
+      }
+    ]
+  }
+}
+
+= Checking Bitmap bitmap0 =
+expecting 13 dirty sectors; have 13. OK!
+
+--- Reference Backup #2 ---
+
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-file-job"}}
+{"return": {}}
+{}
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-fmt-job"}}
+{"return": {}}
+{}
+{"execute": "blockdev-backup", "arguments": {"device": "drive0", "job-id": "ref_backup_2", "sync": "full", "target": "ref_target_2"}}
+{"return": {}}
+{"data": {"device": "ref_backup_2", "len": 67108864, "offset": 67108864, "speed": 0, "type": "backup"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+
+--- Bitmap Backup #2 ---
+
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-file-job"}}
+{"return": {}}
+{}
+{}
+{"execute": "job-dismiss", "arguments": {"id": "bdc-fmt-job"}}
+{"return": {}}
+{}
+{"execute": "blockdev-backup", "arguments": {"auto-finalize": false, "bitmap": "bitmap0", "bitmap-mode": "always", "device": "drive0", "job-id": "bitmap_backup_2", "sync": "bitmap", "target": "bitmap_target_2"}}
+{"return": {}}
+{"execute": "job-finalize", "arguments": {"id": "bitmap_backup_2"}}
+{"return": {}}
+{"data": {"id": "bitmap_backup_2", "type": "backup"}, "event": "BLOCK_JOB_PENDING", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"data": {"device": "bitmap_backup_2", "len": 67108864, "offset": 67108864, "speed": 0, "type": "backup"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{
+  "bitmaps": {
+    "device0": [
+      {
+        "busy": false,
+        "count": 0,
+        "granularity": 65536,
+        "name": "bitmap0",
+        "persistent": false,
+        "recording": true,
+        "status": "active"
+      }
+    ]
+  }
+}
+
+= Checking Bitmap bitmap0 =
+expecting 0 dirty sectors; have 0. OK!
+
+--- Cleanup ---
+
+{"execute": "block-dirty-bitmap-remove", "arguments": {"name": "bitmap0", "node": "drive0"}}
+{"return": {}}
+{
+  "bitmaps": {
+    "device0": []
+  }
+}
+
+--- Verification ---
+
+qemu_img compare "TEST_DIR/PID-bsync1" "TEST_DIR/PID-fbackup1" ==> Mismatch, OK!
+qemu_img compare "TEST_DIR/PID-bsync2" "TEST_DIR/PID-fbackup2" ==> Identical, OK!
+qemu_img compare "TEST_DIR/PID-img" "TEST_DIR/PID-fbackup2" ==> Identical, OK!
+
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index b34c8e3c0c..b53afcd149 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -269,3 +269,4 @@
 254 rw auto backing quick
 255 rw auto quick
 256 rw auto quick
+257 rw auto
-- 
2.21.0

Re: [Qemu-devel] [PATCH 11/12] iotests: add test 257 for bitmap-mode backups

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 7358 bytes --]

On 20.06.19 03:03, John Snow wrote:
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  tests/qemu-iotests/257     |  412 +++++++
>  tests/qemu-iotests/257.out | 2199 ++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/group   |    1 +
>  3 files changed, 2612 insertions(+)
>  create mode 100755 tests/qemu-iotests/257
>  create mode 100644 tests/qemu-iotests/257.out

This test is actually quite nicely written.

I like that I don’t have to read the reference output but can just grep
for “error”.

Only minor notes below.

> diff --git a/tests/qemu-iotests/257 b/tests/qemu-iotests/257
> new file mode 100755
> index 0000000000..5f7f388504
> --- /dev/null
> +++ b/tests/qemu-iotests/257

[...]

> +class PatternGroup:
> +    """Grouping of Pattern objects. Initialize with an iterable of Patterns."""
> +    def __init__(self, patterns):
> +        self.patterns = patterns
> +
> +    def bits(self, granularity):
> +        """Calculate the unique bits dirtied by this pattern grouping"""
> +        res = set()
> +        for pattern in self.patterns:
> +            lower = math.floor(pattern.offset / granularity)
> +            upper = math.floor((pattern.offset + pattern.size - 1) / granularity)
> +            res = res | set(range(lower, upper + 1))

Why you’d do floor((x - 1) / y) + 1 has confused me quite a while.
Until I realized that oh yeah, Python’s range() is a right-open
interval.  I don’t like Python’s range().

(Yes, you’re right, this is better to read than just ceil(x / y).
Because it reminds people like me that range() is weird.)

> +        return res
> +
> +GROUPS = [
> +    PatternGroup([
> +        # Batch 0: 4 clusters
> +        mkpattern('0x49', 0x0000000),
> +        mkpattern('0x6c', 0x0100000),   # 1M
> +        mkpattern('0x6f', 0x2000000),   # 32M
> +        mkpattern('0x76', 0x3ff0000)]), # 64M - 64K
> +    PatternGroup([
> +        # Batch 1: 6 clusters (3 new)
> +        mkpattern('0x65', 0x0000000),   # Full overwrite
> +        mkpattern('0x77', 0x00f8000),   # Partial-left (1M-32K)
> +        mkpattern('0x72', 0x2008000),   # Partial-right (32M+32K)
> +        mkpattern('0x69', 0x3fe0000)]), # Adjacent-left (64M - 128K)
> +    PatternGroup([
> +        # Batch 2: 7 clusters (3 new)
> +        mkpattern('0x74', 0x0010000),   # Adjacent-right
> +        mkpattern('0x69', 0x00e8000),   # Partial-left  (1M-96K)
> +        mkpattern('0x6e', 0x2018000),   # Partial-right (32M+96K)
> +        mkpattern('0x67', 0x3fe0000,
> +                  2*GRANULARITY)]),     # Overwrite [(64M-128K)-64M)
> +    PatternGroup([
> +        # Batch 3: 8 clusters (5 new)
> +        # Carefully chosen such that nothing re-dirties the one cluster
> +        # that copies out successfully before failure in Group #1.
> +        mkpattern('0xaa', 0x0010000,
> +                  3*GRANULARITY),       # Overwrite and 2x Adjacent-right
> +        mkpattern('0xbb', 0x00d8000),   # Partial-left (1M-160K)
> +        mkpattern('0xcc', 0x2028000),   # Partial-right (32M+160K)
> +        mkpattern('0xdd', 0x3fc0000)]), # New; leaving a gap to the right
> +    ]

I’d place this four spaces to the left.  But maybe placing it here is
proper Python indentation, while moving it to the left would be C
indentation.

> +class Drive:
> +    """Represents, vaguely, a drive attached to a VM.
> +    Includes format, graph, and device information."""
> +
> +    def __init__(self, path, vm=None):
> +        self.path = path
> +        self.vm = vm
> +        self.fmt = None
> +        self.size = None
> +        self.node = None
> +        self.device = None
> +
> +    @property
> +    def name(self):
> +        return self.node or self.device
> +
> +    def img_create(self, fmt, size):
> +        self.fmt = fmt
> +        self.size = size
> +        iotests.qemu_img_create('-f', self.fmt, self.path, str(self.size))
> +
> +    def create_target(self, name, fmt, size):
> +        basename = os.path.basename(self.path)
> +        file_node_name = "file_{}".format(basename)
> +        vm = self.vm
> +
> +        log(vm.command('blockdev-create', job_id='bdc-file-job',
> +                       options={
> +                           'driver': 'file',
> +                           'filename': self.path,
> +                           'size': 0,
> +                       }))
> +        vm.run_job('bdc-file-job')
> +        log(vm.command('blockdev-add', driver='file',
> +                       node_name=file_node_name, filename=self.path))
> +
> +        log(vm.command('blockdev-create', job_id='bdc-fmt-job',
> +                       options={
> +                           'driver': fmt,
> +                           'file': file_node_name,
> +                           'size': size,
> +                       }))
> +        vm.run_job('bdc-fmt-job')
> +        log(vm.command('blockdev-add', driver=fmt,
> +                       node_name=name,
> +                       file=file_node_name))
> +        self.fmt = fmt
> +        self.size = size
> +        self.node = name

It’s cool that you use blockdev-create here, but would it not have been
easier to just use self.img_create() + blockdev-add?

I mean, there’s no point in changing it now, I’m just wondering.

> +
> +def query_bitmaps(vm):
> +    res = vm.qmp("query-block")
> +    return {"bitmaps": {device['device'] or device['qdev']:
> +                        device.get('dirty-bitmaps', []) for
> +                        device in res['return']}}

Python’s just not for me if I find this syntax unintuitive and
confusing, hu?

[...]

> +def bitmap_comparison(bitmap, groups=None, want=0):
> +    """
> +    Print a nice human-readable message checking that this bitmap has as
> +    many bits set as we expect it to.
> +    """
> +    log("= Checking Bitmap {:s} =".format(bitmap.get('name', '(anonymous)')))
> +
> +    if groups:
> +        want = calculate_bits(groups)
> +    have = int(bitmap['count'] / bitmap['granularity'])

Or just bitmap['count'] // bitmap['granularity']?

[...]

> +def test_bitmap_sync(bsync_mode, failure=None):

[...]

> +        log('--- Preparing image & VM ---\n')
> +        drive0 = Drive(img_path, vm=vm)
> +        drive0.img_create(iotests.imgfmt, SIZE)
> +        vm.add_device('virtio-scsi-pci,id=scsi0')

Judging from 238, this should be virtio-scsi-ccw on s390-ccw-virtio.

(This is the reason I cannot give an R-b.)

[...]

> +        vm.run_job(job, auto_dismiss=True, auto_finalize=False,
> +                   pre_finalize=_callback,
> +                   cancel=failure == 'simulated')

I’d prefer “cancel=(failure == 'simulated')”.  (Or spaces around =).

Also in other places elsewhere that are of the form x=y where y contains
spaces.

[...]

> +def main():
> +    for bsync_mode in ("never", "conditional", "always"):
> +        for failure in ("simulated", None):
> +            test_bitmap_sync(bsync_mode, failure)
> +
> +    for bsync_mode in ("never", "conditional", "always"):
> +        test_bitmap_sync(bsync_mode, "intermediate")

Why are these separate?  Couldn’t you just iterate over
("simulated", None, "intermediate")?

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 11/12] iotests: add test 257 for bitmap-mode backups

[John Snow]

On 6/20/19 2:35 PM, Max Reitz wrote:
> On 20.06.19 03:03, John Snow wrote:
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  tests/qemu-iotests/257     |  412 +++++++
>>  tests/qemu-iotests/257.out | 2199 ++++++++++++++++++++++++++++++++++++
>>  tests/qemu-iotests/group   |    1 +
>>  3 files changed, 2612 insertions(+)
>>  create mode 100755 tests/qemu-iotests/257
>>  create mode 100644 tests/qemu-iotests/257.out
> 
> This test is actually quite nicely written.
> 

Thanks!

> I like that I don’t have to read the reference output but can just grep
> for “error”.
> 

Me too!! Actually, doing the math for what to expect and verifying the
output by hand was becoming a major burden, so partially this test
infrastructure was my attempt to procedurally verify that the results I
was seeing were what made sense.

At the end of it, I felt it was nice to keep it in there.

> Only minor notes below.
> 
>> diff --git a/tests/qemu-iotests/257 b/tests/qemu-iotests/257
>> new file mode 100755
>> index 0000000000..5f7f388504
>> --- /dev/null
>> +++ b/tests/qemu-iotests/257
> 
> [...]
> 
>> +class PatternGroup:
>> +    """Grouping of Pattern objects. Initialize with an iterable of Patterns."""
>> +    def __init__(self, patterns):
>> +        self.patterns = patterns
>> +
>> +    def bits(self, granularity):
>> +        """Calculate the unique bits dirtied by this pattern grouping"""
>> +        res = set()
>> +        for pattern in self.patterns:
>> +            lower = math.floor(pattern.offset / granularity)
>> +            upper = math.floor((pattern.offset + pattern.size - 1) / granularity)
>> +            res = res | set(range(lower, upper + 1))
> 
> Why you’d do floor((x - 1) / y) + 1 has confused me quite a while.
> Until I realized that oh yeah, Python’s range() is a right-open
> interval.  I don’t like Python’s range().
> 

It confuses me constantly, but it's really meant to be used for
iterating over lengths. This is somewhat of an abuse of it. I always
test it out in a console first before using it, just in case.

> (Yes, you’re right, this is better to read than just ceil(x / y).
> Because it reminds people like me that range() is weird.)
> 
>> +        return res
>> +
>> +GROUPS = [
>> +    PatternGroup([
>> +        # Batch 0: 4 clusters
>> +        mkpattern('0x49', 0x0000000),
>> +        mkpattern('0x6c', 0x0100000),   # 1M
>> +        mkpattern('0x6f', 0x2000000),   # 32M
>> +        mkpattern('0x76', 0x3ff0000)]), # 64M - 64K
>> +    PatternGroup([
>> +        # Batch 1: 6 clusters (3 new)
>> +        mkpattern('0x65', 0x0000000),   # Full overwrite
>> +        mkpattern('0x77', 0x00f8000),   # Partial-left (1M-32K)
>> +        mkpattern('0x72', 0x2008000),   # Partial-right (32M+32K)
>> +        mkpattern('0x69', 0x3fe0000)]), # Adjacent-left (64M - 128K)
>> +    PatternGroup([
>> +        # Batch 2: 7 clusters (3 new)
>> +        mkpattern('0x74', 0x0010000),   # Adjacent-right
>> +        mkpattern('0x69', 0x00e8000),   # Partial-left  (1M-96K)
>> +        mkpattern('0x6e', 0x2018000),   # Partial-right (32M+96K)
>> +        mkpattern('0x67', 0x3fe0000,
>> +                  2*GRANULARITY)]),     # Overwrite [(64M-128K)-64M)
>> +    PatternGroup([
>> +        # Batch 3: 8 clusters (5 new)
>> +        # Carefully chosen such that nothing re-dirties the one cluster
>> +        # that copies out successfully before failure in Group #1.
>> +        mkpattern('0xaa', 0x0010000,
>> +                  3*GRANULARITY),       # Overwrite and 2x Adjacent-right
>> +        mkpattern('0xbb', 0x00d8000),   # Partial-left (1M-160K)
>> +        mkpattern('0xcc', 0x2028000),   # Partial-right (32M+160K)
>> +        mkpattern('0xdd', 0x3fc0000)]), # New; leaving a gap to the right
>> +    ]
> 
> I’d place this four spaces to the left.  But maybe placing it here is
> proper Python indentation, while moving it to the left would be C
> indentation.
> 

Either is fine, I think. In this case it affords us more room for the
commentary on the bit ranges. Maybe it's not necessary, but at least
personally I get woozy looking at the bit patterns.

>> +class Drive:
>> +    """Represents, vaguely, a drive attached to a VM.
>> +    Includes format, graph, and device information."""
>> +
>> +    def __init__(self, path, vm=None):
>> +        self.path = path
>> +        self.vm = vm
>> +        self.fmt = None
>> +        self.size = None
>> +        self.node = None
>> +        self.device = None
>> +
>> +    @property
>> +    def name(self):
>> +        return self.node or self.device
>> +
>> +    def img_create(self, fmt, size):
>> +        self.fmt = fmt
>> +        self.size = size
>> +        iotests.qemu_img_create('-f', self.fmt, self.path, str(self.size))
>> +
>> +    def create_target(self, name, fmt, size):
>> +        basename = os.path.basename(self.path)
>> +        file_node_name = "file_{}".format(basename)
>> +        vm = self.vm
>> +
>> +        log(vm.command('blockdev-create', job_id='bdc-file-job',
>> +                       options={
>> +                           'driver': 'file',
>> +                           'filename': self.path,
>> +                           'size': 0,
>> +                       }))
>> +        vm.run_job('bdc-file-job')
>> +        log(vm.command('blockdev-add', driver='file',
>> +                       node_name=file_node_name, filename=self.path))
>> +
>> +        log(vm.command('blockdev-create', job_id='bdc-fmt-job',
>> +                       options={
>> +                           'driver': fmt,
>> +                           'file': file_node_name,
>> +                           'size': size,
>> +                       }))
>> +        vm.run_job('bdc-fmt-job')
>> +        log(vm.command('blockdev-add', driver=fmt,
>> +                       node_name=name,
>> +                       file=file_node_name))
>> +        self.fmt = fmt
>> +        self.size = size
>> +        self.node = name
> 
> It’s cool that you use blockdev-create here, but would it not have been
> easier to just use self.img_create() + blockdev-add?
> 
> I mean, there’s no point in changing it now, I’m just wondering.
> 

Mostly just because I already wrote this for the last test, and we
already test incremental backups the other way. I figured this would
just be nice for code coverage purposes, and also because using the
blockdev interfaces exclusively does tend to reveal little gotchas
everywhere.

I also kind of want to refactor a lot of my tests and share some of the
common code. I was tinkering with the idea of adding some common objects
to iotests, like "Drive" "Bitmap" and "Backup".

That's why there's a kind of superfluous "Drive" object here.

>> +
>> +def query_bitmaps(vm):
>> +    res = vm.qmp("query-block")
>> +    return {"bitmaps": {device['device'] or device['qdev']:
>> +                        device.get('dirty-bitmaps', []) for
>> +                        device in res['return']}}
> 
> Python’s just not for me if I find this syntax unintuitive and
> confusing, hu?
> 
> [...]
> 

Sorry. It's a very functional-esque way of processing iterables.

I've been doing a lot of FP stuff lately and that skews what I find
readable...

It's a dict comprehension of the form:

{key: value for atom in iterable}

Key here is either the device or qdev name,
the value is the 'dirty-bitmaps' field of the atom, and
res['return'] is the iterable.

Effectively it turns a list of devices into a dict keyed by the device
name, and the only field (if any) was its dirty-bitmap object.

However, in explaining this I do notice I have a bug -- the default
value for the bitmap object ought to be {}, not [].

This is code that should become common testing code too, as I've
re-written it a few times now...

>> +def bitmap_comparison(bitmap, groups=None, want=0):
>> +    """
>> +    Print a nice human-readable message checking that this bitmap has as
>> +    many bits set as we expect it to.
>> +    """
>> +    log("= Checking Bitmap {:s} =".format(bitmap.get('name', '(anonymous)')))
>> +
>> +    if groups:
>> +        want = calculate_bits(groups)
>> +    have = int(bitmap['count'] / bitmap['granularity'])
> 
> Or just bitmap['count'] // bitmap['granularity']?
> 
> [...]
> 

I forget that exists. If you prefer that, OK.

>> +def test_bitmap_sync(bsync_mode, failure=None):
> 
> [...]
> 
>> +        log('--- Preparing image & VM ---\n')
>> +        drive0 = Drive(img_path, vm=vm)
>> +        drive0.img_create(iotests.imgfmt, SIZE)
>> +        vm.add_device('virtio-scsi-pci,id=scsi0')
> 
> Judging from 238, this should be virtio-scsi-ccw on s390-ccw-virtio.
> 
> (This is the reason I cannot give an R-b.)
> 
> [...]
> 

Oh, good catch. Alright.

>> +        vm.run_job(job, auto_dismiss=True, auto_finalize=False,
>> +                   pre_finalize=_callback,
>> +                   cancel=failure == 'simulated')
> 
> I’d prefer “cancel=(failure == 'simulated')”.  (Or spaces around =).
> 
> Also in other places elsewhere that are of the form x=y where y contains
> spaces.
> 
> [...]
> 

OK; I might need to make a pylintrc file to allow that style. Python is
VERY aggressively tuned to omitting parentheses.

(I do actually try to run pylint on my python patches now to make sure I
am targeting SOME kind of style. I realize this is not standardized in
this project.)

>> +def main():
>> +    for bsync_mode in ("never", "conditional", "always"):
>> +        for failure in ("simulated", None):
>> +            test_bitmap_sync(bsync_mode, failure)
>> +
>> +    for bsync_mode in ("never", "conditional", "always"):
>> +        test_bitmap_sync(bsync_mode, "intermediate")
> 
> Why are these separate?  Couldn’t you just iterate over
> ("simulated", None, "intermediate")?
> 
> Max
> 

Haha, oops! That's a holdover from when those two routines weren't
unified. I wrote it like this while I was unifying them to avoid
reordering the test output, which helped me polish the merging of the
routines.

You are right, though, it should be one loop.

Re: [Qemu-devel] [PATCH 11/12] iotests: add test 257 for bitmap-mode backups

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 12386 bytes --]

On 20.06.19 21:08, John Snow wrote:
> 
> 
> On 6/20/19 2:35 PM, Max Reitz wrote:
>> On 20.06.19 03:03, John Snow wrote:
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>>  tests/qemu-iotests/257     |  412 +++++++
>>>  tests/qemu-iotests/257.out | 2199 ++++++++++++++++++++++++++++++++++++
>>>  tests/qemu-iotests/group   |    1 +
>>>  3 files changed, 2612 insertions(+)
>>>  create mode 100755 tests/qemu-iotests/257
>>>  create mode 100644 tests/qemu-iotests/257.out
>>
>> This test is actually quite nicely written.
>>
> 
> Thanks!
> 
>> I like that I don’t have to read the reference output but can just grep
>> for “error”.
>>
> 
> Me too!! Actually, doing the math for what to expect and verifying the
> output by hand was becoming a major burden, so partially this test
> infrastructure was my attempt to procedurally verify that the results I
> was seeing were what made sense.
> 
> At the end of it, I felt it was nice to keep it in there.
> 
>> Only minor notes below.
>>
>>> diff --git a/tests/qemu-iotests/257 b/tests/qemu-iotests/257
>>> new file mode 100755
>>> index 0000000000..5f7f388504
>>> --- /dev/null
>>> +++ b/tests/qemu-iotests/257
>>
>> [...]
>>
>>> +class PatternGroup:
>>> +    """Grouping of Pattern objects. Initialize with an iterable of Patterns."""
>>> +    def __init__(self, patterns):
>>> +        self.patterns = patterns
>>> +
>>> +    def bits(self, granularity):
>>> +        """Calculate the unique bits dirtied by this pattern grouping"""
>>> +        res = set()
>>> +        for pattern in self.patterns:
>>> +            lower = math.floor(pattern.offset / granularity)
>>> +            upper = math.floor((pattern.offset + pattern.size - 1) / granularity)
>>> +            res = res | set(range(lower, upper + 1))
>>
>> Why you’d do floor((x - 1) / y) + 1 has confused me quite a while.
>> Until I realized that oh yeah, Python’s range() is a right-open
>> interval.  I don’t like Python’s range().
>>
> 
> It confuses me constantly, but it's really meant to be used for
> iterating over lengths.

I can see the use for range(x), but not for range(a, b).

(At least it’s not Rust, where [a..b] is [a, b), too – it’s enclosed in
square brackets, it should be closed, damnit.)

> This is somewhat of an abuse of it. I always
> test it out in a console first before using it, just in case.
> 
>> (Yes, you’re right, this is better to read than just ceil(x / y).
>> Because it reminds people like me that range() is weird.)
>>
>>> +        return res
>>> +
>>> +GROUPS = [
>>> +    PatternGroup([
>>> +        # Batch 0: 4 clusters
>>> +        mkpattern('0x49', 0x0000000),
>>> +        mkpattern('0x6c', 0x0100000),   # 1M
>>> +        mkpattern('0x6f', 0x2000000),   # 32M
>>> +        mkpattern('0x76', 0x3ff0000)]), # 64M - 64K
>>> +    PatternGroup([
>>> +        # Batch 1: 6 clusters (3 new)
>>> +        mkpattern('0x65', 0x0000000),   # Full overwrite
>>> +        mkpattern('0x77', 0x00f8000),   # Partial-left (1M-32K)
>>> +        mkpattern('0x72', 0x2008000),   # Partial-right (32M+32K)
>>> +        mkpattern('0x69', 0x3fe0000)]), # Adjacent-left (64M - 128K)
>>> +    PatternGroup([
>>> +        # Batch 2: 7 clusters (3 new)
>>> +        mkpattern('0x74', 0x0010000),   # Adjacent-right
>>> +        mkpattern('0x69', 0x00e8000),   # Partial-left  (1M-96K)
>>> +        mkpattern('0x6e', 0x2018000),   # Partial-right (32M+96K)
>>> +        mkpattern('0x67', 0x3fe0000,
>>> +                  2*GRANULARITY)]),     # Overwrite [(64M-128K)-64M)
>>> +    PatternGroup([
>>> +        # Batch 3: 8 clusters (5 new)
>>> +        # Carefully chosen such that nothing re-dirties the one cluster
>>> +        # that copies out successfully before failure in Group #1.
>>> +        mkpattern('0xaa', 0x0010000,
>>> +                  3*GRANULARITY),       # Overwrite and 2x Adjacent-right
>>> +        mkpattern('0xbb', 0x00d8000),   # Partial-left (1M-160K)
>>> +        mkpattern('0xcc', 0x2028000),   # Partial-right (32M+160K)
>>> +        mkpattern('0xdd', 0x3fc0000)]), # New; leaving a gap to the right
>>> +    ]
>>
>> I’d place this four spaces to the left.  But maybe placing it here is
>> proper Python indentation, while moving it to the left would be C
>> indentation.
>>
> 
> Either is fine, I think. In this case it affords us more room for the
> commentary on the bit ranges. Maybe it's not necessary, but at least
> personally I get woozy looking at the bit patterns.

Oh, no, no, I just meant the final closing ”]” of GROUPS.

(I did wonder about why you didn’t place every PatternGroups closing ])
on a separate line, too, but I decided not to say anything, because it
looks Python-y this way.  But you’re right, this gives a nice excuse why
to put more space between the patterns and the comments, which helps.)

>>> +class Drive:
>>> +    """Represents, vaguely, a drive attached to a VM.
>>> +    Includes format, graph, and device information."""
>>> +
>>> +    def __init__(self, path, vm=None):
>>> +        self.path = path
>>> +        self.vm = vm
>>> +        self.fmt = None
>>> +        self.size = None
>>> +        self.node = None
>>> +        self.device = None
>>> +
>>> +    @property
>>> +    def name(self):
>>> +        return self.node or self.device
>>> +
>>> +    def img_create(self, fmt, size):
>>> +        self.fmt = fmt
>>> +        self.size = size
>>> +        iotests.qemu_img_create('-f', self.fmt, self.path, str(self.size))
>>> +
>>> +    def create_target(self, name, fmt, size):
>>> +        basename = os.path.basename(self.path)
>>> +        file_node_name = "file_{}".format(basename)
>>> +        vm = self.vm
>>> +
>>> +        log(vm.command('blockdev-create', job_id='bdc-file-job',
>>> +                       options={
>>> +                           'driver': 'file',
>>> +                           'filename': self.path,
>>> +                           'size': 0,
>>> +                       }))
>>> +        vm.run_job('bdc-file-job')
>>> +        log(vm.command('blockdev-add', driver='file',
>>> +                       node_name=file_node_name, filename=self.path))
>>> +
>>> +        log(vm.command('blockdev-create', job_id='bdc-fmt-job',
>>> +                       options={
>>> +                           'driver': fmt,
>>> +                           'file': file_node_name,
>>> +                           'size': size,
>>> +                       }))
>>> +        vm.run_job('bdc-fmt-job')
>>> +        log(vm.command('blockdev-add', driver=fmt,
>>> +                       node_name=name,
>>> +                       file=file_node_name))
>>> +        self.fmt = fmt
>>> +        self.size = size
>>> +        self.node = name
>>
>> It’s cool that you use blockdev-create here, but would it not have been
>> easier to just use self.img_create() + blockdev-add?
>>
>> I mean, there’s no point in changing it now, I’m just wondering.
>>
> 
> Mostly just because I already wrote this for the last test, and we
> already test incremental backups the other way. I figured this would
> just be nice for code coverage purposes, and also because using the
> blockdev interfaces exclusively does tend to reveal little gotchas
> everywhere.
> 
> I also kind of want to refactor a lot of my tests and share some of the
> common code. I was tinkering with the idea of adding some common objects
> to iotests, like "Drive" "Bitmap" and "Backup".
> 
> That's why there's a kind of superfluous "Drive" object here.
> 
>>> +
>>> +def query_bitmaps(vm):
>>> +    res = vm.qmp("query-block")
>>> +    return {"bitmaps": {device['device'] or device['qdev']:
>>> +                        device.get('dirty-bitmaps', []) for
>>> +                        device in res['return']}}
>>
>> Python’s just not for me if I find this syntax unintuitive and
>> confusing, hu?
>>
>> [...]
>>
> 
> Sorry. It's a very functional-esque way of processing iterables.
I’m not blaming the basic idea, I’m blaming the syntax.  In fact, I’m
blaming exactly that it isn’t literally functional because there are no
functions here (but .get() and []).  I would like to have functions
because function names would tell me what’s going on.

I can never remember what {:} means (why do they use such nice words
everywhere else, like “or”, “for”, or “in”, and then they do that?).
And I find it weird that postfix-for can introduce variables after they
are used.  That may be the way I would read it aloud, but that’s
definitely not the way I *think*.

> I've been doing a lot of FP stuff lately and that skews what I find
> readable...
> 
> It's a dict comprehension of the form:
> 
> {key: value for atom in iterable}

Ah.  Thanks.  I thought it was some filter where it would only return
elements where 'device' or 'qdev' is set.  So that seemed completely
stupid to me, to have the iteration in the end, but the filter in the
beginning.

> Key here is either the device or qdev name,
> the value is the 'dirty-bitmaps' field of the atom, and
> res['return'] is the iterable.
> 
> Effectively it turns a list of devices into a dict keyed by the device
> name, and the only field (if any) was its dirty-bitmap object.

Why can’t they write it like normal human beings.  Like

Hash[res['return'].map { |device| [device['device'] || device['qdev'],
                                   device['dirty-bitmaps'] or {}]}]

By the way, this is the reason why you’ll always see me using map() and
filter() and then someone saying that there is a more Python-y way of
doing themes, namely postfix-for.  I hate postfix-for.  I also hate
postfix-if-else, by the way, but I felt like I didn’t want to go there.

> However, in explaining this I do notice I have a bug -- the default
> value for the bitmap object ought to be {}, not [].
> 
> This is code that should become common testing code too, as I've
> re-written it a few times now...
> 
>>> +def bitmap_comparison(bitmap, groups=None, want=0):
>>> +    """
>>> +    Print a nice human-readable message checking that this bitmap has as
>>> +    many bits set as we expect it to.
>>> +    """
>>> +    log("= Checking Bitmap {:s} =".format(bitmap.get('name', '(anonymous)')))
>>> +
>>> +    if groups:
>>> +        want = calculate_bits(groups)
>>> +    have = int(bitmap['count'] / bitmap['granularity'])
>>
>> Or just bitmap['count'] // bitmap['granularity']?
>>
>> [...]
>>
> 
> I forget that exists. If you prefer that, OK.

Well, it is shorter and more optimal!!!  (Saves two conversions to FP,
then an FP division, and then one conversion back to integer!!)

>>> +def test_bitmap_sync(bsync_mode, failure=None):
>>
>> [...]
>>
>>> +        log('--- Preparing image & VM ---\n')
>>> +        drive0 = Drive(img_path, vm=vm)
>>> +        drive0.img_create(iotests.imgfmt, SIZE)
>>> +        vm.add_device('virtio-scsi-pci,id=scsi0')
>>
>> Judging from 238, this should be virtio-scsi-ccw on s390-ccw-virtio.
>>
>> (This is the reason I cannot give an R-b.)
>>
>> [...]
>>
> 
> Oh, good catch. Alright.
> 
>>> +        vm.run_job(job, auto_dismiss=True, auto_finalize=False,
>>> +                   pre_finalize=_callback,
>>> +                   cancel=failure == 'simulated')
>>
>> I’d prefer “cancel=(failure == 'simulated')”.  (Or spaces around =).
>>
>> Also in other places elsewhere that are of the form x=y where y contains
>> spaces.
>>
>> [...]
>>
> 
> OK; I might need to make a pylintrc file to allow that style. Python is
> VERY aggressively tuned to omitting parentheses.

It seems to me more and more like Python is very aggressively tuned to
what I find difficult to read.

(You’re also still free to write “cancel = failure == 'simulated'”.  I
wouldn’t write that in C, but well.)

> (I do actually try to run pylint on my python patches now to make sure I
> am targeting SOME kind of style. I realize this is not standardized in
> this project.)

Sorry for becoming very grumpy here (can’t help myself), but why would I
run it when apparently Python has just bad opinions about what’s
readable and what isn’t.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 11/12] iotests: add test 257 for bitmap-mode backups

[John Snow]

On 6/20/19 3:48 PM, Max Reitz wrote:
> On 20.06.19 21:08, John Snow wrote:
>>
>>
>> On 6/20/19 2:35 PM, Max Reitz wrote:
>>> On 20.06.19 03:03, John Snow wrote:
>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>> ---
>>>>  tests/qemu-iotests/257     |  412 +++++++
>>>>  tests/qemu-iotests/257.out | 2199 ++++++++++++++++++++++++++++++++++++
>>>>  tests/qemu-iotests/group   |    1 +
>>>>  3 files changed, 2612 insertions(+)
>>>>  create mode 100755 tests/qemu-iotests/257
>>>>  create mode 100644 tests/qemu-iotests/257.out
>>>
>>> This test is actually quite nicely written.
>>>
>>
>> Thanks!
>>
>>> I like that I don’t have to read the reference output but can just grep
>>> for “error”.
>>>
>>
>> Me too!! Actually, doing the math for what to expect and verifying the
>> output by hand was becoming a major burden, so partially this test
>> infrastructure was my attempt to procedurally verify that the results I
>> was seeing were what made sense.
>>
>> At the end of it, I felt it was nice to keep it in there.
>>
>>> Only minor notes below.
>>>
>>>> diff --git a/tests/qemu-iotests/257 b/tests/qemu-iotests/257
>>>> new file mode 100755
>>>> index 0000000000..5f7f388504
>>>> --- /dev/null
>>>> +++ b/tests/qemu-iotests/257
>>>
>>> [...]
>>>
>>>> +class PatternGroup:
>>>> +    """Grouping of Pattern objects. Initialize with an iterable of Patterns."""
>>>> +    def __init__(self, patterns):
>>>> +        self.patterns = patterns
>>>> +
>>>> +    def bits(self, granularity):
>>>> +        """Calculate the unique bits dirtied by this pattern grouping"""
>>>> +        res = set()
>>>> +        for pattern in self.patterns:
>>>> +            lower = math.floor(pattern.offset / granularity)
>>>> +            upper = math.floor((pattern.offset + pattern.size - 1) / granularity)
>>>> +            res = res | set(range(lower, upper + 1))
>>>
>>> Why you’d do floor((x - 1) / y) + 1 has confused me quite a while.
>>> Until I realized that oh yeah, Python’s range() is a right-open
>>> interval.  I don’t like Python’s range().
>>>
>>
>> It confuses me constantly, but it's really meant to be used for
>> iterating over lengths.
> 
> I can see the use for range(x), but not for range(a, b).
> 
> (At least it’s not Rust, where [a..b] is [a, b), too – it’s enclosed in
> square brackets, it should be closed, damnit.)
> 
>> This is somewhat of an abuse of it. I always
>> test it out in a console first before using it, just in case.
>>
>>> (Yes, you’re right, this is better to read than just ceil(x / y).
>>> Because it reminds people like me that range() is weird.)
>>>
>>>> +        return res
>>>> +
>>>> +GROUPS = [
>>>> +    PatternGroup([
>>>> +        # Batch 0: 4 clusters
>>>> +        mkpattern('0x49', 0x0000000),
>>>> +        mkpattern('0x6c', 0x0100000),   # 1M
>>>> +        mkpattern('0x6f', 0x2000000),   # 32M
>>>> +        mkpattern('0x76', 0x3ff0000)]), # 64M - 64K
>>>> +    PatternGroup([
>>>> +        # Batch 1: 6 clusters (3 new)
>>>> +        mkpattern('0x65', 0x0000000),   # Full overwrite
>>>> +        mkpattern('0x77', 0x00f8000),   # Partial-left (1M-32K)
>>>> +        mkpattern('0x72', 0x2008000),   # Partial-right (32M+32K)
>>>> +        mkpattern('0x69', 0x3fe0000)]), # Adjacent-left (64M - 128K)
>>>> +    PatternGroup([
>>>> +        # Batch 2: 7 clusters (3 new)
>>>> +        mkpattern('0x74', 0x0010000),   # Adjacent-right
>>>> +        mkpattern('0x69', 0x00e8000),   # Partial-left  (1M-96K)
>>>> +        mkpattern('0x6e', 0x2018000),   # Partial-right (32M+96K)
>>>> +        mkpattern('0x67', 0x3fe0000,
>>>> +                  2*GRANULARITY)]),     # Overwrite [(64M-128K)-64M)
>>>> +    PatternGroup([
>>>> +        # Batch 3: 8 clusters (5 new)
>>>> +        # Carefully chosen such that nothing re-dirties the one cluster
>>>> +        # that copies out successfully before failure in Group #1.
>>>> +        mkpattern('0xaa', 0x0010000,
>>>> +                  3*GRANULARITY),       # Overwrite and 2x Adjacent-right
>>>> +        mkpattern('0xbb', 0x00d8000),   # Partial-left (1M-160K)
>>>> +        mkpattern('0xcc', 0x2028000),   # Partial-right (32M+160K)
>>>> +        mkpattern('0xdd', 0x3fc0000)]), # New; leaving a gap to the right
>>>> +    ]
>>>
>>> I’d place this four spaces to the left.  But maybe placing it here is
>>> proper Python indentation, while moving it to the left would be C
>>> indentation.
>>>
>>
>> Either is fine, I think. In this case it affords us more room for the
>> commentary on the bit ranges. Maybe it's not necessary, but at least
>> personally I get woozy looking at the bit patterns.
> 
> Oh, no, no, I just meant the final closing ”]” of GROUPS.
> 
> (I did wonder about why you didn’t place every PatternGroups closing ])
> on a separate line, too, but I decided not to say anything, because it
> looks Python-y this way.  But you’re right, this gives a nice excuse why
> to put more space between the patterns and the comments, which helps.)
> 
>>>> +class Drive:
>>>> +    """Represents, vaguely, a drive attached to a VM.
>>>> +    Includes format, graph, and device information."""
>>>> +
>>>> +    def __init__(self, path, vm=None):
>>>> +        self.path = path
>>>> +        self.vm = vm
>>>> +        self.fmt = None
>>>> +        self.size = None
>>>> +        self.node = None
>>>> +        self.device = None
>>>> +
>>>> +    @property
>>>> +    def name(self):
>>>> +        return self.node or self.device
>>>> +
>>>> +    def img_create(self, fmt, size):
>>>> +        self.fmt = fmt
>>>> +        self.size = size
>>>> +        iotests.qemu_img_create('-f', self.fmt, self.path, str(self.size))
>>>> +
>>>> +    def create_target(self, name, fmt, size):
>>>> +        basename = os.path.basename(self.path)
>>>> +        file_node_name = "file_{}".format(basename)
>>>> +        vm = self.vm
>>>> +
>>>> +        log(vm.command('blockdev-create', job_id='bdc-file-job',
>>>> +                       options={
>>>> +                           'driver': 'file',
>>>> +                           'filename': self.path,
>>>> +                           'size': 0,
>>>> +                       }))
>>>> +        vm.run_job('bdc-file-job')
>>>> +        log(vm.command('blockdev-add', driver='file',
>>>> +                       node_name=file_node_name, filename=self.path))
>>>> +
>>>> +        log(vm.command('blockdev-create', job_id='bdc-fmt-job',
>>>> +                       options={
>>>> +                           'driver': fmt,
>>>> +                           'file': file_node_name,
>>>> +                           'size': size,
>>>> +                       }))
>>>> +        vm.run_job('bdc-fmt-job')
>>>> +        log(vm.command('blockdev-add', driver=fmt,
>>>> +                       node_name=name,
>>>> +                       file=file_node_name))
>>>> +        self.fmt = fmt
>>>> +        self.size = size
>>>> +        self.node = name
>>>
>>> It’s cool that you use blockdev-create here, but would it not have been
>>> easier to just use self.img_create() + blockdev-add?
>>>
>>> I mean, there’s no point in changing it now, I’m just wondering.
>>>
>>
>> Mostly just because I already wrote this for the last test, and we
>> already test incremental backups the other way. I figured this would
>> just be nice for code coverage purposes, and also because using the
>> blockdev interfaces exclusively does tend to reveal little gotchas
>> everywhere.
>>
>> I also kind of want to refactor a lot of my tests and share some of the
>> common code. I was tinkering with the idea of adding some common objects
>> to iotests, like "Drive" "Bitmap" and "Backup".
>>
>> That's why there's a kind of superfluous "Drive" object here.
>>
>>>> +
>>>> +def query_bitmaps(vm):
>>>> +    res = vm.qmp("query-block")
>>>> +    return {"bitmaps": {device['device'] or device['qdev']:
>>>> +                        device.get('dirty-bitmaps', []) for
>>>> +                        device in res['return']}}
>>>
>>> Python’s just not for me if I find this syntax unintuitive and
>>> confusing, hu?
>>>
>>> [...]
>>>
>>
>> Sorry. It's a very functional-esque way of processing iterables.
> I’m not blaming the basic idea, I’m blaming the syntax.  In fact, I’m
> blaming exactly that it isn’t literally functional because there are no
> functions here (but .get() and []).  I would like to have functions
> because function names would tell me what’s going on.
> 
> I can never remember what {:} means (why do they use such nice words
> everywhere else, like “or”, “for”, or “in”, and then they do that?).
> And I find it weird that postfix-for can introduce variables after they
> are used.  That may be the way I would read it aloud, but that’s
> definitely not the way I *think*.
> 
>> I've been doing a lot of FP stuff lately and that skews what I find
>> readable...
>>
>> It's a dict comprehension of the form:
>>
>> {key: value for atom in iterable}
> 
> Ah.  Thanks.  I thought it was some filter where it would only return
> elements where 'device' or 'qdev' is set.  So that seemed completely
> stupid to me, to have the iteration in the end, but the filter in the
> beginning.
> 
>> Key here is either the device or qdev name,
>> the value is the 'dirty-bitmaps' field of the atom, and
>> res['return'] is the iterable.
>>
>> Effectively it turns a list of devices into a dict keyed by the device
>> name, and the only field (if any) was its dirty-bitmap object.
> 
> Why can’t they write it like normal human beings.  Like
> 
> Hash[res['return'].map { |device| [device['device'] || device['qdev'],
>                                    device['dirty-bitmaps'] or {}]}]
> 

🤠

> By the way, this is the reason why you’ll always see me using map() and
> filter() and then someone saying that there is a more Python-y way of
> doing themes, namely postfix-for.  I hate postfix-for.  I also hate
> postfix-if-else, by the way, but I felt like I didn’t want to go there.
> 

I actually really dislike the postfix-if-else too. I prefer C's ternary
there, but when in Rome, etc.

>> However, in explaining this I do notice I have a bug -- the default
>> value for the bitmap object ought to be {}, not [].
>>
>> This is code that should become common testing code too, as I've
>> re-written it a few times now...
>>
>>>> +def bitmap_comparison(bitmap, groups=None, want=0):
>>>> +    """
>>>> +    Print a nice human-readable message checking that this bitmap has as
>>>> +    many bits set as we expect it to.
>>>> +    """
>>>> +    log("= Checking Bitmap {:s} =".format(bitmap.get('name', '(anonymous)')))
>>>> +
>>>> +    if groups:
>>>> +        want = calculate_bits(groups)
>>>> +    have = int(bitmap['count'] / bitmap['granularity'])
>>>
>>> Or just bitmap['count'] // bitmap['granularity']?
>>>
>>> [...]
>>>
>>
>> I forget that exists. If you prefer that, OK.
> 
> Well, it is shorter and more optimal!!!  (Saves two conversions to FP,
> then an FP division, and then one conversion back to integer!!)
> 

ok!!!!!

>>>> +def test_bitmap_sync(bsync_mode, failure=None):
>>>
>>> [...]
>>>
>>>> +        log('--- Preparing image & VM ---\n')
>>>> +        drive0 = Drive(img_path, vm=vm)
>>>> +        drive0.img_create(iotests.imgfmt, SIZE)
>>>> +        vm.add_device('virtio-scsi-pci,id=scsi0')
>>>
>>> Judging from 238, this should be virtio-scsi-ccw on s390-ccw-virtio.
>>>
>>> (This is the reason I cannot give an R-b.)
>>>
>>> [...]
>>>
>>
>> Oh, good catch. Alright.
>>
>>>> +        vm.run_job(job, auto_dismiss=True, auto_finalize=False,
>>>> +                   pre_finalize=_callback,
>>>> +                   cancel=failure == 'simulated')
>>>
>>> I’d prefer “cancel=(failure == 'simulated')”.  (Or spaces around =).
>>>
>>> Also in other places elsewhere that are of the form x=y where y contains
>>> spaces.
>>>
>>> [...]
>>>
>>
>> OK; I might need to make a pylintrc file to allow that style. Python is
>> VERY aggressively tuned to omitting parentheses.
> 
> It seems to me more and more like Python is very aggressively tuned to
> what I find difficult to read.
> 
> (You’re also still free to write “cancel = failure == 'simulated'”.  I
> wouldn’t write that in C, but well.)
> 

It turns out that your suggestion is fine. I do agree, though: I like my
unnecessary parentheses a lot.

Python wants:

assert a == b

And will get mad about:

assert (a == b)

And that's just so hard to deal with when working with C-brain.

>> (I do actually try to run pylint on my python patches now to make sure I
>> am targeting SOME kind of style. I realize this is not standardized in
>> this project.)
> 
> Sorry for becoming very grumpy here (can’t help myself), but why would I
> run it when apparently Python has just bad opinions about what’s
> readable and what isn’t.
> 
> Max
> 

[Qemu-devel] [PATCH 12/12] block/backup: loosen restriction on readonly bitmaps

[John Snow]

With the "never" sync policy, we actually can utilize readonly bitmaps
now. Loosen the check at the QMP level, and tighten it based on
provided arguments down at the job creation level instead.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 block/backup.c | 6 ++++++
 blockdev.c     | 4 ++--
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index beb2078696..ead9a049e2 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -614,6 +614,12 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
             return NULL;
         }
 
+        /* If we need to write to this bitmap, check that we can: */
+        if (bitmap_mode != BITMAP_SYNC_MODE_NEVER &&
+            bdrv_dirty_bitmap_check(sync_bitmap, BDRV_BITMAP_DEFAULT, errp)) {
+            return NULL;
+        }
+
         /* Create a new bitmap, and freeze/disable this one. */
         if (bdrv_dirty_bitmap_create_successor(bs, sync_bitmap, errp) < 0) {
             return NULL;
diff --git a/blockdev.c b/blockdev.c
index 7abbd6bbf2..173a6b85c6 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3560,7 +3560,7 @@ static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn,
             error_setg(errp, "Bitmap '%s' could not be found", backup->bitmap);
             goto unref;
         }
-        if (bdrv_dirty_bitmap_check(bmap, BDRV_BITMAP_DEFAULT, errp)) {
+        if (bdrv_dirty_bitmap_check(bmap, BDRV_BITMAP_ALLOW_RO, errp)) {
             goto unref;
         }
     }
@@ -3667,7 +3667,7 @@ BlockJob *do_blockdev_backup(BlockdevBackup *backup, JobTxn *txn,
             error_setg(errp, "Bitmap '%s' could not be found", backup->bitmap);
             goto out;
         }
-        if (bdrv_dirty_bitmap_check(bmap, BDRV_BITMAP_DEFAULT, errp)) {
+        if (bdrv_dirty_bitmap_check(bmap, BDRV_BITMAP_ALLOW_RO, errp)) {
             goto out;
         }
     }
-- 
2.21.0

Re: [Qemu-devel] [PATCH 12/12] block/backup: loosen restriction on readonly bitmaps

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 457 bytes --]

On 20.06.19 03:03, John Snow wrote:
> With the "never" sync policy, we actually can utilize readonly bitmaps
> now. Loosen the check at the QMP level, and tighten it based on
> provided arguments down at the job creation level instead.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  block/backup.c | 6 ++++++
>  blockdev.c     | 4 ++--
>  2 files changed, 8 insertions(+), 2 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]