[Qemu-devel] [PATCH v3 0/3] qapi: block-dirty-bitmap-remove transaction action

[Qemu-devel] [PATCH v3 0/3] qapi: block-dirty-bitmap-remove transaction action

[John Snow]

Hi, this is a proposal based off of Vladimir's patchset:
[Qemu-devel] [PATCH 0/4] qapi: block-dirty-bitmap-remove transaction action

===
V3:
===

001/3:[----] [--] 'blockdev: reduce aio_context locked sections in bitmap add/remove'
002/3:[0024] [FC] 'qapi: implement block-dirty-bitmap-remove transaction action'
003/3:[----] [--] 'iotests: test bitmap moving inside 254'

- Changed "squelch_persistence" to "skip_store"
- Use Max's suggestion for return expr

===
V2:
===

It replaces patches two and three with a modified patch (now patch 2)
that foregoes the need for a hide()/unhide() bitmap API. I think it's
suitable as a smaller alternative, but I'm not sure if it covers all
of the use cases of the original series.

Patches 1 and 3 (formerly 4) included as-is.

John Snow (1):
  qapi: implement block-dirty-bitmap-remove transaction action

Vladimir Sementsov-Ogievskiy (2):
  blockdev: reduce aio_context locked sections in bitmap add/remove
  iotests: test bitmap moving inside 254

 block.c                        |   2 +-
 block/dirty-bitmap.c           |  15 +++--
 blockdev.c                     | 105 ++++++++++++++++++++++++++-------
 include/block/dirty-bitmap.h   |   2 +-
 migration/block-dirty-bitmap.c |   2 +-
 qapi/transaction.json          |   2 +
 tests/qemu-iotests/254         |  30 +++++++++-
 tests/qemu-iotests/254.out     |  82 +++++++++++++++++++++++++
 8 files changed, 206 insertions(+), 34 deletions(-)

-- 
2.21.0

[Qemu-devel] [PATCH v3 1/3] blockdev: reduce aio_context locked sections in bitmap add/remove

[John Snow]

From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Commit 0a6c86d024c52 returned these locks back to add/remove
functionality, to protect from intersection of persistent bitmap
related IO with other IO. But other bitmap-related functions called
here are unrelated to the problem, and there are no needs to keep these
calls inside critical sections.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
 blockdev.c | 30 +++++++++++++-----------------
 1 file changed, 13 insertions(+), 17 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 4d141e9a1f..01248252ca 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2811,7 +2811,6 @@ void qmp_block_dirty_bitmap_add(const char *node, const char *name,
 {
     BlockDriverState *bs;
     BdrvDirtyBitmap *bitmap;
-    AioContext *aio_context = NULL;
 
     if (!name || name[0] == '\0') {
         error_setg(errp, "Bitmap name cannot be empty");
@@ -2847,16 +2846,20 @@ void qmp_block_dirty_bitmap_add(const char *node, const char *name,
     }
 
     if (persistent) {
-        aio_context = bdrv_get_aio_context(bs);
+        AioContext *aio_context = bdrv_get_aio_context(bs);
+        bool ok;
+
         aio_context_acquire(aio_context);
-        if (!bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp)) {
-            goto out;
+        ok = bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp);
+        aio_context_release(aio_context);
+        if (!ok) {
+            return;
         }
     }
 
     bitmap = bdrv_create_dirty_bitmap(bs, granularity, name, errp);
     if (bitmap == NULL) {
-        goto out;
+        return;
     }
 
     if (disabled) {
@@ -2864,10 +2867,6 @@ void qmp_block_dirty_bitmap_add(const char *node, const char *name,
     }
 
     bdrv_dirty_bitmap_set_persistence(bitmap, persistent);
- out:
-    if (aio_context) {
-        aio_context_release(aio_context);
-    }
 }
 
 void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
@@ -2875,8 +2874,6 @@ void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
 {
     BlockDriverState *bs;
     BdrvDirtyBitmap *bitmap;
-    Error *local_err = NULL;
-    AioContext *aio_context = NULL;
 
     bitmap = block_dirty_bitmap_lookup(node, name, &bs, errp);
     if (!bitmap || !bs) {
@@ -2889,20 +2886,19 @@ void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
     }
 
     if (bdrv_dirty_bitmap_get_persistence(bitmap)) {
-        aio_context = bdrv_get_aio_context(bs);
+        AioContext *aio_context = bdrv_get_aio_context(bs);
+        Error *local_err = NULL;
+
         aio_context_acquire(aio_context);
         bdrv_remove_persistent_dirty_bitmap(bs, name, &local_err);
+        aio_context_release(aio_context);
         if (local_err != NULL) {
             error_propagate(errp, local_err);
-            goto out;
+            return;
         }
     }
 
     bdrv_release_dirty_bitmap(bs, bitmap);
- out:
-    if (aio_context) {
-        aio_context_release(aio_context);
-    }
 }
 
 /**
-- 
2.21.0

[Qemu-devel] [PATCH v3 2/3] qapi: implement block-dirty-bitmap-remove transaction action

[John Snow]

It is used to do transactional movement of the bitmap (which is
possible in conjunction with merge command). Transactional bitmap
movement is needed in scenarios with external snapshot, when we don't
want to leave copy of the bitmap in the base image.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
 block.c                        |  2 +-
 block/dirty-bitmap.c           | 15 +++----
 blockdev.c                     | 79 +++++++++++++++++++++++++++++++---
 include/block/dirty-bitmap.h   |  2 +-
 migration/block-dirty-bitmap.c |  2 +-
 qapi/transaction.json          |  2 +
 6 files changed, 85 insertions(+), 17 deletions(-)

diff --git a/block.c b/block.c
index c139540f2b..5195d4b910 100644
--- a/block.c
+++ b/block.c
@@ -5316,7 +5316,7 @@ static void coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs,
     for (bm = bdrv_dirty_bitmap_next(bs, NULL); bm;
          bm = bdrv_dirty_bitmap_next(bs, bm))
     {
-        bdrv_dirty_bitmap_set_migration(bm, false);
+        bdrv_dirty_bitmap_skip_store(bm, false);
     }
 
     ret = refresh_total_sectors(bs, bs->total_sectors);
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 95a9c2a5d8..a308e1f84b 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -48,10 +48,9 @@ struct BdrvDirtyBitmap {
     bool inconsistent;          /* bitmap is persistent, but inconsistent.
                                    It cannot be used at all in any way, except
                                    a QMP user can remove it. */
-    bool migration;             /* Bitmap is selected for migration, it should
-                                   not be stored on the next inactivation
-                                   (persistent flag doesn't matter until next
-                                   invalidation).*/
+    bool skip_store;            /* We are either migrating or deleting this
+                                 * bitmap; it should not be stored on the next
+                                 * inactivation. */
     QLIST_ENTRY(BdrvDirtyBitmap) list;
 };
 
@@ -757,16 +756,16 @@ void bdrv_dirty_bitmap_set_inconsistent(BdrvDirtyBitmap *bitmap)
 }
 
 /* Called with BQL taken. */
