| Message ID | 1371209999-15579-10-git-send-email-xiawenc@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On 06/14/2013 12:39 PM, Wenchao Xia wrote: > Snapshot ID can't be specified in this interface. Why not? If it is possible to look up by id in isolation, then it should be possible to delete by id in isolation. Also, if it is possible to create a snapshot that has an id but no name (and I think qcow2 allows such snapshots, even if we don't want to expose that corner case of creation via normal QMP), then it is mandatory to have a way to delete by id, since there is no name to delete by. > > Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> > --- > blockdev.c | 12 ++++++++++++ > qapi-schema.json | 19 +++++++++++++++++++ > qmp-commands.hx | 30 ++++++++++++++++++++++++++++++ > 3 files changed, 61 insertions(+), 0 deletions(-) > > +++ b/qapi-schema.json > @@ -1712,6 +1712,25 @@ > 'data': { 'device': 'str', '*name': 'str'} } > > ## > +# @blockdev-snapshot-delete-internal-sync > +# > +# Synchronously delete an internal snapshot of a block device, when the format > +# of the image used support it. Should this command be made available via 'transaction'? That is, if I have a two-disk VM, and use 'transaction' to take a snapshot of both disks at once, shouldn't I also have a way to delete the snapshots of both at once, or gracefully fail without data loss if the second one has problems? > +# > +# @device: the name of the device to delete the snapshot from > +# > +# @name: the snapshot's name to be deleted > +# > +# Returns: nothing on success > +# If @device is not a valid block device, DeviceNotFound > +# If snapshot not found, generic error will be returned > +# > +# Since 1.6 > +## > +{ 'command': 'blockdev-snapshot-delete-internal-sync', > + 'data': { 'device': 'str', 'name': 'str'} } I'm not convinced that this interface is powerful enough.
于 2013-6-15 17:55, Eric Blake 写道: > On 06/14/2013 12:39 PM, Wenchao Xia wrote: >> Snapshot ID can't be specified in this interface. > > Why not? If it is possible to look up by id in isolation, then it > should be possible to delete by id in isolation. Also, if it is > possible to create a snapshot that has an id but no name (and I think > qcow2 allows such snapshots, even if we don't want to expose that corner > case of creation via normal QMP), then it is mandatory to have a way to > delete by id, since there is no name to delete by. > OK, qemu-img allows create snapshot with name="" before, so we need take care for that corener case in this interface. Here is the redesign: ## # @blockdev-snapshot-delete-internal-sync # # Synchronously delete an internal snapshot of a block device, when the ormat # of the image used support it. # # @device: the name of the device to delete the snapshot from # # @name: the snapshot's name to be deleted # # @id: optional the snapshot's ID to be deleted. If specified, qemu will # try delete the snapshot matching both @name and @id. # # Returns: nothing on success # If @device is not a valid block device, DeviceNotFound # If snapshot not found, generic error will be returned # # Since 1.6 ## { 'command': 'blockdev-snapshot-delete-internal-sync', 'data': { 'device': 'str', 'name': 'str', '*id': 'str' } } Then caller specify name='' and id=[ID] to handle the corner case. >> >> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> >> --- >> blockdev.c | 12 ++++++++++++ >> qapi-schema.json | 19 +++++++++++++++++++ >> qmp-commands.hx | 30 ++++++++++++++++++++++++++++++ >> 3 files changed, 61 insertions(+), 0 deletions(-) >> > >> +++ b/qapi-schema.json >> @@ -1712,6 +1712,25 @@ >> 'data': { 'device': 'str', '*name': 'str'} } >> >> ## >> +# @blockdev-snapshot-delete-internal-sync >> +# >> +# Synchronously delete an internal snapshot of a block device, when the format >> +# of the image used support it. > > Should this command be made available via 'transaction'? That is, if I > have a two-disk VM, and use 'transaction' to take a snapshot of both > disks at once, shouldn't I also have a way to delete the snapshots of > both at once, or gracefully fail without data loss if the second one has > problems? I think adding it in transaction is not very useful but brings more complexity. Transcation is used to guareentee all operations are taken in one time point, for example, snapshot creation use it to make sure all are consistent to VM. But for deletion, this requirement do not exist. > >> +# >> +# @device: the name of the device to delete the snapshot from >> +# >> +# @name: the snapshot's name to be deleted >> +# >> +# Returns: nothing on success >> +# If @device is not a valid block device, DeviceNotFound >> +# If snapshot not found, generic error will be returned >> +# >> +# Since 1.6 >> +## >> +{ 'command': 'blockdev-snapshot-delete-internal-sync', >> + 'data': { 'device': 'str', 'name': 'str'} } > > I'm not convinced that this interface is powerful enough. >
On Mon, Jun 17, 2013 at 11:25:26AM +0800, Wenchao Xia wrote: > 于 2013-6-15 17:55, Eric Blake 写道: > >Should this command be made available via 'transaction'? That is, if I > >have a two-disk VM, and use 'transaction' to take a snapshot of both > >disks at once, shouldn't I also have a way to delete the snapshots of > >both at once, or gracefully fail without data loss if the second one has > >problems? > > I think adding it in transaction is not very useful but brings more > complexity. Transcation is used to guareentee all operations are taken > in one time point, for example, snapshot creation use it to make sure > all are consistent to VM. But for deletion, this requirement do not > exist. I guess the problem is: can we make internal snapshot deletion transactional? It's hard to do rollback for snapshot deletion. But batching is definitely useful for doing 'delvm' in QMP. I just don't think transactions help. We just need a 'delvm' equivalent in QMP. Stefan
于 2013-6-18 22:20, Stefan Hajnoczi 写道: > On Mon, Jun 17, 2013 at 11:25:26AM +0800, Wenchao Xia wrote: >> 于 2013-6-15 17:55, Eric Blake 写道: >>> Should this command be made available via 'transaction'? That is, if I >>> have a two-disk VM, and use 'transaction' to take a snapshot of both >>> disks at once, shouldn't I also have a way to delete the snapshots of >>> both at once, or gracefully fail without data loss if the second one has >>> problems? >> >> I think adding it in transaction is not very useful but brings more >> complexity. Transcation is used to guareentee all operations are taken >> in one time point, for example, snapshot creation use it to make sure >> all are consistent to VM. But for deletion, this requirement do not >> exist. > > I guess the problem is: can we make internal snapshot deletion > transactional? It's hard to do rollback for snapshot deletion. > Several deletion in transaction equals to several calls of 'blockdev-snapshot-delete-internal-sync', unlike creation, so I hope not add it which have rollback issue. > But batching is definitely useful for doing 'delvm' in QMP. I just > don't think transactions help. We just need a 'delvm' equivalent in > QMP. > Maybe the caller can encapsulate a batch interface at its level. > Stefan >
On Wed, Jun 19, 2013 at 02:18:48PM +0800, Wenchao Xia wrote: > 于 2013-6-18 22:20, Stefan Hajnoczi 写道: > >On Mon, Jun 17, 2013 at 11:25:26AM +0800, Wenchao Xia wrote: > >>于 2013-6-15 17:55, Eric Blake 写道: > >>>Should this command be made available via 'transaction'? That is, if I > >>>have a two-disk VM, and use 'transaction' to take a snapshot of both > >>>disks at once, shouldn't I also have a way to delete the snapshots of > >>>both at once, or gracefully fail without data loss if the second one has > >>>problems? > >> > >> I think adding it in transaction is not very useful but brings more > >>complexity. Transcation is used to guareentee all operations are taken > >>in one time point, for example, snapshot creation use it to make sure > >>all are consistent to VM. But for deletion, this requirement do not > >>exist. > > > >I guess the problem is: can we make internal snapshot deletion > >transactional? It's hard to do rollback for snapshot deletion. > > > Several deletion in transaction equals to several calls of > 'blockdev-snapshot-delete-internal-sync', unlike creation, so I hope > not add it which have rollback issue. > > > >But batching is definitely useful for doing 'delvm' in QMP. I just > >don't think transactions help. We just need a 'delvm' equivalent in > >QMP. > > > Maybe the caller can encapsulate a batch interface at its level. 'delvm' is a batch interface - it deletes internal snapshots that have the same name across multiple devices. It's not as flexible as: blkdev-internal-snapshot-delete drive0 drive2 drive4 Because that would allow you to specify specific drives. Stefan
于 2013-6-19 15:46, Stefan Hajnoczi 写道: > On Wed, Jun 19, 2013 at 02:18:48PM +0800, Wenchao Xia wrote: >> 于 2013-6-18 22:20, Stefan Hajnoczi 写道: >>> On Mon, Jun 17, 2013 at 11:25:26AM +0800, Wenchao Xia wrote: >>>> 于 2013-6-15 17:55, Eric Blake 写道: >>>>> Should this command be made available via 'transaction'? That is, if I >>>>> have a two-disk VM, and use 'transaction' to take a snapshot of both >>>>> disks at once, shouldn't I also have a way to delete the snapshots of >>>>> both at once, or gracefully fail without data loss if the second one has >>>>> problems? >>>> >>>> I think adding it in transaction is not very useful but brings more >>>> complexity. Transcation is used to guareentee all operations are taken >>>> in one time point, for example, snapshot creation use it to make sure >>>> all are consistent to VM. But for deletion, this requirement do not >>>> exist. >>> >>> I guess the problem is: can we make internal snapshot deletion >>> transactional? It's hard to do rollback for snapshot deletion. >>> >> Several deletion in transaction equals to several calls of >> 'blockdev-snapshot-delete-internal-sync', unlike creation, so I hope >> not add it which have rollback issue. >> >> >>> But batching is definitely useful for doing 'delvm' in QMP. I just >>> don't think transactions help. We just need a 'delvm' equivalent in >>> QMP. >>> >> Maybe the caller can encapsulate a batch interface at its level. > > 'delvm' is a batch interface - it deletes internal snapshots that have > the same name across multiple devices. > > It's not as flexible as: > blkdev-internal-snapshot-delete drive0 drive2 drive4 > > Because that would allow you to specify specific drives. > Do you mean this interface should be changed as blkdev-internal-snapshot-delete devices_array name *id? It seems not much difference with following methods: for device in device_list: blkdev-internal-snapshot-delete device name > Stefan >
On Wed, Jun 19, 2013 at 04:53:38PM +0800, Wenchao Xia wrote: > 于 2013-6-19 15:46, Stefan Hajnoczi 写道: > >On Wed, Jun 19, 2013 at 02:18:48PM +0800, Wenchao Xia wrote: > >>于 2013-6-18 22:20, Stefan Hajnoczi 写道: > >>>On Mon, Jun 17, 2013 at 11:25:26AM +0800, Wenchao Xia wrote: > >>>>于 2013-6-15 17:55, Eric Blake 写道: > >>>>>Should this command be made available via 'transaction'? That is, if I > >>>>>have a two-disk VM, and use 'transaction' to take a snapshot of both > >>>>>disks at once, shouldn't I also have a way to delete the snapshots of > >>>>>both at once, or gracefully fail without data loss if the second one has > >>>>>problems? > >>>> > >>>> I think adding it in transaction is not very useful but brings more > >>>>complexity. Transcation is used to guareentee all operations are taken > >>>>in one time point, for example, snapshot creation use it to make sure > >>>>all are consistent to VM. But for deletion, this requirement do not > >>>>exist. > >>> > >>>I guess the problem is: can we make internal snapshot deletion > >>>transactional? It's hard to do rollback for snapshot deletion. > >>> > >> Several deletion in transaction equals to several calls of > >>'blockdev-snapshot-delete-internal-sync', unlike creation, so I hope > >>not add it which have rollback issue. > >> > >> > >>>But batching is definitely useful for doing 'delvm' in QMP. I just > >>>don't think transactions help. We just need a 'delvm' equivalent in > >>>QMP. > >>> > >> Maybe the caller can encapsulate a batch interface at its level. > > > >'delvm' is a batch interface - it deletes internal snapshots that have > >the same name across multiple devices. > > > >It's not as flexible as: > >blkdev-internal-snapshot-delete drive0 drive2 drive4 > > > >Because that would allow you to specify specific drives. > > > Do you mean this interface should be changed as > blkdev-internal-snapshot-delete devices_array name *id? > > It seems not much difference with following methods: > > for device in device_list: > blkdev-internal-snapshot-delete device name The ability to batch in a single QMP command feels a little nicer. There are less opportunities for the operation to stop half-way through. It would be usable as a QMP 'delvm' and in a more flexible way to delete multiple internal snapshots. Would be interesting to see what Eric Blake thinks from a libvirt perspective. Stefan
于 2013-6-19 22:24, Stefan Hajnoczi 写道: > On Wed, Jun 19, 2013 at 04:53:38PM +0800, Wenchao Xia wrote: >> 于 2013-6-19 15:46, Stefan Hajnoczi 写道: >>> On Wed, Jun 19, 2013 at 02:18:48PM +0800, Wenchao Xia wrote: >>>> 于 2013-6-18 22:20, Stefan Hajnoczi 写道: >>>>> On Mon, Jun 17, 2013 at 11:25:26AM +0800, Wenchao Xia wrote: >>>>>> 于 2013-6-15 17:55, Eric Blake 写道: >>>>>>> Should this command be made available via 'transaction'? That is, if I >>>>>>> have a two-disk VM, and use 'transaction' to take a snapshot of both >>>>>>> disks at once, shouldn't I also have a way to delete the snapshots of >>>>>>> both at once, or gracefully fail without data loss if the second one has >>>>>>> problems? >>>>>> >>>>>> I think adding it in transaction is not very useful but brings more >>>>>> complexity. Transcation is used to guareentee all operations are taken >>>>>> in one time point, for example, snapshot creation use it to make sure >>>>>> all are consistent to VM. But for deletion, this requirement do not >>>>>> exist. >>>>> >>>>> I guess the problem is: can we make internal snapshot deletion >>>>> transactional? It's hard to do rollback for snapshot deletion. >>>>> >>>> Several deletion in transaction equals to several calls of >>>> 'blockdev-snapshot-delete-internal-sync', unlike creation, so I hope >>>> not add it which have rollback issue. >>>> >>>> >>>>> But batching is definitely useful for doing 'delvm' in QMP. I just >>>>> don't think transactions help. We just need a 'delvm' equivalent in >>>>> QMP. >>>>> >>>> Maybe the caller can encapsulate a batch interface at its level. >>> >>> 'delvm' is a batch interface - it deletes internal snapshots that have >>> the same name across multiple devices. >>> >>> It's not as flexible as: >>> blkdev-internal-snapshot-delete drive0 drive2 drive4 >>> >>> Because that would allow you to specify specific drives. >>> >> Do you mean this interface should be changed as >> blkdev-internal-snapshot-delete devices_array name *id? >> >> It seems not much difference with following methods: >> >> for device in device_list: >> blkdev-internal-snapshot-delete device name > > The ability to batch in a single QMP command feels a little nicer. > There are less opportunities for the operation to stop half-way through. > > It would be usable as a QMP 'delvm' and in a more flexible way to delete > multiple internal snapshots. > > Would be interesting to see what Eric Blake thinks from a libvirt > perspective. > > Stefan > Just want to mention: other interfaces for block device take single device as parameter now, use array here would be a little strange, and increase the complexity in qmp and hmp interface. My opinion is let caller do: for device in device_list: blkdev-internal-snapshot-delete device name Still, hope to know Eric's opinion.
diff --git a/blockdev.c b/blockdev.c index 6a952cd..0277482 100644 --- a/blockdev.c +++ b/blockdev.c @@ -802,6 +802,18 @@ void qmp_blockdev_snapshot_internal_sync(const char *device, &snapshot, errp); } +void qmp_blockdev_snapshot_delete_internal_sync(const char *device, + const char *name, + Error **errp) +{ + BlockDriverState *bs = bdrv_find(device); + if (!bs) { + error_set(errp, QERR_DEVICE_NOT_FOUND, device); + return; + }; + + bdrv_snapshot_delete(bs, NULL, name, errp); +} /* New and old BlockDriverState structs for group snapshots */ diff --git a/qapi-schema.json b/qapi-schema.json index 749a349..b5e4f87 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -1712,6 +1712,25 @@ 'data': { 'device': 'str', '*name': 'str'} } ## +# @blockdev-snapshot-delete-internal-sync +# +# Synchronously delete an internal snapshot of a block device, when the format +# of the image used support it. +# +# @device: the name of the device to delete the snapshot from +# +# @name: the snapshot's name to be deleted +# +# Returns: nothing on success +# If @device is not a valid block device, DeviceNotFound +# If snapshot not found, generic error will be returned +# +# Since 1.6 +## +{ 'command': 'blockdev-snapshot-delete-internal-sync', + 'data': { 'device': 'str', 'name': 'str'} } + +## # @human-monitor-command: # # Execute a command on the human monitor and return the output. diff --git a/qmp-commands.hx b/qmp-commands.hx index 3d5f996..904950f 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -1075,6 +1075,36 @@ Example: EQMP { + .name = "blockdev-snapshot-delete-internal-sync", + .args_type = "device:B,name:s", + .mhandler.cmd_new = + qmp_marshal_input_blockdev_snapshot_delete_internal_sync, + }, + +SQMP +blockdev-snapshot-delete-internal-sync +-------------------------------------- + +Synchronously delete an internal snapshot of a block device when the format of +image used support it. If the snapshot matching name is not found, operation +will fail. + +Arguments: + +- "device": device name (json-string) +- "name": name of the snapshot (json-string) + +Example: + +-> { "execute": "blockdev-snapshot-delete-internal-sync", + "arguments": { "device": "ide-hd0", + "name": "snapshot0" } + } +<- { "return": {} } + +EQMP + + { .name = "drive-mirror", .args_type = "sync:s,device:B,target:s,speed:i?,mode:s?,format:s?," "on-source-error:s?,on-target-error:s?,"
Snapshot ID can't be specified in this interface. Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> --- blockdev.c | 12 ++++++++++++ qapi-schema.json | 19 +++++++++++++++++++ qmp-commands.hx | 30 ++++++++++++++++++++++++++++++ 3 files changed, 61 insertions(+), 0 deletions(-)