Re: [Qemu-block] [PATCH v6 19/25] block: Add BlockDriver.bdrv_gather_child_options

[Max Reitz]

On 2017-11-08 18:18, Kevin Wolf wrote:
> Am 29.09.2017 um 18:53 hat Max Reitz geschrieben:
>> Some follow-up patches will rework the way bs->full_open_options is
>> refreshed in bdrv_refresh_filename(). The new implementation will remove
>> the need for the block drivers' bdrv_refresh_filename() implementations
>> to set bs->full_open_options; instead, it will be generic and use static
>> information from each block driver.
>>
>> However, by implementing bdrv_gather_child_options(), block drivers will
>> still be able to override the way the full_open_options of their
>> children are incorporated into their own.
>>
>> We need to implement this function for VMDK because we have to prevent
>> the generic implementation from gathering the options of all children:
>> It is not possible to specify options for the extents through the
>> runtime options.
> 
> Sounds more like a bug than a feature.

Want to fix it? :-)

>> For quorum, the child names that would be used by the generic
>> implementation and the ones that we actually want to use differ. See
>> quorum_gather_child_options() for more information.
> 
> :-/
> 
> What was the conclusion of our discussion at KVM Forum again? I just
> remember that this caused problems in other contexts like dynamic
> reconfiguration, too.

I might have just winced a little.

The short conclusion was that it's all broken.

The long conclusion...  Is that I might try to wrestle with quorum
before this series? :-/

Let's see...  The conclusion was that the user needs to specify a unique
name for each quorum child.  Then, this name would be equivalent to the
runtime name.  Sounds good so far?  Well, I might have just screamed a
little when I remembered another issue:

The generic implementation gathers the options like so:

{ /* node options */
  "child0": { /* child0 options */ },
  "child1": { /* child1 options */ },
  ... }

However, specifying child options like so doesn't work easily with QAPI
and quorum, for two reasons:

(1) Specifying arbitrary keys with specific values would need new QAPI
infrastructure.  Who is going to implement this? :-)

(2) Quorum needs a fixed order for its children, at least for FIFO mode.
 A JSON object does not have inherent ordering.

The simple way to fix this is to make the children list a list of
objects which contain the child name and the options:

{ /* quorum node options */
  "children": [
{ "child-name": "child0",
  "options": { /* child0 options */ } },
{ "child-name": "child1",
  "options": { /* child1 options */ } },
... ] }

But this would still require bdrv_gather_child_options() to generate.


The other way would be to add the QAPI infrastructure to make the
generic thing work, and add a "children-order" list or something which
gives order to the children.  Then the generic implementation should
work...  As long as the quorum driver makes sure to update this list
whenever a child is added or removed.

Maybe the latter is actually the better choice? :-/

But it definitely means more work (because of the QAPI modifications)...

Max


>> Signed-off-by: Max Reitz 
>> ---
>>  include/block/block_int.h | 13 +
>>  block/quorum.c| 30 ++
>>  block/vmdk.c  | 13 +
>>  3 files changed, 56 insertions(+)
> 
> Kevin
> 




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v6 19/25] block: Add BlockDriver.bdrv_gather_child_options

[Kevin Wolf]

Am 29.09.2017 um 18:53 hat Max Reitz geschrieben:
> Some follow-up patches will rework the way bs->full_open_options is
> refreshed in bdrv_refresh_filename(). The new implementation will remove
> the need for the block drivers' bdrv_refresh_filename() implementations
> to set bs->full_open_options; instead, it will be generic and use static
> information from each block driver.
> 
> However, by implementing bdrv_gather_child_options(), block drivers will
> still be able to override the way the full_open_options of their
> children are incorporated into their own.
> 
> We need to implement this function for VMDK because we have to prevent
> the generic implementation from gathering the options of all children:
> It is not possible to specify options for the extents through the
> runtime options.

Sounds more like a bug than a feature.

> For quorum, the child names that would be used by the generic
> implementation and the ones that we actually want to use differ. See
> quorum_gather_child_options() for more information.

:-/

What was the conclusion of our discussion at KVM Forum again? I just
remember that this caused problems in other contexts like dynamic
reconfiguration, too.

> Signed-off-by: Max Reitz 
> ---
>  include/block/block_int.h | 13 +
>  block/quorum.c| 30 ++
>  block/vmdk.c  | 13 +
>  3 files changed, 56 insertions(+)

Kevin

Re: [Qemu-block] [PATCH v6 19/25] block: Add BlockDriver.bdrv_gather_child_options

[Alberto Garcia]

On Fri 29 Sep 2017 06:53:41 PM CEST, Max Reitz wrote:
> Some follow-up patches will rework the way bs->full_open_options is
> refreshed in bdrv_refresh_filename(). The new implementation will remove
> the need for the block drivers' bdrv_refresh_filename() implementations
> to set bs->full_open_options; instead, it will be generic and use static
> information from each block driver.
>
> However, by implementing bdrv_gather_child_options(), block drivers will
> still be able to override the way the full_open_options of their
> children are incorporated into their own.
>
> We need to implement this function for VMDK because we have to prevent
> the generic implementation from gathering the options of all children:
> It is not possible to specify options for the extents through the
> runtime options.
>
> For quorum, the child names that would be used by the generic
> implementation and the ones that we actually want to use differ. See
> quorum_gather_child_options() for more information.
>
> Signed-off-by: Max Reitz 

Reviewed-by: Alberto Garcia 

Berto