-void bdrv_dirty_bitmap_set_migration(BdrvDirtyBitmap *bitmap, bool migration)
+void bdrv_dirty_bitmap_skip_store(BdrvDirtyBitmap *bitmap, bool skip)
 {
     qemu_mutex_lock(bitmap->mutex);
-    bitmap->migration = migration;
+    bitmap->skip_store = skip;
     qemu_mutex_unlock(bitmap->mutex);
 }
 
 bool bdrv_dirty_bitmap_get_persistence(BdrvDirtyBitmap *bitmap)
 {
-    return bitmap->persistent && !bitmap->migration;
+    return bitmap->persistent && !bitmap->skip_store;
 }
 
 bool bdrv_dirty_bitmap_inconsistent(const BdrvDirtyBitmap *bitmap)
@@ -778,7 +777,7 @@ bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs)
 {
     BdrvDirtyBitmap *bm;
     QLIST_FOREACH(bm, &bs->dirty_bitmaps, list) {
-        if (bm->persistent && !bm->readonly && !bm->migration) {
+        if (bm->persistent && !bm->readonly && !bm->skip_store) {
             return true;
         }
     }
diff --git a/blockdev.c b/blockdev.c
index 01248252ca..800b3dcb42 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2134,6 +2134,51 @@ static void block_dirty_bitmap_merge_prepare(BlkActionState *common,
                                                 errp);
 }
 
+static BdrvDirtyBitmap *do_block_dirty_bitmap_remove(
+        const char *node, const char *name, bool release,
+        BlockDriverState **bitmap_bs, Error **errp);
+
+static void block_dirty_bitmap_remove_prepare(BlkActionState *common,
+                                              Error **errp)
+{
+    BlockDirtyBitmap *action;
+    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
+                                             common, common);
+
+    if (action_check_completion_mode(common, errp) < 0) {
+        return;
+    }
+
+    action = common->action->u.block_dirty_bitmap_remove.data;
+
+    state->bitmap = do_block_dirty_bitmap_remove(action->node, action->name,
+                                                 false, &state->bs, errp);
+    if (state->bitmap) {
+        bdrv_dirty_bitmap_skip_store(state->bitmap, true);
+        bdrv_dirty_bitmap_set_busy(state->bitmap, true);
+    }
+}
+
+static void block_dirty_bitmap_remove_abort(BlkActionState *common)
+{
+    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
+                                             common, common);
+
+    if (state->bitmap) {
+        bdrv_dirty_bitmap_skip_store(state->bitmap, false);
+        bdrv_dirty_bitmap_set_busy(state->bitmap, false);
+    }
+}
+
+static void block_dirty_bitmap_remove_commit(BlkActionState *common)
+{
+    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
+                                             common, common);
+
+    bdrv_dirty_bitmap_set_busy(state->bitmap, false);
+    bdrv_release_dirty_bitmap(state->bs, state->bitmap);
+}
+
 static void abort_prepare(BlkActionState *common, Error **errp)
 {
     error_setg(errp, "Transaction aborted using Abort action");
@@ -2211,6 +2256,12 @@ static const BlkActionOps actions[] = {
         .commit = block_dirty_bitmap_free_backup,
         .abort = block_dirty_bitmap_restore,
     },
+    [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_REMOVE] = {
+        .instance_size = sizeof(BlockDirtyBitmapState),
+        .prepare = block_dirty_bitmap_remove_prepare,
+        .commit = block_dirty_bitmap_remove_commit,
+        .abort = block_dirty_bitmap_remove_abort,
+    },
     /* Where are transactions for MIRROR, COMMIT and STREAM?
      * Although these blockjobs use transaction callbacks like the backup job,
      * these jobs do not necessarily adhere to transaction semantics.
@@ -2869,20 +2920,21 @@ void qmp_block_dirty_bitmap_add(const char *node, const char *name,
     bdrv_dirty_bitmap_set_persistence(bitmap, persistent);
 }
 
-void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
-                                   Error **errp)
+static BdrvDirtyBitmap *do_block_dirty_bitmap_remove(
+        const char *node, const char *name, bool release,
+        BlockDriverState **bitmap_bs, Error **errp)
 {
     BlockDriverState *bs;
     BdrvDirtyBitmap *bitmap;
 
     bitmap = block_dirty_bitmap_lookup(node, name, &bs, errp);
     if (!bitmap || !bs) {
-        return;
+        return NULL;
     }
 
     if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_BUSY | BDRV_BITMAP_RO,
                                 errp)) {
-        return;
+        return NULL;
     }
 
     if (bdrv_dirty_bitmap_get_persistence(bitmap)) {
@@ -2892,13 +2944,28 @@ void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
         aio_context_acquire(aio_context);
         bdrv_remove_persistent_dirty_bitmap(bs, name, &local_err);
         aio_context_release(aio_context);
+
         if (local_err != NULL) {
             error_propagate(errp, local_err);
-            return;
+            return NULL;
         }
     }
 
-    bdrv_release_dirty_bitmap(bs, bitmap);
+    if (release) {
+        bdrv_release_dirty_bitmap(bs, bitmap);
+    }
+
+    if (bitmap_bs) {
+        *bitmap_bs = bs;
+    }
+
+    return release ? NULL : bitmap;
+}
+
+void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
+                                   Error **errp)
+{
+    do_block_dirty_bitmap_remove(node, name, true, NULL, errp);
 }
 
 /**
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 62682eb865..a21d54a090 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -83,7 +83,7 @@ void bdrv_dirty_bitmap_set_inconsistent(BdrvDirtyBitmap *bitmap);
 void bdrv_dirty_bitmap_set_busy(BdrvDirtyBitmap *bitmap, bool busy);
 void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
                              HBitmap **backup, Error **errp);
-void bdrv_dirty_bitmap_set_migration(BdrvDirtyBitmap *bitmap, bool migration);
+void bdrv_dirty_bitmap_skip_store(BdrvDirtyBitmap *bitmap, bool skip);
 
 /* Functions that require manual locking.  */
 void bdrv_dirty_bitmap_lock(BdrvDirtyBitmap *bitmap);
diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index 4a896a09eb..d650ba4c23 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -326,7 +326,7 @@ static int init_dirty_bitmap_migration(void)
 
     /* unset migration flags here, to not roll back it */
     QSIMPLEQ_FOREACH(dbms, &dirty_bitmap_mig_state.dbms_list, entry) {
-        bdrv_dirty_bitmap_set_migration(dbms->bitmap, true);
+        bdrv_dirty_bitmap_skip_store(dbms->bitmap, true);
     }
 
     if (QSIMPLEQ_EMPTY(&dirty_bitmap_mig_state.dbms_list)) {
diff --git a/qapi/transaction.json b/qapi/transaction.json
index 95edb78227..da95b804aa 100644
--- a/qapi/transaction.json
+++ b/qapi/transaction.json
@@ -45,6 +45,7 @@
 #
 # - @abort: since 1.6
 # - @block-dirty-bitmap-add: since 2.5
+# - @block-dirty-bitmap-remove: since 4.1
 # - @block-dirty-bitmap-clear: since 2.5
 # - @block-dirty-bitmap-enable: since 4.0
 # - @block-dirty-bitmap-disable: since 4.0
@@ -61,6 +62,7 @@
   'data': {
        'abort': 'Abort',
        'block-dirty-bitmap-add': 'BlockDirtyBitmapAdd',
+       'block-dirty-bitmap-remove': 'BlockDirtyBitmap',
        'block-dirty-bitmap-clear': 'BlockDirtyBitmap',
        'block-dirty-bitmap-enable': 'BlockDirtyBitmap',
        'block-dirty-bitmap-disable': 'BlockDirtyBitmap',
-- 
2.21.0

[Qemu-devel] [PATCH v3 3/3] iotests: test bitmap moving inside 254

[John Snow]

From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Test persistent bitmap copying with and without removal of original
bitmap.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/254     | 30 +++++++++++++-
 tests/qemu-iotests/254.out | 82 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 110 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/254 b/tests/qemu-iotests/254
index 8edba91c5d..9a57bccc26 100755
--- a/tests/qemu-iotests/254
+++ b/tests/qemu-iotests/254
@@ -1,6 +1,6 @@
 #!/usr/bin/env python
 #
-# Test external snapshot with bitmap copying.
+# Test external snapshot with bitmap copying and moving.
 #
 # Copyright (c) 2019 Virtuozzo International GmbH. All rights reserved.
 #
@@ -32,6 +32,10 @@ vm = iotests.VM().add_drive(disk, opts='node-name=base')
 vm.launch()
 
 vm.qmp_log('block-dirty-bitmap-add', node='drive0', name='bitmap0')
+vm.qmp_log('block-dirty-bitmap-add', node='drive0', name='bitmap1',
+           persistent=True)
+vm.qmp_log('block-dirty-bitmap-add', node='drive0', name='bitmap2',
+           persistent=True)
 
 vm.hmp_qemu_io('drive0', 'write 0 512K')
 
@@ -39,16 +43,38 @@ vm.qmp_log('transaction', indent=2, actions=[
     {'type': 'blockdev-snapshot-sync',
      'data': {'device': 'drive0', 'snapshot-file': top,
               'snapshot-node-name': 'snap'}},
+
+    # copy non-persistent bitmap0
     {'type': 'block-dirty-bitmap-add',
      'data': {'node': 'snap', 'name': 'bitmap0'}},
     {'type': 'block-dirty-bitmap-merge',
      'data': {'node': 'snap', 'target': 'bitmap0',
-              'bitmaps': [{'node': 'base', 'name': 'bitmap0'}]}}
+              'bitmaps': [{'node': 'base', 'name': 'bitmap0'}]}},
+
+    # copy persistent bitmap1, original will be saved to base image
+    {'type': 'block-dirty-bitmap-add',
+     'data': {'node': 'snap', 'name': 'bitmap1', 'persistent': True}},
+    {'type': 'block-dirty-bitmap-merge',
+     'data': {'node': 'snap', 'target': 'bitmap1',
+              'bitmaps': [{'node': 'base', 'name': 'bitmap1'}]}},
+
+    # move persistent bitmap1, original will be removed and not saved
+    # to base image
+    {'type': 'block-dirty-bitmap-add',
+     'data': {'node': 'snap', 'name': 'bitmap2', 'persistent': True}},
+    {'type': 'block-dirty-bitmap-merge',
+     'data': {'node': 'snap', 'target': 'bitmap2',
+              'bitmaps': [{'node': 'base', 'name': 'bitmap2'}]}},
+    {'type': 'block-dirty-bitmap-remove',
+     'data': {'node': 'base', 'name': 'bitmap2'}}
 ], filters=[iotests.filter_qmp_testfiles])
 
 result = vm.qmp('query-block')['return'][0]
 log("query-block: device = {}, node-name = {}, dirty-bitmaps:".format(
     result['device'], result['inserted']['node-name']))
 log(result['dirty-bitmaps'], indent=2)
+log("\nbitmaps in backing image:")
+log(result['inserted']['image']['backing-image']['format-specific'] \
+    ['data']['bitmaps'], indent=2)
 
 vm.shutdown()
diff --git a/tests/qemu-iotests/254.out b/tests/qemu-iotests/254.out
index d7394cf002..d185c0532f 100644
--- a/tests/qemu-iotests/254.out
+++ b/tests/qemu-iotests/254.out
@@ -1,5 +1,9 @@
 {"execute": "block-dirty-bitmap-add", "arguments": {"name": "bitmap0", "node": "drive0"}}
 {"return": {}}
+{"execute": "block-dirty-bitmap-add", "arguments": {"name": "bitmap1", "node": "drive0", "persistent": true}}
+{"return": {}}
+{"execute": "block-dirty-bitmap-add", "arguments": {"name": "bitmap2", "node": "drive0", "persistent": true}}
+{"return": {}}
 {
   "execute": "transaction",
   "arguments": {
@@ -31,6 +35,55 @@
           "target": "bitmap0"
         },
         "type": "block-dirty-bitmap-merge"
+      },
+      {
+        "data": {
+          "name": "bitmap1",
+          "node": "snap",
+          "persistent": true
+        },
+        "type": "block-dirty-bitmap-add"
+      },
+      {
+        "data": {
+          "bitmaps": [
+            {
+              "name": "bitmap1",
+              "node": "base"
+            }
+          ],
+          "node": "snap",
+          "target": "bitmap1"
+        },
+        "type": "block-dirty-bitmap-merge"
+      },
+      {
+        "data": {
+          "name": "bitmap2",
+          "node": "snap",
+          "persistent": true
+        },
+        "type": "block-dirty-bitmap-add"
+      },
+      {
+        "data": {
+          "bitmaps": [
+            {
+              "name": "bitmap2",
+              "node": "base"
+            }
+          ],
+          "node": "snap",
+          "target": "bitmap2"
+        },
+        "type": "block-dirty-bitmap-merge"
+      },
+      {
+        "data": {
+          "name": "bitmap2",
+          "node": "base"
+        },
+        "type": "block-dirty-bitmap-remove"
       }
     ]
   }
