Re: [Qemu-block] [Qemu-devel] [PATCH v2] migration/dirty-bitmaps: change bitmap enumeration method

[John Snow]

On 5/16/19 6:12 AM, Vladimir Sementsov-Ogievskiy wrote:
> 14.05.2019 23:19, John Snow wrote:
>> Shift from looking at every root BDS to *every* BDS. This will migrate
>> bitmaps that are attached to blockdev created nodes instead of just ones
>> attached to emulated storage devices.
>>
>> Note that this will not migrate anonymous or internal-use bitmaps, as
>> those are defined as having no name.
>>
>> This will also fix the Coverity issues Peter Maydell has been asking
>> about for the past several releases, as well as fixing a real bug.
>>
>> Reported-by: Peter Maydell <address@hidden>
>> Reported-by: Coverity 😅
>> Reported-by: aihua liang <address@hidden>
>> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1652490
>> Fixes: Coverity CID 1390625
>> CC: Stefan Hajnoczi <address@hidden>
>> Signed-off-by: John Snow <address@hidden>
>> ---
>>   migration/block-dirty-bitmap.c | 14 ++++----------
>>   1 file changed, 4 insertions(+), 10 deletions(-)
>>
>> diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
>> index d1bb863cb6..4a896a09eb 100644
>> --- a/migration/block-dirty-bitmap.c
>> +++ b/migration/block-dirty-bitmap.c
>> @@ -273,7 +273,6 @@ static int init_dirty_bitmap_migration(void)
>>       BlockDriverState *bs;
>>       BdrvDirtyBitmap *bitmap;
>>       DirtyBitmapMigBitmapState *dbms;
>> -    BdrvNextIterator it;
>>       Error *local_err = NULL;
>>   
>>       dirty_bitmap_mig_state.bulk_completed = false;
>> @@ -281,13 +280,8 @@ static int init_dirty_bitmap_migration(void)
>>       dirty_bitmap_mig_state.prev_bitmap = NULL;
>>       dirty_bitmap_mig_state.no_bitmaps = false;
>>   
>> -    for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
>> -        const char *drive_name = bdrv_get_device_or_node_name(bs);
>> -
>> -        /* skip automatically inserted nodes */
>> -        while (bs && bs->drv && bs->implicit) {
>> -            bs = backing_bs(bs);
>> -        }
> 
> hm, so, after the patch, for implicitly-filtered nodes we'll have node_name 
> instead of device name..
> 

Oh, I see -- this does change what we send over the wire for
interior/leaf nodes; that was unintentional on my part.

After my patch, this requires that if you have a manually constructed
tree such that you have attached a bitmap to an interior or leaf node,
you *need* to name that node so that it can be consistently
reconstructed at the target.

I think that's a reasonable requirement and is actually superior to
re-attaching all bitmaps to the root on migration (which would have
happened before.)

Codewise, what we have currently (both before and after this patch) is:

    if (flags & DIRTY_BITMAP_MIG_FLAG_DEVICE_NAME) {
        qemu_put_counted_string(f, dbms->node_name);
    }

So we named the constant "DEVICE_NAME", but the field was already named
node_name, so this seems fine on the sending end. In practice, pre-patch
we sent a device_name for any node that was the root attached to a
device. Post-patch, that doesn't change because I am using the same API
call to retrieve the name.

For interior/leaf nodes, we now send the node-name specifically instead
of the name of the device root. This requires identically constructed
(or at least compatibly named) graphs on the source and destination,
which is a reasonable requirement for migration.

On the receiving end, we have this code:

    if (s->flags & DIRTY_BITMAP_MIG_FLAG_DEVICE_NAME) {
        if (!qemu_get_counted_string(f, s->node_name)) {
            error_report("Unable to read node name string");
            return -EINVAL;
        }
        s->bs = bdrv_lookup_bs(s->node_name, s->node_name, &local_err);

which looks like a correct mapping. I think this is a safe change, even
though I made it somewhat unintentionally.

> But, on the other, hand, if we have implicitly-filtered node on target, we 
> were doing wrong thing anyway,
> as dirty_bitmap_load_header don't skip implicit nodes.
> 
>> +    for (bs = bdrv_next_all_states(NULL); bs; bs = 
>> bdrv_next_all_states(bs)) {
> 
> As I understand, difference with bdrv_next_node is that we don't skip unnamed 
> nodes [...]
> 

The difference is that we iterate over states that aren't roots of
trees; so not just unnamed nodes but rather intermediate nodes as well
as nodes not attached to a storage device.

bdrv_next says: `Iterates over all top-level BlockDriverStates, i.e.
BDSs that are owned by the monitor or attached to a BlockBackend`

So this loop is going to iterate over *everything*, not just top-level
nodes. This lets me skip the tree-crawling loop that didn't work quite
right.

>> +        const char *name = bdrv_get_device_or_node_name(bs);
>>   
>>           for (bitmap = bdrv_dirty_bitmap_next(bs, NULL); bitmap;
>>                bitmap = bdrv_dirty_bitmap_next(bs, bitmap))
>> @@ -296,7 +290,7 @@ static int init_dirty_bitmap_migration(void)
>>                   continue;
>>               }
>>   
>> -            if (drive_name == NULL) {
>> +            if (!name || strcmp(name, "") == 0) {
> 
> [...] to do this (may be paranoiac, but why not?) check
> 

Because bdrv_get_device_or_node_name technically has the capability to
return an empty string, though I believe in contemporary block layer
that we always generate a node-name. So it might be paranoid, but it
matches the API I'm calling as documented.

>>                   error_report("Found bitmap '%s' in unnamed node %p. It 
>> can't "
>>                                "be migrated", 
>> bdrv_dirty_bitmap_name(bitmap), bs);
>>                   goto fail;
>> @@ -313,7 +307,7 @@ static int init_dirty_bitmap_migration(void)
>>   
>>               dbms = g_new0(DirtyBitmapMigBitmapState, 1);
>>               dbms->bs = bs;
>> -            dbms->node_name = drive_name;
>> +            dbms->node_name = name;
>>               dbms->bitmap = bitmap;
>>               dbms->total_sectors = bdrv_nb_sectors(bs);
>>               dbms->sectors_per_chunk = CHUNK_SIZE * 8 *
>>
> 
> There is still some mess about device name vs node name, and I don't know, 
> could we somehow
> solve it, but patch looks OK for me anyway:
> 

Yes, there's more mess, but I think this is Less Wrong™️. I will try to
combat some of this confusion soon; but I'll look at your cross-node
merge patch first.

> Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> 

Does this review still stand after my clarifications?

--js