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

[Max Reitz <mreitz@redhat.com>]

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

On 10.08.19 18:34, Vladimir Sementsov-Ogievskiy wrote:
> 09.08.2019 19:13, Max Reitz wrote:
>> 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;
>> +}
>> +
>>   int bdrv_can_snapshot(BlockDriverState *bs)
>>   {
>>       BlockDriver *drv = bs->drv;
>> @@ -154,8 +180,9 @@ int bdrv_can_snapshot(BlockDriverState *bs)
>>       }
>>   
>>       if (!drv->bdrv_snapshot_create) {
>> -        if (bs->file != NULL) {
>> -            return bdrv_can_snapshot(bs->file->bs);
>> +        BlockDriverState *fallback_bs = bdrv_snapshot_fallback(bs);
>> +        if (fallback_bs) {
>> +            return bdrv_can_snapshot(fallback_bs);
>>           }
>>           return 0;
>>       }
>> @@ -167,14 +194,15 @@ int bdrv_snapshot_create(BlockDriverState *bs,
>>                            QEMUSnapshotInfo *sn_info)
>>   {
>>       BlockDriver *drv = bs->drv;
>> +    BlockDriverState *fallback_bs = bdrv_snapshot_fallback(bs);
>>       if (!drv) {
>>           return -ENOMEDIUM;
>>       }
>>       if (drv->bdrv_snapshot_create) {
>>           return drv->bdrv_snapshot_create(bs, sn_info);
>>       }
>> -    if (bs->file) {
>> -        return bdrv_snapshot_create(bs->file->bs, sn_info);
>> +    if (fallback_bs) {
>> +        return bdrv_snapshot_create(fallback_bs, sn_info);
>>       }
>>       return -ENOTSUP;
>>   }
>> @@ -184,6 +212,7 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
>>                          Error **errp)
>>   {
>>       BlockDriver *drv = bs->drv;
>> +    BlockDriverState *fallback_bs;
>>       int ret, open_ret;
>>   
>>       if (!drv) {
>> @@ -204,39 +233,66 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
>>           return ret;
>>       }
>>   
>> -    if (bs->file) {
>> -        BlockDriverState *file;
>> -        QDict *options = qdict_clone_shallow(bs->options);
>> +    fallback_bs = bdrv_snapshot_fallback(bs);
>> +    if (fallback_bs) {
>> +        QDict *options;
>>           QDict *file_options;
>>           Error *local_err = NULL;
>> +        bool is_backing_child;
>> +        BdrvChild **child_pointer;
>> +
>> +        /*
>> +         * We need a pointer to the fallback child pointer, so let us
>> +         * see whether the child is referenced by a field in the BDS
>> +         * object.
>> +         */
>> +        if (fallback_bs == bs->file->bs) {
>> +            is_backing_child = false;
>> +            child_pointer = &bs->file;
>> +        } else if (fallback_bs == bs->backing->bs) {
>> +            is_backing_child = true;
>> +            child_pointer = &bs->backing;
>> +        } else {
>> +            /*
>> +             * The fallback child is not referenced by a field in the
>> +             * BDS object.  We cannot go on then.
>> +             */
>> +            error_setg(errp, "Block driver does not support snapshots");
>> +            return -ENOTSUP;
>> +        }
>> +
> 
> Hmm.. Should not this check be included into bdrv_snapshot_fallback(), to
> work only with file and backing?

I was under the impression that this was just special code for what
turned out to be bdrv_snapshot_load_tmp() now, because it seems so
weird.  (So I thought just making the restriction here wouldn’t really
be a limit.)

I was wrong.  This is used when applying snapshots, so it is important.
 If we make a restriction here, we should have it in all fallback code, yes.

> And could we allow fallback only for filters? Is there real usecase except filters?
> Or may be, drop fallback at all?

raw isn’t a filter driver.  And rbd as a protocol supports snapshotting.
 Hence the fallback code, I presume.

Max


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