@@ -40,6 +93,24 @@
 }
 query-block: device = drive0, node-name = snap, dirty-bitmaps:
 [
+  {
+    "busy": false,
+    "count": 524288,
+    "granularity": 65536,
+    "name": "bitmap2",
+    "persistent": true,
+    "recording": true,
+    "status": "active"
+  },
+  {
+    "busy": false,
+    "count": 524288,
+    "granularity": 65536,
+    "name": "bitmap1",
+    "persistent": true,
+    "recording": true,
+    "status": "active"
+  },
   {
     "busy": false,
     "count": 524288,
@@ -50,3 +121,14 @@ query-block: device = drive0, node-name = snap, dirty-bitmaps:
     "status": "active"
   }
 ]
+
+bitmaps in backing image:
+[
+  {
+    "flags": [
+      "auto"
+    ],
+    "granularity": 65536,
+    "name": "bitmap1"
+  }
+]
-- 
2.21.0

Re: [Qemu-devel] [PATCH v3 1/3] blockdev: reduce aio_context locked sections in bitmap add/remove

[Max Reitz]

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

On 09.07.19 00:05, John Snow wrote:
> From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
> Commit 0a6c86d024c52 returned these locks back to add/remove
> functionality, to protect from intersection of persistent bitmap
> related IO with other IO. But other bitmap-related functions called
> here are unrelated to the problem, and there are no needs to keep these
> calls inside critical sections.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: John Snow <jsnow@redhat.com>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  blockdev.c | 30 +++++++++++++-----------------
>  1 file changed, 13 insertions(+), 17 deletions(-)

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


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

