[PATCH v4 0/6] fix migration with bitmaps and mirror

[PATCH v4 0/6] fix migration with bitmaps and mirror

[Vladimir Sementsov-Ogievskiy]

v4: (Max's patch marking filters as filters already in master)
03: careful select child of filter, avoid crash 
04: add Eric's r-b
05-06: tweak error message, keep Andrey's r-b, add Eric's r-b

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/dirty-bitmap.c           |  13 ++++
 migration/block-dirty-bitmap.c | 130 ++++++++++++++++++++++++---------
 tests/qemu-iotests/194         |  14 +++-
 tests/qemu-iotests/194.out     |   6 ++
 5 files changed, 127 insertions(+), 37 deletions(-)

-- 
2.21.0

[PATCH v4 1/6] 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 v4 2/6] 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 5a8d52e4de..36e8da4fc2 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -95,6 +95,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 f9bfc77985..c01319b188 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -818,6 +818,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 v4 3/6] 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 doesn't work
with auto-generated node names.

Let's fix it by using 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 | 45 +++++++++++++++++++++++++++++++++-
 1 file changed, 44 insertions(+), 1 deletion(-)

diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index 7e93718086..04a5525fd1 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -319,14 +319,54 @@ 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))
+        {
+            if (bs->backing) {
+                bs = bs->backing->bs;
+            } else if (bs->file) {
+                bs = bs->file->bs;
+            } else {
+                bs = NULL;
+            }
+        }
+
+        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 +380,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

Re: [PATCH v4 3/6] migration/block-dirty-bitmap: fix bitmaps pre-blockdev migration during mirror job

[Eric Blake]

On 5/21/20 5:06 PM, 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 doesn't work
> with auto-generated node names.
> 
> Let's fix it by using 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 | 45 +++++++++++++++++++++++++++++++++-
>   1 file changed, 44 insertions(+), 1 deletion(-)

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

Thanks; I'm queuing this series through my bitmaps tree, and will send a 
pull request probably on Monday.


> +
> +        /* Skip filters without bitmaos */

bitmaps - I'm touching that up

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

[PATCH v4 4/6] 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>
Reviewed-by: Eric Blake <eblake@redhat.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

Re: [PATCH v4 4/6] iotests: 194: test also migration of dirty bitmap

[Thomas Huth]

On 22/05/2020 00.06, Vladimir Sementsov-Ogievskiy wrote:
> Test that dirty bitmap migration works when we deal with mirror.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  tests/qemu-iotests/194     | 14 ++++++++++----
>  tests/qemu-iotests/194.out |  6 ++++++
>  2 files changed, 16 insertions(+), 4 deletions(-)

 Hi!

This test broke the iotest in the gitlab CI:

 https://gitlab.com/huth/qemu/-/jobs/578520599#L3780

it works again when I revert this commit.

Could the test be reworked so that it works in CI pipelines, too?
Otherwise, I think it's best if we disable it in the .gitlab-ci.yml file...

 Thomas

Re: [PATCH v4 4/6] iotests: 194: test also migration of dirty bitmap

[Vladimir Sementsov-Ogievskiy]

03.06.2020 10:52, Thomas Huth wrote:
> On 22/05/2020 00.06, Vladimir Sementsov-Ogievskiy wrote:
>> Test that dirty bitmap migration works when we deal with mirror.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> ---
>>   tests/qemu-iotests/194     | 14 ++++++++++----
>>   tests/qemu-iotests/194.out |  6 ++++++
>>   2 files changed, 16 insertions(+), 4 deletions(-)
> 
>   Hi!
> 
> This test broke the iotest in the gitlab CI:
> 
>   https://gitlab.com/huth/qemu/-/jobs/578520599#L3780
> 
> it works again when I revert this commit.
> 
> Could the test be reworked so that it works in CI pipelines, too?
> Otherwise, I think it's best if we disable it in the .gitlab-ci.yml file...
> 
>   Thomas
> 

Hi!

from the link you provided:

...

../configure --cc=clang --enable-werror --disable-tcg --audio-drv-list=""

...


194      fail       [06:31:15] [06:31:16]                    output mismatch (see 194.out.bad)
--- /builds/huth/qemu/tests/qemu-iotests/194.out	2020-06-03 06:18:10.000000000 +0000
+++ /builds/huth/qemu/build/tests/qemu-iotests/194.out.bad	2020-06-03 06:31:16.000000000 +0000
@@ -22,3 +22,4 @@
  Stopping the NBD server on destination...
  {"return": {}}
  [{"busy": false, "count": 0, "granularity": 65536, "name": "bitmap0", "persistent": false, "recording": true, "status": "active"}]
+WARNING:qemu.machine:qemu received signal 6: /builds/huth/qemu/build/tests/qemu-iotests/../../x86_64-softmmu/qemu-system-x86_64 -display none -vga none -chardev socket,id=mon,path=/tmp/tmp.e9BvOjVl1W/qemudest-15756-monitor.sock -mon chardev=mon,mode=control -qtest unix:path=/tmp/tmp.e9BvOjVl1W/qemudest-15756-qtest.sock -accel qtest -nodefaults -display none -accel qtest -drive if=virtio,id=drive0,file=/builds/huth/qemu/build/tests/qemu-iotests/scratch/15756-dest.img,format=raw,cache=writeback,aio=threads -incoming unix:/tmp/tmp.e9BvOjVl1W/15756-migration.sock



