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

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3409 bytes --]

On 10.09.19 14:49, Kevin Wolf wrote:
> 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 <vsementsov@virtuozzo.com>
>>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>>> ---
>>>>  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?

Well, the snapshot is still there, it’s just on a different node.  So in
theory, you could take a snapshot in a live VM (where the snapshotting
node is not at the top), and then later revert to it with qemu-img (by
accessing the file with the snapshot directly).

Though in practice this is just a fallback anyway, and I don’t think we
currently have anything where it would make sense to fall through some
node to any child but .file or .backing.  So why not.  It would be
shorter, too.  (Well, plus the short comment why looking at .file and
.backing is sufficient.)

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]