[Qemu-devel] [FIX V2] repair snapshots

[Qemu-devel] [FIX V2] repair snapshots

[Benoît Canet]

This patch fix the snapshoting code.

When initially testing block filter snapshots patches and quorum I used a wrong
quorum command line which containned an extra top level "file."

As a result quorum was encapsulated in a top level bs.

This caused me to write and test against the wrong configuration and
break snapshot.

This patch implement Kevin sugestion for the fix.

I tested

snapshot of a Blockbackend: ex virtio0 ok
snapshot of a quorum file instantiated in the command line. ok
snapshot of a quorum file instantiated via blockdev_add by references ok
snapshot of a quorum file instianciated via blockdev without references ok

A comment update for quorum will be needed to avoid user to use the wrong
command line.

Benoît Canet (1):
  block: Fix device snapshots broken by the block filter snapshots
    patchset.

 block.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

-- 
1.8.3.2

[Qemu-devel] [FIX V2] block: Fix device snapshots broken by the block filter snapshots patchset.

[Benoît Canet]

Signed-off-by: Benoit Canet <benoit@irqsave.net>
---
 block.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/block.c b/block.c
index 70d7f35..d9553ba 100644
--- a/block.c
+++ b/block.c
@@ -5419,11 +5419,7 @@ bool bdrv_is_first_non_filter(BlockDriverState *candidate)
     QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
         bool perm;
 
-        if (!bs->file) {
-            continue;
-        }
-
-        perm = bdrv_recurse_is_first_non_filter(bs->file, candidate);
+        perm = bdrv_recurse_is_first_non_filter(bs, candidate);
 
         /* candidate is the first non filter */
         if (perm) {
-- 
1.8.3.2

Re: [Qemu-devel] [FIX V2] block: Fix device snapshots broken by the block filter snapshots patchset.

[Kevin Wolf]

Am 13.02.2014 um 02:51 hat Benoît Canet geschrieben:
> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> ---
>  block.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)

Should be [PATCH v3] in the subject line.