- Qemu aborted. Not good. Definitely is better to fix it than just exclude the test.. I can't reproduce. Could you provide backtrace from coredump?


-- 
Best regards,
Vladimir

Re: [PATCH v4 4/6] iotests: 194: test also migration of dirty bitmap

[Thomas Huth]

On 03/06/2020 10.06, Vladimir Sementsov-Ogievskiy wrote:
> 03.06.2020 10:52, Thomas Huth wrote:
>> On 22/05/2020 00.06, Vladimir Sementsov-Ogievskiy wrote:
>>> Test that dirty bitmap migration works when we deal with mirror.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>> ---
>>>   tests/qemu-iotests/194     | 14 ++++++++++----
>>>   tests/qemu-iotests/194.out |  6 ++++++
>>>   2 files changed, 16 insertions(+), 4 deletions(-)
>>
>>   Hi!
>>
>> This test broke the iotest in the gitlab CI:
>>
>>   https://gitlab.com/huth/qemu/-/jobs/578520599#L3780
>>
>> it works again when I revert this commit.
>>
>> Could the test be reworked so that it works in CI pipelines, too?
>> Otherwise, I think it's best if we disable it in the .gitlab-ci.yml
>> file...
[...]
> - Qemu aborted. Not good. Definitely is better to fix it than just
> exclude the test.. I can't reproduce. Could you provide backtrace from
> coredump?

It aborted in block/dirty-bitmap.c, line 295, that's the
"assert(!bdrv_dirty_bitmap_busy(bitmap));" if I got it right.

Full backtrace here:

 https://gitlab.com/huth/qemu/-/jobs/580553686#L3638

HTH,
 Thomas

Re: [PATCH v4 4/6] iotests: 194: test also migration of dirty bitmap

[Vladimir Sementsov-Ogievskiy]

04.06.2020 10:21, Thomas Huth wrote:
> On 03/06/2020 10.06, Vladimir Sementsov-Ogievskiy wrote:
>> 03.06.2020 10:52, Thomas Huth wrote:
>>> On 22/05/2020 00.06, Vladimir Sementsov-Ogievskiy wrote:
>>>> Test that dirty bitmap migration works when we deal with mirror.
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>>> ---
>>>>    tests/qemu-iotests/194     | 14 ++++++++++----
>>>>    tests/qemu-iotests/194.out |  6 ++++++
>>>>    2 files changed, 16 insertions(+), 4 deletions(-)
>>>
>>>    Hi!
>>>
>>> This test broke the iotest in the gitlab CI:
>>>
>>>    https://gitlab.com/huth/qemu/-/jobs/578520599#L3780
>>>
>>> it works again when I revert this commit.
>>>
>>> Could the test be reworked so that it works in CI pipelines, too?
>>> Otherwise, I think it's best if we disable it in the .gitlab-ci.yml
>>> file...
> [...]
>> - Qemu aborted. Not good. Definitely is better to fix it than just
>> exclude the test.. I can't reproduce. Could you provide backtrace from
>> coredump?
> 
> It aborted in block/dirty-bitmap.c, line 295, that's the
> "assert(!bdrv_dirty_bitmap_busy(bitmap));" if I got it right.
> 
> Full backtrace here:
> 
>   https://gitlab.com/huth/qemu/-/jobs/580553686#L3638
> 

Aha, missed it, thanks.

