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

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3456 bytes --]

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 <mreitz@redhat.com>
>> ---
>>  include/block/block_int.h | 13 +++++++++++++
>>  block/quorum.c            | 30 ++++++++++++++++++++++++++++++
>>  block/vmdk.c              | 13 +++++++++++++
>>  3 files changed, 56 insertions(+)
> 
> Kevin
> 



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