> diff --git a/block.c b/block.c
> index 70d7f35..d9553ba 100644
> --- a/block.c
> +++ b/block.c
> @@ -5419,11 +5419,7 @@ bool bdrv_is_first_non_filter(BlockDriverState *candidate)
>      QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
>          bool perm;
>  
> -        if (!bs->file) {
> -            continue;
> -        }
> -
> -        perm = bdrv_recurse_is_first_non_filter(bs->file, candidate);
> +        perm = bdrv_recurse_is_first_non_filter(bs, candidate);
>  
>          /* candidate is the first non filter */
>          if (perm) {

So we're back to my patch, just with different author? I thought you
said something was broken with it?

Kevin

Re: [Qemu-devel] [FIX V2] block: Fix device snapshots broken by the block filter snapshots patchset.

[Benoît Canet]

The Thursday 13 Feb 2014 à 09:33:52 (+0100), Kevin Wolf wrote :
> Am 13.02.2014 um 02:51 hat Benoît Canet geschrieben:
> > Signed-off-by: Benoit Canet <benoit@irqsave.net>
> > ---
> >  block.c | 6 +-----
> >  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> Should be [PATCH v3] in the subject line.
> 
> > diff --git a/block.c b/block.c
> > index 70d7f35..d9553ba 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -5419,11 +5419,7 @@ bool bdrv_is_first_non_filter(BlockDriverState *candidate)
> >      QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
> >          bool perm;
> >  
> > -        if (!bs->file) {
> > -            continue;
> > -        }
> > -
> > -        perm = bdrv_recurse_is_first_non_filter(bs->file, candidate);
> > +        perm = bdrv_recurse_is_first_non_filter(bs, candidate);
> >  
> >          /* candidate is the first non filter */
> >          if (perm) {
> 
> So we're back to my patch, just with different author? I thought you
> said something was broken with it?

Yes it's your patch.

My quorum command line had extra "file." in front of the quorum parameters.
(the user error you mentioned)
That's the reason I initially broke snapshot's because I was testing against
something like bs->file thinking it was normal to get top bs in bs->file.
And that's also the reason why I though your patch was broken.

The quorum series also need a repost to get rid of the "file." of the commit
comment.

Best regards

Benoît

> 
> Kevin
> 

[Qemu-devel] [FIX V2] Fix broken external snapshots

[Benoît Canet]

This is a more cleaner version of the patch repairing the externals snapshots
broken after the introduction of block filters snapshots.

It's based on the presence of the authorization table in the filters.

Forget the first post I was just ashamed an wanted to repair the damage asap.

Best regards

Benoît

Benoît Canet (1):
  block: Fix device snapshots broken by the block filter snapshots
    patchset.

 block.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

-- 
1.8.3.2

[Qemu-devel] [FIX V2] block: Fix device snapshots broken by the block filter snapshots patchset.

[Benoît Canet]

Take into account the fact that a block filter like quorum will be in bs->file
while a regular block driver device is really on the top level.

Signed-off-by: Benoit Canet <benoit@irqsave.net>
---
 block.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/block.c b/block.c
index 07ac50a..d04f535 100644
--- a/block.c
+++ b/block.c
@@ -5400,14 +5400,16 @@ bool bdrv_is_first_non_filter(BlockDriverState *candidate)
 
     /* walk down the bs forest recursively */
     QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
-        bool perm;
-
-        if (!bs->file) {
-            continue;
+        bool perm = false;
+
+        if (bs->file &&
+            bs->file->drv &&
+            bs->file->drv->authorizations[BS_IS_A_FILTER]) {
+            perm = bdrv_recurse_is_first_non_filter(bs->file, candidate);
+        } else if (bs == candidate) {
+            perm = true;
         }
 
-        perm = bdrv_recurse_is_first_non_filter(bs->file, candidate);
-
         /* candidate is the first non filter */
         if (perm) {
             return true;
-- 
1.8.3.2

Re: [Qemu-devel] [FIX V2] block: Fix device snapshots broken by the block filter snapshots patchset.

[Fam Zheng]

On Mon, 02/10 22:49, Benoît Canet wrote:
> Take into account the fact that a block filter like quorum will be in bs->file
> while a regular block driver device is really on the top level.
> 
> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> ---
>  block.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 07ac50a..d04f535 100644
> --- a/block.c
> +++ b/block.c
> @@ -5400,14 +5400,16 @@ bool bdrv_is_first_non_filter(BlockDriverState *candidate)
>  
>      /* walk down the bs forest recursively */
>      QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
> -        bool perm;
> -
> -        if (!bs->file) {
> -            continue;
> +        bool perm = false;
> +
> +        if (bs->file &&
> +            bs->file->drv &&
> +            bs->file->drv->authorizations[BS_IS_A_FILTER]) {
> +            perm = bdrv_recurse_is_first_non_filter(bs->file, candidate);
> +        } else if (bs == candidate) {
> +            perm = true;
>          }
>  
> -        perm = bdrv_recurse_is_first_non_filter(bs->file, candidate);
> -
>          /* candidate is the first non filter */
>          if (perm) {
>              return true;

With this change, if the top level driver has ->file, its implementation of
.bdrv_recurse_is_first_non_filter() is skipped and bs->file is the start point.

So we have an implication that single child block drivers (that has ->file)
doesn't need to, and shouldn't implement this operation, as commentted above
bdrv_generic_is_first_non_filter.

Tested that this patch fixes the external snapshot problem, but didn't test
the "quorum as bs->file case".

Thanks,

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [FIX V2] block: Fix device snapshots broken by the block filter snapshots patchset.

[Benoît Canet]

The Tuesday 11 Feb 2014 à 16:12:17 (+0800), Fam Zheng wrote :
> On Mon, 02/10 22:49, Benoît Canet wrote:
> > Take into account the fact that a block filter like quorum will be in bs->file
> > while a regular block driver device is really on the top level.
> > 
> > Signed-off-by: Benoit Canet <benoit@irqsave.net>
> > ---
> >  block.c | 14 ++++++++------
> >  1 file changed, 8 insertions(+), 6 deletions(-)
> > 
> > diff --git a/block.c b/block.c
> > index 07ac50a..d04f535 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -5400,14 +5400,16 @@ bool bdrv_is_first_non_filter(BlockDriverState *candidate)
> >  
> >      /* walk down the bs forest recursively */
> >      QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
> > -        bool perm;
> > -
> > -        if (!bs->file) {
> > -            continue;
> > +        bool perm = false;
> > +
> > +        if (bs->file &&
> > +            bs->file->drv &&
> > +            bs->file->drv->authorizations[BS_IS_A_FILTER]) {
> > +            perm = bdrv_recurse_is_first_non_filter(bs->file, candidate);
> > +        } else if (bs == candidate) {
> > +            perm = true;
> >          }
> >  
> > -        perm = bdrv_recurse_is_first_non_filter(bs->file, candidate);
> > -
> >          /* candidate is the first non filter */
> >          if (perm) {
> >              return true;
> 
> With this change, if the top level driver has ->file, its implementation of
> .bdrv_recurse_is_first_non_filter() is skipped and bs->file is the start point.
> 
> So we have an implication that single child block drivers (that has ->file)
> doesn't need to, and shouldn't implement this operation, as commentted above
> bdrv_generic_is_first_non_filter.
> 
> Tested that this patch fixes the external snapshot problem, but didn't test
> the "quorum as bs->file case".
> 
> Thanks,
> 
> Reviewed-by: Fam Zheng <famz@redhat.com>

After extensive testing of all type of quorum instanciations and snapshots I
discovered that we are not done yet with this issue.

When instantiating quorum from the command line the quorum driver is in
bs->file->drv.
When using QMP's blockdev_add at once or by using references the quorum driver
is in bs->drv.

As a result this patch work in half the cases: regular file and command line
quorum but not when quorum is instantiated via QMP.

I don't understand why the block layer have this irregular behavior regarding
quorum instantiation.

What would be the best way to fix this ?

Best regards

Benoît

Re: [Qemu-devel] [FIX V2] block: Fix device snapshots broken by the block filter snapshots patchset.

[Benoît Canet]

The Wednesday 12 Feb 2014 à 20:49:18 (+0100), Benoît Canet wrote :
> The Tuesday 11 Feb 2014 à 16:12:17 (+0800), Fam Zheng wrote :
> > On Mon, 02/10 22:49, Benoît Canet wrote:
> > > Take into account the fact that a block filter like quorum will be in bs->file
> > > while a regular block driver device is really on the top level.
> > > 
> > > Signed-off-by: Benoit Canet <benoit@irqsave.net>
> > > ---
> > >  block.c | 14 ++++++++------
> > >  1 file changed, 8 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/block.c b/block.c
> > > index 07ac50a..d04f535 100644
> > > --- a/block.c
> > > +++ b/block.c
> > > @@ -5400,14 +5400,16 @@ bool bdrv_is_first_non_filter(BlockDriverState *candidate)
> > >  
> > >      /* walk down the bs forest recursively */
> > >      QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
> > > -        bool perm;
> > > -
> > > -        if (!bs->file) {
> > > -            continue;
> > > +        bool perm = false;
> > > +
> > > +        if (bs->file &&
> > > +            bs->file->drv &&
> > > +            bs->file->drv->authorizations[BS_IS_A_FILTER]) {
> > > +            perm = bdrv_recurse_is_first_non_filter(bs->file, candidate);
> > > +        } else if (bs == candidate) {
> > > +            perm = true;
> > >          }
> > >  
> > > -        perm = bdrv_recurse_is_first_non_filter(bs->file, candidate);
> > > -
> > >          /* candidate is the first non filter */
> > >          if (perm) {
> > >              return true;
> > 
> > With this change, if the top level driver has ->file, its implementation of
> > .bdrv_recurse_is_first_non_filter() is skipped and bs->file is the start point.
> > 
> > So we have an implication that single child block drivers (that has ->file)
> > doesn't need to, and shouldn't implement this operation, as commentted above
> > bdrv_generic_is_first_non_filter.
> > 
> > Tested that this patch fixes the external snapshot problem, but didn't test
> > the "quorum as bs->file case".
> > 
> > Thanks,
> > 
> > Reviewed-by: Fam Zheng <famz@redhat.com>
> 
> After extensive testing of all type of quorum instanciations and snapshots I
> discovered that we are not done yet with this issue.
> 
> When instantiating quorum from the command line the quorum driver is in
> bs->file->drv.
> When using QMP's blockdev_add at once or by using references the quorum driver
> is in bs->drv.
> 
> As a result this patch work in half the cases: regular file and command line
> quorum but not when quorum is instantiated via QMP.
> 
> I don't understand why the block layer have this irregular behavior regarding
> quorum instantiation.
> 
> What would be the best way to fix this ?

I have a pending resping of this patch which tests for the two cases as a
workaround.

Should I post it ?

Best regards

Benoît

> 
> Best regards
> 
> Benoît
> 

Re: [Qemu-devel] [FIX V2] block: Fix device snapshots broken by the block filter snapshots patchset.

[Benoît Canet]

The Wednesday 12 Feb 2014 à 22:43:15 (+0100), Benoît Canet wrote :
> The Wednesday 12 Feb 2014 à 20:49:18 (+0100), Benoît Canet wrote :
> > The Tuesday 11 Feb 2014 à 16:12:17 (+0800), Fam Zheng wrote :
> > > On Mon, 02/10 22:49, Benoît Canet wrote:
> > > > Take into account the fact that a block filter like quorum will be in bs->file
> > > > while a regular block driver device is really on the top level.
> > > > 
> > > > Signed-off-by: Benoit Canet <benoit@irqsave.net>
> > > > ---
> > > >  block.c | 14 ++++++++------
> > > >  1 file changed, 8 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/block.c b/block.c
> > > > index 07ac50a..d04f535 100644
> > > > --- a/block.c
> > > > +++ b/block.c
> > > > @@ -5400,14 +5400,16 @@ bool bdrv_is_first_non_filter(BlockDriverState *candidate)
> > > >  
> > > >      /* walk down the bs forest recursively */
> > > >      QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
> > > > -        bool perm;
> > > > -
> > > > -        if (!bs->file) {
> > > > -            continue;
> > > > +        bool perm = false;
> > > > +
> > > > +        if (bs->file &&
> > > > +            bs->file->drv &&
> > > > +            bs->file->drv->authorizations[BS_IS_A_FILTER]) {
> > > > +            perm = bdrv_recurse_is_first_non_filter(bs->file, candidate);
> > > > +        } else if (bs == candidate) {
> > > > +            perm = true;
> > > >          }
> > > >  
> > > > -        perm = bdrv_recurse_is_first_non_filter(bs->file, candidate);
> > > > -
> > > >          /* candidate is the first non filter */
> > > >          if (perm) {
> > > >              return true;
> > > 
> > > With this change, if the top level driver has ->file, its implementation of
> > > .bdrv_recurse_is_first_non_filter() is skipped and bs->file is the start point.
> > > 
> > > So we have an implication that single child block drivers (that has ->file)
> > > doesn't need to, and shouldn't implement this operation, as commentted above
> > > bdrv_generic_is_first_non_filter.
> > > 
> > > Tested that this patch fixes the external snapshot problem, but didn't test
> > > the "quorum as bs->file case".
> > > 
> > > Thanks,
> > > 
> > > Reviewed-by: Fam Zheng <famz@redhat.com>
> > 
> > After extensive testing of all type of quorum instanciations and snapshots I
> > discovered that we are not done yet with this issue.
> > 
> > When instantiating quorum from the command line the quorum driver is in
> > bs->file->drv.
> > When using QMP's blockdev_add at once or by using references the quorum driver
> > is in bs->drv.
> > 
> > As a result this patch work in half the cases: regular file and command line
> > quorum but not when quorum is instantiated via QMP.
> > 
> > I don't understand why the block layer have this irregular behavior regarding
> > quorum instantiation.
> > 
> > What would be the best way to fix this ?
> 
> I have a pending resping of this patch which tests for the two cases as a
> workaround.
> 
> Should I post it ?
> 
> Best regards
> 
> Benoît
> 
> > 
> > Best regards
> > 
> > Benoît
> > 
> 

I found that the used the command line init in a wrong way.
I will repost an updated version of the patch.

Quorum will need to be reposted since one commit message showed the wrong way of
doing the initialization.

Best regards

Benoît

Re: [Qemu-devel] [FIX V2] block: Fix device snapshots broken by the block filter snapshots patchset.

[Kevin Wolf]

Am 12.02.2014 um 20:49 hat Benoît Canet geschrieben:
> The Tuesday 11 Feb 2014 à 16:12:17 (+0800), Fam Zheng wrote :
> > On Mon, 02/10 22:49, Benoît Canet wrote:
> > > Take into account the fact that a block filter like quorum will be in bs->file
> > > while a regular block driver device is really on the top level.
> > > 
> > > Signed-off-by: Benoit Canet <benoit@irqsave.net>
> > > ---
> > >  block.c | 14 ++++++++------
> > >  1 file changed, 8 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/block.c b/block.c
> > > index 07ac50a..d04f535 100644
> > > --- a/block.c
> > > +++ b/block.c
> > > @@ -5400,14 +5400,16 @@ bool bdrv_is_first_non_filter(BlockDriverState *candidate)
> > >  
> > >      /* walk down the bs forest recursively */
> > >      QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
> > > -        bool perm;
> > > -
> > > -        if (!bs->file) {
> > > -            continue;
> > > +        bool perm = false;
> > > +
> > > +        if (bs->file &&
> > > +            bs->file->drv &&
> > > +            bs->file->drv->authorizations[BS_IS_A_FILTER]) {
> > > +            perm = bdrv_recurse_is_first_non_filter(bs->file, candidate);
> > > +        } else if (bs == candidate) {
> > > +            perm = true;
> > >          }
> > >  
> > > -        perm = bdrv_recurse_is_first_non_filter(bs->file, candidate);
> > > -
> > >          /* candidate is the first non filter */
> > >          if (perm) {
> > >              return true;
> > 
> > With this change, if the top level driver has ->file, its implementation of
> > .bdrv_recurse_is_first_non_filter() is skipped and bs->file is the start point.
> > 
> > So we have an implication that single child block drivers (that has ->file)
> > doesn't need to, and shouldn't implement this operation, as commentted above
> > bdrv_generic_is_first_non_filter.
> > 
> > Tested that this patch fixes the external snapshot problem, but didn't test
> > the "quorum as bs->file case".
> > 
> > Thanks,
> > 
> > Reviewed-by: Fam Zheng <famz@redhat.com>
> 
> After extensive testing of all type of quorum instanciations and snapshots I
> discovered that we are not done yet with this issue.
> 
> When instantiating quorum from the command line the quorum driver is in
> bs->file->drv.
> When using QMP's blockdev_add at once or by using references the quorum driver
> is in bs->drv.

I'm pretty sure this is a user error.

Command line and blockdev-add are pretty much equivalent, they even use
exactly the same code path once they have converted their input into an
options QDict.

Both ways should be able to instantiate both setups you mentioned. This
is required, because these setups have different semantics: quorum above
qcow2 redirects and compares the guest data, whereas below qcow2 it
redirects image file accesses including metadata).

Kevin