Patchwork [V2,09/12] qmp: add interface blockdev-snapshot-delete-internal-sync

login
register
mail settings
Submitter Wayne Xia
Date June 14, 2013, 11:39 a.m.
Message ID <1371209999-15579-10-git-send-email-xiawenc@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/251414/
State New
Headers show

Comments

Wayne Xia - June 14, 2013, 11:39 a.m.
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(-)
Eric Blake - June 15, 2013, 9:55 a.m.
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.
Wayne Xia - June 17, 2013, 3:25 a.m.
于 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.
>
Stefan Hajnoczi - June 18, 2013, 2:20 p.m.
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
Wayne Xia - June 19, 2013, 6:18 a.m.
于 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
>
Stefan Hajnoczi - June 19, 2013, 7:46 a.m.
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
Wayne Xia - June 19, 2013, 8:53 a.m.
于 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
>
Stefan Hajnoczi - June 19, 2013, 2:24 p.m.
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
Wayne Xia - June 20, 2013, 2:37 a.m.
于 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.

Patch

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?,"