Re: [Qemu-devel] [PATCH v5 20/42] block/snapshot: Fall back to storage child

[Vladimir Sementsov-Ogievskiy]

14.06.2019 19:10, Max Reitz wrote:
> 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 <address@hidden>
>>> ---
>>>    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.
> 

Anything prevents format node to store something not in a bdrv child but in 
something separate?

May be the safest way is to fall back only for filter nodes.


-- 
Best regards,
Vladimir