Re: [Qemu-devel] [PATCH v3 2/3] qapi: implement block-dirty-bitmap-remove transaction action

[Max Reitz]

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

On 09.07.19 00:05, John Snow wrote:
> It is used to do transactional movement of the bitmap (which is
> possible in conjunction with merge command). Transactional bitmap
> movement is needed in scenarios with external snapshot, when we don't
> want to leave copy of the bitmap in the base image.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  block.c                        |  2 +-
>  block/dirty-bitmap.c           | 15 +++----
>  blockdev.c                     | 79 +++++++++++++++++++++++++++++++---
>  include/block/dirty-bitmap.h   |  2 +-
>  migration/block-dirty-bitmap.c |  2 +-
>  qapi/transaction.json          |  2 +
>  6 files changed, 85 insertions(+), 17 deletions(-)

[...]

> diff --git a/qapi/transaction.json b/qapi/transaction.json
> index 95edb78227..da95b804aa 100644
> --- a/qapi/transaction.json
> +++ b/qapi/transaction.json
> @@ -45,6 +45,7 @@
>  #
>  # - @abort: since 1.6
>  # - @block-dirty-bitmap-add: since 2.5
> +# - @block-dirty-bitmap-remove: since 4.1

Optimistic.

But anyway:

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

>  # - @block-dirty-bitmap-clear: since 2.5
>  # - @block-dirty-bitmap-enable: since 4.0
>  # - @block-dirty-bitmap-disable: since 4.0


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

Re: [Qemu-devel] [PATCH v3 3/3] iotests: test bitmap moving inside 254

[Max Reitz]

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

On 09.07.19 00:05, John Snow wrote:
> From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
> Test persistent bitmap copying with and without removal of original
> bitmap.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  tests/qemu-iotests/254     | 30 +++++++++++++-
>  tests/qemu-iotests/254.out | 82 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 110 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/qemu-iotests/254 b/tests/qemu-iotests/254
> index 8edba91c5d..9a57bccc26 100755
> --- a/tests/qemu-iotests/254
> +++ b/tests/qemu-iotests/254

[...]

> @@ -39,16 +43,38 @@ vm.qmp_log('transaction', indent=2, actions=[
>      {'type': 'blockdev-snapshot-sync',
>       'data': {'device': 'drive0', 'snapshot-file': top,
>                'snapshot-node-name': 'snap'}},
> +
> +    # copy non-persistent bitmap0
>      {'type': 'block-dirty-bitmap-add',
>       'data': {'node': 'snap', 'name': 'bitmap0'}},
>      {'type': 'block-dirty-bitmap-merge',
>       'data': {'node': 'snap', 'target': 'bitmap0',
> -              'bitmaps': [{'node': 'base', 'name': 'bitmap0'}]}}
> +              'bitmaps': [{'node': 'base', 'name': 'bitmap0'}]}},
> +
> +    # copy persistent bitmap1, original will be saved to base image
> +    {'type': 'block-dirty-bitmap-add',
> +     'data': {'node': 'snap', 'name': 'bitmap1', 'persistent': True}},
> +    {'type': 'block-dirty-bitmap-merge',
> +     'data': {'node': 'snap', 'target': 'bitmap1',
> +              'bitmaps': [{'node': 'base', 'name': 'bitmap1'}]}},
> +
> +    # move persistent bitmap1, original will be removed and not saved

*bitmap2

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

> +    # to base image
> +    {'type': 'block-dirty-bitmap-add',
> +     'data': {'node': 'snap', 'name': 'bitmap2', 'persistent': True}},
> +    {'type': 'block-dirty-bitmap-merge',
> +     'data': {'node': 'snap', 'target': 'bitmap2',
> +              'bitmaps': [{'node': 'base', 'name': 'bitmap2'}]}},
> +    {'type': 'block-dirty-bitmap-remove',
> +     'data': {'node': 'base', 'name': 'bitmap2'}}
>  ], filters=[iotests.filter_qmp_testfiles])


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

