Re: [Qemu-devel] [PATCH V6 5/8] block: Create authorizations mechanism for external snapshot and resize.

[Benoît Canet]

Le Tuesday 04 Feb 2014 à 11:25:52 (+0100), Kevin Wolf a écrit :
> Am 04.02.2014 um 01:15 hat Jeff Cody geschrieben:
> > On Thu, Jan 23, 2014 at 09:31:36PM +0100, Benoît Canet wrote:
> > > From: Benoît Canet <address@hidden>
> > > 
> > > Signed-off-by: Benoit Canet <address@hidden>
> > > ---
> > >  block.c                   | 65 
> > > ++++++++++++++++++++++++++++++++++++++++-------
> > >  block/blkverify.c         |  2 +-
> > >  blockdev.c                |  2 +-
> > >  include/block/block.h     | 20 +++++++--------
> > >  include/block/block_int.h | 12 ++++++---
> > >  5 files changed, 77 insertions(+), 24 deletions(-)
> > > 
> > > diff --git a/block.c b/block.c
> > > index e1bc732..3e0994b 100644
> > > --- a/block.c
> > > +++ b/block.c
> > > @@ -5088,21 +5088,68 @@ int bdrv_amend_options(BlockDriverState *bs, 
> > > QEMUOptionParameter *options)
> > >      return bs->drv->bdrv_amend_options(bs, options);
> > >  }
> > >  
> > > -ExtSnapshotPerm bdrv_check_ext_snapshot(BlockDriverState *bs)
> > > +/* Used to recurse on single child block filters.
> > > + * Single child block filter will store their child in bs->file.
> > > + */
> > > +bool bdrv_generic_is_first_non_filter(BlockDriverState *bs,
> > > +                                      BlockDriverState *candidate)
> > >  {
> > > -    if (bs->drv->bdrv_check_ext_snapshot) {
> > > -        return bs->drv->bdrv_check_ext_snapshot(bs);
> > > +    if (!bs->drv) {
> > > +        return false;
> > > +    }
> > > +
> > > +    if (!bs->drv->authorizations[BS_IS_A_FILTER]) {
> > > +        if (bs == candidate) {
> > > +            return true;
> > > +        } else {
> > > +            return false;
> > > +        }
> > 
> > This seems to break external snapshots; after this patch, I can no
> > longer perform live ext snapshots (on qcow2, raw, etc..), unless I am
> > doing something incorrectly.
> > 
> > Instead of checking for bs == candidate, was it intended to check to
> > see if !strcmp(bs->filename, candidiate->filename) was true?
> 
> If believe the problem is in bdrv_is_first_non_filter(): It starts with
> bs->file, whereas the first non-filter is obviously the top-level BDS
> itself. I suspect the following patch fixes it (it makes the simple
> snapshotting case work again, but I'm not sure if it forbids everything
> that should be forbidden).
> 
> In any case, this shows that...
> 
> - ...our testing is still lacking (no qemu-iotests case for live
>   snapshots? Seriously? Expect it to be broken then.)
> 
> - ...not all patch authors do a good share of manual testing
> 
> - ...I have relaxed my reviewing too much. I wasn't convinced that this
>   patch is right, because the whole logic confused me, but I couldn't
>   point to a bug. I shouldn't have merged it when in doubt.
> 
> Jeff, would you like to submit a qemu-iotests case for snapshotting?
> 
> Benoît, can you check whether the patch below is correct?
> 
> Kevin

I have a patch that I will post.

I was confused by the fact that when a quorum driver is running it's not on the
top of the tree but on bs->file.

I tested regular snapshot and quorum single file snapshots and it works with the
new patch.

Best regards

Benoît

> 
> 
> diff --git a/block.c b/block.c
> index ac0ccac..1299484 100644
> --- a/block.c
> +++ b/block.c
> @@ -5412,11 +5412,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) {
>