[PATCH v3 0/7] fix migration with bitmaps and mirror

[PATCH v3 0/7] fix migration with bitmaps and mirror

[Vladimir Sementsov-Ogievskiy]

v3:
01: updated, just taken from Kevin's block-tree
02: fix grammar, add Eric's r-b
03: add Eric's and Andrey's r-bs (Andrey's one comes from downstrem)
04: update commit message a bit
06-07: new

Max Reitz (1):
  block: Mark commit, mirror, blkreplay as filters

Vladimir Sementsov-Ogievskiy (6):
  migration/block-dirty-bitmap: refactor init_dirty_bitmap_migration
  block/dirty-bitmap: add bdrv_has_named_bitmaps helper
  migration/block-dirty-bitmap: fix bitmaps pre-blockdev migration
    during mirror job
  iotests: 194: test also migration of dirty bitmap
  migration/block-dirty-bitmap: add_bitmaps_to_list: check disk name
    once
  migration/block-dirty-bitmap: forbid migration by generated node-name

 include/block/dirty-bitmap.h   |   1 +
 block/blkreplay.c              |   1 +
 block/commit.c                 |   2 +
 block/dirty-bitmap.c           |  13 ++++
 block/mirror.c                 |   2 +
 migration/block-dirty-bitmap.c | 126 ++++++++++++++++++++++++---------
 tests/qemu-iotests/194         |  14 ++--
 tests/qemu-iotests/194.out     |   6 ++
 8 files changed, 127 insertions(+), 38 deletions(-)

-- 
2.21.0

[PATCH v3 1/7] block: Mark commit, mirror, blkreplay as filters

[Vladimir Sementsov-Ogievskiy]

From: Max Reitz <mreitz@redhat.com>

The commit, mirror, and blkreplay block nodes are filters, so they should
be marked as such.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/blkreplay.c | 1 +
 block/commit.c    | 2 ++
 block/mirror.c    | 2 ++
 3 files changed, 5 insertions(+)

diff --git a/block/blkreplay.c b/block/blkreplay.c
index c96ac8f4bc..131c9e8477 100644
--- a/block/blkreplay.c
+++ b/block/blkreplay.c
@@ -135,6 +135,7 @@ static int blkreplay_snapshot_goto(BlockDriverState *bs,
 static BlockDriver bdrv_blkreplay = {
     .format_name            = "blkreplay",
     .instance_size          = 0,
+    .is_filter              = true,
 
     .bdrv_open              = blkreplay_open,
     .bdrv_child_perm        = bdrv_filter_default_perms,
diff --git a/block/commit.c b/block/commit.c
index 87f6096d90..445a28069b 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -240,6 +240,8 @@ static BlockDriver bdrv_commit_top = {
     .bdrv_co_block_status       = bdrv_co_block_status_from_backing,
     .bdrv_refresh_filename      = bdrv_commit_top_refresh_filename,
     .bdrv_child_perm            = bdrv_commit_top_child_perm,
+
+    .is_filter                  = true,
 };
 
 void commit_start(const char *job_id, BlockDriverState *bs,
diff --git a/block/mirror.c b/block/mirror.c
index aca95c9bc9..b6de24bc3c 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1527,6 +1527,8 @@ static BlockDriver bdrv_mirror_top = {
     .bdrv_co_block_status       = bdrv_co_block_status_from_backing,
     .bdrv_refresh_filename      = bdrv_mirror_top_refresh_filename,
     .bdrv_child_perm            = bdrv_mirror_top_child_perm,
+
+    .is_filter                  = true,
 };
 
 static BlockJob *mirror_start_job(
-- 
2.21.0

[PATCH v3 2/7] migration/block-dirty-bitmap: refactor init_dirty_bitmap_migration

[Vladimir Sementsov-Ogievskiy]

Split out handling one bs, it is needed for the following commit, which
will handle BlockBackends separately.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 migration/block-dirty-bitmap.c | 89 +++++++++++++++++++---------------
 1 file changed, 49 insertions(+), 40 deletions(-)

diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index 7eafface61..7e93718086 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -268,57 +268,66 @@ static void dirty_bitmap_mig_cleanup(void)
 }
 
 /* Called with iothread lock taken. */
-static int init_dirty_bitmap_migration(void)
+static int add_bitmaps_to_list(BlockDriverState *bs, const char *bs_name)
 {
-    BlockDriverState *bs;
     BdrvDirtyBitmap *bitmap;
     DirtyBitmapMigBitmapState *dbms;
     Error *local_err = NULL;
 
-    dirty_bitmap_mig_state.bulk_completed = false;
-    dirty_bitmap_mig_state.prev_bs = NULL;
-    dirty_bitmap_mig_state.prev_bitmap = NULL;
-    dirty_bitmap_mig_state.no_bitmaps = false;
+    FOR_EACH_DIRTY_BITMAP(bs, bitmap) {
+        if (!bdrv_dirty_bitmap_name(bitmap)) {
+            continue;
+        }
 
-    for (bs = bdrv_next_all_states(NULL); bs; bs = bdrv_next_all_states(bs)) {
-        const char *name = bdrv_get_device_or_node_name(bs);
+        if (!bs_name || strcmp(bs_name, "") == 0) {
+            error_report("Found bitmap '%s' in unnamed node %p. It can't "
+                         "be migrated", bdrv_dirty_bitmap_name(bitmap), bs);
+            return -1;
+        }
 
-        FOR_EACH_DIRTY_BITMAP(bs, bitmap) {
-            if (!bdrv_dirty_bitmap_name(bitmap)) {
-                continue;
-            }
+        if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_DEFAULT, &local_err)) {
+            error_report_err(local_err);
+            return -1;
+        }
 
-            if (!name || strcmp(name, "") == 0) {
-                error_report("Found bitmap '%s' in unnamed node %p. It can't "
-                             "be migrated", bdrv_dirty_bitmap_name(bitmap), bs);
-                goto fail;
-            }
+        bdrv_ref(bs);
+        bdrv_dirty_bitmap_set_busy(bitmap, true);
+
+        dbms = g_new0(DirtyBitmapMigBitmapState, 1);
+        dbms->bs = bs;
+        dbms->node_name = bs_name;
+        dbms->bitmap = bitmap;
+        dbms->total_sectors = bdrv_nb_sectors(bs);
+        dbms->sectors_per_chunk = CHUNK_SIZE * 8 *
+            bdrv_dirty_bitmap_granularity(bitmap) >> BDRV_SECTOR_BITS;
+        if (bdrv_dirty_bitmap_enabled(bitmap)) {
+            dbms->flags |= DIRTY_BITMAP_MIG_START_FLAG_ENABLED;
+        }
+        if (bdrv_dirty_bitmap_get_persistence(bitmap)) {
+            dbms->flags |= DIRTY_BITMAP_MIG_START_FLAG_PERSISTENT;
+        }
 
-            if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_DEFAULT,
-                                        &local_err)) {
-                error_report_err(local_err);
-                goto fail;
-            }
+        QSIMPLEQ_INSERT_TAIL(&dirty_bitmap_mig_state.dbms_list,
+                             dbms, entry);
+    }
 
-            bdrv_ref(bs);
-            bdrv_dirty_bitmap_set_busy(bitmap, true);
-
-            dbms = g_new0(DirtyBitmapMigBitmapState, 1);
-            dbms->bs = bs;
-            dbms->node_name = name;
-            dbms->bitmap = bitmap;
-            dbms->total_sectors = bdrv_nb_sectors(bs);
-            dbms->sectors_per_chunk = CHUNK_SIZE * 8 *
-                bdrv_dirty_bitmap_granularity(bitmap) >> BDRV_SECTOR_BITS;
-            if (bdrv_dirty_bitmap_enabled(bitmap)) {
-                dbms->flags |= DIRTY_BITMAP_MIG_START_FLAG_ENABLED;
-            }
-            if (bdrv_dirty_bitmap_get_persistence(bitmap)) {
-                dbms->flags |= DIRTY_BITMAP_MIG_START_FLAG_PERSISTENT;
-            }
+    return 0;
+}
+
+/* Called with iothread lock taken. */
+static int init_dirty_bitmap_migration(void)
+{
+    BlockDriverState *bs;
+    DirtyBitmapMigBitmapState *dbms;
+
+    dirty_bitmap_mig_state.bulk_completed = false;
+    dirty_bitmap_mig_state.prev_bs = NULL;
+    dirty_bitmap_mig_state.prev_bitmap = NULL;
+    dirty_bitmap_mig_state.no_bitmaps = false;
 
-            QSIMPLEQ_INSERT_TAIL(&dirty_bitmap_mig_state.dbms_list,
-                                 dbms, entry);
+    for (bs = bdrv_next_all_states(NULL); bs; bs = bdrv_next_all_states(bs)) {
+        if (add_bitmaps_to_list(bs, bdrv_get_device_or_node_name(bs))) {
+            goto fail;
         }
     }
 
-- 
2.21.0

[PATCH v3 3/7] block/dirty-bitmap: add bdrv_has_named_bitmaps helper

[Vladimir Sementsov-Ogievskiy]

To be used for bitmap migration in further commit.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 include/block/dirty-bitmap.h |  1 +
 block/dirty-bitmap.c         | 13 +++++++++++++
 2 files changed, 14 insertions(+)

diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 8a10029418..eee70df873 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -94,6 +94,7 @@ int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap);
 void bdrv_dirty_bitmap_truncate(BlockDriverState *bs, int64_t bytes);
 bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap);
 bool bdrv_has_readonly_bitmaps(BlockDriverState *bs);
+bool bdrv_has_named_bitmaps(BlockDriverState *bs);
 bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap);
 bool bdrv_dirty_bitmap_get_persistence(BdrvDirtyBitmap *bitmap);
 bool bdrv_dirty_bitmap_inconsistent(const BdrvDirtyBitmap *bitmap);
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 063793e316..c788ca7c69 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -809,6 +809,19 @@ bool bdrv_has_readonly_bitmaps(BlockDriverState *bs)
     return false;
 }
 