Re: [Qemu-devel] [PATCH v3 2/3] qapi: implement block-dirty-bitmap-remove transaction action

[John Snow]

On 7/15/19 7:40 AM, Max Reitz wrote:
> On 09.07.19 00:05, John Snow wrote:
>> It is used to do transactional movement of the bitmap (which is
>> possible in conjunction with merge command). Transactional bitmap
>> movement is needed in scenarios with external snapshot, when we don't
>> want to leave copy of the bitmap in the base image.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  block.c                        |  2 +-
>>  block/dirty-bitmap.c           | 15 +++----
>>  blockdev.c                     | 79 +++++++++++++++++++++++++++++++---
>>  include/block/dirty-bitmap.h   |  2 +-
>>  migration/block-dirty-bitmap.c |  2 +-
>>  qapi/transaction.json          |  2 +
>>  6 files changed, 85 insertions(+), 17 deletions(-)
> 
> [...]
> 
>> diff --git a/qapi/transaction.json b/qapi/transaction.json
>> index 95edb78227..da95b804aa 100644
>> --- a/qapi/transaction.json
>> +++ b/qapi/transaction.json
>> @@ -45,6 +45,7 @@
>>  #
>>  # - @abort: since 1.6
>>  # - @block-dirty-bitmap-add: since 2.5
>> +# - @block-dirty-bitmap-remove: since 4.1
> 
> Optimistic.
> 
> But anyway:
> 
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> 
>>  # - @block-dirty-bitmap-clear: since 2.5
>>  # - @block-dirty-bitmap-enable: since 4.0
>>  # - @block-dirty-bitmap-disable: since 4.0
> 

Hah, oops. OK, I'll adjust that (and your comment in patch 3) and stage
them.

Thank you!

Re: [Qemu-devel] [PATCH v3 0/3] qapi: block-dirty-bitmap-remove transaction action

[John Snow]

On 7/8/19 6:04 PM, John Snow wrote:
> Hi, this is a proposal based off of Vladimir's patchset:
> [Qemu-devel] [PATCH 0/4] qapi: block-dirty-bitmap-remove transaction action
> 
> ===
> V3:
> ===
> 
> 001/3:[----] [--] 'blockdev: reduce aio_context locked sections in bitmap add/remove'
> 002/3:[0024] [FC] 'qapi: implement block-dirty-bitmap-remove transaction action'
> 003/3:[----] [--] 'iotests: test bitmap moving inside 254'
> 
> - Changed "squelch_persistence" to "skip_store"
> - Use Max's suggestion for return expr
> 
> ===
> V2:
> ===
> 
> It replaces patches two and three with a modified patch (now patch 2)
> that foregoes the need for a hide()/unhide() bitmap API. I think it's
> suitable as a smaller alternative, but I'm not sure if it covers all
> of the use cases of the original series.
> 
> Patches 1 and 3 (formerly 4) included as-is.
> 
> John Snow (1):
>   qapi: implement block-dirty-bitmap-remove transaction action
> 
> Vladimir Sementsov-Ogievskiy (2):
>   blockdev: reduce aio_context locked sections in bitmap add/remove
>   iotests: test bitmap moving inside 254
> 
>  block.c                        |   2 +-
>  block/dirty-bitmap.c           |  15 +++--
>  blockdev.c                     | 105 ++++++++++++++++++++++++++-------
>  include/block/dirty-bitmap.h   |   2 +-
>  migration/block-dirty-bitmap.c |   2 +-
>  qapi/transaction.json          |   2 +
>  tests/qemu-iotests/254         |  30 +++++++++-
>  tests/qemu-iotests/254.out     |  82 +++++++++++++++++++++++++
>  8 files changed, 206 insertions(+), 34 deletions(-)
> 

Thanks, applied to my bitmaps tree:

https://github.com/jnsnow/qemu/commits/bitmaps
https://github.com/jnsnow/qemu.git

--js


(Vladimir: if this isn't amenable to you, it's going in for 4.2, so we
have until the next freeze to change it. Let me know, OK?)

Re: [Qemu-devel] [PATCH v3 0/3] qapi: block-dirty-bitmap-remove transaction action

[Vladimir Sementsov-Ogievskiy]

15.07.2019 22:48, John Snow wrote:
> 
> 
> On 7/8/19 6:04 PM, John Snow wrote:
>> Hi, this is a proposal based off of Vladimir's patchset:
>> [Qemu-devel] [PATCH 0/4] qapi: block-dirty-bitmap-remove transaction action
>>
>> ===
>> V3:
>> ===
>>
>> 001/3:[----] [--] 'blockdev: reduce aio_context locked sections in bitmap add/remove'
>> 002/3:[0024] [FC] 'qapi: implement block-dirty-bitmap-remove transaction action'
>> 003/3:[----] [--] 'iotests: test bitmap moving inside 254'
>>
>> - Changed "squelch_persistence" to "skip_store"
>> - Use Max's suggestion for return expr
>>
>> ===
>> V2:
>> ===
>>
>> It replaces patches two and three with a modified patch (now patch 2)
>> that foregoes the need for a hide()/unhide() bitmap API. I think it's
>> suitable as a smaller alternative, but I'm not sure if it covers all
>> of the use cases of the original series.
>>
>> Patches 1 and 3 (formerly 4) included as-is.
>>
>> John Snow (1):
>>    qapi: implement block-dirty-bitmap-remove transaction action
>>
>> Vladimir Sementsov-Ogievskiy (2):
>>    blockdev: reduce aio_context locked sections in bitmap add/remove
>>    iotests: test bitmap moving inside 254
>>
>>   block.c                        |   2 +-
>>   block/dirty-bitmap.c           |  15 +++--
>>   blockdev.c                     | 105 ++++++++++++++++++++++++++-------
>>   include/block/dirty-bitmap.h   |   2 +-
>>   migration/block-dirty-bitmap.c |   2 +-
>>   qapi/transaction.json          |   2 +
>>   tests/qemu-iotests/254         |  30 +++++++++-
>>   tests/qemu-iotests/254.out     |  82 +++++++++++++++++++++++++
>>   8 files changed, 206 insertions(+), 34 deletions(-)
>>
> 
> Thanks, applied to my bitmaps tree:
> 
> https://github.com/jnsnow/qemu/commits/bitmaps
> https://github.com/jnsnow/qemu.git
> 
> --js
> 
> 
> (Vladimir: if this isn't amenable to you, it's going in for 4.2, so we
> have until the next freeze to change it. Let me know, OK?)
> 


