[Qemu-devel] Merging the quorum block driver

[Qemu-devel] Merging the quorum block driver

[Stefan Hajnoczi]

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

Here is pseudo-code for quorum_open().  It extracts the children from
the command-line and opens BlockDriverStates for them:

def quorum_open(qdict):
    for key in qdict:
        # Parse out child index
        match = key.match('^children(\d+)\.')
        if not match:
            continue
        idx = match.group(0)

        # block.c:extract_subqdict() already lets you extract a new
        # qdict with the fields starting with a prefix.  You could make
        # this function common.
        child_dict = extract_subqdict(qdict, 'children%d.' % idx)

        # Finally open the child with its specific options
        children[idx] = bdrv_open(child_dict)

After doing this we no longer need to embed the children into the file=
option as a string and driver-specific options can be used for the
children.

2. Prevent external snapshots

The user may wish to snapshot above or below the quorum BDS.  There is
no syntax to express the BDS tree node where snapshotting should happen.
Implementing this fully is a distraction and requires additional work.

For quorum it is simplest to add a BDS interface so block drivers can
forbid snapshots.  This is called recursively on the whole BDS tree, so
quorum can veto the snapshot operation.  An error is returned saying the
device does not support snapshots.

In the future this limitation can be lifted but for the near-term this
is a simple step to make quorum mergable.

With these changes quorum should be mergable.  Implementing full
BlockFilter support is not necessary yet but we can discuss it in
separate thread, if you want.

Stefan

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

[Benoît Canet]

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
> 
> Here is pseudo-code for quorum_open().  It extracts the children from
> the command-line and opens BlockDriverStates for them:
> 
> def quorum_open(qdict):
>     for key in qdict:
>         # Parse out child index
>         match = key.match('^children(\d+)\.')
>         if not match:
>             continue
>         idx = match.group(0)
> 
>         # block.c:extract_subqdict() already lets you extract a new
>         # qdict with the fields starting with a prefix.  You could make
>         # this function common.
>         child_dict = extract_subqdict(qdict, 'children%d.' % idx)
> 
>         # Finally open the child with its specific options
>         children[idx] = bdrv_open(child_dict)
> 
> After doing this we no longer need to embed the children into the file=
> option as a string and driver-specific options can be used for the
> children.
> 
> 2. Prevent external snapshots
> 
> The user may wish to snapshot above or below the quorum BDS.  There is
> no syntax to express the BDS tree node where snapshotting should happen.
> Implementing this fully is a distraction and requires additional work.
> 
> For quorum it is simplest to add a BDS interface so block drivers can
> forbid snapshots.  This is called recursively on the whole BDS tree, so
> quorum can veto the snapshot operation.  An error is returned saying the
> device does not support snapshots.
> 
> In the future this limitation can be lifted but for the near-term this
> is a simple step to make quorum mergable.
> 
> With these changes quorum should be mergable.  Implementing full
> BlockFilter support is not necessary yet but we can discuss it in
> separate thread, if you want.

Thanks Stefan !!

I will do these changes to make quorum mergeable and yes I am still interested
in writing BlockFilter support given I have a plan to do so.

Best regards

Benoît
> 
> Stefan
> 

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

[Benoît Canet]

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

Hello Stefan,

I started writing the code.

How would the quorum treshold be specified with this command line style ?

Best regards

Benoît

> 
> Here is pseudo-code for quorum_open().  It extracts the children from
> the command-line and opens BlockDriverStates for them:
> 
> def quorum_open(qdict):
>     for key in qdict:
>         # Parse out child index
>         match = key.match('^children(\d+)\.')
>         if not match:
>             continue
>         idx = match.group(0)
> 
>         # block.c:extract_subqdict() already lets you extract a new
>         # qdict with the fields starting with a prefix.  You could make
>         # this function common.
>         child_dict = extract_subqdict(qdict, 'children%d.' % idx)
> 
>         # Finally open the child with its specific options
>         children[idx] = bdrv_open(child_dict)
> 
> After doing this we no longer need to embed the children into the file=
> option as a string and driver-specific options can be used for the
> children.
> 
> 2. Prevent external snapshots
> 
> The user may wish to snapshot above or below the quorum BDS.  There is
> no syntax to express the BDS tree node where snapshotting should happen.
> Implementing this fully is a distraction and requires additional work.
> 
> For quorum it is simplest to add a BDS interface so block drivers can
> forbid snapshots.  This is called recursively on the whole BDS tree, so
> quorum can veto the snapshot operation.  An error is returned saying the
> device does not support snapshots.
> 
> In the future this limitation can be lifted but for the near-term this
> is a simple step to make quorum mergable.
> 
> With these changes quorum should be mergable.  Implementing full
> BlockFilter support is not necessary yet but we can discuss it in
> separate thread, if you want.
> 
> Stefan

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

