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

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

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

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

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

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

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

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

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

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

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

> with auto-generated node names

trailing '.'

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

s/allowing use/using/

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

Is this correct, or should it be:

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

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

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

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

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

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


-- 
Best regards,
Vladimir

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

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

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

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