| Message ID | 20190809161407.11920-21-mreitz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: Deal with filters | expand |
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? 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_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); > + *child_pointer = NULL; > + > + ret = bdrv_snapshot_goto(fallback_bs, snapshot_id, errp); > open_ret = drv->bdrv_open(bs, options, bs->open_flags, &local_err); > qobject_unref(options); > if (open_ret < 0) { > - bdrv_unref(file); > + bdrv_unref(fallback_bs); > bs->drv = NULL; > /* A bdrv_snapshot_goto() error takes precedence */ > error_propagate(errp, local_err); > return ret < 0 ? ret : open_ret; > } > > - assert(bs->file->bs == file); > - bdrv_unref(file); > + assert(fallback_bs == (*child_pointer)->bs); > + bdrv_unref(fallback_bs); > return ret; > } > > @@ -272,6 +328,7 @@ int bdrv_snapshot_delete(BlockDriverState *bs, > Error **errp) > { > BlockDriver *drv = bs->drv; > + BlockDriverState *fallback_bs = bdrv_snapshot_fallback(bs); > int ret; > > if (!drv) { > @@ -288,8 +345,8 @@ int bdrv_snapshot_delete(BlockDriverState *bs, > > if (drv->bdrv_snapshot_delete) { > ret = drv->bdrv_snapshot_delete(bs, snapshot_id, name, errp); > - } else if (bs->file) { > - ret = bdrv_snapshot_delete(bs->file->bs, snapshot_id, name, errp); > + } else if (fallback_bs) { > + ret = bdrv_snapshot_delete(fallback_bs, snapshot_id, name, errp); > } else { > error_setg(errp, "Block format '%s' used by device '%s' " > "does not support internal snapshot deletion", > @@ -305,14 +362,15 @@ int bdrv_snapshot_list(BlockDriverState *bs, > QEMUSnapshotInfo **psn_info) > { > BlockDriver *drv = bs->drv; > + BlockDriverState *fallback_bs = bdrv_snapshot_fallback(bs); > if (!drv) { > return -ENOMEDIUM; > } > if (drv->bdrv_snapshot_list) { > return drv->bdrv_snapshot_list(bs, psn_info); > } > - if (bs->file) { > - return bdrv_snapshot_list(bs->file->bs, psn_info); > + if (fallback_bs) { > + return bdrv_snapshot_list(fallback_bs, psn_info); > } > return -ENOTSUP; > } >
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
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? Kevin
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. 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
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? Kevin
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
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); + *child_pointer = NULL; + + ret = bdrv_snapshot_goto(fallback_bs, snapshot_id, errp); open_ret = drv->bdrv_open(bs, options, bs->open_flags, &local_err); qobject_unref(options); if (open_ret < 0) { - bdrv_unref(file); + bdrv_unref(fallback_bs); bs->drv = NULL; /* A bdrv_snapshot_goto() error takes precedence */ error_propagate(errp, local_err); return ret < 0 ? ret : open_ret; } - assert(bs->file->bs == file); - bdrv_unref(file); + assert(fallback_bs == (*child_pointer)->bs); + bdrv_unref(fallback_bs); return ret; } @@ -272,6 +328,7 @@ int bdrv_snapshot_delete(BlockDriverState *bs, Error **errp) { BlockDriver *drv = bs->drv; + BlockDriverState *fallback_bs = bdrv_snapshot_fallback(bs); int ret; if (!drv) { @@ -288,8 +345,8 @@ int bdrv_snapshot_delete(BlockDriverState *bs, if (drv->bdrv_snapshot_delete) { ret = drv->bdrv_snapshot_delete(bs, snapshot_id, name, errp); - } else if (bs->file) { - ret = bdrv_snapshot_delete(bs->file->bs, snapshot_id, name, errp); + } else if (fallback_bs) { + ret = bdrv_snapshot_delete(fallback_bs, snapshot_id, name, errp); } else { error_setg(errp, "Block format '%s' used by device '%s' " "does not support internal snapshot deletion", @@ -305,14 +362,15 @@ int bdrv_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_info) { BlockDriver *drv = bs->drv; + BlockDriverState *fallback_bs = bdrv_snapshot_fallback(bs); if (!drv) { return -ENOMEDIUM; } if (drv->bdrv_snapshot_list) { return drv->bdrv_snapshot_list(bs, psn_info); } - if (bs->file) { - return bdrv_snapshot_list(bs->file->bs, psn_info); + if (fallback_bs) { + return bdrv_snapshot_list(fallback_bs, psn_info); } return -ENOTSUP; }
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(-)