[Stefan Hajnoczi]

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
> 
> Hello Stefan,
> 
> I started writing the code.
> 
> How would the quorum treshold be specified with this command line style ?

Kevin can correct me but I think:

  -drive ...,file.vote_threshold=2

BlockDrivers can have custom options.  The voting threshold is a good
example.

quorum_open() can check for "vote_threshold" in its options qdict.

Stefan

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

[Kevin Wolf]

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.

You can use my blockdev development branch if you want to play with the
QAPI part: git://repo.or.cz/qemu/kevin.git blockdev

> > Hello Stefan,
> > 
> > I started writing the code.
> > 
> > How would the quorum treshold be specified with this command line style ?
> 
> Kevin can correct me but I think:
> 
>   -drive ...,file.vote_threshold=2
> 
> BlockDrivers can have custom options.  The voting threshold is a good
> example.
> 
> quorum_open() can check for "vote_threshold" in its options qdict.

Yup, that's the right way.

Kevin

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

[Benoît Canet]

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

Best regards

Benoît

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

[Kevin Wolf]

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.

Kevin

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

[Benoît Canet]

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 }

Best regards

Benoît

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

[Kevin Wolf]

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.

Kevin

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

[Eric Blake]

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

On 09/19/2013 07:21 AM, Kevin Wolf wrote:

> 
> I was thinking more along the lines of:
> 
>     -drive if=none,file.driver=quorum,format=raw,\
>            file.children[0].file.filename=/nfs1/test.qcow2, \

Note that this requires shell quoting to protect you from globbing
changing the argv handed to qemu based on an (unusual) file name in the
current directory.  Not necessarily the end of the world (someone like
libvirt that uses exec rather than a shell command line isn't bothered
by it, and the command line is so long that most users will not be
typing it directly), but worth thinking about whether we can come up
with an alternate syntax that doesn't require shell quoting.  (As is,
your use of \-newline and whitespace is not quite right for how the
actual command line would appear; but it does make discussion easier in
the meantime)

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

Indeed, those aspects (mappability to QAPI, and fine-tuning of each
child) are essential.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

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

[Daniel P. Berrange]

On Thu, Sep 19, 2013 at 08:51:39AM -0600, Eric Blake wrote:
> On 09/19/2013 07:21 AM, Kevin Wolf wrote:
> 
> > 
> > I was thinking more along the lines of:
> > 
> >     -drive if=none,file.driver=quorum,format=raw,\
> >            file.children[0].file.filename=/nfs1/test.qcow2, \
> 
> Note that this requires shell quoting to protect you from globbing
> changing the argv handed to qemu based on an (unusual) file name in the
> current directory.  Not necessarily the end of the world (someone like
> libvirt that uses exec rather than a shell command line isn't bothered
> by it, and the command line is so long that most users will not be
> typing it directly), but worth thinking about whether we can come up
> with an alternate syntax that doesn't require shell quoting.  (As is,
> your use of \-newline and whitespace is not quite right for how the
> actual command line would appear; but it does make discussion easier in
> the meantime)

Just don't use brackets - use a . for the array index separator too
eg

 -drive if=none,file.driver=quorum,format=raw,\
    file.children.0.file.filename=foo,\
    file.children.1.file.filename=bar,\
    file.children.2.file.filename=wiz,\

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

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

[Benoît Canet]

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 ?

Best regards

Benoît

>
> Kevin
>

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