And finally I'm here :)

Thanks a lot for doing this job and for your explanations in other threads which
I'm reading today and sorry for the delay! I'll look through these series soon.

Actually, my second child (girl:) was born a month ago, and then her elder brother
was ill, so I took two weeks sick leave after about two weeks vacation and forget
about work for a month.

Hmm. And Nikolay, who doing libvirt part is on vocation now (I started bitmap remove
transaction series by his request), so I don't know about the end of the story with
release and this functionality..

Anyway, it's cool, thanks!

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v3 0/3] qapi: block-dirty-bitmap-remove transaction action

[John Snow]

On 7/24/19 7:12 AM, Vladimir Sementsov-Ogievskiy wrote:
> 15.07.2019 22:48, John Snow wrote:
>>
>>
>> On 7/8/19 6:04 PM, John Snow wrote:
>>> Hi, this is a proposal based off of Vladimir's patchset:
>>> [Qemu-devel] [PATCH 0/4] qapi: block-dirty-bitmap-remove transaction action
>>>
>>> ===
>>> V3:
>>> ===
>>>
>>> 001/3:[----] [--] 'blockdev: reduce aio_context locked sections in bitmap add/remove'
>>> 002/3:[0024] [FC] 'qapi: implement block-dirty-bitmap-remove transaction action'
>>> 003/3:[----] [--] 'iotests: test bitmap moving inside 254'
>>>
>>> - Changed "squelch_persistence" to "skip_store"
>>> - Use Max's suggestion for return expr
>>>
>>> ===
>>> V2:
>>> ===
>>>
>>> It replaces patches two and three with a modified patch (now patch 2)
>>> that foregoes the need for a hide()/unhide() bitmap API. I think it's
>>> suitable as a smaller alternative, but I'm not sure if it covers all
>>> of the use cases of the original series.
>>>
>>> Patches 1 and 3 (formerly 4) included as-is.
>>>
>>> John Snow (1):
>>>    qapi: implement block-dirty-bitmap-remove transaction action
>>>
>>> Vladimir Sementsov-Ogievskiy (2):
>>>    blockdev: reduce aio_context locked sections in bitmap add/remove
>>>    iotests: test bitmap moving inside 254
>>>
>>>   block.c                        |   2 +-
>>>   block/dirty-bitmap.c           |  15 +++--
>>>   blockdev.c                     | 105 ++++++++++++++++++++++++++-------
>>>   include/block/dirty-bitmap.h   |   2 +-
>>>   migration/block-dirty-bitmap.c |   2 +-
>>>   qapi/transaction.json          |   2 +
>>>   tests/qemu-iotests/254         |  30 +++++++++-
>>>   tests/qemu-iotests/254.out     |  82 +++++++++++++++++++++++++
>>>   8 files changed, 206 insertions(+), 34 deletions(-)
>>>
>>
>> Thanks, applied to my bitmaps tree:
>>
>> https://github.com/jnsnow/qemu/commits/bitmaps
>> https://github.com/jnsnow/qemu.git
>>
>> --js
>>
>>
>> (Vladimir: if this isn't amenable to you, it's going in for 4.2, so we
>> have until the next freeze to change it. Let me know, OK?)
>>
> 
> 
> And finally I'm here :)
> 
> Thanks a lot for doing this job and for your explanations in other threads which
> I'm reading today and sorry for the delay! I'll look through these series soon.
> 
> Actually, my second child (girl:) was born a month ago, and then her elder brother
> was ill, so I took two weeks sick leave after about two weeks vacation and forget
> about work for a month.
> 
> Hmm. And Nikolay, who doing libvirt part is on vocation now (I started bitmap remove
> transaction series by his request), so I don't know about the end of the story with
> release and this functionality..
> 
> Anyway, it's cool, thanks!
> 

Wow!

Congratulations Vladimir!

Re: [Qemu-devel] [PATCH v3 2/3] qapi: implement block-dirty-bitmap-remove transaction action

[Vladimir Sementsov-Ogievskiy]

