[Qemu-devel] [PATCH 0/2] transaction support for bitmap merge

[Qemu-devel] [PATCH 0/2] transaction support for bitmap merge

[Vladimir Sementsov-Ogievskiy]

This is a last brick, necessary to play with nbd bitmap export in
conjunction with image fleecing.

Vladimir Sementsov-Ogievskiy (2):
  drity-bitmap: refactor merge: separte can_merge
  qapi: add transaction support for x-block-dirty-bitmap-merge

 qapi/transaction.json        |  2 ++
 include/block/dirty-bitmap.h |  3 +++
 include/qemu/hbitmap.h       |  8 ++++++
 block/dirty-bitmap.c         | 32 ++++++++++++++++++----
 blockdev.c                   | 63 ++++++++++++++++++++++++++++++++++++--------
 util/hbitmap.c               |  6 ++++-
 6 files changed, 97 insertions(+), 17 deletions(-)

-- 
2.11.1

[Qemu-devel] [PATCH 1/2] drity-bitmap: refactor merge: separte can_merge

[Vladimir Sementsov-Ogievskiy]

Separate can_merge, to reuse it for transaction support for merge
command.

Also, switch some asserts to errors.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 include/block/dirty-bitmap.h |  3 +++
 include/qemu/hbitmap.h       |  8 ++++++++
 block/dirty-bitmap.c         | 32 +++++++++++++++++++++++++++-----
 blockdev.c                   | 10 ----------
 util/hbitmap.c               |  6 +++++-
 5 files changed, 43 insertions(+), 16 deletions(-)

diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 288dc6adb6..412a333c02 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -71,6 +71,9 @@ void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap,
 void bdrv_dirty_bitmap_set_qmp_locked(BdrvDirtyBitmap *bitmap, bool qmp_locked);
 void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
                              Error **errp);
+bool bdrv_can_merge_dirty_bitmap(BdrvDirtyBitmap *dst,
+                                 const BdrvDirtyBitmap *src,
+                                 Error **errp);
 
 /* Functions that require manual locking.  */
 void bdrv_dirty_bitmap_lock(BdrvDirtyBitmap *bitmap);
diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
index ddca52c48e..d08bc20ea3 100644
--- a/include/qemu/hbitmap.h
+++ b/include/qemu/hbitmap.h
@@ -85,6 +85,14 @@ void hbitmap_truncate(HBitmap *hb, uint64_t size);
 bool hbitmap_merge(HBitmap *a, const HBitmap *b);
 
 /**
+ * hbitmap_can_merge:
+ *
+ * Returns same value as hbitmap_merge, but do not do actual merge.
+ *
+ */
+bool hbitmap_can_merge(HBitmap *a, const HBitmap *b);
+
+/**
  * hbitmap_empty:
  * @hb: HBitmap to operate on.
  *
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index db1782ec1f..1137224aaa 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -784,6 +784,30 @@ int64_t bdrv_dirty_bitmap_next_zero(BdrvDirtyBitmap *bitmap, uint64_t offset)
     return hbitmap_next_zero(bitmap->bitmap, offset);
 }
 
+bool bdrv_can_merge_dirty_bitmap(BdrvDirtyBitmap *dst,
+                                 const BdrvDirtyBitmap *src,
+                                 Error **errp)
+{
+    if (bdrv_dirty_bitmap_frozen(dst)) {
+        error_setg(errp, "Bitmap '%s' is frozen and cannot be modified",
+                   dst->name);
+        return false;
+    }
+
+    if (bdrv_dirty_bitmap_readonly(dst)) {
+        error_setg(errp, "Bitmap '%s' is readonly and cannot be modified",
+                   dst->name);
+        return false;
+    }
+
+    if (!hbitmap_can_merge(dst->bitmap, src->bitmap)) {
+        error_setg(errp, "Bitmaps are incompatible and can't be merged");
+        return false;
+    }
+
+    return true;
+}
+
 void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
                              Error **errp)
 {
@@ -792,11 +816,9 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
 
     qemu_mutex_lock(dest->mutex);
 
-    assert(bdrv_dirty_bitmap_enabled(dest));
-    assert(!bdrv_dirty_bitmap_readonly(dest));
-
-    if (!hbitmap_merge(dest->bitmap, src->bitmap)) {
-        error_setg(errp, "Bitmaps are incompatible and can't be merged");
+    if (bdrv_can_merge_dirty_bitmap(dest, src, errp)) {
+        bool ret = hbitmap_merge(dest->bitmap, src->bitmap);
+        assert(ret);
     }
 
     qemu_mutex_unlock(dest->mutex);
diff --git a/blockdev.c b/blockdev.c
index 58d7570932..63c4d33124 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2955,16 +2955,6 @@ void qmp_x_block_dirty_bitmap_merge(const char *node, const char *dst_name,
         return;
     }
 
-    if (bdrv_dirty_bitmap_frozen(dst)) {
-        error_setg(errp, "Bitmap '%s' is frozen and cannot be modified",
-                   dst_name);
-        return;
-    } else if (bdrv_dirty_bitmap_readonly(dst)) {
-        error_setg(errp, "Bitmap '%s' is readonly and cannot be modified",
-                   dst_name);
-        return;
-    }
-
     src = bdrv_find_dirty_bitmap(bs, src_name);
     if (!src) {
         error_setg(errp, "Dirty bitmap '%s' not found", src_name);
diff --git a/util/hbitmap.c b/util/hbitmap.c
index bcd304041a..b56377b043 100644
--- a/util/hbitmap.c
+++ b/util/hbitmap.c
@@ -723,6 +723,10 @@ void hbitmap_truncate(HBitmap *hb, uint64_t size)
     }
 }
 
+bool hbitmap_can_merge(HBitmap *a, const HBitmap *b)
+{
+    return (a->size == b->size) && (a->granularity == b->granularity);
+}
 
 /**
  * Given HBitmaps A and B, let A := A (BITOR) B.
@@ -736,7 +740,7 @@ bool hbitmap_merge(HBitmap *a, const HBitmap *b)
     int i;
     uint64_t j;
 
-    if ((a->size != b->size) || (a->granularity != b->granularity)) {
+    if (!hbitmap_can_merge(a, b)) {
         return false;
     }
 
-- 
2.11.1

Re: [Qemu-devel] [PATCH 1/2] drity-bitmap: refactor merge: separte can_merge

[Eric Blake]

On 07/03/2018 05:53 AM, Vladimir Sementsov-Ogievskiy wrote:

In the subject: s/drity/dirty/, s/separte/separate/

> Separate can_merge, to reuse it for transaction support for merge
> command.
> 
> Also, switch some asserts to errors.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 1/2] drity-bitmap: refactor merge: separte can_merge

[John Snow]

On 07/03/2018 06:53 AM, Vladimir Sementsov-Ogievskiy wrote:
> Separate can_merge, to reuse it for transaction support for merge
> command.
> 
> Also, switch some asserts to errors.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  include/block/dirty-bitmap.h |  3 +++
>  include/qemu/hbitmap.h       |  8 ++++++++
>  block/dirty-bitmap.c         | 32 +++++++++++++++++++++++++++-----
>  blockdev.c                   | 10 ----------
>  util/hbitmap.c               |  6 +++++-
>  5 files changed, 43 insertions(+), 16 deletions(-)
> 
> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
> index 288dc6adb6..412a333c02 100644
> --- a/include/block/dirty-bitmap.h
> +++ b/include/block/dirty-bitmap.h
> @@ -71,6 +71,9 @@ void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap,
>  void bdrv_dirty_bitmap_set_qmp_locked(BdrvDirtyBitmap *bitmap, bool qmp_locked);
>  void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
>                               Error **errp);
> +bool bdrv_can_merge_dirty_bitmap(BdrvDirtyBitmap *dst,
> +                                 const BdrvDirtyBitmap *src,
> +                                 Error **errp);
>  
>  /* Functions that require manual locking.  */
>  void bdrv_dirty_bitmap_lock(BdrvDirtyBitmap *bitmap);
> diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
> index ddca52c48e..d08bc20ea3 100644
> --- a/include/qemu/hbitmap.h
> +++ b/include/qemu/hbitmap.h
> @@ -85,6 +85,14 @@ void hbitmap_truncate(HBitmap *hb, uint64_t size);
>  bool hbitmap_merge(HBitmap *a, const HBitmap *b);
>  
>  /**
> + * hbitmap_can_merge:
> + *
> + * Returns same value as hbitmap_merge, but do not do actual merge.
> + *
> + */
> +bool hbitmap_can_merge(HBitmap *a, const HBitmap *b);
> +
> +/**
>   * hbitmap_empty:
>   * @hb: HBitmap to operate on.
>   *
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index db1782ec1f..1137224aaa 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -784,6 +784,30 @@ int64_t bdrv_dirty_bitmap_next_zero(BdrvDirtyBitmap *bitmap, uint64_t offset)
>      return hbitmap_next_zero(bitmap->bitmap, offset);
>  }
>  
> +bool bdrv_can_merge_dirty_bitmap(BdrvDirtyBitmap *dst,
> +                                 const BdrvDirtyBitmap *src,
> +                                 Error **errp)
> +{
> +    if (bdrv_dirty_bitmap_frozen(dst)) {
> +        error_setg(errp, "Bitmap '%s' is frozen and cannot be modified",
> +                   dst->name);
> +        return false;
> +    }
> +
> +    if (bdrv_dirty_bitmap_readonly(dst)) {
> +        error_setg(errp, "Bitmap '%s' is readonly and cannot be modified",
> +                   dst->name);
> +        return false;
> +    }
> +
> +    if (!hbitmap_can_merge(dst->bitmap, src->bitmap)) {
> +        error_setg(errp, "Bitmaps are incompatible and can't be merged");
> +        return false;
> +    }
> +
> +    return true;
> +}
> +
>  void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
>                               Error **errp)
>  {
> @@ -792,11 +816,9 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
>  
>      qemu_mutex_lock(dest->mutex);
>  
> -    assert(bdrv_dirty_bitmap_enabled(dest));

Slight behavior change by removing this, but it makes sense.

> -    assert(!bdrv_dirty_bitmap_readonly(dest));
> -
> -    if (!hbitmap_merge(dest->bitmap, src->bitmap)) {
> -        error_setg(errp, "Bitmaps are incompatible and can't be merged");
> +    if (bdrv_can_merge_dirty_bitmap(dest, src, errp)) {
> +        bool ret = hbitmap_merge(dest->bitmap, src->bitmap);
> +        assert(ret);

Might as well just assert(hbitmap_merge(...));

>      }
>  
>      qemu_mutex_unlock(dest->mutex);
> diff --git a/blockdev.c b/blockdev.c
> index 58d7570932..63c4d33124 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2955,16 +2955,6 @@ void qmp_x_block_dirty_bitmap_merge(const char *node, const char *dst_name,
>          return;
>      }
>  
> -    if (bdrv_dirty_bitmap_frozen(dst)) {
> -        error_setg(errp, "Bitmap '%s' is frozen and cannot be modified",
> -                   dst_name);
> -        return;
> -    } else if (bdrv_dirty_bitmap_readonly(dst)) {
> -        error_setg(errp, "Bitmap '%s' is readonly and cannot be modified",
> -                   dst_name);
> -        return;
> -    }
> -
>      src = bdrv_find_dirty_bitmap(bs, src_name);
>      if (!src) {
>          error_setg(errp, "Dirty bitmap '%s' not found", src_name);
> diff --git a/util/hbitmap.c b/util/hbitmap.c
> index bcd304041a..b56377b043 100644
> --- a/util/hbitmap.c
> +++ b/util/hbitmap.c
> @@ -723,6 +723,10 @@ void hbitmap_truncate(HBitmap *hb, uint64_t size)
>      }
>  }
>  
> +bool hbitmap_can_merge(HBitmap *a, const HBitmap *b)
> +{
> +    return (a->size == b->size) && (a->granularity == b->granularity);
> +}
>  
>  /**
>   * Given HBitmaps A and B, let A := A (BITOR) B.
> @@ -736,7 +740,7 @@ bool hbitmap_merge(HBitmap *a, const HBitmap *b)
>      int i;
>      uint64_t j;
>  
> -    if ((a->size != b->size) || (a->granularity != b->granularity)) {
> +    if (!hbitmap_can_merge(a, b)) {
>          return false;
>      }
>  
> 

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [PATCH 1/2] drity-bitmap: refactor merge: separte can_merge

[Eric Blake]

On 07/05/2018 01:51 PM, John Snow wrote:

> 
>> -    assert(!bdrv_dirty_bitmap_readonly(dest));
>> -
>> -    if (!hbitmap_merge(dest->bitmap, src->bitmap)) {
>> -        error_setg(errp, "Bitmaps are incompatible and can't be merged");
>> +    if (bdrv_can_merge_dirty_bitmap(dest, src, errp)) {
>> +        bool ret = hbitmap_merge(dest->bitmap, src->bitmap);
>> +        assert(ret);
> 
> Might as well just assert(hbitmap_merge(...));

Except that side effects inside assert() are bad programming practice, 
even if in qemu assert()s are guaranteed to always be enabled by osdep.h.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 1/2] drity-bitmap: refactor merge: separte can_merge

[John Snow]

On 07/05/2018 02:55 PM, Eric Blake wrote:
> On 07/05/2018 01:51 PM, John Snow wrote:
> 
>>
>>> -    assert(!bdrv_dirty_bitmap_readonly(dest));
>>> -
>>> -    if (!hbitmap_merge(dest->bitmap, src->bitmap)) {
>>> -        error_setg(errp, "Bitmaps are incompatible and can't be
>>> merged");
>>> +    if (bdrv_can_merge_dirty_bitmap(dest, src, errp)) {
>>> +        bool ret = hbitmap_merge(dest->bitmap, src->bitmap);
>>> +        assert(ret);
>>
>> Might as well just assert(hbitmap_merge(...));
> 
> Except that side effects inside assert() are bad programming practice,
> even if in qemu assert()s are guaranteed to always be enabled by osdep.h.
> 

Oh, good point. NEVERMIND!

[Qemu-devel] [PATCH 2/2] qapi: add transaction support for x-block-dirty-bitmap-merge

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 qapi/transaction.json |  2 ++
 blockdev.c            | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 54 insertions(+), 1 deletion(-)

diff --git a/qapi/transaction.json b/qapi/transaction.json
index d7e4274550..f9da6ad47f 100644
--- a/qapi/transaction.json
+++ b/qapi/transaction.json
@@ -48,6 +48,7 @@
 # - @block-dirty-bitmap-clear: since 2.5
 # - @x-block-dirty-bitmap-enable: since 3.0
 # - @x-block-dirty-bitmap-disable: since 3.0
+# - @x-block-dirty-bitmap-merge: since 3.0
 # - @blockdev-backup: since 2.3
 # - @blockdev-snapshot: since 2.5
 # - @blockdev-snapshot-internal-sync: since 1.7
@@ -63,6 +64,7 @@
        'block-dirty-bitmap-clear': 'BlockDirtyBitmap',
        'x-block-dirty-bitmap-enable': 'BlockDirtyBitmap',
        'x-block-dirty-bitmap-disable': 'BlockDirtyBitmap',
+       'x-block-dirty-bitmap-merge': 'BlockDirtyBitmapMerge',
        'blockdev-backup': 'BlockdevBackup',
        'blockdev-snapshot': 'BlockdevSnapshot',
        'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal',
diff --git a/blockdev.c b/blockdev.c
index 63c4d33124..d0f2d1a4e9 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1946,6 +1946,13 @@ typedef struct BlockDirtyBitmapState {
     bool was_enabled;
 } BlockDirtyBitmapState;
 
+typedef struct BlockDirtyBitmapMergeState {
+    BlkActionState common;
+    BlockDriverState *bs;
+    BdrvDirtyBitmap *dst;
+    BdrvDirtyBitmap *src;
+} BlockDirtyBitmapMergeState;
+
 static void block_dirty_bitmap_add_prepare(BlkActionState *common,
                                            Error **errp)
 {
@@ -2112,6 +2119,45 @@ static void block_dirty_bitmap_disable_abort(BlkActionState *common)
     }
 }
 
+static void block_dirty_bitmap_merge_prepare(BlkActionState *common,
+                                             Error **errp)
+{
+    BlockDirtyBitmapMerge *action;
+    BlockDirtyBitmapMergeState *state = DO_UPCAST(BlockDirtyBitmapMergeState,
+                                                  common, common);
+
+    if (action_check_completion_mode(common, errp) < 0) {
+        return;
+    }
+
+    action = common->action->u.x_block_dirty_bitmap_merge.data;
+    state->dst = block_dirty_bitmap_lookup(action->node,
+                                           action->dst_name,
+                                           &state->bs,
+                                           errp);
+    if (!state->dst) {
+        return;
+    }
+
+    state->src = bdrv_find_dirty_bitmap(state->bs, action->src_name);
+    if (!state->src) {
+        return;
+    }
+
+    if (!bdrv_can_merge_dirty_bitmap(state->dst, state->src, errp)) {
+        return;
+    }
+}
+
+static void block_dirty_bitmap_merge_commit(BlkActionState *common)
+{
+    BlockDirtyBitmapMergeState *state = DO_UPCAST(BlockDirtyBitmapMergeState,
+                                                  common, common);
+
+    /* success is guaranteed by bdrv_can_merge_dirty_bitmap() */
+    bdrv_merge_dirty_bitmap(state->dst, state->src, &error_abort);
+}
+
 static void abort_prepare(BlkActionState *common, Error **errp)
 {
     error_setg(errp, "Transaction aborted using Abort action");
@@ -2182,7 +2228,12 @@ static const BlkActionOps actions[] = {
         .instance_size = sizeof(BlockDirtyBitmapState),
         .prepare = block_dirty_bitmap_disable_prepare,
         .abort = block_dirty_bitmap_disable_abort,
-     }
+    },
+    [TRANSACTION_ACTION_KIND_X_BLOCK_DIRTY_BITMAP_MERGE] = {
+        .instance_size = sizeof(BlockDirtyBitmapMergeState),
+        .prepare = block_dirty_bitmap_merge_prepare,
+        .commit = block_dirty_bitmap_merge_commit,
+    }
 };
 
 /**
-- 
2.11.1

Re: [Qemu-devel] [PATCH 2/2] qapi: add transaction support for x-block-dirty-bitmap-merge

[Eric Blake]

On 07/03/2018 05:53 AM, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   qapi/transaction.json |  2 ++
>   blockdev.c            | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>   2 files changed, 54 insertions(+), 1 deletion(-)

I'll let John handle this one, but it looks reasonable to me.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 2/2] qapi: add transaction support for x-block-dirty-bitmap-merge

[John Snow]

On 07/03/2018 06:53 AM, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  qapi/transaction.json |  2 ++
>  blockdev.c            | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 54 insertions(+), 1 deletion(-)
> 
> diff --git a/qapi/transaction.json b/qapi/transaction.json
> index d7e4274550..f9da6ad47f 100644
> --- a/qapi/transaction.json
> +++ b/qapi/transaction.json
> @@ -48,6 +48,7 @@
>  # - @block-dirty-bitmap-clear: since 2.5
>  # - @x-block-dirty-bitmap-enable: since 3.0
>  # - @x-block-dirty-bitmap-disable: since 3.0
> +# - @x-block-dirty-bitmap-merge: since 3.0
>  # - @blockdev-backup: since 2.3
>  # - @blockdev-snapshot: since 2.5
>  # - @blockdev-snapshot-internal-sync: since 1.7
> @@ -63,6 +64,7 @@
>         'block-dirty-bitmap-clear': 'BlockDirtyBitmap',
>         'x-block-dirty-bitmap-enable': 'BlockDirtyBitmap',
>         'x-block-dirty-bitmap-disable': 'BlockDirtyBitmap',
> +       'x-block-dirty-bitmap-merge': 'BlockDirtyBitmapMerge',
>         'blockdev-backup': 'BlockdevBackup',
>         'blockdev-snapshot': 'BlockdevSnapshot',
>         'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal',
> diff --git a/blockdev.c b/blockdev.c
> index 63c4d33124..d0f2d1a4e9 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1946,6 +1946,13 @@ typedef struct BlockDirtyBitmapState {
>      bool was_enabled;
>  } BlockDirtyBitmapState;
>  
> +typedef struct BlockDirtyBitmapMergeState {
> +    BlkActionState common;
> +    BlockDriverState *bs;
> +    BdrvDirtyBitmap *dst;
> +    BdrvDirtyBitmap *src;
> +} BlockDirtyBitmapMergeState;
> +
>  static void block_dirty_bitmap_add_prepare(BlkActionState *common,
>                                             Error **errp)
>  {
> @@ -2112,6 +2119,45 @@ static void block_dirty_bitmap_disable_abort(BlkActionState *common)
>      }
>  }
>  
> +static void block_dirty_bitmap_merge_prepare(BlkActionState *common,
> +                                             Error **errp)
> +{
> +    BlockDirtyBitmapMerge *action;
> +    BlockDirtyBitmapMergeState *state = DO_UPCAST(BlockDirtyBitmapMergeState,
> +                                                  common, common);
> +
> +    if (action_check_completion_mode(common, errp) < 0) {
> +        return;
> +    }
> +
> +    action = common->action->u.x_block_dirty_bitmap_merge.data;
> +    state->dst = block_dirty_bitmap_lookup(action->node,
> +                                           action->dst_name,
> +                                           &state->bs,
> +                                           errp);
> +    if (!state->dst) {
> +        return;
> +    }
> +
> +    state->src = bdrv_find_dirty_bitmap(state->bs, action->src_name);
> +    if (!state->src) {
> +        return;
> +    }
> +
> +    if (!bdrv_can_merge_dirty_bitmap(state->dst, state->src, errp)) {
> +        return;
> +    }
> +}
> +
> +static void block_dirty_bitmap_merge_commit(BlkActionState *common)
> +{
> +    BlockDirtyBitmapMergeState *state = DO_UPCAST(BlockDirtyBitmapMergeState,
> +                                                  common, common);
> +
> +    /* success is guaranteed by bdrv_can_merge_dirty_bitmap() */
> +    bdrv_merge_dirty_bitmap(state->dst, state->src, &error_abort);
> +}
> +
>  static void abort_prepare(BlkActionState *common, Error **errp)
>  {
>      error_setg(errp, "Transaction aborted using Abort action");
> @@ -2182,7 +2228,12 @@ static const BlkActionOps actions[] = {
>          .instance_size = sizeof(BlockDirtyBitmapState),
>          .prepare = block_dirty_bitmap_disable_prepare,
>          .abort = block_dirty_bitmap_disable_abort,
> -     }
> +    },
> +    [TRANSACTION_ACTION_KIND_X_BLOCK_DIRTY_BITMAP_MERGE] = {
> +        .instance_size = sizeof(BlockDirtyBitmapMergeState),
> +        .prepare = block_dirty_bitmap_merge_prepare,
> +        .commit = block_dirty_bitmap_merge_commit,
> +    }
>  };
>  
>  /**
> 

It's not easy to tell, and we've had this discussion a few times on-list
now to no clear conclusion, but the ordering of bitmap manipulation in
transactions matters.

I believe it matters because drive-backup and blockdev-backup both will
create a job (but not "start" it) during the .prepare phase, but this
job creation itself takes ownership of the bitmap (freezing it, for
instance.)

If you modify a bitmap in a transaction:

[bitmap-command-x
 action-job-y]

you may expect that "bitmap-command-x" will apply to "action-job-y", but
if we were to delay the bitmap modification to .commit(), the
"action-job-y" might take ownership of the bitmap FIRST, and then the
bitmap modification command would fail. I think that's unexpected behavior.

For the same reasons, I think "merge" ought to act early and unwind if
necessary -- at least as the transaction system exists now.

Further, all of the existing bitmap modification commands are
"undo-on-abort" semantics, including add, clear, enable and disable.

We discussed this once in 2015:
https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg01229.html

"
These semantics don't work in this example:

[block-dirty-bitmap-clear,
 drive-backup]

Since drive-backup starts the blockjob in .prepare() but
block-dirty-bitmap-clear only clears the bitmap in .commit() the order
is wrong.

.prepare() has to do something non-destructive, like stashing away the
HBitmap and replacing it with an empty one.  Then .commit() can discard
the old bitmap while .abort() can move the old bitmap back to undo the
operation.
"

Although we no longer START the blockjob in .prepare(), we're still
creating it -- and that creation does freeze the bitmap, so we are still
beholden to this ordering.

...unless, perhaps, we modify the way in which other job actions consume
and utilize bitmaps. They ought not "use" them until they actually
"start" but this might leave us open to failures on .start unless we're
particularly very careful.


I think for now, the best policy is:

- Assume that the actions of transactions are strictly ordered
- Any action that can effect a subsequent action must be visible by
subsequent actions
- Since blockdev/drive-backup use bitmaps in .prepare(), all bitmap
modification commands must take effect in .prepare() as well.


Thoughts?

Re: [Qemu-devel] [PATCH 2/2] qapi: add transaction support for x-block-dirty-bitmap-merge

[Vladimir Sementsov-Ogievskiy]

05.07.2018 23:40, John Snow wrote:
>
> On 07/03/2018 06:53 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   qapi/transaction.json |  2 ++
>>   blockdev.c            | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>>   2 files changed, 54 insertions(+), 1 deletion(-)
>>
>> diff --git a/qapi/transaction.json b/qapi/transaction.json
>> index d7e4274550..f9da6ad47f 100644
>> --- a/qapi/transaction.json
>> +++ b/qapi/transaction.json
>> @@ -48,6 +48,7 @@
>>   # - @block-dirty-bitmap-clear: since 2.5
>>   # - @x-block-dirty-bitmap-enable: since 3.0
>>   # - @x-block-dirty-bitmap-disable: since 3.0
>> +# - @x-block-dirty-bitmap-merge: since 3.0
>>   # - @blockdev-backup: since 2.3
>>   # - @blockdev-snapshot: since 2.5
>>   # - @blockdev-snapshot-internal-sync: since 1.7
>> @@ -63,6 +64,7 @@
>>          'block-dirty-bitmap-clear': 'BlockDirtyBitmap',
>>          'x-block-dirty-bitmap-enable': 'BlockDirtyBitmap',
>>          'x-block-dirty-bitmap-disable': 'BlockDirtyBitmap',
>> +       'x-block-dirty-bitmap-merge': 'BlockDirtyBitmapMerge',
>>          'blockdev-backup': 'BlockdevBackup',
>>          'blockdev-snapshot': 'BlockdevSnapshot',
>>          'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal',
>> diff --git a/blockdev.c b/blockdev.c
>> index 63c4d33124..d0f2d1a4e9 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -1946,6 +1946,13 @@ typedef struct BlockDirtyBitmapState {
>>       bool was_enabled;
>>   } BlockDirtyBitmapState;
>>   
>> +typedef struct BlockDirtyBitmapMergeState {
>> +    BlkActionState common;
>> +    BlockDriverState *bs;
>> +    BdrvDirtyBitmap *dst;
>> +    BdrvDirtyBitmap *src;
>> +} BlockDirtyBitmapMergeState;
>> +
>>   static void block_dirty_bitmap_add_prepare(BlkActionState *common,
>>                                              Error **errp)
>>   {
>> @@ -2112,6 +2119,45 @@ static void block_dirty_bitmap_disable_abort(BlkActionState *common)
>>       }
>>   }
>>   
>> +static void block_dirty_bitmap_merge_prepare(BlkActionState *common,
>> +                                             Error **errp)
>> +{
>> +    BlockDirtyBitmapMerge *action;
>> +    BlockDirtyBitmapMergeState *state = DO_UPCAST(BlockDirtyBitmapMergeState,
>> +                                                  common, common);
>> +
>> +    if (action_check_completion_mode(common, errp) < 0) {
>> +        return;
>> +    }
>> +
>> +    action = common->action->u.x_block_dirty_bitmap_merge.data;
>> +    state->dst = block_dirty_bitmap_lookup(action->node,
>> +                                           action->dst_name,
>> +                                           &state->bs,
>> +                                           errp);
>> +    if (!state->dst) {
>> +        return;
>> +    }
>> +
>> +    state->src = bdrv_find_dirty_bitmap(state->bs, action->src_name);
>> +    if (!state->src) {
>> +        return;
>> +    }
>> +
>> +    if (!bdrv_can_merge_dirty_bitmap(state->dst, state->src, errp)) {
>> +        return;
>> +    }
>> +}
>> +
>> +static void block_dirty_bitmap_merge_commit(BlkActionState *common)
>> +{
>> +    BlockDirtyBitmapMergeState *state = DO_UPCAST(BlockDirtyBitmapMergeState,
>> +                                                  common, common);
>> +
>> +    /* success is guaranteed by bdrv_can_merge_dirty_bitmap() */
>> +    bdrv_merge_dirty_bitmap(state->dst, state->src, &error_abort);
>> +}
>> +
>>   static void abort_prepare(BlkActionState *common, Error **errp)
>>   {
>>       error_setg(errp, "Transaction aborted using Abort action");
>> @@ -2182,7 +2228,12 @@ static const BlkActionOps actions[] = {
>>           .instance_size = sizeof(BlockDirtyBitmapState),
>>           .prepare = block_dirty_bitmap_disable_prepare,
>>           .abort = block_dirty_bitmap_disable_abort,
>> -     }
>> +    },
>> +    [TRANSACTION_ACTION_KIND_X_BLOCK_DIRTY_BITMAP_MERGE] = {
>> +        .instance_size = sizeof(BlockDirtyBitmapMergeState),
>> +        .prepare = block_dirty_bitmap_merge_prepare,
>> +        .commit = block_dirty_bitmap_merge_commit,
>> +    }
>>   };
>>   
>>   /**
>>
> It's not easy to tell, and we've had this discussion a few times on-list
> now to no clear conclusion, but the ordering of bitmap manipulation in
> transactions matters.
>
> I believe it matters because drive-backup and blockdev-backup both will
> create a job (but not "start" it) during the .prepare phase, but this
> job creation itself takes ownership of the bitmap (freezing it, for
> instance.)
>
> If you modify a bitmap in a transaction:
>
> [bitmap-command-x
>   action-job-y]
>
> you may expect that "bitmap-command-x" will apply to "action-job-y", but
> if we were to delay the bitmap modification to .commit(), the
> "action-job-y" might take ownership of the bitmap FIRST, and then the
> bitmap modification command would fail. I think that's unexpected behavior.
>
> For the same reasons, I think "merge" ought to act early and unwind if
> necessary -- at least as the transaction system exists now.
>
> Further, all of the existing bitmap modification commands are
> "undo-on-abort" semantics, including add, clear, enable and disable.
>
> We discussed this once in 2015:
> https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg01229.html
>
> "
> These semantics don't work in this example:
>
> [block-dirty-bitmap-clear,
>   drive-backup]
>
> Since drive-backup starts the blockjob in .prepare() but
> block-dirty-bitmap-clear only clears the bitmap in .commit() the order
> is wrong.
>
> .prepare() has to do something non-destructive, like stashing away the
> HBitmap and replacing it with an empty one.  Then .commit() can discard
> the old bitmap while .abort() can move the old bitmap back to undo the
> operation.
> "
>
> Although we no longer START the blockjob in .prepare(), we're still
> creating it -- and that creation does freeze the bitmap, so we are still
> beholden to this ordering.
>
> ...unless, perhaps, we modify the way in which other job actions consume
> and utilize bitmaps. They ought not "use" them until they actually
> "start" but this might leave us open to failures on .start unless we're
> particularly very careful.
>
>
> I think for now, the best policy is:
>
> - Assume that the actions of transactions are strictly ordered
> - Any action that can effect a subsequent action must be visible by
> subsequent actions
> - Since blockdev/drive-backup use bitmaps in .prepare(), all bitmap
> modification commands must take effect in .prepare() as well.
>
>
> Thoughts?

Ok, let's go this way for now, I'll rewrite it.

Last requirement looks a bit strange for me in transaction context. We 
should not assume that action is done before commit.
What is main idea of transaction action, do everything which will not 
fail in commit, or do everything which may be undone in prepare? Is 
there more formal way to manage dependencies between actions in one 
transaction?

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 2/2] qapi: add transaction support for x-block-dirty-bitmap-merge

[John Snow]

On 07/06/2018 06:12 AM, Vladimir Sementsov-Ogievskiy wrote:
> 
> Ok, let's go this way for now, I'll rewrite it.
> 
> Last requirement looks a bit strange for me in transaction context. We
> should not assume that action is done before commit.
> What is main idea of transaction action, do everything which will not
> fail in commit, or do everything which may be undone in prepare? Is
> there more formal way to manage dependencies between actions in one
> transaction?

Honestly, that's what I was wondering -- is there some formal model
people would be familiar with to help explain inter-dependencies between
transactional actions?

I don't know of one, presently, so I'm making it up as I go along.

I think this statement is intuitively true, though:

"For actions that are affected by other actions, the order of those two
actions matter, with the first action having an effect on the second."

We process actions in a linear order, first .prepare and then .commit --
so actions that come first in the list do have their callbacks executed
first. There's no way to express priority otherwise unless we give
certain actions a priority score, which we don't have right now.

Next, backup actions take ownership of the bitmap in .prepare()... so in
order for bitmap manipulation commands to affect these actions, they
also need to modify the bitmap in .prepare().

This may feel counter-intuitive to how transactions appear to be
designed to work, but on this subject Stefan said (May 2015):

> The ambiguity is whether "commit the changes" for .commit() means
> "changes take effect" or "discard stashed state, making undo
> impossible".
> 
> I think the "discard stashed state, making undo impossible"
> interpretation is good because .commit() is not allowed to fail.  That
> function should only do things that never fail.

I think this is probably the correct way to proceed, and we ought to
formalize this model. I think it's actually nearly impossible to do it
the other way around, too.

Let's say we've got two actions that both do error-checking in .prepare,
but perform their actual stateful changes in .commit, but are
interdependent.

{ a, b }

- a.prepare() checks state and confirms we will be able to proceed, but
changes no state.
- b.prepare() checks state and confirms it will be able to proceed, but
can't see what a is planning to do.
- a.commit() succeeds as expected.
- b.commit() does not fail as it is not allowed to, but the effects are
undetermined because we have not checked the state change from a.commit()

In general we can get around this, but the more actions we add, the
harder it is to do proper error checking while considering the
hypothetical state after some actions have committed.

I think the model where we take effect in .prepare() and undo it if
necessary in .abort() is actually easier to model in your head, because
error checking is simpler. That's probably the right model, then.



At the very least, I think that's correct for 3.0 and the immediate
future. If you disagree, please speak up because I've long been
particularly uncertain about this aspect.

--John

Re: [Qemu-devel] [PATCH 2/2] qapi: add transaction support for x-block-dirty-bitmap-merge

[Vladimir Sementsov-Ogievskiy]

06.07.2018 18:38, John Snow wrote:
>
> On 07/06/2018 06:12 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Ok, let's go this way for now, I'll rewrite it.
>>
>> Last requirement looks a bit strange for me in transaction context. We
>> should not assume that action is done before commit.
>> What is main idea of transaction action, do everything which will not
>> fail in commit, or do everything which may be undone in prepare? Is
>> there more formal way to manage dependencies between actions in one
>> transaction?
> Honestly, that's what I was wondering -- is there some formal model
> people would be familiar with to help explain inter-dependencies between
> transactional actions?
>
> I don't know of one, presently, so I'm making it up as I go along.
>
> I think this statement is intuitively true, though:
>
> "For actions that are affected by other actions, the order of those two
> actions matter, with the first action having an effect on the second."
>
> We process actions in a linear order, first .prepare and then .commit --
> so actions that come first in the list do have their callbacks executed
> first. There's no way to express priority otherwise unless we give
> certain actions a priority score, which we don't have right now.
>
> Next, backup actions take ownership of the bitmap in .prepare()... so in
> order for bitmap manipulation commands to affect these actions, they
> also need to modify the bitmap in .prepare().
>
> This may feel counter-intuitive to how transactions appear to be
> designed to work, but on this subject Stefan said (May 2015):
>
>> The ambiguity is whether "commit the changes" for .commit() means
>> "changes take effect" or "discard stashed state, making undo
>> impossible".
>>
>> I think the "discard stashed state, making undo impossible"
>> interpretation is good because .commit() is not allowed to fail.  That
>> function should only do things that never fail.
> I think this is probably the correct way to proceed, and we ought to
> formalize this model. I think it's actually nearly impossible to do it
> the other way around, too.
>
> Let's say we've got two actions that both do error-checking in .prepare,
> but perform their actual stateful changes in .commit, but are
> interdependent.
>
> { a, b }
>
> - a.prepare() checks state and confirms we will be able to proceed, but
> changes no state.
> - b.prepare() checks state and confirms it will be able to proceed, but
> can't see what a is planning to do.
> - a.commit() succeeds as expected.
> - b.commit() does not fail as it is not allowed to, but the effects are
> undetermined because we have not checked the state change from a.commit()
>
> In general we can get around this, but the more actions we add, the
> harder it is to do proper error checking while considering the
> hypothetical state after some actions have committed.
>
> I think the model where we take effect in .prepare() and undo it if
> necessary in .abort() is actually easier to model in your head, because
> error checking is simpler. That's probably the right model, then.
>
>
>
> At the very least, I think that's correct for 3.0 and the immediate
> future. If you disagree, please speak up because I've long been
> particularly uncertain about this aspect.
>
> --John

Ok.

But a question about some formalization mechanism or at least writing 
corresponding documentation still exists.

The simplest formalization may be to allow only one action with .commit 
in transaction. In this case, ordering is enough to satisfy 
dependencies. (all operations with dirty bitmaps are actually operations 
without .commit, freeing temporary HBitmap may be done in .clean not in 
.commit, as it don't change external state)

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 2/2] qapi: add transaction support for x-block-dirty-bitmap-merge

[Eric Blake]

On 07/06/2018 10:38 AM, John Snow wrote:

>> I think the "discard stashed state, making undo impossible"
>> interpretation is good because .commit() is not allowed to fail.  That
>> function should only do things that never fail.
> 
> I think this is probably the correct way to proceed, and we ought to
> formalize this model. I think it's actually nearly impossible to do it
> the other way around, too.
> 
> Let's say we've got two actions that both do error-checking in .prepare,
> but perform their actual stateful changes in .commit, but are
> interdependent.
> 
> { a, b }
> 
> - a.prepare() checks state and confirms we will be able to proceed, but
> changes no state.
> - b.prepare() checks state and confirms it will be able to proceed, but
> can't see what a is planning to do.
> - a.commit() succeeds as expected.
> - b.commit() does not fail as it is not allowed to, but the effects are
> undetermined because we have not checked the state change from a.commit()

I'm also trying to figure out if we guarantee that .abort is always 
called in reverse order of .prepare.  If we have { a, b, c }, where 
'a'.prepare makes one change, 'b'.prepare makes another that 'a' cannot 
directly undo, then 'c' fails, we're still okay if 'b'.abort can roll 
back to the state it had during prepare, so that 'a'.abort can then undo 
state cleanly.  And for consistency, I'd argue that .commit should have 
the same ordering as .abort (that is, every .prepare - .abort/.commit 
pair is a nested stack to track for unwinding purposes, and should be 
unwound in reverse order of setting aside resources up front).

> 
> In general we can get around this, but the more actions we add, the
> harder it is to do proper error checking while considering the
> hypothetical state after some actions have committed.
> 
> I think the model where we take effect in .prepare() and undo it if
> necessary in .abort() is actually easier to model in your head, because
> error checking is simpler. That's probably the right model, then.
> 

Yes, I'm also leaning heavily towards strong guarantees.  In addition to 
our examples of which operations depend on which bitmaps, we have this 
additional case:

{ blockdev-snapshot, blockdev-snapshot-internal-sync }

vs.

{ blockdev-snapshot-internal-sync, blockdev-snapshot }

Yes, I know that mixing internal and external snapshots is currently 
VERY dangerous 
(https://lists.gnu.org/archive/html/qemu-devel/2018-04/msg00865.html) if 
you aren't careful - but there are two plausible outcomes that can be 
chosen, where the order matters.  In the first instance, we want to 
create a new external snapshot (going from 'A' to 'A <- B') and THEN 
create an internal snapshot (image 'B' gets an internal snapshot); in 
the second instance we want to create an internal snapshot (image 'A' 
gets an internal snapshot) and THEN create an external snapshot (going 
from 'A' to 'A <- B').  Since the file where the internal snapshot is 
created is indeterminate unless we strictly process the transaction in 
the order that it was given, that argues that we want the strong order 
guarantees.

> 
> 
> At the very least, I think that's correct for 3.0 and the immediate
> future. If you disagree, please speak up because I've long been
> particularly uncertain about this aspect.
> 
> --John
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org