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 <ben...@irqsave.net>
> > >
> > > Signed-off-by: Benoit Canet <ben...@irqsave.net>
> > > ---
> > > 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) {
>