09.07.2019 1:05, John Snow wrote:
> It is used to do transactional movement of the bitmap (which is
> possible in conjunction with merge command). Transactional bitmap
> movement is needed in scenarios with external snapshot, when we don't
> want to leave copy of the bitmap in the base image.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   block.c                        |  2 +-
>   block/dirty-bitmap.c           | 15 +++----
>   blockdev.c                     | 79 +++++++++++++++++++++++++++++++---
>   include/block/dirty-bitmap.h   |  2 +-
>   migration/block-dirty-bitmap.c |  2 +-
>   qapi/transaction.json          |  2 +
>   6 files changed, 85 insertions(+), 17 deletions(-)
> 
> diff --git a/block.c b/block.c
> index c139540f2b..5195d4b910 100644
> --- a/block.c
> +++ b/block.c
> @@ -5316,7 +5316,7 @@ static void coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs,
>       for (bm = bdrv_dirty_bitmap_next(bs, NULL); bm;
>            bm = bdrv_dirty_bitmap_next(bs, bm))
>       {
> -        bdrv_dirty_bitmap_set_migration(bm, false);
> +        bdrv_dirty_bitmap_skip_store(bm, false);
>       }
>   
>       ret = refresh_total_sectors(bs, bs->total_sectors);
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 95a9c2a5d8..a308e1f84b 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -48,10 +48,9 @@ struct BdrvDirtyBitmap {
>       bool inconsistent;          /* bitmap is persistent, but inconsistent.
>                                      It cannot be used at all in any way, except
>                                      a QMP user can remove it. */
> -    bool migration;             /* Bitmap is selected for migration, it should
> -                                   not be stored on the next inactivation
> -                                   (persistent flag doesn't matter until next
> -                                   invalidation).*/
> +    bool skip_store;            /* We are either migrating or deleting this
> +                                 * bitmap; it should not be stored on the next
> +                                 * inactivation. */
>       QLIST_ENTRY(BdrvDirtyBitmap) list;
>   };
>   
> @@ -757,16 +756,16 @@ void bdrv_dirty_bitmap_set_inconsistent(BdrvDirtyBitmap *bitmap)
>   }
>   
>   /* Called with BQL taken. */
> -void bdrv_dirty_bitmap_set_migration(BdrvDirtyBitmap *bitmap, bool migration)
> +void bdrv_dirty_bitmap_skip_store(BdrvDirtyBitmap *bitmap, bool skip)
>   {
>       qemu_mutex_lock(bitmap->mutex);
> -    bitmap->migration = migration;
> +    bitmap->skip_store = skip;
>       qemu_mutex_unlock(bitmap->mutex);
>   }
>   
>   bool bdrv_dirty_bitmap_get_persistence(BdrvDirtyBitmap *bitmap)
>   {
> -    return bitmap->persistent && !bitmap->migration;
> +    return bitmap->persistent && !bitmap->skip_store;
>   }
>   
>   bool bdrv_dirty_bitmap_inconsistent(const BdrvDirtyBitmap *bitmap)
> @@ -778,7 +777,7 @@ bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs)
>   {
>       BdrvDirtyBitmap *bm;
>       QLIST_FOREACH(bm, &bs->dirty_bitmaps, list) {
> -        if (bm->persistent && !bm->readonly && !bm->migration) {
> +        if (bm->persistent && !bm->readonly && !bm->skip_store) {
>               return true;
>           }
>       }
> diff --git a/blockdev.c b/blockdev.c
> index 01248252ca..800b3dcb42 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2134,6 +2134,51 @@ static void block_dirty_bitmap_merge_prepare(BlkActionState *common,
>                                                   errp);
>   }
>   
> +static BdrvDirtyBitmap *do_block_dirty_bitmap_remove(
> +        const char *node, const char *name, bool release,
> +        BlockDriverState **bitmap_bs, Error **errp);
> +
> +static void block_dirty_bitmap_remove_prepare(BlkActionState *common,
> +                                              Error **errp)
> +{
> +    BlockDirtyBitmap *action;
> +    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
> +                                             common, common);
> +
> +    if (action_check_completion_mode(common, errp) < 0) {
> +        return;
> +    }
> +
> +    action = common->action->u.block_dirty_bitmap_remove.data;
> +
> +    state->bitmap = do_block_dirty_bitmap_remove(action->node, action->name,
> +                                                 false, &state->bs, errp);
> +    if (state->bitmap) {
> +        bdrv_dirty_bitmap_skip_store(state->bitmap, true);
> +        bdrv_dirty_bitmap_set_busy(state->bitmap, true);
> +    }
> +}
> +
> +static void block_dirty_bitmap_remove_abort(BlkActionState *common)
> +{
> +    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
> +                                             common, common);
> +
> +    if (state->bitmap) {

Hmm, interesting, I thought, abort should not be called, if prepare failed, so the
following may be done unconditionally?

> +        bdrv_dirty_bitmap_skip_store(state->bitmap, false);
> +        bdrv_dirty_bitmap_set_busy(state->bitmap, false);
> +    }
> +}
> +

[..]

OK, I agree, good idea to reuse BUSY and migration field here and avoid new API.
Now release-prepare is less "release", but I don't see any real problems with it.
Maybe, we need something to be noted in docs.

Hmm, as we are not in a hurry now, we may wait for Nikolay to check this on his
scenarios.


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v3 2/3] qapi: implement block-dirty-bitmap-remove transaction action

[John Snow]

On 7/24/19 9:58 AM, Vladimir Sementsov-Ogievskiy wrote:
> 09.07.2019 1:05, John Snow wrote:
>> It is used to do transactional movement of the bitmap (which is
>> possible in conjunction with merge command). Transactional bitmap
>> movement is needed in scenarios with external snapshot, when we don't
>> want to leave copy of the bitmap in the base image.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   block.c                        |  2 +-
>>   block/dirty-bitmap.c           | 15 +++----
>>   blockdev.c                     | 79 +++++++++++++++++++++++++++++++---
>>   include/block/dirty-bitmap.h   |  2 +-
>>   migration/block-dirty-bitmap.c |  2 +-
>>   qapi/transaction.json          |  2 +
>>   6 files changed, 85 insertions(+), 17 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index c139540f2b..5195d4b910 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -5316,7 +5316,7 @@ static void coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs,
>>       for (bm = bdrv_dirty_bitmap_next(bs, NULL); bm;
>>            bm = bdrv_dirty_bitmap_next(bs, bm))
>>       {
>> -        bdrv_dirty_bitmap_set_migration(bm, false);
>> +        bdrv_dirty_bitmap_skip_store(bm, false);
>>       }
>>   
>>       ret = refresh_total_sectors(bs, bs->total_sectors);
>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>> index 95a9c2a5d8..a308e1f84b 100644
>> --- a/block/dirty-bitmap.c
>> +++ b/block/dirty-bitmap.c
>> @@ -48,10 +48,9 @@ struct BdrvDirtyBitmap {
>>       bool inconsistent;          /* bitmap is persistent, but inconsistent.
>>                                      It cannot be used at all in any way, except
>>                                      a QMP user can remove it. */
>> -    bool migration;             /* Bitmap is selected for migration, it should
>> -                                   not be stored on the next inactivation
>> -                                   (persistent flag doesn't matter until next
>> -                                   invalidation).*/
>> +    bool skip_store;            /* We are either migrating or deleting this
>> +                                 * bitmap; it should not be stored on the next
>> +                                 * inactivation. */
>>       QLIST_ENTRY(BdrvDirtyBitmap) list;
>>   };
>>   
>> @@ -757,16 +756,16 @@ void bdrv_dirty_bitmap_set_inconsistent(BdrvDirtyBitmap *bitmap)
>>   }
>>   
>>   /* Called with BQL taken. */
>> -void bdrv_dirty_bitmap_set_migration(BdrvDirtyBitmap *bitmap, bool migration)
>> +void bdrv_dirty_bitmap_skip_store(BdrvDirtyBitmap *bitmap, bool skip)
>>   {
>>       qemu_mutex_lock(bitmap->mutex);
>> -    bitmap->migration = migration;
>> +    bitmap->skip_store = skip;
>>       qemu_mutex_unlock(bitmap->mutex);
>>   }
>>   
>>   bool bdrv_dirty_bitmap_get_persistence(BdrvDirtyBitmap *bitmap)
>>   {
>> -    return bitmap->persistent && !bitmap->migration;
>> +    return bitmap->persistent && !bitmap->skip_store;
>>   }
>>   
>>   bool bdrv_dirty_bitmap_inconsistent(const BdrvDirtyBitmap *bitmap)
>> @@ -778,7 +777,7 @@ bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs)
>>   {
>>       BdrvDirtyBitmap *bm;
>>       QLIST_FOREACH(bm, &bs->dirty_bitmaps, list) {
>> -        if (bm->persistent && !bm->readonly && !bm->migration) {
>> +        if (bm->persistent && !bm->readonly && !bm->skip_store) {
>>               return true;
>>           }
>>       }
>> diff --git a/blockdev.c b/blockdev.c
>> index 01248252ca..800b3dcb42 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -2134,6 +2134,51 @@ static void block_dirty_bitmap_merge_prepare(BlkActionState *common,
>>                                                   errp);
>>   }
>>   
>> +static BdrvDirtyBitmap *do_block_dirty_bitmap_remove(
>> +        const char *node, const char *name, bool release,
>> +        BlockDriverState **bitmap_bs, Error **errp);
>> +
>> +static void block_dirty_bitmap_remove_prepare(BlkActionState *common,
>> +                                              Error **errp)
>> +{
>> +    BlockDirtyBitmap *action;
>> +    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
>> +                                             common, common);
>> +
>> +    if (action_check_completion_mode(common, errp) < 0) {
>> +        return;
>> +    }
>> +
>> +    action = common->action->u.block_dirty_bitmap_remove.data;
>> +
>> +    state->bitmap = do_block_dirty_bitmap_remove(action->node, action->name,
>> +                                                 false, &state->bs, errp);
>> +    if (state->bitmap) {
>> +        bdrv_dirty_bitmap_skip_store(state->bitmap, true);
>> +        bdrv_dirty_bitmap_set_busy(state->bitmap, true);
>> +    }
>> +}
>> +
>> +static void block_dirty_bitmap_remove_abort(BlkActionState *common)
>> +{
>> +    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
>> +                                             common, common);
>> +
>> +    if (state->bitmap) {
> 
> Hmm, interesting, I thought, abort should not be called, if prepare failed, so the
> following may be done unconditionally?
> 

I don't think so:

qmp_transaction:
	...
        state->ops->prepare(state, &local_err);
        if (local_err) {
            error_propagate(errp, local_err);
	    goto delete_and_fail;
	}
	...
delete_and_fail:
    ...
    QTAILQ_FOREACH_REVERSE(state, &snap_bdrv_states, entry) {
        if (state->ops->abort) {
            state->ops->abort(state);
        }
    }

>> +        bdrv_dirty_bitmap_skip_store(state->bitmap, false);
>> +        bdrv_dirty_bitmap_set_busy(state->bitmap, false);
>> +    }
>> +}
>> +
> 
> [..]
> 
> OK, I agree, good idea to reuse BUSY and migration field here and avoid new API.
> Now release-prepare is less "release", but I don't see any real problems with it.
> Maybe, we need something to be noted in docs.
> 
> Hmm, as we are not in a hurry now, we may wait for Nikolay to check this on his
> scenarios.
> 

