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

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 6046 bytes --]

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

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]