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

2019-09-10 Thread Max Reitz 
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 
 Signed-off-by: Max Reitz 
 ---
  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



signature.asc
Description: OpenPGP digital signature

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

2019-09-10 Thread 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 
> >> Signed-off-by: Max Reitz 
> >> ---
> >>  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

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

2019-09-10 Thread Max Reitz 
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 
>> Signed-off-by: Max Reitz 
>> ---
>>  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.

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().

Max



signature.asc
Description: OpenPGP digital signature

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

2019-09-10 Thread Kevin Wolf 
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 
> Signed-off-by: Max Reitz 
> ---
>  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?

Kevin

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

2019-08-12 Thread Max Reitz 
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 
>> Signed-off-by: Max Reitz 
>> ---
>>   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

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

2019-08-10 Thread Vladimir Sementsov-Ogievskiy 
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 
> Signed-off-by: Max Reitz 
> ---
>   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?

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


>   
> -file = bs->file->bs;
>   /* Prevent it from getting deleted when detached from bs */
> -bdrv_ref(file);
> +bdrv_ref(fallback_bs);
>   
> -qdict_extract_subqdict(options, &file_options, "file.");
> +qdict_extract_subqdict(options, &file_options,
> +   is_backing_child ? "backing." : "file.");
>   qobject_unr

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

2019-08-09 Thread Max Reitz 
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 
Signed-off-by: Max Reitz 
---
 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;
+}
+
+options = qdict_clone_shallow(bs->options);
 
-file = bs->file->bs;
 /* Prevent it from getting deleted when detached from bs */
-bdrv_ref(file);
+bdrv_ref(fallback_bs);
 
-qdict_extract_subqdict(options, &file_options, "file.");
+qdict_extract_subqdict(options, &file_options,
+   is_backing_child ? "backing." : "file.");
 qobject_unref(file_options);
-qdict_put_str(options, "file", bdrv_get_node_name(file));
+qdict_put_str(options, is_backing_child ? "backing" : "file",
+  bdrv_get_node_name(fallback_bs));
 
 if (drv->bdrv_close) {
 drv->bdrv_close(bs);
 }
-bdrv_unref_child(bs, bs->file);
-bs->file = NULL;
 
-ret = bdrv_snapshot_goto(file, snapshot_id, errp);
+assert(fallback_bs == (*child_pointer)->bs);
+bdrv_unref_child(bs, *child_pointer);
+