[PATCH v2 0/5] fix migration with bitmaps and mirror

[PATCH v2 0/5] fix migration with bitmaps and mirror

[Vladimir Sementsov-Ogievskiy]

Hi all!

It's a continuation for
"bitmap migration bug with -drive while block mirror runs"
<315cff78-dcdb-a3ce-2742-da3cc9f0ca97@redhat.com>
https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg07241.html

The problem is that bitmaps migrated to node with same node-name or
blk-parent name. And currently only the latter actually work in libvirt.
And with mirror-top filter it doesn't work, because
bdrv_get_device_or_node_name don't go through filters.

Fix this by handling filtered children of block backends in separate.

v2: rebase on current master

Max Reitz (1):
  block: Mark commit and mirror as filter drivers

Vladimir Sementsov-Ogievskiy (4):
  migretion/block-dirty-bitmap: refactor init_dirty_bitmap_migration
  block/dirty-bitmap: add bdrv_has_named_bitmaps helper
  migration/block-dirty-bitmap: fix bitmaps migration during mirror job
  iotests: 194: test also migration of dirty bitmap

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

-- 
2.21.0

[PATCH v2 1/5] block: Mark commit and mirror as filter drivers

[Vladimir Sementsov-Ogievskiy]

From: Max Reitz <mreitz@redhat.com>

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

