Re: [Qemu-devel] Merging the quorum block driver

[Kevin Wolf]

Am 20.09.2013 um 14:08 hat Benoît Canet geschrieben:
> Le Thursday 19 Sep 2013 à 16:23:54 (+0200), Kevin Wolf a écrit :
> > Am 19.09.2013 um 15:38 hat Benoît Canet geschrieben:
> > > Le Thursday 19 Sep 2013 à 15:21:46 (+0200), Kevin Wolf a écrit :
> > > > Am 19.09.2013 um 14:55 hat Benoît Canet geschrieben:
> > > > > Le Thursday 19 Sep 2013 à 10:57:50 (+0200), Kevin Wolf a écrit :
> > > > > > Am 19.09.2013 um 10:26 hat Stefan Hajnoczi geschrieben:
> > > > > > > On Wed, Sep 18, 2013 at 05:05:27PM +0200, Benoît Canet wrote:
> > > > > > > > Le Tuesday 17 Sep 2013 à 14:44:13 (+0200), Stefan Hajnoczi a 
> > > > > > > > écrit :
> > > > > > > > > Hi Benoit,
> > > > > > > > > Kevin and I had a chance to chat face-to-face and we 
> > > > > > > > > discussed what
> > > > > > > > > concrete changes are necessary to merge quorum (without 
> > > > > > > > > solving all the
> > > > > > > > > other block layers problems at once).
> > > > > > > > >
> > > > > > > > > I think quorum could be merged relatively quickly (and 
> > > > > > > > > without massive
> > > > > > > > > BlockFilter investments) by changing the following:
> > > > > > > > >
> > > > > > > > > 1. Defining children on the command-line
> > > > > > > > >
> > > > > > > > > Existing "filter" drivers use the protocol filename to embed 
> > > > > > > > > their
> > > > > > > > > children, for example the blkverify driver.  This is a big 
> > > > > > > > > hack because
> > > > > > > > > we have no proper syntax or escaping for the embedded drive 
> > > > > > > > > definitions
> > > > > > > > > in the file= option.
> > > > > > > > >
> > > > > > > > > This was one of the main arguments against merging quorum.  
> > > > > > > > > Now that
> > > > > > > > > Kevin has implemented driver-specific open options (see
> > > > > > > > > block/qcow2.c:qcow2_runtime_opts), it is possible for the 
> > > > > > > > > quorum driver
> > > > > > > > > to open children specific on the command-line:
> > > > > > > > >
> > > > > > > > >   -drive if=none,file.driver=quorum,format=raw,\
> > > > > > > > >          file.children0.file=/nfs1/test.qcow2,\
> > > > > > > > >          file.children1.file=/nfs2/test.qcow2,\
> > > > > > > > >          file.children2.file=/nfs3/test.qcow2
> > > > > >
> > > > > > By the way, I think he concrete syntax might have to be a bit 
> > > > > > different
> > > > > > so it can be mapped to QAPI for blockdev-add. I think we'll want to 
> > > > > > use
> > > > > > a JSON list for the children; a mapping to QDict and to the command 
> > > > > > line
> > > > > > is yet to be implemented I guess.
> > > > >
> > > > > Hi,
> > > > >
> > > > > Just to make sure I'll code the right thing; is the following correct 
> > > > > ?
> > > > >
> > > > > -drive if=none,file.driver=quorum,format=raw,\
> > > > >        file.children=[/nfs1/test.qcow2, /nfs2/test.qcow2, 
> > > > > /nfs3/test.qcow2], \
> > > > >        file.vote_threshold=2
> > > >
> > > > I was thinking more along the lines of:
> > > >
> > > >     -drive if=none,file.driver=quorum,format=raw,\
> > > >            file.children[0].file.filename=/nfs1/test.qcow2, \
> > > >            file.children[1].file.filename=/nfs2/test.qcow2, \
> > > >            file.children[1].file.cache.direct=on, \
> > > >            file.children[2].file.driver=nbd, \
> > > >            file.children[2].file.host=localhost, \
> > > >            file.vote_threshold=2
> > > >
> > > > The exact syntax doesn't matter all that much as long as it can be
> > > > mapped to QAPI, but as you can see this allows specifying detailed
> > > > options even for the children. I think this is a requirement.
> > >
> > > So does it mean that on top of your blockdev branch qemu_opts_to_qdict 
> > > should do
> > > a little parsing of opt->name to regroup options of the same children 
> > > together ?
> > >
> > > e.g: converting the incoming qoption into the qdict equivalent of:
> > > dict = { "driver": "quorum",
> > >          "children[0]": "file.filename=/nfs1/test.qcow2",
> > >          "children[1]": 
> > > "file.filename=/nfs2/test.qcow2,file.cache.direct=on",
> > >          "children[2]": "file.driver=nbd,file.host=localhost",
> > >          "vote_threshold": 2 }
> >
> > No, the qdict is supposed to be completely flat. I think there are two
> > parts to it:
> >
> > 1. Extend qdict_flatten() so that it converts QLists into an array
> >    representation. The resulting qdict should look pretty much like the
> >    command line:
> >
> >     dict = { "driver": "quorum",
> >              "children[0].file.filename": "/nfs1/test.qcow2",
> >              "children[1].file.filename": "/nfs2/test.qcow2",
> >              "children[1].file.cache.direct": "on",
> >              "children[2].file.driver": "nbd",
> >              "children[2].file.host": "localhost",
> >              "vote_threshold": 2 }
> >
> > 2. The option parsing code in quorum must find a way to walk this
> >    flattened children array. Basically it will end up checking if
> >    the keys start with a "children[%d]", and if so, use
> >    extract_subqdict() to get the full array element back.
> 
> I made this.
> The block driver complains that no filename is provided at startup.
> ---
>     } else if (!drv->bdrv_parse_filename && !filename) {
>         qerror_report(ERROR_CLASS_GENERIC_ERROR,
>                       "The '%s' block driver requires a file name",
>                       drv->format_name);
>         ret = -EINVAL;
>         goto fail;
>     }
> ----
> 
> How can I write a patch to fix this ?

There's a hidden assumption here: All drivers that support
driver-specific options have drv->bdrv_parse_filename implemented.
This isn't true for quorum, obviously, so the condition doesn't work
here any more.

Perhaps introduce a bool drv->bdrv_needs_filename to be checked here
instead of drv->bdrv_parse_filename?

Kevin