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 <address@hidden>
> > > ---
> > >  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 <address@hidden>
> 
> 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