+bool bdrv_has_named_bitmaps(BlockDriverState *bs)
+{
+    BdrvDirtyBitmap *bm;
+
+    QLIST_FOREACH(bm, &bs->dirty_bitmaps, list) {
+        if (bdrv_dirty_bitmap_name(bm)) {
+            return true;
+        }
+    }
+
+    return false;
+}
+
 /* Called with BQL taken. */
 void bdrv_dirty_bitmap_set_persistence(BdrvDirtyBitmap *bitmap, bool persistent)
 {
-- 
2.21.0

[PATCH v3 4/7] migration/block-dirty-bitmap: fix bitmaps pre-blockdev migration during mirror job

[Vladimir Sementsov-Ogievskiy]

Important thing for bitmap migration is to select destination block
node to obtain the migrated bitmap.

Prepatch, on source we use bdrv_get_device_or_node_name() to identify
the node, and on target we do bdrv_lookup_bs.
bdrv_get_device_or_node_name() returns blk name only for direct
children of blk. So, bitmaps of direct children of blks are migrated by
blk name and others - by node name.

Old libvirt is unprepared to bitmap migration by node-name,
node-names are mostly auto-generated. So actually only migration by blk
name works for it.

Newer libvirt will use new interface (which will be added soon) to
specify node-mapping for bitmaps migration explicitly. Still, let's
improve the current behavior a bit.

Now, consider classic libvirt migrations assisted by mirror block job:
mirror block job inserts filter, so our source is not a direct child of
blk, and bitmaps are migrated by node-names. And this just don't work
with auto-generated node names

Let's fix it by allowing use blk-name even if some implicit filters are
inserted.

Note2: we, of course, can't skip filters and use blk name to migrate
bitmaps in filtered node by blk name for this blk if these filters have
named bitmaps which should be migrated.

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1652424
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 migration/block-dirty-bitmap.c | 39 +++++++++++++++++++++++++++++++++-
 1 file changed, 38 insertions(+), 1 deletion(-)

diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index 7e93718086..5d3a7d2b07 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -319,14 +319,48 @@ static int init_dirty_bitmap_migration(void)
 {
     BlockDriverState *bs;
     DirtyBitmapMigBitmapState *dbms;
+    GHashTable *handled_by_blk = g_hash_table_new(NULL, NULL);
+    BlockBackend *blk;
 
     dirty_bitmap_mig_state.bulk_completed = false;
     dirty_bitmap_mig_state.prev_bs = NULL;
     dirty_bitmap_mig_state.prev_bitmap = NULL;
     dirty_bitmap_mig_state.no_bitmaps = false;
 
+    /*
+     * Use blockdevice name for direct (or filtered) children of named block
+     * backends.
+     */
+    for (blk = blk_next(NULL); blk; blk = blk_next(blk)) {
+        const char *name = blk_name(blk);
+
+        if (!name || strcmp(name, "") == 0) {
+            continue;
+        }
+
+        bs = blk_bs(blk);
+
+        /* Skip filters without bitmaos */
+        while (bs && bs->drv && bs->drv->is_filter &&
+               !bdrv_has_named_bitmaps(bs))
+        {
+            bs = bs->backing->bs ?: bs->file->bs;
+        }
+
+        if (bs && bs->drv && !bs->drv->is_filter) {
+            if (add_bitmaps_to_list(bs, name)) {
+                goto fail;
+            }
+            g_hash_table_add(handled_by_blk, bs);
+        }
+    }
+
     for (bs = bdrv_next_all_states(NULL); bs; bs = bdrv_next_all_states(bs)) {
-        if (add_bitmaps_to_list(bs, bdrv_get_device_or_node_name(bs))) {
+        if (g_hash_table_contains(handled_by_blk, bs)) {
+            continue;
+        }
+
+        if (add_bitmaps_to_list(bs, bdrv_get_node_name(bs))) {
             goto fail;
         }
     }
@@ -340,9 +374,12 @@ static int init_dirty_bitmap_migration(void)
         dirty_bitmap_mig_state.no_bitmaps = true;
     }
 
+    g_hash_table_destroy(handled_by_blk);
+
     return 0;
 
 fail:
+    g_hash_table_destroy(handled_by_blk);
     dirty_bitmap_mig_cleanup();
 
     return -1;
-- 
2.21.0

[PATCH v3 5/7] iotests: 194: test also migration of dirty bitmap

[Vladimir Sementsov-Ogievskiy]

Test that dirty bitmap migration works when we deal with mirror.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/194     | 14 ++++++++++----
 tests/qemu-iotests/194.out |  6 ++++++
 2 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/194 b/tests/qemu-iotests/194
index 8b1f720af4..3fad7c6c1a 100755
--- a/tests/qemu-iotests/194
+++ b/tests/qemu-iotests/194
@@ -42,6 +42,8 @@ with iotests.FilePath('source.img') as source_img_path, \
             .add_incoming('unix:{0}'.format(migration_sock_path))
             .launch())
 
+    source_vm.qmp_log('block-dirty-bitmap-add', node='drive0', name='bitmap0')
+
     iotests.log('Launching NBD server on destination...')
     iotests.log(dest_vm.qmp('nbd-server-start', addr={'type': 'unix', 'data': {'path': nbd_sock_path}}))
     iotests.log(dest_vm.qmp('nbd-server-add', device='drive0', writable=True))
@@ -61,12 +63,14 @@ with iotests.FilePath('source.img') as source_img_path, \
                 filters=[iotests.filter_qmp_event])
 
     iotests.log('Starting migration...')
