Re: [Qemu-devel] [PATCH v6 20/42] block/snapshot: Fix fallback

[Kevin Wolf]

Am 10.09.2019 um 14:04 hat Max Reitz geschrieben:
> On 10.09.19 13:56, Kevin Wolf wrote:
> > Am 09.08.2019 um 18:13 hat Max Reitz geschrieben:
> >> If the top node's driver does not provide snapshot functionality and we
> >> want to fall back to a node down the chain, we need to snapshot all
> >> non-COW children.  For simplicity's sake, just do not fall back if there
> >> is more than one such child.
> >>
> >> bdrv_snapshot_goto() becomes a bit weird because we may have to redirect
> >> the actual child pointer, so it only works if the fallback child is
> >> bs->file or bs->backing (and then we have to find out which it is).
> >>
> >> Suggested-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> >> Signed-off-by: Max Reitz <address@hidden>
> >> ---
> >>  block/snapshot.c | 100 +++++++++++++++++++++++++++++++++++++----------
> >>  1 file changed, 79 insertions(+), 21 deletions(-)
> >>
> >> diff --git a/block/snapshot.c b/block/snapshot.c
> >> index f2f48f926a..35403c167f 100644
> >> --- a/block/snapshot.c
> >> +++ b/block/snapshot.c
> >> @@ -146,6 +146,32 @@ bool 
> >> bdrv_snapshot_find_by_id_and_name(BlockDriverState *bs,
> >>      return ret;
> >>  }
> >>  
> >> +/**
> >> + * Return the child BDS to which we can fall back if the given BDS
> >> + * does not support snapshots.
> >> + * Return NULL if there is no BDS to (safely) fall back to.
> >> + */
> >> +static BlockDriverState *bdrv_snapshot_fallback(BlockDriverState *bs)
> >> +{
> >> +    BlockDriverState *child_bs = NULL;
> >> +    BdrvChild *child;
> >> +
> >> +    QLIST_FOREACH(child, &bs->children, next) {
> >> +        if (child == bdrv_filtered_cow_child(bs)) {
> >> +            /* Ignore: COW children need not be included in snapshots */
> >> +            continue;
> >> +        }
> >> +
> >> +        if (child_bs) {
> >> +            /* Cannot fall back to a single child if there are multiple */
> >> +            return NULL;
> >> +        }
> >> +        child_bs = child->bs;
> >> +    }
> >> +
> >> +    return child_bs;
> >> +}
> > 
> > Why do we return child->bs here when bdrv_snapshot_goto() then needs to
> > reconstruct what the associated BdrvChild was? Wouldn't it make more
> > sense to return BdrvChild** from here and maybe have a small wrapper for
> > the other functions that only need a BDS?
> 
> What would you return instead?  &child doesn’t work.

Oops, brain fart. :-)

> We could limit ourselves to bs->file and bs->backing.  It just seemed
> like a bit of an artificial limit to me, because we only really have it
> for bdrv_snapshot_goto().

Hm, but then, what use is supporting other children for creating a
snapshot when you can't load it any more afterwards?

Kevin

signature.asc
Description: PGP signature