Add 'blockdev-snapshot' command
PATCH v2 2/2 block: add a 'blockdev-snapshot' QMP command
In reply to Alberto GarciaOne of the limitations of the 'blockdev-snapshot-sync' command is that it does not allow passing BlockdevOptions to the newly created snapshots, so they are always opened using the default values.
Extending the command to allow passing options is not a practical solution because there is overlap between those options and some of the existing parameters of the command.
This patch introduces a new 'blockdev-snapshot' command with a simpler
interface: it just takes two references to existing block devices that will be used as the source and target for the snapshot.
Since the main difference between the two commands is that one of them creates and opens the target image, while the other uses an already opened one, the bulk of the implementation is shared.
--- blockdev.c | 154 ++++++++++++++++++++++++++++++++------------------- qapi-schema.json | 1 + qapi/block-core.json | 26 +++++++++ qmp-commands.hx | 28 ++++++++++ 4 files changed, 153 insertions(+), 56 deletions(-)
blockdev.c +98 -56
@@ -1183,6 +1183,18 @@ void qmp_blockdev_snapshot_sync(bool has_device, const char *device, &snapshot, errp); } +void qmp_blockdev_snapshot (const char *device,const char *snapshot, +Error **errp) +{ +BlockdevSnapshot snapshot_data = { + .device = (char *) device, + .snapshot = (char *) snapshot + }; + +blockdev_do_action (TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT , + &snapshot_data, errp); +} +void qmp_blockdev_snapshot_internal_sync (const char *device,const char *name,Error **errp)
@@ -1521,60 +1533,52 @@ typedef struct ExternalSnapshotState {static void external_snapshot_prepare (BlkTransactionState *common,Error **errp) { -int flags, ret; -QDict *options; +int flags =0 , ret; +QDict *options =NULL ;Error *local_err =NULL ; -bool has_device = false; +/* Device and node name of the image to generate the snapshot from */ const char *device; -bool has_node_name = false;const char *node_name; -bool has_snapshot_node_name = false; -const char *snapshot_node_name; +/* Reference to the new image (for 'blockdev-snapshot') */ +const char *snapshot_ref; +/* File name of the new image (for 'blockdev-snapshot-sync') */ const char *new_image_file; -const char *format ="qcow2" ; -enum NewImageMode mode =NEW_IMAGE_MODE_ABSOLUTE_PATHS ;ExternalSnapshotState *state =DO_UPCAST (ExternalSnapshotState, common, common);TransactionAction *action = common->action; -/* get parameters */ -g_assert (action->kind ==TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC ); - - has_device = action->blockdev_snapshot_sync->has_device; - device = action->blockdev_snapshot_sync->device; - has_node_name = action->blockdev_snapshot_sync->has_node_name; - node_name = action->blockdev_snapshot_sync->node_name; - has_snapshot_node_name = - action->blockdev_snapshot_sync->has_snapshot_node_name; - snapshot_node_name = action->blockdev_snapshot_sync->snapshot_node_name; - - new_image_file = action->blockdev_snapshot_sync->snapshot_file; -if (action->blockdev_snapshot_sync->has_format) { - format = action->blockdev_snapshot_sync->format; - } -if (action->blockdev_snapshot_sync->has_mode) { - mode = action->blockdev_snapshot_sync->mode; +/* 'blockdev-snapshot' and 'blockdev-snapshot-sync' have similar +* purpose but a different set of parameters */ +switch (action->kind) { +case TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT : + { +BlockdevSnapshot *s = action->blockdev_snapshot; + device = s->device; + node_name = s->device; + new_image_file =NULL ; + snapshot_ref = s->snapshot; + } +break ; +case TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC : + { +BlockdevSnapshotSync *s = action->blockdev_snapshot_sync; + device = s->has_device ? s->device :NULL ; + node_name = s->has_node_name ? s->node_name :NULL ; + new_image_file = s->snapshot_file; + snapshot_ref =NULL ; + } +break ; +default : +g_assert_not_reached (); }/* start processing */ - state->old_bs =bdrv_lookup_bs (has_device ? device :NULL , - has_node_name ? node_name :NULL , - &local_err); + state->old_bs =bdrv_lookup_bs (device, node_name, &local_err);if (local_err) {error_propagate (errp, local_err);return ; } -if (has_node_name && !has_snapshot_node_name) { -error_setg (errp,"New snapshot node name missing" ); -return ; - } - -if (has_snapshot_node_name &&bdrv_find_node (snapshot_node_name)) { -error_setg (errp,"New snapshot node name already existing" ); -return ; - } -/* Acquire AioContext now so any threads operating on old_bs stop */ state->aio_context =bdrv_get_aio_context (state->old_bs);aio_context_acquire (state->aio_context);
@@ -1601,35 +1605,67 @@ static void external_snapshot_prepare(BlkTransactionState *common,return ; } - flags = state->old_bs->open_flags; - -/* create new image w/backing file */ -if (mode !=NEW_IMAGE_MODE_EXISTING ) { -bdrv_img_create (new_image_file, format, - state->old_bs->filename, - state->old_bs->drv->format_name, -NULL ,-1 , flags, &local_err, false); -if (local_err) { +if (snapshot_ref) { +if (!bdrv_lookup_bs (snapshot_ref, snapshot_ref, &local_err)) {error_propagate (errp, local_err);return ; } } - options =qdict_new (); -if (has_snapshot_node_name) { -qdict_put (options,"node-name" , -qstring_from_str (snapshot_node_name)); +if (action->kind ==TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC ) { +BlockdevSnapshotSync *s = action->blockdev_snapshot_sync; +const char *format = s->has_format ? s->format :"qcow2" ; +enum NewImageMode mode; +const char *snapshot_node_name = + s->has_snapshot_node_name ? s->snapshot_node_name :NULL ; + +if (node_name && !snapshot_node_name) { +error_setg (errp,"New snapshot node name missing" ); +return ; + } + +if (snapshot_node_name &&bdrv_find_node (snapshot_node_name)) { +error_setg (errp,"New snapshot node name already existing" ); +return ; + } + + flags = state->old_bs->open_flags; + +/* create new image w/backing file */ + mode = s->has_mode ? s->mode :NEW_IMAGE_MODE_ABSOLUTE_PATHS ; +if (mode !=NEW_IMAGE_MODE_EXISTING ) { +bdrv_img_create (new_image_file, format, + state->old_bs->filename, + state->old_bs->drv->format_name, +NULL ,-1 , flags, &local_err, false); +if (local_err) { +error_propagate (errp, local_err); +return ; + } + } + + options =qdict_new (); +if (s->has_snapshot_node_name) { +qdict_put (options,"node-name" , +qstring_from_str (snapshot_node_name)); + } +qdict_put (options,"driver" ,qstring_from_str (format)); + + flags |=BDRV_O_NO_BACKING ; } -qdict_put (options,"driver" ,qstring_from_str (format)); -/* TODO Inherit bs->options or only take explicit options with an -* extended QMP command? */ assert (state->new_bs ==NULL ); - ret =bdrv_open (&state->new_bs, new_image_file,NULL , options, - flags |BDRV_O_NO_BACKING , &local_err); + ret =bdrv_open (&state->new_bs, new_image_file, snapshot_ref, options, + flags, &local_err);/* We will manually add the backing_hd field to the bs later */ if (ret !=0 ) {error_propagate (errp, local_err); +return ; + } + +if (state->new_bs->blk !=NULL ) { +error_setg (errp,"The snapshot is already in use by %s" , +blk_name (state->new_bs->blk)); } }
@@ -1818,6 +1854,12 @@ static void abort_commit(BlkTransactionState *common) }static const BdrvActionOps actions[] = { + [TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT ] = { + .instance_size =sizeof (ExternalSnapshotState), + .prepare = external_snapshot_prepare, + .commit = external_snapshot_commit, + .abort = external_snapshot_abort, + }, [TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC ] = { .instance_size =sizeof (ExternalSnapshotState), .prepare = external_snapshot_prepare,
qapi-schema.json +1 -0
@@ -1496,6 +1496,7 @@ ## { 'union': 'TransactionAction', 'data': { + 'blockdev-snapshot': 'BlockdevSnapshot', 'blockdev-snapshot-sync': 'BlockdevSnapshotSync', 'drive-backup': 'DriveBackup', 'blockdev-backup': 'BlockdevBackup',
qapi/block-core.json +26 -0
@@ -705,6 +705,19 @@ '*format': 'str', '*mode': 'NewImageMode' } } ## +# @BlockdevSnapshot +# +# @device: device or node name to generate the snapshot from. +# +# @snapshot: reference to the existing block device that will be used +# for the snapshot. +# +# Since 2.5 +## +{ 'struct': 'BlockdevSnapshot', + 'data': { 'device': 'str', 'snapshot': 'str' } } + +## # @DriveBackup # # @device: the name of the device which should be copied.
@@ -800,6 +813,19 @@ { 'command': 'blockdev-snapshot-sync', 'data': 'BlockdevSnapshotSync' } + +## +# @blockdev-snapshot +# +# Generates a snapshot of a block device. +# +# For the arguments, see the documentation of BlockdevSnapshot. +# +# Since 2.5 +## +{ 'command': 'blockdev-snapshot', + 'data': 'BlockdevSnapshot' } + ## # @change-backing-file #
qmp-commands.hx +28 -0
@@ -1454,6 +1454,34 @@ Example: EQMP { + .name = "blockdev-snapshot", + .args_type = "device:s,snapshot:s", + .mhandler.cmd_new = qmp_marshal_input_blockdev_snapshot, + }, + +SQMP +blockdev-snapshot +----------------- +Since 2.5 + +Create a snapshot of a block device. 'device' and 'snapshot' both +refer to existing block devices. The former is the one to generate the +snapshot from, and the latter is the target of the snapshot. + +Arguments: + +- "device": snapshot source (json-string) +- "snapshot": snapshot target (json-string) + +Example: + +-> { "execute": "blockdev-snapshot", "arguments": { "device": "ide-hd0", + "snapshot": "node1534" } } +<- { "return": {} } + +EQMP + + { .name = "blockdev-snapshot-internal-sync", .args_type = "device:B,name:s", .mhandler.cmd_new = qmp_marshal_input_blockdev_snapshot_internal_sync,
Signature · 1 line
2.5.1
v2 cover Add 'blockdev-snapshot' command
Previous attempt to extend blockdev-snapshot-sync:
https://lists.gnu.org/archive/html/qemu-block/2015-08/msg00236.html
As discussed in the previous thread, this adds a new command called 'blockdev-snapshot', that simply takes references to two existing block devices. I decided to share most of the implementation with the previous 'blockdev-snapshot-sync' since the only differences are in the 'prepare' step.
The tests are missing, but I wanted to know first if you are otherwise OK with this approach. If the patches look good I'll write the tests next week.
These patches apply on top of two series from Max:
- This one allows creating BDS that are not attached to a BlockBackend
https://lists.gnu.org/archive/html/qemu-block/2015-07/msg00382.html
- This one drops the drv parameter from bdrv_open(), which was preventing us from creating snapshots of images opened with custom runtime options:
https://lists.gnu.org/archive/html/qemu-block/2015-08/msg00189.html
You cannot apply these two series cleanly on top of each other, so if you want to try my patches without having to merge everything manually, you can get the whole thing here:
https://github.com/bertogg/qemu/commits/blockdev-snapshot
Comments are welcome!
Berto
Alberto Garcia (2):
block: rename BlockdevSnapshot to BlockdevSnapshotSync
block: add a 'blockdev-snapshot' QMP command
blockdev.c | 156 ++++++++++++++++++++++++++++++++------------------- qapi-schema.json | 3 +- qapi/block-core.json | 32 ++++++++++- qmp-commands.hx | 28 +++++++++ 4 files changed, 158 insertions(+), 61 deletions(-)
Signature · 1 line
2.5.1
PATCH v2 1/2 block: rename BlockdevSnapshot to BlockdevSnapshotSync
We will introduce the 'blockdev-snapshot' command that will require its own struct for the parameters, so we need to rename this one in order to avoid name clashes.
--- blockdev.c | 2 +- qapi-schema.json | 2 +- qapi/block-core.json | 8 ++++---- 3 files changed, 6 insertions(+), 6 deletions(-)
blockdev.c +1 -1
@@ -1166,7 +1166,7 @@ void qmp_blockdev_snapshot_sync(bool has_device, const char *device, bool has_format, const char *format, bool has_mode, NewImageMode mode, Error **errp) { - BlockdevSnapshot snapshot = { + BlockdevSnapshotSync snapshot = { .has_device = has_device, .device = (char *) device, .has_node_name = has_node_name,
qapi-schema.json +1 -1
@@ -1496,7 +1496,7 @@ ## { 'union': 'TransactionAction', 'data': { - 'blockdev-snapshot-sync': 'BlockdevSnapshot', + 'blockdev-snapshot-sync': 'BlockdevSnapshotSync', 'drive-backup': 'DriveBackup', 'blockdev-backup': 'BlockdevBackup', 'abort': 'Abort',
qapi/block-core.json +4 -4
@@ -682,7 +682,7 @@ 'data': [ 'existing', 'absolute-paths' ] } ## -# @BlockdevSnapshot +# @BlockdevSnapshotSync # # Either @device or @node-name must be set but not both. #
@@ -699,7 +699,7 @@ # @mode: #optional whether and how QEMU should create a new image, default is # 'absolute-paths'. ## -{ 'struct': 'BlockdevSnapshot', +{ 'struct': 'BlockdevSnapshotSync', 'data': { '*device': 'str', '*node-name': 'str', 'snapshot-file': 'str', '*snapshot-node-name': 'str', '*format': 'str', '*mode': 'NewImageMode' } }
@@ -790,7 +790,7 @@ # # Generates a synchronous snapshot of a block device. # -# For the arguments, see the documentation of BlockdevSnapshot. +# For the arguments, see the documentation of BlockdevSnapshotSync. # # Returns: nothing on success # If @device is not a valid block device, DeviceNotFound
@@ -798,7 +798,7 @@ # Since 0.14.0 ## { 'command': 'blockdev-snapshot-sync', - 'data': 'BlockdevSnapshot' } + 'data': 'BlockdevSnapshotSync' } ## # @change-backing-file
Signature · 1 line
2.5.1
Re: [Qemu-devel] [PATCH v2 1/2] block: rename BlockdevSnapshot to BlockdevSnapshotSync
Quoted 10 lines · Alberto Garcia
We will introduce the 'blockdev-snapshot' command that will require its own struct for the parameters, so we need to rename this one in order to avoid name clashes.
Signed-off-by Alberto Garcia <[email protected]>--- blockdev.c | 2 +- qapi-schema.json | 2 +- qapi/block-core.json | 8 ++++---- 3 files changed, 6 insertions(+), 6 deletions(-)
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [PATCH v2 2/2] block: add a 'blockdev-snapshot' QMP command
In reply to Alberto GarciaQuoted 8 lines · Alberto Garcia
One of the limitations of the 'blockdev-snapshot-sync' command is that it does not allow passing BlockdevOptions to the newly created snapshots, so they are always opened using the default values.
Extending the command to allow passing options is not a practical solution because there is overlap between those options and some of the existing parameters of the command.
Quoted 7 lines
This patch introduces a new 'blockdev-snapshot' command with a simpler
interface: it just takes two references to existing block devices that will be used as the source and target for the snapshot.Since the main difference between the two commands is that one of them creates and opens the target image, while the other uses an already opened one, the bulk of the implementation is shared.
I like it. Obviously, we have to get the pre-requisites in first, to allow opening a BDS without a BB, but it is nicer overall.
Quoted 14 lines
Signed-off-by Alberto Garcia <[email protected]>--- blockdev.c | 154 ++++++++++++++++++++++++++++++++------------------- qapi-schema.json | 1 + qapi/block-core.json | 26 +++++++++ qmp-commands.hx | 28 ++++++++++ 4 files changed, 153 insertions(+), 56 deletions(-)blockdev.c +0
-0
blockdev.c void qmp_blockdev_snapshot_sync(bool has_device, const char *device, · line 1186
Quoted 4 lines
blockdev.c +1
-0@@ -1183,6 +1183,18 @@ void qmp_blockdev_snapshot_sync(bool has_device, const char *device, &snapshot, errp); } +void qmp_blockdev_snapshot(const char *device, const char *snapshot,
Is 'node' a better name than 'device' here?
Quoted 4 lines
blockdev.c +0
-0Error **errp)@@ -1521,60 +1533,52 @@ typedef struct ExternalSnapshotState {static void external_snapshot_prepare (BlkTransactionState *common,Error **errp)
blockdev.c typedef struct ExternalSnapshotState { · line 1544
Quoted 10 lines
blockdev.c +8
-0{ +/* 'blockdev-snapshot' and 'blockdev-snapshot-sync' have similar +* purpose but a different set of parameters */ +switch (action->kind) { +case TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT : + { +BlockdevSnapshot *s = action->blockdev_snapshot; + device = s->device; + node_name = s->device;
Again, I think this should be setting s->node_name, not s->device.
Quoted 49 lines
blockdev.c +16
-18+ new_image_file =NULL ; + snapshot_ref = s->snapshot; + } +break ; +case TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC : + { +BlockdevSnapshotSync *s = action->blockdev_snapshot_sync; + device = s->has_device ? s->device :NULL ; + node_name = s->has_node_name ? s->node_name :NULL ; + new_image_file = s->snapshot_file; + snapshot_ref =NULL ; + } +break ; +default : +g_assert_not_reached (); }/* start processing */ - state->old_bs =bdrv_lookup_bs (has_device ? device :NULL , - has_node_name ? node_name :NULL , - &local_err); + state->old_bs =bdrv_lookup_bs (device, node_name, &local_err);if (local_err) {error_propagate (errp, local_err);return ; } -if (has_node_name && !has_snapshot_node_name) { -error_setg (errp,"New snapshot node name missing" ); -return ; - } - -if (has_snapshot_node_name &&bdrv_find_node (snapshot_node_name)) { -error_setg (errp,"New snapshot node name already existing" ); -return ; - } -/* Acquire AioContext now so any threads operating on old_bs stop */ state->aio_context =bdrv_get_aio_context (state->old_bs);aio_context_acquire (state->aio_context);@@ -1601,35 +1605,67 @@ static void external_snapshot_prepare(BlkTransactionState *common,return ; } - flags = state->old_bs->open_flags; - -/* create new image w/backing file */ -if (mode !=NEW_IMAGE_MODE_EXISTING ) { - bdrv_img_create(new_image_file, format,
blockdev.c static void external_snapshot_prepare(BlkTransactionState *common, · line 1613
Quoted 10 lines
blockdev.c +2
-4- state->old_bs->filename, - state->old_bs->drv->format_name, - NULL, -1, flags, &local_err, false); -if (local_err) { +if (snapshot_ref) { +if (!bdrv_lookup_bs (snapshot_ref, snapshot_ref, &local_err)) {error_propagate (errp, local_err);return ; } }
Shouldn't you also check that snapshot_ref does not currently have a backing BDS (as it is the act of creating the snapshot that sets the current device as the backing of the snapshot_ref BDS before altering the BB to point to snapshot_ref as its new BDS)?
blockdev.c line 1499
Quoted 6 lines
blockdev.c +2
-0+++ b/qapi-schema.json@@ -1496,6 +1496,7 @@ ## { 'union': 'TransactionAction', 'data': { + 'blockdev-snapshot': 'BlockdevSnapshot',
Missing 'since 2.5' documentation.
Quoted 8 lines
blockdev.c +0
-0'blockdev-snapshot-sync': 'BlockdevSnapshotSync', 'drive-backup': 'DriveBackup', 'blockdev-backup': 'BlockdevBackup',qapi/block-core.json +0
-0
qapi/block-core.json line 710
Quoted 6 lines
qapi/block-core.json +3
-0@@ -705,6 +705,19 @@ '*format': 'str', '*mode': 'NewImageMode' } } ## +# @BlockdevSnapshot +# +# @device: device or node name to generate the snapshot from.
Would naming this 'node' make more sense?
Quoted 20 lines
qapi/block-core.json +13
-0+# +# @snapshot: reference to the existing block device that will be used +# for the snapshot. +# +# Since 2.5 +## +{ 'struct': 'BlockdevSnapshot', + 'data': { 'device': 'str', 'snapshot': 'str' } } + +## # @DriveBackup # # @device: the name of the device which should be copied.@@ -800,6 +813,19 @@ { 'command': 'blockdev-snapshot-sync', 'data': 'BlockdevSnapshotSync' } + +## +# @blockdev-snapshot
qapi/block-core.json line 828
Quoted 10 lines
qapi/block-core.json +10
-0+# +# Generates a snapshot of a block device. +# +# For the arguments, see the documentation of BlockdevSnapshot. +# +# Since 2.5 +## +{ 'command': 'blockdev-snapshot', + 'data': 'BlockdevSnapshot' } +
Overall, I like the syntax.
qapi/block-core.json
Quoted 8 lines
qapi/block-core.json +8
-0+SQMP +blockdev-snapshot +----------------- +Since 2.5 + +Create a snapshot of a block device. 'device' and 'snapshot' both +refer to existing block devices. The former is the one to generate the +snapshot from, and the latter is the target of the snapshot.
Is there any better terminology? Maybe:
The act of creating a snapshot installs 'device' as the backing image of 'snapshot'; additionally, if 'device' is associated with a block device, the block device changes to using 'snapshot' as its new active image.
Hmm - I wonder if that means we should have an optional boolean parameter that allows us to avoid the automatic pivot. After all, with 'blockdev-snapshot-sync', you can specify 'device' and omit 'node-name' to update the device's active layer, or you can omit 'device' and specify 'node-name' to create another qcow2 file but NOT install it as the active layer, regardless of which 'node-name' that serves as the starting point. So when 'node-name' is the BDS node that 'device' is currently visiting, you have control over whether 'device' auto-updates to the new BDS.
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [PATCH v2 2/2] block: add a 'blockdev-snapshot' QMP command
Quoted 7 lines · Eric Blake
blockdev.c void qmp_blockdev_snapshot_sync(bool has_device, const char *device, · line 1186
Quoted 5 lines
blockdev.c +1
-0@@ -1183,6 +1183,18 @@ void qmp_blockdev_snapshot_sync(bool has_device, const char *device, &snapshot, errp); } +void qmp_blockdev_snapshot(const char *device, const char *snapshot,Is 'node' a better name than 'device' here?
I don't have a strong preference (I was just following Kevin's suggestion), but 'device' seems to be the most common name for this parameter. This one can take a node name as well, but it will only accept an active layer anyway... I can change the name if you prefer.
Quoted 11 lines
blockdev.c static void external_snapshot_prepare(BlkTransactionState *common, · line 1613
Quoted 6 lines
blockdev.c +2
-0+if (snapshot_ref) { +if (!bdrv_lookup_bs (snapshot_ref, snapshot_ref, &local_err)) {error_propagate (errp, local_err);return ; } }Shouldn't you also check that
snapshot_refdoes not currently have a backing BDS (as it is the act of creating the snapshot that sets the current device as the backing of thesnapshot_refBDS before altering the BB to point tosnapshot_refas its new BDS)?
I think you're right, thanks!
Quoted 11 lines
qmp-commands.hx Example:
Quoted 8 lines
qmp-commands.hx +8
-0+SQMP +blockdev-snapshot +----------------- +Since 2.5 + +Create a snapshot of a block device. 'device' and 'snapshot' both +refer to existing block devices. The former is the one to generate +the snapshot from, and the latter is the target of the snapshot.Is there any better terminology? Maybe:
Quoted 4 lines
The act of creating a snapshot installs 'device' as the backing image of 'snapshot'; additionally, if 'device' is associated with a block device, the block device changes to using 'snapshot' as its new active image.
Sounds good.
Quoted 9 lines
Hmm - I wonder if that means we should have an optional boolean parameter that allows us to avoid the automatic pivot. After all, with 'blockdev-snapshot-sync', you can specify 'device' and omit 'node-name' to update the device's active layer, or you can omit 'device' and specify 'node-name' to create another qcow2 file but NOT install it as the active layer, regardless of which 'node-name' that serves as the starting point. So when 'node-name' is the BDS node that 'device' is currently visiting, you have control over whether 'device' auto-updates to the new BDS.
What's the use case for that?
Berto
Re: [Qemu-devel] [PATCH v2 2/2] block: add a 'blockdev-snapshot' QMP command
In reply to Eric BlakeQuoted 11 lines · Eric Blake
blockdev.c static void external_snapshot_prepare(BlkTransactionState *common, · line 1613
Quoted 6 lines
blockdev.c +2
-0+if (snapshot_ref) { +if (!bdrv_lookup_bs (snapshot_ref, snapshot_ref, &local_err)) {error_propagate (errp, local_err);return ; } }Shouldn't you also check that
snapshot_refdoes not currently have a backing BDS (as it is the act of creating the snapshot that sets the current device as the backing of thesnapshot_refBDS before altering the BB to point tosnapshot_refas its new BDS)?
Wait, I actually think that it should have a backing BDS. This new command is roughly equivalent to the 'existing' mode of the 'blockdev-snapshot-sync' command, so the new image must be created externally in both cases having the current image as its backing file.
The difference is that 'blockdev-snapshot-sync' will open the new image and not its backing chain (using the BDRV_O_NO_BACKING flag), but in 'blockdev-snapshot' the image must be opened previously with 'blockdev-add', which will open the whole chain.
So I think that we should expect a backing image and we have to unref it during the 'commit' part of the transaction (before bdrv_append()).
Berto
Re: [Qemu-devel] [PATCH v2 2/2] block: add a 'blockdev-snapshot' QMP command
Am 07.09.2015 um 13:31 hat Alberto Garcia geschrieben:
Quoted 18 lines · Eric Blake
Quoted 11 lines
blockdev.c static void external_snapshot_prepare(BlkTransactionState *common, · line 1613
Quoted 6 lines
blockdev.c +2
-0+if (snapshot_ref) { +if (!bdrv_lookup_bs (snapshot_ref, snapshot_ref, &local_err)) {error_propagate (errp, local_err);return ; } }Shouldn't you also check that
snapshot_refdoes not currently have a backing BDS (as it is the act of creating the snapshot that sets the current device as the backing of thesnapshot_refBDS before altering the BB to point tosnapshot_refas its new BDS)?Wait, I actually think that it should have a backing BDS. This new command is roughly equivalent to the 'existing' mode of the 'blockdev-snapshot-sync' command, so the new image must be created externally in both cases having the current image as its backing file.
Quoted 7 lines
The difference is that 'blockdev-snapshot-sync' will open the new image and not its backing chain (using the
BDRV_O_NO_BACKINGflag), but in 'blockdev-snapshot' the image must be opened previously with 'blockdev-add', which will open the whole chain.So I think that we should expect a backing image and we have to unref it during the 'commit' part of the transaction (before
bdrv_append()).
This means that the images in the backing chain are opened twice before doing the bdrv_append(). This definitely includes the active layer as one example of an illegal setup, and with block jobs running on the backing files it might involve even more writable image files.
I think Eric's approach is better, where we check the required conditions instead of adding magic to somehow transfer a questionable state into what we want.
Of course, this means that we need to provide a way to actually fulfill the right conditions, and you might be right that it doesn't exist yet. Could the same effect as BDRV_O_NO_BACKING possibly be achieved with blockdev-add with something like '"backing": {}'?
We might actually have the same problem in more places: In optional fields, we need to distinguish "use the default" from "I don't want any". Especially if we want to support reopen in QMP at some point, we need to figure this out (default is leaving options unchanged, but unsetting must be possible).
Kevin
Re: [Qemu-devel] [PATCH v2 2/2] block: add a 'blockdev-snapshot' QMP command
Quoted 4 lines · Kevin Wolf
Of course, this means that we need to provide a way to actually fulfill the right conditions, and you might be right that it doesn't exist yet. Could the same effect as
BDRV_O_NO_BACKINGpossibly be achieved with blockdev-add with something like '"backing": {}'?
I'm not sure it that would be the best API, but the overall idea looks good to me.
Berto