OK, you have time. These patches are all sitting in my bitmaps branch
and I haven't sent the PR for them yet.

--js

Re: [Qemu-devel] [PATCH v3 0/3] qapi: block-dirty-bitmap-remove transaction action

[Vladimir Sementsov-Ogievskiy]

24.07.2019 15:52, John Snow wrote:
> 
> 
> On 7/24/19 7:12 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 15.07.2019 22:48, John Snow wrote:
>>>
>>>
>>> On 7/8/19 6:04 PM, John Snow wrote:
>>>> Hi, this is a proposal based off of Vladimir's patchset:
>>>> [Qemu-devel] [PATCH 0/4] qapi: block-dirty-bitmap-remove transaction action
>>>>
>>>> ===
>>>> V3:
>>>> ===
>>>>
>>>> 001/3:[----] [--] 'blockdev: reduce aio_context locked sections in bitmap add/remove'
>>>> 002/3:[0024] [FC] 'qapi: implement block-dirty-bitmap-remove transaction action'
>>>> 003/3:[----] [--] 'iotests: test bitmap moving inside 254'
>>>>
>>>> - Changed "squelch_persistence" to "skip_store"
>>>> - Use Max's suggestion for return expr
>>>>
>>>> ===
>>>> V2:
>>>> ===
>>>>
>>>> It replaces patches two and three with a modified patch (now patch 2)
>>>> that foregoes the need for a hide()/unhide() bitmap API. I think it's
>>>> suitable as a smaller alternative, but I'm not sure if it covers all
>>>> of the use cases of the original series.
>>>>
>>>> Patches 1 and 3 (formerly 4) included as-is.
>>>>
>>>> John Snow (1):
>>>>     qapi: implement block-dirty-bitmap-remove transaction action
>>>>
>>>> Vladimir Sementsov-Ogievskiy (2):
>>>>     blockdev: reduce aio_context locked sections in bitmap add/remove
>>>>     iotests: test bitmap moving inside 254
>>>>
>>>>    block.c                        |   2 +-
>>>>    block/dirty-bitmap.c           |  15 +++--
>>>>    blockdev.c                     | 105 ++++++++++++++++++++++++++-------
>>>>    include/block/dirty-bitmap.h   |   2 +-
>>>>    migration/block-dirty-bitmap.c |   2 +-
>>>>    qapi/transaction.json          |   2 +
>>>>    tests/qemu-iotests/254         |  30 +++++++++-
>>>>    tests/qemu-iotests/254.out     |  82 +++++++++++++++++++++++++
>>>>    8 files changed, 206 insertions(+), 34 deletions(-)
>>>>
>>>
>>> Thanks, applied to my bitmaps tree:
>>>
>>> https://github.com/jnsnow/qemu/commits/bitmaps
>>> https://github.com/jnsnow/qemu.git
>>>
>>> --js
>>>
>>>
>>> (Vladimir: if this isn't amenable to you, it's going in for 4.2, so we
>>> have until the next freeze to change it. Let me know, OK?)
>>>
>>
>>
>> And finally I'm here :)
>>
>> Thanks a lot for doing this job and for your explanations in other threads which
>> I'm reading today and sorry for the delay! I'll look through these series soon.
>>
>> Actually, my second child (girl:) was born a month ago, and then her elder brother
>> was ill, so I took two weeks sick leave after about two weeks vacation and forget
>> about work for a month.
>>
>> Hmm. And Nikolay, who doing libvirt part is on vocation now (I started bitmap remove
>> transaction series by his request), so I don't know about the end of the story with
>> release and this functionality..
>>
>> Anyway, it's cool, thanks!
>>
> 
> Wow!
> 
> Congratulations Vladimir!
> 

Thank you!

-- 
Best regards,
Vladimir