Re: bitmap migration bug with -drive while block mirror runs

[Vladimir Sementsov-Ogievskiy]

01.10.2019 19:17, Max Reitz wrote:
> On 01.10.19 18:12, Kevin Wolf wrote:
>> Am 01.10.2019 um 17:27 hat Max Reitz geschrieben:
>>> On 01.10.19 17:09, Kevin Wolf wrote:
>>>> Am 01.10.2019 um 16:34 hat Max Reitz geschrieben:
>>>>> On 01.10.19 16:27, Vladimir Sementsov-Ogievskiy wrote:
>>>>>> 01.10.2019 17:13, Max Reitz wrote:
>>>>>>> On 01.10.19 16:00, Vladimir Sementsov-Ogievskiy wrote:
>>>>>>>> 01.10.2019 3:09, John Snow wrote:
>>>>>>>>> Hi folks, I identified a problem with the migration code that Red Hat 
>>>>>>>>> QE
>>>>>>>>> found and thought you'd like to see it:
>>>>>>>>>
>>>>>>>>> https://bugzilla.redhat.com/show_bug.cgi?id=1652424#c20
>>>>>>>>>
>>>>>>>>> Very, very briefly: drive-mirror inserts a filter node that changes 
>>>>>>>>> what
>>>>>>>>> bdrv_get_device_or_node_name() returns, which causes a migration 
>>>>>>>>> problem.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Ignorant question #1: Can we multi-parent the filter node and
>>>>>>>>> source-node? It looks like at the moment both consider their only 
>>>>>>>>> parent
>>>>>>>>> to be the block-job and don't have a link back to their parents 
>>>>>>>>> otherwise.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Otherwise: I have a lot of cloudy ideas on how to solve this, but
>>>>>>>>> ultimately what we want is to be able to find the "addressable" name 
>>>>>>>>> for
>>>>>>>>> the node the bitmap is attached to, which would be the name of the 
>>>>>>>>> first
>>>>>>>>> ancestor node that isn't a filter. (OR, the name of the block-backend
>>>>>>>>> above that node.)
>>>>>>>>
>>>>>>>> Not the name of ancestor node, it will break mapping: it must be name 
>>>>>>>> of the
>>>>>>>> node itself or name of parent (may be through several filters) 
>>>>>>>> block-backend
>>>>>>>>
>>>>>>>>>
>>>>>>>>> A simple way to do this might be a "child_unfiltered" BdrvChild role
>>>>>>>>> that simply bypasses the filter that was inserted and serves no real
>>>>>>>>> purpose other than to allow the child to have a parent link and find 
>>>>>>>>> who
>>>>>>>>> it's """real""" parent is.
>>>>>>>>>
>>>>>>>>> Because of flushing, reopen, sync, drain &c &c &c I'm not sure how
>>>>>>>>> feasible this quick idea might be, though.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> - Corollary fix #1: call error_setg if the bitmap node name that's 
>>>>>>>>> about
>>>>>>>>> to go over the wire is an autogenerated node: this is never correct!
>>>>>>>>>
>>>>>>>>> (Why not? because the target is incapable of matching the node-name
>>>>>>>>> because they are randomly generated AND you cannot specify node-names
>>>>>>>>> with # prefixes as they are especially reserved!
>>>>>>>>>
>>>>>>>>> (This raises a related problem: if you explicitly add bitmaps to nodes
>>>>>>>>> with autogenerated names, you will be unable to migrate them.))
>>>>>>>>>
>>>>>>>>> --js
>>>>>>>>>
>>>>>>>>
>>>>>>>> What about the following:
>>>>>>>>
>>>>>>>> diff --git a/block.c b/block.c
>>>>>>>> index 5944124845..6739c19be9 100644
>>>>>>>> --- a/block.c
>>>>>>>> +++ b/block.c
>>>>>>>> @@ -1009,8 +1009,20 @@ static void bdrv_inherited_options(int 
>>>>>>>> *child_flags, QDict *child_options,
>>>>>>>>         *child_flags = flags;
>>>>>>>>     }
>>>>>>>>
>>>>>>>> +static const char *bdrv_child_get_name(BdrvChild *child)
>>>>>>>> +{
>>>>>>>> +    BlockDriverState *parent = child->opaque;
>>>>>>>> +
>>>>>>>> +    if (parent->drv && parent->drv->is_filter) {
>>>>>>>> +        return bdrv_get_parent_name(parent);
>>>>>>>> +    }
>>>>>>>> +
>>>>>>>> +    return NULL;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>
>>>>>>> Why would we skip filters explicitly added by the user?
>>>>>>>
>>>>>>
>>>>>> Why not? Otherwise migration of bitmaps will not work: we may have 
>>>>>> different set
>>>>>> of filters on source and destination, and we still should map nodes with 
>>>>>> bitmaps
>>>>>> automatically.
>>>>>
>>>>> Why would we have a different set of explicitly added filters on source
>>>>> and destination and allow them to be automatically changed during
>>>>> migration?  Shouldn’t users only change them pre or post migration?
>>>>
>>>> We never made a requirement that the backend must be the same on the
>>>> source and the destination. Basically, migration copies the state of
>>>> frontends and the user is responsible for having these frontends created
>>>> and connected to the right backends on the destination.
>>>>
>>>> Using different paths on the destination is a very obvious requirement
>>>> for block devices. It's less obvious for the graph structure, but I
>>>> don't see a reason why it couldn't change on migration. Say we were
>>>> using local storage on the source, but now we did storage migration to
>>>> some network storage, access to which should be throttled.
>>>
>>> I don’t quite see why we couldn’t add such filters before or after
>>> migration.
>>
>> Possibly. But why would we when the source doesn't need the filter? We
>> don't change the image path before migration either.
>>
>> I think the tricky part is coming up with rules and "keep the frontend
>> the same, the backend can change arbitrarily" is a very easy rule.
> 
> OK, indeed.
> 
>>> And it was my impression that bitmap migration was a problem now
>>> precisely because it is bound to the graph structure.
>>
>> So apparently I wasn't completely wrong when I preferred just writing
>> bitmaps back to the image instead of transferring them in the migration
>> stream...
>>
>> It's not really bound to the graph structure per se, but to node names
>> and for non-anonymous BlockBackends to the link between the BB and its
>> root node. The latter is part of the graph structure, but only a very
>> small part, and it exists only for legacy (non-blockdev) configurations.
>>
>>> But anyway.  I’ll gladly remove myself from this discussion because I
>>> don’t know much about migration and actually I’d prefer to keep it that
>>> way.  (Sorry.)
>>
>> Good idea, let's have the migration maintainers handle this.
> 
> :-)
> 
> That’s always how it goes, isn’t it?  Migration maintainers don’t know
> the block side, and we don’t know the migration side...

Haha, luckily I'm not a maintainer :)

> 
> Anyway.  It’s just a fact that I don’t have much to add to the
> discussion, whereas there seems to be a productive discussion without me
> already.
> 
> Max
> 


-- 
Best regards,
Vladimir