-    source_vm.qmp('migrate-set-capabilities',
-                  capabilities=[{'capability': 'events', 'state': True}])
-    dest_vm.qmp('migrate-set-capabilities',
-                capabilities=[{'capability': 'events', 'state': True}])
+    capabilities = [{'capability': 'events', 'state': True},
+                    {'capability': 'dirty-bitmaps', 'state': True}]
+    source_vm.qmp('migrate-set-capabilities', capabilities=capabilities)
+    dest_vm.qmp('migrate-set-capabilities', capabilities=capabilities)
     iotests.log(source_vm.qmp('migrate', uri='unix:{0}'.format(migration_sock_path)))
 
+    source_vm.qmp_log('migrate-start-postcopy')
+
     while True:
         event1 = source_vm.event_wait('MIGRATION')
         iotests.log(event1, filters=[iotests.filter_qmp_event])
@@ -82,3 +86,5 @@ with iotests.FilePath('source.img') as source_img_path, \
             iotests.log('Stopping the NBD server on destination...')
             iotests.log(dest_vm.qmp('nbd-server-stop'))
             break
+
+    iotests.log(source_vm.qmp('query-block')['return'][0]['dirty-bitmaps'])
diff --git a/tests/qemu-iotests/194.out b/tests/qemu-iotests/194.out
index 71857853fb..dd60dcc14f 100644
--- a/tests/qemu-iotests/194.out
+++ b/tests/qemu-iotests/194.out
@@ -1,4 +1,6 @@
 Launching VMs...