Hm. in 194 iotest we wait for migration finish on source, but not on target. I can assume, that in your setup target shutdown occurs earlier than migration finish, so we have busy bitmap on shutdown.
It's known bug (at least for me :), patch is in list:
[PATCH v2 00/22] Fix error handling during bitmap postcopy
   [..]
   [PATCH v2 10/22] migration/block-dirty-bitmap: cancel migration on shutdown  (https://lists.gnu.org/archive/html/qemu-devel/2020-02/msg04288.html)
      If target is turned of prior to postcopy finished, target crashes
      because busy bitmaps are found at shutdown.
      Canceling incoming migration helps, as it removes all unfinished (and
      therefore busy) bitmaps.
   [..]


Still, of course iotest 194 should be fixed too, to wait for migration finish on target too, I'll send a patch.

-- 
Best regards,
Vladimir

Re: [PATCH v4 4/6] iotests: 194: test also migration of dirty bitmap

[Thomas Huth]

On 04/06/2020 09.51, Vladimir Sementsov-Ogievskiy wrote:
> 04.06.2020 10:21, Thomas Huth wrote:
>> On 03/06/2020 10.06, Vladimir Sementsov-Ogievskiy wrote:
>>> 03.06.2020 10:52, Thomas Huth wrote:
>>>> On 22/05/2020 00.06, Vladimir Sementsov-Ogievskiy wrote:
>>>>> Test that dirty bitmap migration works when we deal with mirror.
>>>>>
>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>>>> ---
>>>>>    tests/qemu-iotests/194     | 14 ++++++++++----
>>>>>    tests/qemu-iotests/194.out |  6 ++++++
>>>>>    2 files changed, 16 insertions(+), 4 deletions(-)
>>>>
>>>>    Hi!
>>>>
>>>> This test broke the iotest in the gitlab CI:
>>>>
>>>>    https://gitlab.com/huth/qemu/-/jobs/578520599#L3780
>>>>
>>>> it works again when I revert this commit.
>>>>
>>>> Could the test be reworked so that it works in CI pipelines, too?
>>>> Otherwise, I think it's best if we disable it in the .gitlab-ci.yml
>>>> file...
>> [...]
>>> - Qemu aborted. Not good. Definitely is better to fix it than just
>>> exclude the test.. I can't reproduce. Could you provide backtrace from
>>> coredump?
>>
>> It aborted in block/dirty-bitmap.c, line 295, that's the
>> "assert(!bdrv_dirty_bitmap_busy(bitmap));" if I got it right.
>>
>> Full backtrace here:
>>
>>   https://gitlab.com/huth/qemu/-/jobs/580553686#L3638
>>
> 
> Aha, missed it, thanks.

No, you didn't miss it. That was a new run where I added some more
debugging magic to the gitlab-ci.yml file:

build-tcg-disabled:
 [...]
 - make -j"$JOBS"
 - sysctl -w kernel.core_pattern=/tmp/core
 - ulimit -c
 - cd tests/qemu-iotests/
 - ./check -raw 194 || find /tmp -name "*core*"
 - gdb -ex bt ../../x86_64-softmmu/qemu-system-x86_64 /tmp/core*

> Hm. in 194 iotest we wait for migration finish on source, but not on
> target. I can assume, that in your setup target shutdown occurs earlier
> than migration finish, so we have busy bitmap on shutdown.

Sounds plausible. Especially if you consider that these gitlab CI
containers only run with one virtual CPU, so there is likely more delay
here.

> It's known bug (at least for me :), patch is in list:
> [PATCH v2 00/22] Fix error handling during bitmap postcopy
>   [..]
>   [PATCH v2 10/22] migration/block-dirty-bitmap: cancel migration on
> shutdown 
> (https://lists.gnu.org/archive/html/qemu-devel/2020-02/msg04288.html)
>      If target is turned of prior to postcopy finished, target crashes
>      because busy bitmaps are found at shutdown.
>      Canceling incoming migration helps, as it removes all unfinished (and
>      therefore busy) bitmaps.
>   [..]
> 
> 
> Still, of course iotest 194 should be fixed too, to wait for migration
> finish on target too, I'll send a patch.

Thanks, I'll give it a try!

 Thomas

[PATCH v4 5/6] 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>
Reviewed-by: Eric Blake <eblake@redhat.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 04a5525fd1..65f2948f07 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("Bitmap '%s' in unnamed node can't be migrated",
+                     bdrv_dirty_bitmap_name(bitmap));
+        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 v4 6/6] 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>
Reviewed-by: Eric Blake <eblake@redhat.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 65f2948f07..71528581e4 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("Bitmap '%s' in a node with auto-generated "
+                     "name '%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;
-- 
2.21.0

Re: [PATCH v4 0/6] fix migration with bitmaps and mirror

[Eric Blake]

On 5/21/20 5:06 PM, Vladimir Sementsov-Ogievskiy wrote:
> v4: (Max's patch marking filters as filters already in master)
> 03: careful select child of filter, avoid crash
> 04: add Eric's r-b
> 05-06: tweak error message, keep Andrey's r-b, add Eric's r-b
> 
> 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

3 and 5 have rather long subject lines, as shown by the wrapping 
inserted by git (3 even exceeds 80 columns on its own, even before git 
adds prefixing).  Should I try to touch this up in my staging queue? 
For example:

migration: fix non-blockdev bitmap migration with mirror

doesn't lose too much information, but is definitely shorter for 3.

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

Re: [PATCH v4 0/6] fix migration with bitmaps and mirror

[Vladimir Sementsov-Ogievskiy]

22.05.2020 18:24, Eric Blake wrote:
> On 5/21/20 5:06 PM, Vladimir Sementsov-Ogievskiy wrote:
>> v4: (Max's patch marking filters as filters already in master)
>> 03: careful select child of filter, avoid crash
>> 04: add Eric's r-b
>> 05-06: tweak error message, keep Andrey's r-b, add Eric's r-b
>>
>> 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
> 
> 3 and 5 have rather long subject lines, as shown by the wrapping inserted by git (3 even exceeds 80 columns on its own, even before git adds prefixing).  Should I try to touch this up in my staging queue? For example:
> 
> migration: fix non-blockdev bitmap migration with mirror
> 
> doesn't lose too much information, but is definitely shorter for 3.
> 

No objections, of course

-- 
Best regards,
Vladimir