Patchwork [V2,3/4] blockdev: Allow snapshoting of protocols.

login
register
mail settings
Submitter Benoît Canet
Date Jan. 28, 2013, 5:04 p.m.
Message ID <1359392680-15838-4-git-send-email-benoit@irqsave.net>
Download mbox | patch
Permalink /patch/216289/
State New
Headers show

Comments

Benoît Canet - Jan. 28, 2013, 5:04 p.m.
Signed-off-by: Benoit Canet <benoit@irqsave.net>
---
 blockdev.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
Kevin Wolf - Jan. 29, 2013, 12:19 p.m.
Am 28.01.2013 18:04, schrieb Benoît Canet:
> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> ---
>  blockdev.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 0ce45c5..b1f388b 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -800,7 +800,8 @@ void qmp_transaction(BlockdevActionList *dev_list, Error **errp)
>          /* We will manually add the backing_hd field to the bs later */
>          states->new_bs = bdrv_new("");
>          ret = bdrv_open(states->new_bs, new_image_file,
> -                        flags | BDRV_O_NO_BACKING, drv);
> +                        flags | BDRV_O_NO_BACKING,
> +                        path_has_protocol(new_image_file) ?  NULL : drv);
>          if (ret != 0) {
>              error_set(errp, QERR_OPEN_FILE_FAILED, new_image_file);
>              goto delete_and_fail;

Wait, what's happening here? I don't understand this patch and how it's
related to snapshotting non-file protocols (if this is even what you
mean). What is your exact scenario, what does the existing code do in
it, and how does this change improve it? An empty commit message is
definitely not appropriate for such a change.

In any case, using NULL as drv for bdrv_open() looks plain wrong.

Kevin
Benoît Canet - Jan. 29, 2013, 1:07 p.m.
> Wait, what's happening here? I don't understand this patch and how it's
> related to snapshotting non-file protocols (if this is even what you
> mean). What is your exact scenario, what does the existing code do in
> it, and how does this change improve it? An empty commit message is
> definitely not appropriate for such a change.
> 
> In any case, using NULL as drv for bdrv_open() looks plain wrong.

When passing drv bdrv_open tries to open the url as qcow2 or another plain file
format and fail.

bdrv_file_open is not a better option because it won't return a BlockDriverState
constructed in the same way as the old one.
(raw as bs and quorum as ->file)

I agree that this patch is a hack and I am looking for a better way of doing it.

Benoît
Kevin Wolf - Jan. 29, 2013, 1:25 p.m.
Am 29.01.2013 14:07, schrieb Benoît Canet:
>> Wait, what's happening here? I don't understand this patch and how it's
>> related to snapshotting non-file protocols (if this is even what you
>> mean). What is your exact scenario, what does the existing code do in
>> it, and how does this change improve it? An empty commit message is
>> definitely not appropriate for such a change.
>>
>> In any case, using NULL as drv for bdrv_open() looks plain wrong.
> 
> When passing drv bdrv_open tries to open the url as qcow2 or another plain file
> format and fail.

What exactly is a "plain file format" and what is the difference between
it and other file formats?

Anyway, this is not a problem description, let alone a description of
your exact scenario.

If qcow2 is specified as the format in the QMP command, why is it then
wrong to try and open the file as qcow2? Why does it fail? What is the
correct driver?

> bdrv_file_open is not a better option because it won't return a BlockDriverState
> constructed in the same way as the old one.
> (raw as bs and quorum as ->file)
> 
> I agree that this patch is a hack and I am looking for a better way of doing it.

Let's talk about the problem first before discussing solutions.

Kevin
Benoît Canet - Jan. 29, 2013, 1:45 p.m.
Le Tuesday 29 Jan 2013 à 14:25:42 (+0100), Kevin Wolf a écrit :
> Am 29.01.2013 14:07, schrieb Benoît Canet:
> >> Wait, what's happening here? I don't understand this patch and how it's
> >> related to snapshotting non-file protocols (if this is even what you
> >> mean). What is your exact scenario, what does the existing code do in
> >> it, and how does this change improve it? An empty commit message is
> >> definitely not appropriate for such a change.
> >>
> >> In any case, using NULL as drv for bdrv_open() looks plain wrong.
> > 
> > When passing drv bdrv_open tries to open the url as qcow2 or another plain file
> > format and fail.
> 
> What exactly is a "plain file format" and what is the difference between
> it and other file formats?

Quorum is a protocol which encapsulate n BlockDriverState.

> 
> Anyway, this is not a problem description, let alone a description of
> your exact scenario.

A scenario would be:
A user want to create a new quorum snapshot made of n qcow2 snapshots.
The user specify qcow2 as format so the new quorum files are created as qcow2.
Then the code try to open the quorum file as a qcow2 file and it fail.

> 
> If qcow2 is specified as the format in the QMP command, why is it then
> wrong to try and open the file as qcow2? Why does it fail? What is the
> correct driver?

It's correct to format the individual quorum files as qcow2 so specifying qcow2
in the qmp command seems ok.
But opening a "quorum:" url which agregate these qcow2 files will fail if drv is
the qcow2 driver.
Would passing the quorum protocol driver to bdrv_open be ok ?

Benoît

> 
> > bdrv_file_open is not a better option because it won't return a BlockDriverState
> > constructed in the same way as the old one.
> > (raw as bs and quorum as ->file)
> > 
> > I agree that this patch is a hack and I am looking for a better way of doing it.
> 
> Let's talk about the problem first before discussing solutions.
> 
> Kevin
Kevin Wolf - Jan. 29, 2013, 2:05 p.m.
Am 29.01.2013 14:45, schrieb Benoît Canet:
> Le Tuesday 29 Jan 2013 à 14:25:42 (+0100), Kevin Wolf a écrit :
>> Am 29.01.2013 14:07, schrieb Benoît Canet:
>>>> Wait, what's happening here? I don't understand this patch and how it's
>>>> related to snapshotting non-file protocols (if this is even what you
>>>> mean). What is your exact scenario, what does the existing code do in
>>>> it, and how does this change improve it? An empty commit message is
>>>> definitely not appropriate for such a change.
>>>>
>>>> In any case, using NULL as drv for bdrv_open() looks plain wrong.
>>>
>>> When passing drv bdrv_open tries to open the url as qcow2 or another plain file
>>> format and fail.
>>
>> What exactly is a "plain file format" and what is the difference between
>> it and other file formats?
> 
> Quorum is a protocol which encapsulate n BlockDriverState.

So it's not a file format, neither plain nor otherwise.

>> Anyway, this is not a problem description, let alone a description of
>> your exact scenario.
> 
> A scenario would be:
> A user want to create a new quorum snapshot made of n qcow2 snapshots.
> The user specify qcow2 as format so the new quorum files are created as qcow2.
> Then the code try to open the quorum file as a qcow2 file and it fail.

Like this?

base1 [file] --- base1 [qcow2] --\
base2 [file] --- base2 [qcow2] --->-- quorum --- snap.qcow2
base3 [file] --- base3 [qcow2] --/

Or like this? (This is the only one where quorum is really used as  a
protocol)

base1 [file] --\
base2 [file] --->-- quorum --- base [qcow2] --- snap.qcow2
base3 [file] --/

Or even this one?

base1 [file] --- base1 [qcow2] --- snap1.qcow2 --\
base2 [file] --- base2 [qcow2] --- snap2.qcow2 --->-- quorum
base3 [file] --- base3 [qcow2] --- snap3.qcow2 --/

I think the last one is what you really want, but it's certainly not the
case that is enabled by this patch.

>> If qcow2 is specified as the format in the QMP command, why is it then
>> wrong to try and open the file as qcow2? Why does it fail? What is the
>> correct driver?
> 
> It's correct to format the individual quorum files as qcow2 so specifying qcow2
> in the qmp command seems ok.
> But opening a "quorum:" url which agregate these qcow2 files will fail if drv is
> the qcow2 driver.
> Would passing the quorum protocol driver to bdrv_open be ok ?

A quorum: URL opened with the qcow2 driver should succeed and result in
something like the second case mentioned above.

Kevin
Benoît Canet - Jan. 29, 2013, 2:27 p.m.
> base1 [file] --- base1 [qcow2] --- snap1.qcow2 --\
> base2 [file] --- base2 [qcow2] --- snap2.qcow2 --->-- quorum
> base3 [file] --- base3 [qcow2] --- snap3.qcow2 --/

> 
> I think the last one is what you really want, but it's certainly not the
> case that is enabled by this patch.

Yes I am trying to get this configuration: quorum always on top.

Benoît

> 
> >> If qcow2 is specified as the format in the QMP command, why is it then
> >> wrong to try and open the file as qcow2? Why does it fail? What is the
> >> correct driver?
> > 
> > It's correct to format the individual quorum files as qcow2 so specifying qcow2
> > in the qmp command seems ok.
> > But opening a "quorum:" url which agregate these qcow2 files will fail if drv is
> > the qcow2 driver.
> > Would passing the quorum protocol driver to bdrv_open be ok ?
> 
> A quorum: URL opened with the qcow2 driver should succeed and result in
> something like the second case mentioned above.
> 
> Kevin
Benoît Canet - Jan. 29, 2013, 3:03 p.m.
> base1 [file] --- base1 [qcow2] --- snap1.qcow2 --\
> base2 [file] --- base2 [qcow2] --- snap2.qcow2 --->-- quorum
> base3 [file] --- base3 [qcow2] --- snap3.qcow2 --/

I tried to default snapshot creation to qcow2 in quorum.c and specify
quorum as format in the snapshot_blkdev command line.
It doesn't work (crash).
Do you have any idea on how to open while obtaining the third case ?

Benoît
Kevin Wolf - Jan. 29, 2013, 3:21 p.m.
Am 29.01.2013 16:03, schrieb Benoît Canet:
>> base1 [file] --- base1 [qcow2] --- snap1.qcow2 --\
>> base2 [file] --- base2 [qcow2] --- snap2.qcow2 --->-- quorum
>> base3 [file] --- base3 [qcow2] --- snap3.qcow2 --/
> 
> I tried to default snapshot creation to qcow2 in quorum.c and specify
> quorum as format in the snapshot_blkdev command line.
> It doesn't work (crash).
> Do you have any idea on how to open while obtaining the third case ?

Wait for BlockBackends and then implement proper stay-on-top filters.

Kevin
Eric Blake - Jan. 29, 2013, 4:24 p.m.
On 01/29/2013 07:27 AM, Benoît Canet wrote:
>> base1 [file] --- base1 [qcow2] --- snap1.qcow2 --\
>> base2 [file] --- base2 [qcow2] --- snap2.qcow2 --->-- quorum
>> base3 [file] --- base3 [qcow2] --- snap3.qcow2 --/

For that matter, I think it would be useful to have a quorum where the
data is duplicated between different formats, such as:

file1 [raw]                         --\
base2 [raw] --- snap2.qcow2 [qcow2] -->-- quorum
base3 [qed] --- snap3.qed [qed]     --/

In that case, you NEED to be able to pass in the protocol of each
underlying device _as part of opening the quorum_, something like:

quorum:2/3:file1[raw]:snap2.qcow2[qcow2]:snap3.qed[qed]

or some such punctuation.  And of course, that means you'd have to
extend your escaping mechanism to allow escaping of whatever syntax you
use for declaring a per-member format.
Benoît Canet - Jan. 29, 2013, 5:02 p.m.
Le Tuesday 29 Jan 2013 à 16:21:43 (+0100), Kevin Wolf a écrit :
> Am 29.01.2013 16:03, schrieb Benoît Canet:
> >> base1 [file] --- base1 [qcow2] --- snap1.qcow2 --\
> >> base2 [file] --- base2 [qcow2] --- snap2.qcow2 --->-- quorum
> >> base3 [file] --- base3 [qcow2] --- snap3.qcow2 --/
> > 
> > I tried to default snapshot creation to qcow2 in quorum.c and specify
> > quorum as format in the snapshot_blkdev command line.
> > It doesn't work (crash).
> > Do you have any idea on how to open while obtaining the third case ?
> 
> Wait for BlockBackends and then implement proper stay-on-top filters.

Any idea to disable properly snapshot creation for quorum and allowing it
to be merged in a not so far future ?

Benoît

> 
> Kevin
>
Kevin Wolf - Feb. 5, 2013, 5:25 p.m.
Am 29.01.2013 17:24, schrieb Eric Blake:
> On 01/29/2013 07:27 AM, Benoît Canet wrote:
>>> base1 [file] --- base1 [qcow2] --- snap1.qcow2 --\
>>> base2 [file] --- base2 [qcow2] --- snap2.qcow2 --->-- quorum
>>> base3 [file] --- base3 [qcow2] --- snap3.qcow2 --/
> 
> For that matter, I think it would be useful to have a quorum where the
> data is duplicated between different formats, such as:
> 
> file1 [raw]                         --\
> base2 [raw] --- snap2.qcow2 [qcow2] -->-- quorum
> base3 [qed] --- snap3.qed [qed]     --/
> 
> In that case, you NEED to be able to pass in the protocol of each
> underlying device _as part of opening the quorum_, something like:
> 
> quorum:2/3:file1[raw]:snap2.qcow2[qcow2]:snap3.qed[qed]
> 
> or some such punctuation.  And of course, that means you'd have to
> extend your escaping mechanism to allow escaping of whatever syntax you
> use for declaring a per-member format.

Let me rephrase that: quorum really needs -blockdev. And quite possibly
the data that can be passed to the snapshot QMP commands today isn't
sufficient to describe what we want to describe. :-/

Kevin

Patch

diff --git a/blockdev.c b/blockdev.c
index 0ce45c5..b1f388b 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -800,7 +800,8 @@  void qmp_transaction(BlockdevActionList *dev_list, Error **errp)
         /* We will manually add the backing_hd field to the bs later */
         states->new_bs = bdrv_new("");
         ret = bdrv_open(states->new_bs, new_image_file,
-                        flags | BDRV_O_NO_BACKING, drv);
+                        flags | BDRV_O_NO_BACKING,
+                        path_has_protocol(new_image_file) ?  NULL : drv);
         if (ret != 0) {
             error_set(errp, QERR_OPEN_FILE_FAILED, new_image_file);
             goto delete_and_fail;