+{"execute": "block-dirty-bitmap-add", "arguments": {"name": "bitmap0", "node": "drive0"}}
+{"return": {}}
 Launching NBD server on destination...
 {"return": {}}
 {"return": {}}
@@ -8,11 +10,15 @@ Waiting for `drive-mirror` to complete...
 {"data": {"device": "mirror-job0", "len": 1073741824, "offset": 1073741824, "speed": 0, "type": "mirror"}, "event": "BLOCK_JOB_READY", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
 Starting migration...
 {"return": {}}
+{"execute": "migrate-start-postcopy", "arguments": {}}
+{"return": {}}
 {"data": {"status": "setup"}, "event": "MIGRATION", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
 {"data": {"status": "active"}, "event": "MIGRATION", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"data": {"status": "postcopy-active"}, "event": "MIGRATION", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
 {"data": {"status": "completed"}, "event": "MIGRATION", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
 Gracefully ending the `drive-mirror` job on source...
 {"return": {}}
 {"data": {"device": "mirror-job0", "len": 1073741824, "offset": 1073741824, "speed": 0, "type": "mirror"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
 Stopping the NBD server on destination...
 {"return": {}}
+[{"busy": false, "count": 0, "granularity": 65536, "name": "bitmap0", "persistent": false, "recording": true, "status": "active"}]
-- 
2.21.0

[PATCH v3 6/7] migration/block-dirty-bitmap: add_bitmaps_to_list: check disk name once

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---
 migration/block-dirty-bitmap.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index 5d3a7d2b07..e0e081ce60 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -274,17 +274,22 @@ static int add_bitmaps_to_list(BlockDriverState *bs, const char *bs_name)
     DirtyBitmapMigBitmapState *dbms;
     Error *local_err = NULL;
 
+    bitmap = bdrv_dirty_bitmap_first(bs);
+    if (!bitmap) {
+        return 0;
+    }
+
+    if (!bs_name || strcmp(bs_name, "") == 0) {
+        error_report("Found bitmap '%s' in unnamed node %p. It can't "
+                     "be migrated", bdrv_dirty_bitmap_name(bitmap), bs);
+        return -1;
+    }
+
     FOR_EACH_DIRTY_BITMAP(bs, bitmap) {
         if (!bdrv_dirty_bitmap_name(bitmap)) {
             continue;
         }
 
-        if (!bs_name || strcmp(bs_name, "") == 0) {
-            error_report("Found bitmap '%s' in unnamed node %p. It can't "
-                         "be migrated", bdrv_dirty_bitmap_name(bitmap), bs);
-            return -1;
-        }
-
         if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_DEFAULT, &local_err)) {
             error_report_err(local_err);
             return -1;
-- 
2.21.0

[PATCH v3 7/7] migration/block-dirty-bitmap: forbid migration by generated node-name

[Vladimir Sementsov-Ogievskiy]

It actually never worked with libvirt, as auto-generated names are
different on source and destination.

It's unsafe and useless to migrate by auto-generated node-names, so
let's forbid it.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---
 migration/block-dirty-bitmap.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index e0e081ce60..f5744c35e6 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -285,6 +285,13 @@ static int add_bitmaps_to_list(BlockDriverState *bs, const char *bs_name)
         return -1;
     }
 
+    if (bs_name[0] == '#') {
+        error_report("Found bitmap '%s' in a node with auto-generated "
+                     "name: %s. It can't be migrated",
+                     bdrv_dirty_bitmap_name(bitmap), bs_name);
+        return -1;
+    }
+
     FOR_EACH_DIRTY_BITMAP(bs, bitmap) {
         if (!bdrv_dirty_bitmap_name(bitmap)) {
             continue;
-- 
2.21.0

Re: [PATCH v3 4/7] migration/block-dirty-bitmap: fix bitmaps pre-blockdev migration during mirror job

[Eric Blake]

On 5/15/20 7:40 AM, Vladimir Sementsov-Ogievskiy wrote:
> Important thing for bitmap migration is to select destination block
> node to obtain the migrated bitmap.
> 
> Prepatch, on source we use bdrv_get_device_or_node_name() to identify
> the node, and on target we do bdrv_lookup_bs.
> bdrv_get_device_or_node_name() returns blk name only for direct
> children of blk. So, bitmaps of direct children of blks are migrated by
> blk name and others - by node name.
> 
> Old libvirt is unprepared to bitmap migration by node-name,
> node-names are mostly auto-generated. So actually only migration by blk
> name works for it.
> 
> Newer libvirt will use new interface (which will be added soon) to
> specify node-mapping for bitmaps migration explicitly. Still, let's
> improve the current behavior a bit.
> 
> Now, consider classic libvirt migrations assisted by mirror block job:
> mirror block job inserts filter, so our source is not a direct child of
> blk, and bitmaps are migrated by node-names. And this just don't work

either "won't" or "doesn't"

> with auto-generated node names

trailing '.'

> 
> Let's fix it by allowing use blk-name even if some implicit filters are
> inserted.

s/allowing use/using/

> 
> Note2: we, of course, can't skip filters and use blk name to migrate
> bitmaps in filtered node by blk name for this blk if these filters have
> named bitmaps which should be migrated.
> 
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1652424
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   migration/block-dirty-bitmap.c | 39 +++++++++++++++++++++++++++++++++-
>   1 file changed, 38 insertions(+), 1 deletion(-)
> 
> diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
> index 7e93718086..5d3a7d2b07 100644
> --- a/migration/block-dirty-bitmap.c
> +++ b/migration/block-dirty-bitmap.c
> @@ -319,14 +319,48 @@ static int init_dirty_bitmap_migration(void)
>   {
>       BlockDriverState *bs;
>       DirtyBitmapMigBitmapState *dbms;
> +    GHashTable *handled_by_blk = g_hash_table_new(NULL, NULL);
> +    BlockBackend *blk;
>   
>       dirty_bitmap_mig_state.bulk_completed = false;
>       dirty_bitmap_mig_state.prev_bs = NULL;
>       dirty_bitmap_mig_state.prev_bitmap = NULL;
>       dirty_bitmap_mig_state.no_bitmaps = false;
>   
> +    /*
> +     * Use blockdevice name for direct (or filtered) children of named block
> +     * backends.
> +     */
> +    for (blk = blk_next(NULL); blk; blk = blk_next(blk)) {
> +        const char *name = blk_name(blk);
> +
> +        if (!name || strcmp(name, "") == 0) {
> +            continue;
> +        }
> +
> +        bs = blk_bs(blk);
> +
> +        /* Skip filters without bitmaos */
> +        while (bs && bs->drv && bs->drv->is_filter &&
> +               !bdrv_has_named_bitmaps(bs))
> +        {
> +            bs = bs->backing->bs ?: bs->file->bs;

Is this correct, or should it be:

bs = bs->backing ? bs->backing->bs : bs->file->bs;

Otherwise looks reasonable, but I'm hesitant to include it in today's 
bitmap pull request in order to give it more review/testing time.  It 
should be ready for a pull request next week, though.

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

Re: [PATCH v3 4/7] migration/block-dirty-bitmap: fix bitmaps pre-blockdev migration during mirror job

[Vladimir Sementsov-Ogievskiy]

18.05.2020 23:36, Eric Blake wrote:
> On 5/15/20 7:40 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Important thing for bitmap migration is to select destination block
>> node to obtain the migrated bitmap.
>>
>> Prepatch, on source we use bdrv_get_device_or_node_name() to identify
>> the node, and on target we do bdrv_lookup_bs.
>> bdrv_get_device_or_node_name() returns blk name only for direct
>> children of blk. So, bitmaps of direct children of blks are migrated by
>> blk name and others - by node name.
>>
>> Old libvirt is unprepared to bitmap migration by node-name,
>> node-names are mostly auto-generated. So actually only migration by blk
>> name works for it.
>>
>> Newer libvirt will use new interface (which will be added soon) to
>> specify node-mapping for bitmaps migration explicitly. Still, let's
>> improve the current behavior a bit.
>>
>> Now, consider classic libvirt migrations assisted by mirror block job:
>> mirror block job inserts filter, so our source is not a direct child of
>> blk, and bitmaps are migrated by node-names. And this just don't work
> 
> either "won't" or "doesn't"
> 
>> with auto-generated node names
> 
> trailing '.'
> 
>>
>> Let's fix it by allowing use blk-name even if some implicit filters are
>> inserted.
> 
> s/allowing use/using/
> 
>>
>> Note2: we, of course, can't skip filters and use blk name to migrate
>> bitmaps in filtered node by blk name for this blk if these filters have
>> named bitmaps which should be migrated.
>>
>> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1652424
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   migration/block-dirty-bitmap.c | 39 +++++++++++++++++++++++++++++++++-
>>   1 file changed, 38 insertions(+), 1 deletion(-)
>>
>> diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
>> index 7e93718086..5d3a7d2b07 100644
>> --- a/migration/block-dirty-bitmap.c
>> +++ b/migration/block-dirty-bitmap.c
>> @@ -319,14 +319,48 @@ static int init_dirty_bitmap_migration(void)
>>   {
>>       BlockDriverState *bs;
>>       DirtyBitmapMigBitmapState *dbms;
>> +    GHashTable *handled_by_blk = g_hash_table_new(NULL, NULL);
>> +    BlockBackend *blk;
>>       dirty_bitmap_mig_state.bulk_completed = false;
>>       dirty_bitmap_mig_state.prev_bs = NULL;
>>       dirty_bitmap_mig_state.prev_bitmap = NULL;
>>       dirty_bitmap_mig_state.no_bitmaps = false;
>> +    /*
>> +     * Use blockdevice name for direct (or filtered) children of named block
>> +     * backends.
>> +     */
>> +    for (blk = blk_next(NULL); blk; blk = blk_next(blk)) {
>> +        const char *name = blk_name(blk);
>> +
>> +        if (!name || strcmp(name, "") == 0) {
>> +            continue;
>> +        }
>> +
>> +        bs = blk_bs(blk);
>> +
>> +        /* Skip filters without bitmaos */
>> +        while (bs && bs->drv && bs->drv->is_filter &&
>> +               !bdrv_has_named_bitmaps(bs))
>> +        {
>> +            bs = bs->backing->bs ?: bs->file->bs;
> 
> Is this correct, or should it be:
> 
> bs = bs->backing ? bs->backing->bs : bs->file->bs;

Hmm, yes, otherwise it should crash on file-based filter :)

> 
> Otherwise looks reasonable, but I'm hesitant to include it in today's bitmap pull request in order to give it more review/testing time.  It should be ready for a pull request next week, though.
> 


-- 
Best regards,
Vladimir

Re: [PATCH v3 4/7] migration/block-dirty-bitmap: fix bitmaps pre-blockdev migration during mirror job

[Eric Blake]

On 5/19/20 5:51 AM, Vladimir Sementsov-Ogievskiy wrote:
> 18.05.2020 23:36, Eric Blake wrote:
>> On 5/15/20 7:40 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> Important thing for bitmap migration is to select destination block
>>> node to obtain the migrated bitmap.
>>>

>>> +        /* Skip filters without bitmaos */
>>> +        while (bs && bs->drv && bs->drv->is_filter &&
>>> +               !bdrv_has_named_bitmaps(bs))
>>> +        {
>>> +            bs = bs->backing->bs ?: bs->file->bs;
>>
>> Is this correct, or should it be:
>>
>> bs = bs->backing ? bs->backing->bs : bs->file->bs;
> 
> Hmm, yes, otherwise it should crash on file-based filter :)
> 
>>
>> Otherwise looks reasonable, but I'm hesitant to include it in today's 
>> bitmap pull request in order to give it more review/testing time.  It 
>> should be ready for a pull request next week, though.

Can you post a v4, to make it easier for me to build?

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

Re: [PATCH v3 7/7] migration/block-dirty-bitmap: forbid migration by generated node-name

[Eric Blake]

On 5/15/20 7:40 AM, Vladimir Sementsov-Ogievskiy wrote:
> It actually never worked with libvirt, as auto-generated names are
> different on source and destination.
> 
> It's unsafe and useless to migrate by auto-generated node-names, so
> let's forbid it.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
>   migration/block-dirty-bitmap.c | 7 +++++++
>   1 file changed, 7 insertions(+)
> 

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

> diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
> index e0e081ce60..f5744c35e6 100644
> --- a/migration/block-dirty-bitmap.c
> +++ b/migration/block-dirty-bitmap.c
> @@ -285,6 +285,13 @@ static int add_bitmaps_to_list(BlockDriverState *bs, const char *bs_name)
>           return -1;
>       }
>   
> +    if (bs_name[0] == '#') {
> +        error_report("Found bitmap '%s' in a node with auto-generated "
> +                     "name: %s. It can't be migrated",

Maybe compress to:

"Bitmap '%s' in auto-generated node '%s' can't be migrated"

> +                     bdrv_dirty_bitmap_name(bitmap), bs_name);
> +        return -1;
> +    }
> +
>       FOR_EACH_DIRTY_BITMAP(bs, bitmap) {
>           if (!bdrv_dirty_bitmap_name(bitmap)) {
>               continue;
> 

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

Re: [PATCH v3 6/7] migration/block-dirty-bitmap: add_bitmaps_to_list: check disk name once

[Eric Blake]

On 5/15/20 7:40 AM, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
>   migration/block-dirty-bitmap.c | 17 +++++++++++------
>   1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
> index 5d3a7d2b07..e0e081ce60 100644
> --- a/migration/block-dirty-bitmap.c
> +++ b/migration/block-dirty-bitmap.c
> @@ -274,17 +274,22 @@ static int add_bitmaps_to_list(BlockDriverState *bs, const char *bs_name)
>       DirtyBitmapMigBitmapState *dbms;
>       Error *local_err = NULL;
>   
> +    bitmap = bdrv_dirty_bitmap_first(bs);
> +    if (!bitmap) {
> +        return 0;
> +    }
> +
> +    if (!bs_name || strcmp(bs_name, "") == 0) {
> +        error_report("Found bitmap '%s' in unnamed node %p. It can't "
> +                     "be migrated", bdrv_dirty_bitmap_name(bitmap), bs);

The %p is unusual; it does not help the end user, but only a developer 
with gdb access.

Maybe we could compress to:

"Bitmap '%s' in unnamed node can't be migrated"

> +        return -1;
> +    }
> +
>       FOR_EACH_DIRTY_BITMAP(bs, bitmap) {
>           if (!bdrv_dirty_bitmap_name(bitmap)) {
>               continue;
>           }
>   
> -        if (!bs_name || strcmp(bs_name, "") == 0) {
> -            error_report("Found bitmap '%s' in unnamed node %p. It can't "
> -                         "be migrated", bdrv_dirty_bitmap_name(bitmap), bs);
> -            return -1;
> -        }
> -

But since this is just code motion (hoisting an check outside of a loop, 
for fewer executions of something that does not change within the loop), 
it doesn't matter whether this patch goes in as-is or if you also tweak 
the error message.

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

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

Re: [PATCH v3 5/7] iotests: 194: test also migration of dirty bitmap

[Eric Blake]

On 5/15/20 7:40 AM, Vladimir Sementsov-Ogievskiy wrote:
> Test that dirty bitmap migration works when we deal with mirror.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   tests/qemu-iotests/194     | 14 ++++++++++----
>   tests/qemu-iotests/194.out |  6 ++++++
>   2 files changed, 16 insertions(+), 4 deletions(-)
> 
Definitely needed ;)

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

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