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

[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