Signed-off-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
   [squash comment fix from another Max's patch and adjust commit msg]
---
 include/block/block_int.h | 8 +++++---
 block/commit.c            | 2 ++
 block/mirror.c            | 2 ++
 3 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index dd033d0b37..964ce58481 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -89,9 +89,11 @@ struct BlockDriver {
     int instance_size;
 
     /* set to true if the BlockDriver is a block filter. Block filters pass
-     * certain callbacks that refer to data (see block.c) to their bs->file if
-     * the driver doesn't implement them. Drivers that do not wish to forward
-     * must implement them and return -ENOTSUP.
+     * certain callbacks that refer to data (see block.c) to their bs->file
+     * or bs->backing (whichever one exists) if the driver doesn't implement
+     * them. Drivers that do not wish to forward must implement them and return
+     * -ENOTSUP.
+     * Note that filters are not allowed to modify data.
      */
     bool is_filter;
     /* for snapshots block filter like Quorum can implement the
diff --git a/block/commit.c b/block/commit.c
index 23c90b3b91..9b455c4285 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -253,6 +253,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 f0f2d9dff1..8cbbe1e065 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1505,6 +1505,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 v2 2/5] migretion/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 in separate.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.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 v2 3/5] 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>
---
 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 e2b20ecab9..d93a38c7bc 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 7039e82520..9117556095 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 v2 4/5] migration/block-dirty-bitmap: fix bitmaps 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.

Libvirt currently is unprepared to bitmap migration by node-name,
node-names are mostly auto-generated. So actually only migration by blk
name works.

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.

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

Note, that we possibly want to allow explicit filters skipping too, but
this is another story.

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 v2 5/5] 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 72e47e8833..f48ddf6051 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 v2 0/5] fix migration with bitmaps and mirror

[Peter Krempa]

On Thu, Dec 19, 2019 at 11:51:01 +0300, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
> 
> It's a continuation for
> "bitmap migration bug with -drive while block mirror runs"
> <315cff78-dcdb-a3ce-2742-da3cc9f0ca97@redhat.com>
> https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg07241.html
> 
> The problem is that bitmaps migrated to node with same node-name or
> blk-parent name. And currently only the latter actually work in libvirt.
> And with mirror-top filter it doesn't work, because
> bdrv_get_device_or_node_name don't go through filters.

I want to point out that since libvirt-5.10 we use -blockdev to
configure the backend of storage devices with qemu-4.2 and later. This
means unfortunately that the BlockBackend of the drive does not have a
name any more and thus the above will not work even if you make the
lookup code to see through filters.

As I've pointed out separately node-names are not good idea to use for
matching either as they can be distinct on the destination of migration.

Having same node names for images during migration was not documented as
a requiremend and even if it was the case when the mirror job is used
the destination is a different image and thus having a different node
name is expected.

Since it's not documented, expect the same situation as with
autogenerated nodenames, the destination may have different node-names
and the same node-name may refer to a different image. Implicit matching
based on node-names is thus impossible.

Re: [PATCH v2 0/5] fix migration with bitmaps and mirror

[Vladimir Sementsov-Ogievskiy]

19.12.2019 13:36, Peter Krempa wrote:
> On Thu, Dec 19, 2019 at 11:51:01 +0300, Vladimir Sementsov-Ogievskiy wrote:
>> Hi all!
>>
>> It's a continuation for
>> "bitmap migration bug with -drive while block mirror runs"
>> <315cff78-dcdb-a3ce-2742-da3cc9f0ca97@redhat.com>
>> https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg07241.html
>>
>> The problem is that bitmaps migrated to node with same node-name or
>> blk-parent name. And currently only the latter actually work in libvirt.
>> And with mirror-top filter it doesn't work, because
>> bdrv_get_device_or_node_name don't go through filters.
> 
> I want to point out that since libvirt-5.10 we use -blockdev to
> configure the backend of storage devices with qemu-4.2 and later. This
> means unfortunately that the BlockBackend of the drive does not have a
> name any more and thus the above will not work even if you make the
> lookup code to see through filters.

Should we support qemu-4.2 and later for earlier versions of libvirt?

> 
> As I've pointed out separately node-names are not good idea to use for
> matching either as they can be distinct on the destination of migration.
> 
> Having same node names for images during migration was not documented as
> a requiremend and even if it was the case when the mirror job is used
> the destination is a different image and thus having a different node
> name is expected.
> 
> Since it's not documented, expect the same situation as with
> autogenerated nodenames, the destination may have different node-names
> and the same node-name may refer to a different image. Implicit matching
> based on node-names is thus impossible.
> 

So, it's time to implement explicit matching..

I remember we discussed a command to set this matching on source. So we call
qmp command on source, which specifies where bitmaps go on target..

Is it OK?

Or, is it better to do it symmetrically, calling on source command, which
just binds some migration-ids to the bitmaps. And same command on target, to
bind these ids to bitmaps on target..

Hmmm.

I think better, to still set matching not to ids but to {node_name, bitmap_name}
pair, but allow do it either on source or on target (or both), which will allow
to migrate from old qemu version without such command to new qemu version which
supports it.


-- 
Best regards,
Vladimir

Re: [PATCH v2 0/5] fix migration with bitmaps and mirror

[Vladimir Sementsov-Ogievskiy]

John, I don't quite follow discussion in bugzilla. Do we need these series
as at least temporary workaround, or not? Should I resend?


19.12.2019 11:51, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
> 
> It's a continuation for
> "bitmap migration bug with -drive while block mirror runs"
> <315cff78-dcdb-a3ce-2742-da3cc9f0ca97@redhat.com>
> https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg07241.html
> 
> The problem is that bitmaps migrated to node with same node-name or
> blk-parent name. And currently only the latter actually work in libvirt.
> And with mirror-top filter it doesn't work, because
> bdrv_get_device_or_node_name don't go through filters.
> 
> Fix this by handling filtered children of block backends in separate.
> 
> v2: rebase on current master
> 
> Max Reitz (1):
>    block: Mark commit and mirror as filter drivers
> 
> Vladimir Sementsov-Ogievskiy (4):
>    migretion/block-dirty-bitmap: refactor init_dirty_bitmap_migration
>    block/dirty-bitmap: add bdrv_has_named_bitmaps helper
>    migration/block-dirty-bitmap: fix bitmaps migration during mirror job
>    iotests: 194: test also migration of dirty bitmap
> 
>   include/block/block_int.h      |   8 ++-
>   include/block/dirty-bitmap.h   |   1 +
>   block/commit.c                 |   2 +
>   block/dirty-bitmap.c           |  13 ++++
>   block/mirror.c                 |   2 +
>   migration/block-dirty-bitmap.c | 114 +++++++++++++++++++++++----------
>   tests/qemu-iotests/194         |  14 ++--
>   tests/qemu-iotests/194.out     |   6 ++
>   8 files changed, 119 insertions(+), 41 deletions(-)
> 


-- 
Best regards,
Vladimir

Re: [PATCH v2 1/5] block: Mark commit and mirror as filter drivers

[Eric Blake]

On 12/19/19 2:51 AM, Vladimir Sementsov-Ogievskiy wrote:
> From: Max Reitz <mreitz@redhat.com>
> 
> The commit and mirror block nodes are filters, so they should be marked
> as such.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>     [squash comment fix from another Max's patch and adjust commit msg]
> ---
>   include/block/block_int.h | 8 +++++---
>   block/commit.c            | 2 ++
>   block/mirror.c            | 2 ++
>   3 files changed, 9 insertions(+), 3 deletions(-)

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 v2 2/5] migretion/block-dirty-bitmap: refactor init_dirty_bitmap_migration

[Eric Blake]

On 12/19/19 2:51 AM, Vladimir Sementsov-Ogievskiy wrote:

In the subject: s/migretion/migration/

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

s/in separate/separately/

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

> +++ b/migration/block-dirty-bitmap.c
> @@ -268,57 +268,66 @@ static void dirty_bitmap_mig_cleanup(void)

>   
> -    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);

error_report() generally avoids multiple sentences.

> +            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);

But as this was just code motion,

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 v2 0/5] fix migration with bitmaps and mirror

[Vladimir Sementsov-Ogievskiy]

19.12.2019 13:36, Peter Krempa wrote:
> On Thu, Dec 19, 2019 at 11:51:01 +0300, Vladimir Sementsov-Ogievskiy wrote:
>> Hi all!
>>
>> It's a continuation for
>> "bitmap migration bug with -drive while block mirror runs"
>> <315cff78-dcdb-a3ce-2742-da3cc9f0ca97@redhat.com>
>> https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg07241.html
>>
>> The problem is that bitmaps migrated to node with same node-name or
>> blk-parent name. And currently only the latter actually work in libvirt.
>> And with mirror-top filter it doesn't work, because
>> bdrv_get_device_or_node_name don't go through filters.
> 
> I want to point out that since libvirt-5.10 we use -blockdev to
> configure the backend of storage devices with qemu-4.2 and later. This
> means unfortunately that the BlockBackend of the drive does not have a
> name any more and thus the above will not work even if you make the
> lookup code to see through filters.

Not that this series doesn't make things worse, as it loops through named
block backends when trying to use their name for migration. So with these
patches applied, qemu will just work in more possible scenarios.

> 
> As I've pointed out separately node-names are not good idea to use for
> matching either as they can be distinct on the destination of migration.
> 
> Having same node names for images during migration was not documented as
> a requiremend and even if it was the case when the mirror job is used
> the destination is a different image and thus having a different node
> name is expected.
> 
> Since it's not documented, expect the same situation as with
> autogenerated nodenames, the destination may have different node-names
> and the same node-name may refer to a different image. Implicit matching
> based on node-names is thus impossible.
> 


-- 
Best regards,
Vladimir

Re: [PATCH v2 0/5] fix migration with bitmaps and mirror

[Peter Krempa]

On Fri, Apr 03, 2020 at 14:02:47 +0300, Vladimir Sementsov-Ogievskiy wrote:
> 19.12.2019 13:36, Peter Krempa wrote:
> > On Thu, Dec 19, 2019 at 11:51:01 +0300, Vladimir Sementsov-Ogievskiy wrote:
> > > Hi all!
> > > 
> > > It's a continuation for
> > > "bitmap migration bug with -drive while block mirror runs"
> > > <315cff78-dcdb-a3ce-2742-da3cc9f0ca97@redhat.com>
> > > https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg07241.html
> > > 
> > > The problem is that bitmaps migrated to node with same node-name or
> > > blk-parent name. And currently only the latter actually work in libvirt.
> > > And with mirror-top filter it doesn't work, because
> > > bdrv_get_device_or_node_name don't go through filters.
> > 
> > I want to point out that since libvirt-5.10 we use -blockdev to
> > configure the backend of storage devices with qemu-4.2 and later. This
> > means unfortunately that the BlockBackend of the drive does not have a
> > name any more and thus the above will not work even if you make the
> > lookup code to see through filters.
> 
> Not that this series doesn't make things worse, as it loops through named
> block backends when trying to use their name for migration. So with these
> patches applied, qemu will just work in more possible scenarios.

Okay, if that's so it's fair enough in this case.

I'm just very firmly against baking in the assumption that
node names mean the same thing accross migration, because that will
create a precedent situation and more stuff may be baked in on top of
this in the future. It seems that it has already happened though and
it's wrong. And the worst part is that it's never mentioned that this
might occur. But again, don't do that and preferrably remove the
matching of node names for bitmaps altogether until we can control it
arbitrarily.

We've also seen this already before with the backend name of memory
devices being baked in to the migration stream which creates an unwanted
dependancy.

Re: [PATCH v2 0/5] fix migration with bitmaps and mirror

[Vladimir Sementsov-Ogievskiy]

03.04.2020 14:23, Peter Krempa wrote:
> On Fri, Apr 03, 2020 at 14:02:47 +0300, Vladimir Sementsov-Ogievskiy wrote:
>> 19.12.2019 13:36, Peter Krempa wrote:
>>> On Thu, Dec 19, 2019 at 11:51:01 +0300, Vladimir Sementsov-Ogievskiy wrote:
>>>> Hi all!
>>>>
>>>> It's a continuation for
>>>> "bitmap migration bug with -drive while block mirror runs"
>>>> <315cff78-dcdb-a3ce-2742-da3cc9f0ca97@redhat.com>
>>>> https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg07241.html
>>>>
>>>> The problem is that bitmaps migrated to node with same node-name or
>>>> blk-parent name. And currently only the latter actually work in libvirt.
>>>> And with mirror-top filter it doesn't work, because
>>>> bdrv_get_device_or_node_name don't go through filters.
>>>
>>> I want to point out that since libvirt-5.10 we use -blockdev to
>>> configure the backend of storage devices with qemu-4.2 and later. This
>>> means unfortunately that the BlockBackend of the drive does not have a
>>> name any more and thus the above will not work even if you make the
>>> lookup code to see through filters.
>>
>> Not that this series doesn't make things worse, as it loops through named
>> block backends when trying to use their name for migration. So with these
>> patches applied, qemu will just work in more possible scenarios.
> 
> Okay, if that's so it's fair enough in this case.
> 
> I'm just very firmly against baking in the assumption that
> node names mean the same thing accross migration, because that will
> create a precedent situation and more stuff may be baked in on top of
> this in the future. It seems that it has already happened though and
> it's wrong. And the worst part is that it's never mentioned that this
> might occur. But again, don't do that and preferrably remove the
> matching of node names for bitmaps altogether until we can control it
> arbitrarily.
> 
> We've also seen this already before with the backend name of memory
> devices being baked in to the migration stream which creates an unwanted
> dependancy.
> 

Hmm. Actually, matching by node-name never worked. May be just drop it now, and allow only matching by blk-name?

And then (in 5.1) implement special qmp commands for precise mapping.

-- 
Best regards,
Vladimir

Re: [PATCH v2 0/5] fix migration with bitmaps and mirror

[Vladimir Sementsov-Ogievskiy]

03.04.2020 14:29, Vladimir Sementsov-Ogievskiy wrote:
> 03.04.2020 14:23, Peter Krempa wrote:
>> On Fri, Apr 03, 2020 at 14:02:47 +0300, Vladimir Sementsov-Ogievskiy wrote:
>>> 19.12.2019 13:36, Peter Krempa wrote:
>>>> On Thu, Dec 19, 2019 at 11:51:01 +0300, Vladimir Sementsov-Ogievskiy wrote:
>>>>> Hi all!
>>>>>
>>>>> It's a continuation for
>>>>> "bitmap migration bug with -drive while block mirror runs"
>>>>> <315cff78-dcdb-a3ce-2742-da3cc9f0ca97@redhat.com>
>>>>> https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg07241.html
>>>>>
>>>>> The problem is that bitmaps migrated to node with same node-name or
>>>>> blk-parent name. And currently only the latter actually work in libvirt.
>>>>> And with mirror-top filter it doesn't work, because
>>>>> bdrv_get_device_or_node_name don't go through filters.
>>>>
>>>> I want to point out that since libvirt-5.10 we use -blockdev to
>>>> configure the backend of storage devices with qemu-4.2 and later. This
>>>> means unfortunately that the BlockBackend of the drive does not have a
>>>> name any more and thus the above will not work even if you make the
>>>> lookup code to see through filters.
>>>
>>> Not that this series doesn't make things worse, as it loops through named
>>> block backends when trying to use their name for migration. So with these
>>> patches applied, qemu will just work in more possible scenarios.
>>
>> Okay, if that's so it's fair enough in this case.
>>
>> I'm just very firmly against baking in the assumption that
>> node names mean the same thing accross migration, because that will
>> create a precedent situation and more stuff may be baked in on top of
>> this in the future. It seems that it has already happened though and
>> it's wrong. And the worst part is that it's never mentioned that this
>> might occur. But again, don't do that and preferrably remove the
>> matching of node names for bitmaps altogether until we can control it
>> arbitrarily.
>>
>> We've also seen this already before with the backend name of memory
>> devices being baked in to the migration stream which creates an unwanted
>> dependancy.
>>
> 
> Hmm. Actually, matching by node-name never worked. May be just drop it now, and allow only matching by blk-name?
> 
> And then (in 5.1) implement special qmp commands for precise mapping.
> 

Hmm, it may break someones setup... Bad idea. Probably we can forbid auto-generated node-names.

-- 
Best regards,
Vladimir

Re: [PATCH v2 0/5] fix migration with bitmaps and mirror

[Dr. David Alan Gilbert]

* Vladimir Sementsov-Ogievskiy (vsementsov@virtuozzo.com) wrote:
> 03.04.2020 14:29, Vladimir Sementsov-Ogievskiy wrote:
> > 03.04.2020 14:23, Peter Krempa wrote:
> > > On Fri, Apr 03, 2020 at 14:02:47 +0300, Vladimir Sementsov-Ogievskiy wrote:
> > > > 19.12.2019 13:36, Peter Krempa wrote:
> > > > > On Thu, Dec 19, 2019 at 11:51:01 +0300, Vladimir Sementsov-Ogievskiy wrote:
> > > > > > Hi all!
> > > > > > 
> > > > > > It's a continuation for
> > > > > > "bitmap migration bug with -drive while block mirror runs"
> > > > > > <315cff78-dcdb-a3ce-2742-da3cc9f0ca97@redhat.com>
> > > > > > https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg07241.html
> > > > > > 
> > > > > > The problem is that bitmaps migrated to node with same node-name or
> > > > > > blk-parent name. And currently only the latter actually work in libvirt.
> > > > > > And with mirror-top filter it doesn't work, because
> > > > > > bdrv_get_device_or_node_name don't go through filters.
> > > > > 
> > > > > I want to point out that since libvirt-5.10 we use -blockdev to
> > > > > configure the backend of storage devices with qemu-4.2 and later. This
> > > > > means unfortunately that the BlockBackend of the drive does not have a
> > > > > name any more and thus the above will not work even if you make the
> > > > > lookup code to see through filters.
> > > > 
> > > > Not that this series doesn't make things worse, as it loops through named
> > > > block backends when trying to use their name for migration. So with these
> > > > patches applied, qemu will just work in more possible scenarios.
> > > 
> > > Okay, if that's so it's fair enough in this case.
> > > 
> > > I'm just very firmly against baking in the assumption that
> > > node names mean the same thing accross migration, because that will
> > > create a precedent situation and more stuff may be baked in on top of
> > > this in the future. It seems that it has already happened though and
> > > it's wrong. And the worst part is that it's never mentioned that this
> > > might occur. But again, don't do that and preferrably remove the
> > > matching of node names for bitmaps altogether until we can control it
> > > arbitrarily.
> > > 
> > > We've also seen this already before with the backend name of memory
> > > devices being baked in to the migration stream which creates an unwanted
> > > dependancy.
> > > 
> > 
> > Hmm. Actually, matching by node-name never worked. May be just drop it now, and allow only matching by blk-name?
> > 
> > And then (in 5.1) implement special qmp commands for precise mapping.
> > 
> 
> Hmm, it may break someones setup... Bad idea. Probably we can forbid auto-generated node-names.

If we want to remove it I guess we have to go through a proper
deprecation; but that's OK.

The thing to keep in mind is that when people say 'the commandline
should match' on source/destination - that's just not true;
so we have to define what actually needs to stay the same for bitmap
migration to work.

Dave

> -- 
> Best regards,
> Vladimir
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH v2 0/5] fix migration with bitmaps and mirror

[Vladimir Sementsov-Ogievskiy]

03.04.2020 18:05, Dr. David Alan Gilbert wrote:
> * Vladimir Sementsov-Ogievskiy (vsementsov@virtuozzo.com) wrote:
>> 03.04.2020 14:29, Vladimir Sementsov-Ogievskiy wrote:
>>> 03.04.2020 14:23, Peter Krempa wrote:
>>>> On Fri, Apr 03, 2020 at 14:02:47 +0300, Vladimir Sementsov-Ogievskiy wrote:
>>>>> 19.12.2019 13:36, Peter Krempa wrote:
>>>>>> On Thu, Dec 19, 2019 at 11:51:01 +0300, Vladimir Sementsov-Ogievskiy wrote:
>>>>>>> Hi all!
>>>>>>>
>>>>>>> It's a continuation for
>>>>>>> "bitmap migration bug with -drive while block mirror runs"
>>>>>>> <315cff78-dcdb-a3ce-2742-da3cc9f0ca97@redhat.com>
>>>>>>> https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg07241.html
>>>>>>>
>>>>>>> The problem is that bitmaps migrated to node with same node-name or
>>>>>>> blk-parent name. And currently only the latter actually work in libvirt.
>>>>>>> And with mirror-top filter it doesn't work, because
>>>>>>> bdrv_get_device_or_node_name don't go through filters.
>>>>>>
>>>>>> I want to point out that since libvirt-5.10 we use -blockdev to
>>>>>> configure the backend of storage devices with qemu-4.2 and later. This
>>>>>> means unfortunately that the BlockBackend of the drive does not have a
>>>>>> name any more and thus the above will not work even if you make the
>>>>>> lookup code to see through filters.
>>>>>
>>>>> Not that this series doesn't make things worse, as it loops through named
>>>>> block backends when trying to use their name for migration. So with these
>>>>> patches applied, qemu will just work in more possible scenarios.
>>>>
>>>> Okay, if that's so it's fair enough in this case.
>>>>
>>>> I'm just very firmly against baking in the assumption that
>>>> node names mean the same thing accross migration, because that will
>>>> create a precedent situation and more stuff may be baked in on top of
>>>> this in the future. It seems that it has already happened though and
>>>> it's wrong. And the worst part is that it's never mentioned that this
>>>> might occur. But again, don't do that and preferrably remove the
>>>> matching of node names for bitmaps altogether until we can control it
>>>> arbitrarily.
>>>>
>>>> We've also seen this already before with the backend name of memory
>>>> devices being baked in to the migration stream which creates an unwanted
>>>> dependancy.
>>>>
>>>
>>> Hmm. Actually, matching by node-name never worked. May be just drop it now, and allow only matching by blk-name?
>>>
>>> And then (in 5.1) implement special qmp commands for precise mapping.
>>>
>>
>> Hmm, it may break someones setup... Bad idea. Probably we can forbid auto-generated node-names.
> 
> If we want to remove it I guess we have to go through a proper
> deprecation; but that's OK.
> 
> The thing to keep in mind is that when people say 'the commandline
> should match' on source/destination - that's just not true;
> so we have to define what actually needs to stay the same for bitmap
> migration to work.

Hmm. Let's add two qmp commands

1. migrate-set-outgoing-bitmap-mapping, which can set mapping (node-name, bitmap-name) -> (migration-node-name, migration-bitmap-name)
2. migrate-set-incoming-bitmap-mapping, which can set mapping (migration-node-name, migration-bitmap-name) -> (node-name, bitmap-name)

So, if we want to migrate bitmap B1 from node N1 on source to node M1 on target, we'll have three possibilities:

1. call on source migrate-set-outgoing-bitmap-mapping, to set mapping (N1, B1) -> (M1, B1) (and target will use 'M1' from migration stream to search the node)

2. call on destination migrate-set-incoming-bitmap-mapping, to set mapping (N1, B1) -> (M1, B1) (source will just put 'N1' to migration stream, and target will made transformation)

or even
3. Set mapping both on source and target, to make migration stream abstract, for example, (N1, B1) -> ('_abstract_bitmap_migration_node_', <bitmap-id>) -> (M1, B1)

Using 1 and 2, it is possible to make any migration to/from older Qemu version..

And what should be deprecated is dirty-bitmaps migration capability, which is associated with old behavior. So, newer libvirt will call set-mapping commands both on source and target, instead of enabling capability.


-- 
Best regards,
Vladimir

Re: [PATCH v2 0/5] fix migration with bitmaps and mirror

[Eric Blake]

reviving this thread...

On 4/3/20 6:23 AM, Peter Krempa wrote:
> On Fri, Apr 03, 2020 at 14:02:47 +0300, Vladimir Sementsov-Ogievskiy wrote:
>> 19.12.2019 13:36, Peter Krempa wrote:
>>> On Thu, Dec 19, 2019 at 11:51:01 +0300, Vladimir Sementsov-Ogievskiy wrote:
>>>> Hi all!
>>>>
>>>> It's a continuation for
>>>> "bitmap migration bug with -drive while block mirror runs"
>>>> <315cff78-dcdb-a3ce-2742-da3cc9f0ca97@redhat.com>
>>>> https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg07241.html
>>>>
>>>> The problem is that bitmaps migrated to node with same node-name or
>>>> blk-parent name. And currently only the latter actually work in libvirt.
>>>> And with mirror-top filter it doesn't work, because
>>>> bdrv_get_device_or_node_name don't go through filters.
>>>
>>> I want to point out that since libvirt-5.10 we use -blockdev to
>>> configure the backend of storage devices with qemu-4.2 and later. This
>>> means unfortunately that the BlockBackend of the drive does not have a
>>> name any more and thus the above will not work even if you make the
>>> lookup code to see through filters.
>>
>> Not that this series doesn't make things worse, as it loops through named
>> block backends when trying to use their name for migration. So with these
>> patches applied, qemu will just work in more possible scenarios.
> 
> Okay, if that's so it's fair enough in this case.
> 
> I'm just very firmly against baking in the assumption that
> node names mean the same thing accross migration, because that will
> create a precedent situation and more stuff may be baked in on top of
> this in the future. It seems that it has already happened though and
> it's wrong. And the worst part is that it's never mentioned that this
> might occur. But again, don't do that and preferrably remove the
> matching of node names for bitmaps altogether until we can control it
> arbitrarily.
> 
> We've also seen this already before with the backend name of memory
> devices being baked in to the migration stream which creates an unwanted
> dependancy.

Max is trying to tackle the node-name issue:
https://lists.gnu.org/archive/html/qemu-devel/2020-05/msg03358.html

And trying to apply that patch after staging this series hits a conflict 
in mnigration/block-dirty-bitmap.c.  Which one should go in first?

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

Re: [PATCH v2 3/5] block/dirty-bitmap: add bdrv_has_named_bitmaps helper

[Eric Blake]

On 12/19/19 2:51 AM, Vladimir Sementsov-Ogievskiy wrote:
> To be used for bitmap migration in further commit.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   include/block/dirty-bitmap.h |  1 +
>   block/dirty-bitmap.c         | 13 +++++++++++++
>   2 files changed, 14 insertions(+)
> 

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 v2 1/5] block: Mark commit and mirror as filter drivers

[Eric Blake]

On 1/31/20 1:23 PM, Eric Blake wrote:
> On 12/19/19 2:51 AM, Vladimir Sementsov-Ogievskiy wrote:
>> From: Max Reitz <mreitz@redhat.com>
>>
>> The commit and mirror block nodes are filters, so they should be marked
>> as such.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>     [squash comment fix from another Max's patch and adjust commit msg]
>> ---
>>   include/block/block_int.h | 8 +++++---
>>   block/commit.c            | 2 ++
>>   block/mirror.c            | 2 ++
>>   3 files changed, 9 insertions(+), 3 deletions(-)
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>

It looks like this patch has been updated and is now on Kevin's block 
branch:
https://lists.gnu.org/archive/html/qemu-devel/2020-05/msg03271.html


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

Re: [PATCH v2 4/5] migration/block-dirty-bitmap: fix bitmaps migration during mirror job

[Eric Blake]

On 12/19/19 2:51 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.
> 
> Libvirt currently is unprepared to bitmap migration by node-name,
> node-names are mostly auto-generated. So actually only migration by blk
> name works.

It depends on whether -blockdev is in use.  With -blockdev, libvirt 
should be supplying a node name everywhere, without, it is only device 
names.

> 
> 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.

Does Max' work to improve seeing through filters fix this?

> 
> Let's fix it by allowing use blk-name even if some implicit filters are
> inserted.
> 
> Note, that we possibly want to allow explicit filters skipping too, but
> this is another story.
> 
> 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

That bug has been marked CLOSED in the meantime, but it appears to be 
only because libvirt is now using -blockdev rather than the older drive, 
while the problem affects drive usage.

> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   migration/block-dirty-bitmap.c | 39 +++++++++++++++++++++++++++++++++-
>   1 file changed, 38 insertions(+), 1 deletion(-)
> 
> 

Okay, after reading some more history on this project (the curse of 
coming up to speed after volunteering to become a co-maintainer), it 
looks like Max's idea replaces this patch altogether.  How much of the 
rest of the series is still important?

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

Re: [PATCH v2 0/5] fix migration with bitmaps and mirror

[Vladimir Sementsov-Ogievskiy]

14.05.2020 21:29, Eric Blake wrote:
> reviving this thread...
> 
> On 4/3/20 6:23 AM, Peter Krempa wrote:
>> On Fri, Apr 03, 2020 at 14:02:47 +0300, Vladimir Sementsov-Ogievskiy wrote:
>>> 19.12.2019 13:36, Peter Krempa wrote:
>>>> On Thu, Dec 19, 2019 at 11:51:01 +0300, Vladimir Sementsov-Ogievskiy wrote:
>>>>> Hi all!
>>>>>
>>>>> It's a continuation for
>>>>> "bitmap migration bug with -drive while block mirror runs"
>>>>> <315cff78-dcdb-a3ce-2742-da3cc9f0ca97@redhat.com>
>>>>> https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg07241.html
>>>>>
>>>>> The problem is that bitmaps migrated to node with same node-name or
>>>>> blk-parent name. And currently only the latter actually work in libvirt.
>>>>> And with mirror-top filter it doesn't work, because
>>>>> bdrv_get_device_or_node_name don't go through filters.
>>>>
>>>> I want to point out that since libvirt-5.10 we use -blockdev to
>>>> configure the backend of storage devices with qemu-4.2 and later. This
>>>> means unfortunately that the BlockBackend of the drive does not have a
>>>> name any more and thus the above will not work even if you make the
>>>> lookup code to see through filters.
>>>
>>> Not that this series doesn't make things worse, as it loops through named
>>> block backends when trying to use their name for migration. So with these
>>> patches applied, qemu will just work in more possible scenarios.
>>
>> Okay, if that's so it's fair enough in this case.
>>
>> I'm just very firmly against baking in the assumption that
>> node names mean the same thing accross migration, because that will
>> create a precedent situation and more stuff may be baked in on top of
>> this in the future. It seems that it has already happened though and
>> it's wrong. And the worst part is that it's never mentioned that this
>> might occur. But again, don't do that and preferrably remove the
>> matching of node names for bitmaps altogether until we can control it
>> arbitrarily.
>>
>> We've also seen this already before with the backend name of memory
>> devices being baked in to the migration stream which creates an unwanted
>> dependancy.
> 
> Max is trying to tackle the node-name issue:
> https://lists.gnu.org/archive/html/qemu-devel/2020-05/msg03358.html
> 
> And trying to apply that patch after staging this series hits a conflict in mnigration/block-dirty-bitmap.c.  Which one should go in first?
> 

My patches are needed to fix migration for the pre-blockdev configuration with mirror-top filter.

We ofcrouse need them in Virtuozzo, but it's not hard to keep the in downstream-only.. And it will be not simple to use new command from Max in pre-blockdev libvirt configuration, with auto-generated node-names.

How much we care about pre-blockdev libvirt now in upstream Qemu?

If we don't care, than these series are only for downstreams, and we don't need to apply them upstream..

On the other hand, Max have to resend anyway, to handle old code, which uses device name instead of node-name. And if we don't want to drop now the code which can use device name (needed for old libvirt), why not to apply the series, which just make old code better?

====

In other words: do we still support pre-blockdev libvirt (and any other pre-blockdev users)?

If we support, than, as I said somewhere, I need to resend these series as I have updated version in our downstream. And I think, I can rebase Max's patch by myself and send together with this all, if no objections.

-- 
Best regards,
Vladimir

Re: [PATCH v2 0/5] fix migration with bitmaps and mirror

[Vladimir Sementsov-Ogievskiy]

15.05.2020 08:52, Vladimir Sementsov-Ogievskiy wrote:
> 14.05.2020 21:29, Eric Blake wrote:
>> reviving this thread...
>>
>> On 4/3/20 6:23 AM, Peter Krempa wrote:
>>> On Fri, Apr 03, 2020 at 14:02:47 +0300, Vladimir Sementsov-Ogievskiy wrote:
>>>> 19.12.2019 13:36, Peter Krempa wrote:
>>>>> On Thu, Dec 19, 2019 at 11:51:01 +0300, Vladimir Sementsov-Ogievskiy wrote:
>>>>>> Hi all!
>>>>>>
>>>>>> It's a continuation for
>>>>>> "bitmap migration bug with -drive while block mirror runs"
>>>>>> <315cff78-dcdb-a3ce-2742-da3cc9f0ca97@redhat.com>
>>>>>> https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg07241.html
>>>>>>
>>>>>> The problem is that bitmaps migrated to node with same node-name or
>>>>>> blk-parent name. And currently only the latter actually work in libvirt.
>>>>>> And with mirror-top filter it doesn't work, because
>>>>>> bdrv_get_device_or_node_name don't go through filters.
>>>>>
>>>>> I want to point out that since libvirt-5.10 we use -blockdev to
>>>>> configure the backend of storage devices with qemu-4.2 and later. This
>>>>> means unfortunately that the BlockBackend of the drive does not have a
>>>>> name any more and thus the above will not work even if you make the
>>>>> lookup code to see through filters.
>>>>
>>>> Not that this series doesn't make things worse, as it loops through named
>>>> block backends when trying to use their name for migration. So with these
>>>> patches applied, qemu will just work in more possible scenarios.
>>>
>>> Okay, if that's so it's fair enough in this case.
>>>
>>> I'm just very firmly against baking in the assumption that
>>> node names mean the same thing accross migration, because that will
>>> create a precedent situation and more stuff may be baked in on top of
>>> this in the future. It seems that it has already happened though and
>>> it's wrong. And the worst part is that it's never mentioned that this
>>> might occur. But again, don't do that and preferrably remove the
>>> matching of node names for bitmaps altogether until we can control it
>>> arbitrarily.
>>>
>>> We've also seen this already before with the backend name of memory
>>> devices being baked in to the migration stream which creates an unwanted
>>> dependancy.
>>
>> Max is trying to tackle the node-name issue:
>> https://lists.gnu.org/archive/html/qemu-devel/2020-05/msg03358.html
>>
>> And trying to apply that patch after staging this series hits a conflict in mnigration/block-dirty-bitmap.c.  Which one should go in first?
>>
> 
> My patches are needed to fix migration for the pre-blockdev configuration with mirror-top filter.
> 
> We ofcrouse need them in Virtuozzo, but it's not hard to keep the in downstream-only.. And it will be not simple to use new command from Max in pre-blockdev libvirt configuration, with auto-generated node-names.
> 
> How much we care about pre-blockdev libvirt now in upstream Qemu?
> 
> If we don't care, than these series are only for downstreams, and we don't need to apply them upstream..
> 
> On the other hand, Max have to resend anyway, to handle old code, which uses device name instead of node-name. And if we don't want to drop now the code which can use device name (needed for old libvirt), why not to apply the series, which just make old code better?
> 
> ====
> 
> In other words: do we still support pre-blockdev libvirt (and any other pre-blockdev users)?
> 
> If we support, than, as I said somewhere, I need to resend these series as I have updated version in our downstream. And I think, I can rebase Max's patch by myself and send together with this all, if no objections.
> 

I'm going to resend the series today, let's look at it.

-- 
Best regards,
Vladimir

Re: [PATCH v2 0/5] fix migration with bitmaps and mirror

[Eric Blake]

On 5/15/20 6:15 AM, Vladimir Sementsov-Ogievskiy wrote:

>>> Max is trying to tackle the node-name issue:
>>> https://lists.gnu.org/archive/html/qemu-devel/2020-05/msg03358.html
>>>
>>> And trying to apply that patch after staging this series hits a 
>>> conflict in mnigration/block-dirty-bitmap.c.  Which one should go in 
>>> first?
>>>
>>
>> My patches are needed to fix migration for the pre-blockdev 
>> configuration with mirror-top filter.
>>
>> We ofcrouse need them in Virtuozzo, but it's not hard to keep the in 
>> downstream-only.. And it will be not simple to use new command from 
>> Max in pre-blockdev libvirt configuration, with auto-generated 
>> node-names.

Carrying a downstream fork forever is more work on you.  If the patch is 
easy enough to maintain, incorporating it upstream is best all around, 
even if libvirt has moved on to the point of no longer caring since it 
no longer uses pre-blockdev.

>>
>> How much we care about pre-blockdev libvirt now in upstream Qemu?
>>
>> If we don't care, than these series are only for downstreams, and we 
>> don't need to apply them upstream..

Eventually, we may want to deprecate pre-blockdev, but I don't think we 
are there yet, and even when it does happen, it will be two more 
releases with it being deprecated before it is gone, so we might as well 
make it work correctly in the meantime.

>>
>> On the other hand, Max have to resend anyway, to handle old code, 
>> which uses device name instead of node-name. And if we don't want to 
>> drop now the code which can use device name (needed for old libvirt), 
>> why not to apply the series, which just make old code better?
>>
>> ====
>>
>> In other words: do we still support pre-blockdev libvirt (and any 
>> other pre-blockdev users)?
>>
>> If we support, than, as I said somewhere, I need to resend these 
>> series as I have updated version in our downstream. And I think, I can 
>> rebase Max's patch by myself and send together with this all, if no 
>> objections.
>>
> 
> I'm going to resend the series today, let's look at it.
> 

Sounds reasonable.

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

Re: [PATCH v2 0/5] fix migration with bitmaps and mirror

[Vladimir Sementsov-Ogievskiy]

15.05.2020 20:51, Eric Blake wrote:
> On 5/15/20 6:15 AM, Vladimir Sementsov-Ogievskiy wrote:
> 
>>>> Max is trying to tackle the node-name issue:
>>>> https://lists.gnu.org/archive/html/qemu-devel/2020-05/msg03358.html
>>>>
>>>> And trying to apply that patch after staging this series hits a conflict in mnigration/block-dirty-bitmap.c.  Which one should go in first?
>>>>
>>>
>>> My patches are needed to fix migration for the pre-blockdev configuration with mirror-top filter.
>>>
>>> We ofcrouse need them in Virtuozzo, but it's not hard to keep the in downstream-only.. And it will be not simple to use new command from Max in pre-blockdev libvirt configuration, with auto-generated node-names.
> 
> Carrying a downstream fork forever is more work on you.  If the patch is easy enough to maintain, incorporating it upstream is best all around, even if libvirt has moved on to the point of no longer caring since it no longer uses pre-blockdev.

I hope not forever, when Rhel moves to node-names, we will do it too (hmm, I don't know, may be future already came, and Rhel8 libvirt is node-name oriented?) Still, yes it's always better to reduce the downstream overhead

> 
>>>
>>> How much we care about pre-blockdev libvirt now in upstream Qemu?
>>>
>>> If we don't care, than these series are only for downstreams, and we don't need to apply them upstream..
> 
> Eventually, we may want to deprecate pre-blockdev, but I don't think we are there yet, and even when it does happen, it will be two more releases with it being deprecated before it is gone, so we might as well make it work correctly in the meantime.

Agree. Better to fix old behavior first, and then do proper deprecation if needed.

> 
>>>
>>> On the other hand, Max have to resend anyway, to handle old code, which uses device name instead of node-name. And if we don't want to drop now the code which can use device name (needed for old libvirt), why not to apply the series, which just make old code better?
>>>
>>> ====
>>>
>>> In other words: do we still support pre-blockdev libvirt (and any other pre-blockdev users)?
>>>
>>> If we support, than, as I said somewhere, I need to resend these series as I have updated version in our downstream. And I think, I can rebase Max's patch by myself and send together with this all, if no objections.
>>>
>>
>> I'm going to resend the series today, let's look at it.
>>
> 
> Sounds reasonable.
> 

-- 
Best regards,
Vladimir