On 14.06.19 17:22, Vladimir Sementsov-Ogievskiy wrote:
> 13.06.2019 1:09, Max Reitz wrote:
>> If the top node's driver does not provide snapshot functionality and we
>> want to go down the chain, we should go towards the child which stores
>> the data, i.e. the storage child.
>>
>> bdrv_snapshot_goto() becomes a bit weird because we may have to redirect
>> the actual child pointer, so it only works if the storage child is
>> bs->file or bs->backing (and then we have to find out which it is).
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   block/snapshot.c | 74 ++++++++++++++++++++++++++++++++++--------------
>>   1 file changed, 53 insertions(+), 21 deletions(-)
>>
>> diff --git a/block/snapshot.c b/block/snapshot.c
>> index f2f48f926a..58cd667f3a 100644
>> --- a/block/snapshot.c
>> +++ b/block/snapshot.c
>> @@ -154,8 +154,9 @@ int bdrv_can_snapshot(BlockDriverState *bs)
>>       }
>>   
>>       if (!drv->bdrv_snapshot_create) {
>> -        if (bs->file != NULL) {
>> -            return bdrv_can_snapshot(bs->file->bs);
>> +        BlockDriverState *storage_bs = bdrv_storage_bs(bs);
>> +        if (storage_bs) {
>> +            return bdrv_can_snapshot(storage_bs);
>>           }
>>           return 0;
>>       }
> 
> Hmm is it correct at all doing a snapshot, when top format node doesn't support it,
> metadata child doesn't support it and storage child supports? Doing snapshots of
> storage child seems useless, as data file must be in sync with metadata.

You’re right.

That’s actually a bug already.  VMDK can store data in multiple
children, but it does not support snapshots.  So if you store such a
split VMDK disk on an RBD volume, it is possible that just the
descriptor file is snapshotted, but nothing else.

Hmmm.  I think the best way is to check whether there is exactly one
child that is not the bdrv_filtered_cow_child().  If so, we can go down
to it and snapshot it.  Otherwise, the node does not support snapshots.

Max