| Message ID | 806c66b2f071f03bec0b7d45a71512e394386885.1339617170.git.phrdina@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 06/14/2012 01:35 AM, Pavel Hrdina wrote: > Signed-off-by: Pavel Hrdina <phrdina@redhat.com> > --- > +++ b/qapi-schema.json > @@ -1169,6 +1169,21 @@ > { 'command': 'block_resize', 'data': { 'device': 'str', 'size': 'int' }} > > ## > +# @commit > +# > +# Commit changes to the disk images (if -snapshot is used) or backing files. > +# > +# @device: the name of the device or the "all" to commit all devices > +# > +# Returns: nothing on success > +# If @device is not a valid block device, DeviceNotFound > +# If a long-running operation is using the device, DeviceInUse > +# > +# Since: 1.2 > +## > +{ 'command': 'commit', 'data': { 'device': 'str' }} Should we use this as an opportunity to make the command more powerful? For example, integrating this with the 'transaction' command or a block job queried by 'query-block-jobs' to track its progress would be useful. Also, suppose I have A <- B <- C. Does 'commit' only do one layer (C into B), or all layers (B and C into A)? That argues that we need an optional parameter that says how deep to commit (committing C into B only to repeat and commit B into A is more time-consuming than directly committing both B and C into A to start with). When a commit is complete, which file is backing the device - is it still C (which continues to diverge, but now from the point of the commit) or does qemu pivot things to have the device now backed by B (and C can be discarded, particularly true if changes are now going into B which invalidate C).
On 06/14/2012 02:18 PM, Eric Blake wrote: > On 06/14/2012 01:35 AM, Pavel Hrdina wrote: >> Signed-off-by: Pavel Hrdina<phrdina@redhat.com> >> --- >> +++ b/qapi-schema.json >> @@ -1169,6 +1169,21 @@ >> { 'command': 'block_resize', 'data': { 'device': 'str', 'size': 'int' }} >> >> ## >> +# @commit >> +# >> +# Commit changes to the disk images (if -snapshot is used) or backing files. >> +# >> +# @device: the name of the device or the "all" to commit all devices >> +# >> +# Returns: nothing on success >> +# If @device is not a valid block device, DeviceNotFound >> +# If a long-running operation is using the device, DeviceInUse >> +# >> +# Since: 1.2 >> +## >> +{ 'command': 'commit', 'data': { 'device': 'str' }} > Should we use this as an opportunity to make the command more powerful? > For example, integrating this with the 'transaction' command or a block > job queried by 'query-block-jobs' to track its progress would be useful. > Also, suppose I have A<- B<- C. Does 'commit' only do one layer (C > into B), or all layers (B and C into A)? That argues that we need an > optional parameter that says how deep to commit (committing C into B > only to repeat and commit B into A is more time-consuming than directly > committing both B and C into A to start with). When a commit is > complete, which file is backing the device - is it still C (which > continues to diverge, but now from the point of the commit) or does qemu > pivot things to have the device now backed by B (and C can be discarded, > particularly true if changes are now going into B which invalidate C). What i find out is that 'commit' will commit changes only from C to B and qemu continues with C from the new commit point. I couldn't find a way to commit changes and go back to backing file. This should be supported by parameter and also as you mention that commit all changes through all snapshots should be supported by another parameter. The 'transaction' feature would be nice to have too. Pavel
On 06/14/2012 08:56 AM, Pavel Hrdina wrote: > On 06/14/2012 02:18 PM, Eric Blake wrote: >> On 06/14/2012 01:35 AM, Pavel Hrdina wrote: >>> Signed-off-by: Pavel Hrdina<phrdina@redhat.com> >>> --- >>> +++ b/qapi-schema.json >>> @@ -1169,6 +1169,21 @@ >>> { 'command': 'block_resize', 'data': { 'device': 'str', 'size': >>> 'int' }} >>> >>> ## >>> +# @commit >>> +# >>> +# Commit changes to the disk images (if -snapshot is used) or >>> backing files. >>> +# >>> +# @device: the name of the device or the "all" to commit all devices >>> +# >>> +# Returns: nothing on success >>> +# If @device is not a valid block device, DeviceNotFound >>> +# If a long-running operation is using the device, DeviceInUse >>> +# >>> +# Since: 1.2 >>> +## >>> +{ 'command': 'commit', 'data': { 'device': 'str' }} >> Should we use this as an opportunity to make the command more powerful? >> For example, integrating this with the 'transaction' command or a block >> job queried by 'query-block-jobs' to track its progress would be useful. >> Also, suppose I have A<- B<- C. Does 'commit' only do one layer (C >> into B), or all layers (B and C into A)? That argues that we need an >> optional parameter that says how deep to commit (committing C into B >> only to repeat and commit B into A is more time-consuming than directly >> committing both B and C into A to start with). When a commit is >> complete, which file is backing the device - is it still C (which >> continues to diverge, but now from the point of the commit) or does qemu >> pivot things to have the device now backed by B (and C can be discarded, >> particularly true if changes are now going into B which invalidate C). > > What i find out is that 'commit' will commit changes only from C to B > and qemu continues with C from the new commit point. I couldn't find a > way to commit changes and go back to backing file. This should be > supported by parameter and also as you mention that commit all changes > through all snapshots should be supported by another parameter. > The 'transaction' feature would be nice to have too. Which makes it sound like we're starting to overlap with Jeff's work on 'block-commit'. If 'block-commit' proves to be better all around at doing what we want, do we even need to keep 'commit' in QMP, or would it be okay for HMP only?
On 06/14/2012 05:04 PM, Eric Blake wrote: > On 06/14/2012 08:56 AM, Pavel Hrdina wrote: >> On 06/14/2012 02:18 PM, Eric Blake wrote: >>> On 06/14/2012 01:35 AM, Pavel Hrdina wrote: >>>> Signed-off-by: Pavel Hrdina<phrdina@redhat.com> >>>> --- >>>> +++ b/qapi-schema.json >>>> @@ -1169,6 +1169,21 @@ >>>> { 'command': 'block_resize', 'data': { 'device': 'str', 'size': >>>> 'int' }} >>>> >>>> ## >>>> +# @commit >>>> +# >>>> +# Commit changes to the disk images (if -snapshot is used) or >>>> backing files. >>>> +# >>>> +# @device: the name of the device or the "all" to commit all devices >>>> +# >>>> +# Returns: nothing on success >>>> +# If @device is not a valid block device, DeviceNotFound >>>> +# If a long-running operation is using the device, DeviceInUse >>>> +# >>>> +# Since: 1.2 >>>> +## >>>> +{ 'command': 'commit', 'data': { 'device': 'str' }} >>> Should we use this as an opportunity to make the command more powerful? >>> For example, integrating this with the 'transaction' command or a block >>> job queried by 'query-block-jobs' to track its progress would be useful. >>> Also, suppose I have A<- B<- C. Does 'commit' only do one layer (C >>> into B), or all layers (B and C into A)? That argues that we need an >>> optional parameter that says how deep to commit (committing C into B >>> only to repeat and commit B into A is more time-consuming than directly >>> committing both B and C into A to start with). When a commit is >>> complete, which file is backing the device - is it still C (which >>> continues to diverge, but now from the point of the commit) or does qemu >>> pivot things to have the device now backed by B (and C can be discarded, >>> particularly true if changes are now going into B which invalidate C). >> What i find out is that 'commit' will commit changes only from C to B >> and qemu continues with C from the new commit point. I couldn't find a >> way to commit changes and go back to backing file. This should be >> supported by parameter and also as you mention that commit all changes >> through all snapshots should be supported by another parameter. >> The 'transaction' feature would be nice to have too. > Which makes it sound like we're starting to overlap with Jeff's work on > 'block-commit'. > > If 'block-commit' proves to be better all around at doing what we want, > do we even need to keep 'commit' in QMP, or would it be okay for HMP only? If the 'block-commit' will be better I think that we could drop the 'commit' completely. And have only 'block-commit' for both QMP and HMP. Pavel
On Thu, 14 Jun 2012 09:35:21 +0200 Pavel Hrdina <phrdina@redhat.com> wrote: > > Signed-off-by: Pavel Hrdina <phrdina@redhat.com> > --- > Please ignore previous patch, I forget to change 'Since' version. > > blockdev.c | 9 ++++----- > blockdev.h | 1 - > hmp-commands.hx | 2 +- > hmp.c | 9 +++++++++ > hmp.h | 1 + > qapi-schema.json | 15 +++++++++++++++ > qmp-commands.hx | 24 ++++++++++++++++++++++++ > 7 files changed, 54 insertions(+), 7 deletions(-) > > diff --git a/blockdev.c b/blockdev.c > index 622ecba..d78af31 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -631,27 +631,26 @@ err: > return NULL; > } > > -void do_commit(Monitor *mon, const QDict *qdict) > +void qmp_commit(const char *device, Error **errp) > { > - const char *device = qdict_get_str(qdict, "device"); > BlockDriverState *bs; > int ret; > > if (!strcmp(device, "all")) { This 'all' functionality should be moved to hmp_commit(). You can call qmp_query_block() and loop through the list of devices calling qmp_commit() on them. Note that we should do this even if we go for Jeff's block-commit. > ret = bdrv_commit_all(); > if (ret == -EBUSY) { > - qerror_report(QERR_DEVICE_IN_USE, device); > + error_set(errp, QERR_DEVICE_IN_USE, device); > return; > } > } else { > bs = bdrv_find(device); > if (!bs) { > - qerror_report(QERR_DEVICE_NOT_FOUND, device); > + error_set(errp, QERR_DEVICE_NOT_FOUND, device); > return; > } > ret = bdrv_commit(bs); > if (ret == -EBUSY) { > - qerror_report(QERR_DEVICE_IN_USE, device); > + error_set(errp, QERR_DEVICE_IN_USE, device); > return; > } bdrv_commit() can return more errors, but this will ignore them and report success instead. We want to return qerrors for all possible bdrv_commit() errors. But it's a good idea to wait to see if we'll use Jeff's block-commit before making changed to this patch. > } > diff --git a/blockdev.h b/blockdev.h > index 260e16b..c2e52bb 100644 > --- a/blockdev.h > +++ b/blockdev.h > @@ -60,6 +60,5 @@ DriveInfo *add_init_drive(const char *opts); > > void qmp_change_blockdev(const char *device, const char *filename, > bool has_format, const char *format, Error **errp); > -void do_commit(Monitor *mon, const QDict *qdict); > int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data); > #endif > diff --git a/hmp-commands.hx b/hmp-commands.hx > index f5d9d91..53d2c34 100644 > --- a/hmp-commands.hx > +++ b/hmp-commands.hx > @@ -28,7 +28,7 @@ ETEXI > .args_type = "device:B", > .params = "device|all", > .help = "commit changes to the disk images (if -snapshot is used) or backing files", > - .mhandler.cmd = do_commit, > + .mhandler.cmd = hmp_commit, > }, > > STEXI > diff --git a/hmp.c b/hmp.c > index 2ce8cb9..176defa 100644 > --- a/hmp.c > +++ b/hmp.c > @@ -999,3 +999,12 @@ void hmp_netdev_del(Monitor *mon, const QDict *qdict) > qmp_netdev_del(id, &err); > hmp_handle_error(mon, &err); > } > + > +void hmp_commit(Monitor *mon, const QDict *qdict) > +{ > + const char *device = qdict_get_str(qdict, "device"); > + Error *err = NULL; > + > + qmp_commit(device, &err); > + hmp_handle_error(mon, &err); > +} > diff --git a/hmp.h b/hmp.h > index 79d138d..99d57c0 100644 > --- a/hmp.h > +++ b/hmp.h > @@ -64,5 +64,6 @@ void hmp_device_del(Monitor *mon, const QDict *qdict); > void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict); > void hmp_netdev_add(Monitor *mon, const QDict *qdict); > void hmp_netdev_del(Monitor *mon, const QDict *qdict); > +void hmp_commit(Monitor *mon, const QDict *qdict); > > #endif > diff --git a/qapi-schema.json b/qapi-schema.json > index 3b6e346..10cb22b 100644 > --- a/qapi-schema.json > +++ b/qapi-schema.json > @@ -1169,6 +1169,21 @@ > { 'command': 'block_resize', 'data': { 'device': 'str', 'size': 'int' }} > > ## > +# @commit > +# > +# Commit changes to the disk images (if -snapshot is used) or backing files. > +# > +# @device: the name of the device or the "all" to commit all devices > +# > +# Returns: nothing on success > +# If @device is not a valid block device, DeviceNotFound > +# If a long-running operation is using the device, DeviceInUse > +# > +# Since: 1.2 > +## > +{ 'command': 'commit', 'data': { 'device': 'str' }} > + > +## > # @NewImageMode > # > # An enumeration that tells QEMU how to set the backing file path in > diff --git a/qmp-commands.hx b/qmp-commands.hx > index 2e1a38e..8b6bfbc 100644 > --- a/qmp-commands.hx > +++ b/qmp-commands.hx > @@ -714,6 +714,30 @@ Example: > -> { "execute": "block_resize", "arguments": { "device": "scratch", "size": 1073741824 } } > <- { "return": {} } > > + > +EQMP > + > + { > + .name = "commit", > + .args_type = "device:B", > + .mhandler.cmd_new = qmp_marshal_input_commit, > + }, > + > +SQMP > +commit > +------ > + > +Commit changes to the disk images (if -snapshot is used) or backing files. > + > +Arguments: > + > +- "device": the device's ID, or all (json-string) > + > +Example: > + > +-> { "execute": "commit", "arguments": { "device": "all" } } > +<- { "return": {} } > + > EQMP > > {
On Thu, 14 Jun 2012 17:21:44 +0200 Pavel Hrdina <phrdina@redhat.com> wrote: > On 06/14/2012 05:04 PM, Eric Blake wrote: > > On 06/14/2012 08:56 AM, Pavel Hrdina wrote: > >> On 06/14/2012 02:18 PM, Eric Blake wrote: > >>> On 06/14/2012 01:35 AM, Pavel Hrdina wrote: > >>>> Signed-off-by: Pavel Hrdina<phrdina@redhat.com> > >>>> --- > >>>> +++ b/qapi-schema.json > >>>> @@ -1169,6 +1169,21 @@ > >>>> { 'command': 'block_resize', 'data': { 'device': 'str', 'size': > >>>> 'int' }} > >>>> > >>>> ## > >>>> +# @commit > >>>> +# > >>>> +# Commit changes to the disk images (if -snapshot is used) or > >>>> backing files. > >>>> +# > >>>> +# @device: the name of the device or the "all" to commit all devices > >>>> +# > >>>> +# Returns: nothing on success > >>>> +# If @device is not a valid block device, DeviceNotFound > >>>> +# If a long-running operation is using the device, DeviceInUse > >>>> +# > >>>> +# Since: 1.2 > >>>> +## > >>>> +{ 'command': 'commit', 'data': { 'device': 'str' }} > >>> Should we use this as an opportunity to make the command more powerful? > >>> For example, integrating this with the 'transaction' command or a block > >>> job queried by 'query-block-jobs' to track its progress would be useful. > >>> Also, suppose I have A<- B<- C. Does 'commit' only do one layer (C > >>> into B), or all layers (B and C into A)? That argues that we need an > >>> optional parameter that says how deep to commit (committing C into B > >>> only to repeat and commit B into A is more time-consuming than directly > >>> committing both B and C into A to start with). When a commit is > >>> complete, which file is backing the device - is it still C (which > >>> continues to diverge, but now from the point of the commit) or does qemu > >>> pivot things to have the device now backed by B (and C can be discarded, > >>> particularly true if changes are now going into B which invalidate C). > >> What i find out is that 'commit' will commit changes only from C to B > >> and qemu continues with C from the new commit point. I couldn't find a > >> way to commit changes and go back to backing file. This should be > >> supported by parameter and also as you mention that commit all changes > >> through all snapshots should be supported by another parameter. > >> The 'transaction' feature would be nice to have too. > > Which makes it sound like we're starting to overlap with Jeff's work on > > 'block-commit'. > > > > If 'block-commit' proves to be better all around at doing what we want, > > do we even need to keep 'commit' in QMP, or would it be okay for HMP only? > If the 'block-commit' will be better I think that we could drop the > 'commit' completely. And have only 'block-commit' for both QMP and HMP. I completely agree about the QMP part, but for HMP it's a good idea to maintain the commit command. To achieve this, we can implement hmp_commit() in terms of block-commit. Jeff, can you answer us here? Does block-commit supersedes the commit command we have today?
On 06/15/2012 10:02 AM, Luiz Capitulino wrote: > On Thu, 14 Jun 2012 17:21:44 +0200 > Pavel Hrdina <phrdina@redhat.com> wrote: > >> On 06/14/2012 05:04 PM, Eric Blake wrote: >>> On 06/14/2012 08:56 AM, Pavel Hrdina wrote: >>>> On 06/14/2012 02:18 PM, Eric Blake wrote: >>>>> On 06/14/2012 01:35 AM, Pavel Hrdina wrote: >>>>>> Signed-off-by: Pavel Hrdina<phrdina@redhat.com> >>>>>> --- >>>>>> +++ b/qapi-schema.json >>>>>> @@ -1169,6 +1169,21 @@ >>>>>> { 'command': 'block_resize', 'data': { 'device': 'str', 'size': >>>>>> 'int' }} >>>>>> >>>>>> ## >>>>>> +# @commit >>>>>> +# >>>>>> +# Commit changes to the disk images (if -snapshot is used) or >>>>>> backing files. >>>>>> +# >>>>>> +# @device: the name of the device or the "all" to commit all devices >>>>>> +# >>>>>> +# Returns: nothing on success >>>>>> +# If @device is not a valid block device, DeviceNotFound >>>>>> +# If a long-running operation is using the device, DeviceInUse >>>>>> +# >>>>>> +# Since: 1.2 >>>>>> +## >>>>>> +{ 'command': 'commit', 'data': { 'device': 'str' }} >>>>> Should we use this as an opportunity to make the command more powerful? >>>>> For example, integrating this with the 'transaction' command or a block >>>>> job queried by 'query-block-jobs' to track its progress would be useful. >>>>> Also, suppose I have A<- B<- C. Does 'commit' only do one layer (C >>>>> into B), or all layers (B and C into A)? That argues that we need an >>>>> optional parameter that says how deep to commit (committing C into B >>>>> only to repeat and commit B into A is more time-consuming than directly >>>>> committing both B and C into A to start with). When a commit is >>>>> complete, which file is backing the device - is it still C (which >>>>> continues to diverge, but now from the point of the commit) or does qemu >>>>> pivot things to have the device now backed by B (and C can be discarded, >>>>> particularly true if changes are now going into B which invalidate C). >>>> What i find out is that 'commit' will commit changes only from C to B >>>> and qemu continues with C from the new commit point. I couldn't find a >>>> way to commit changes and go back to backing file. This should be >>>> supported by parameter and also as you mention that commit all changes >>>> through all snapshots should be supported by another parameter. >>>> The 'transaction' feature would be nice to have too. >>> Which makes it sound like we're starting to overlap with Jeff's work on >>> 'block-commit'. >>> >>> If 'block-commit' proves to be better all around at doing what we want, >>> do we even need to keep 'commit' in QMP, or would it be okay for HMP only? >> If the 'block-commit' will be better I think that we could drop the >> 'commit' completely. And have only 'block-commit' for both QMP and HMP. > > I completely agree about the QMP part, but for HMP it's a good idea to > maintain the commit command. To achieve this, we can implement hmp_commit() > in terms of block-commit. > > Jeff, can you answer us here? Does block-commit supersedes the commit command > we have today? The block-commit will supercede in functionality the commit command in place today, but it is a live operation - as such, it will take longer to complete, but it won't pause the guest. In general, I think it would be safe to say that it could supersede the current commit command, unless others see a use case for a commit operation that completes faster yet pauses the guest. I think being able to rely on qemu-img to perform an offline commit would be sufficient. I agree on the HMP command matching the QMP command in the method used, so that there is no confusion.
On Fri, 15 Jun 2012 10:11:46 -0400 Jeff Cody <jcody@redhat.com> wrote: > On 06/15/2012 10:02 AM, Luiz Capitulino wrote: > > On Thu, 14 Jun 2012 17:21:44 +0200 > > Pavel Hrdina <phrdina@redhat.com> wrote: > > > >> On 06/14/2012 05:04 PM, Eric Blake wrote: > >>> On 06/14/2012 08:56 AM, Pavel Hrdina wrote: > >>>> On 06/14/2012 02:18 PM, Eric Blake wrote: > >>>>> On 06/14/2012 01:35 AM, Pavel Hrdina wrote: > >>>>>> Signed-off-by: Pavel Hrdina<phrdina@redhat.com> > >>>>>> --- > >>>>>> +++ b/qapi-schema.json > >>>>>> @@ -1169,6 +1169,21 @@ > >>>>>> { 'command': 'block_resize', 'data': { 'device': 'str', 'size': > >>>>>> 'int' }} > >>>>>> > >>>>>> ## > >>>>>> +# @commit > >>>>>> +# > >>>>>> +# Commit changes to the disk images (if -snapshot is used) or > >>>>>> backing files. > >>>>>> +# > >>>>>> +# @device: the name of the device or the "all" to commit all devices > >>>>>> +# > >>>>>> +# Returns: nothing on success > >>>>>> +# If @device is not a valid block device, DeviceNotFound > >>>>>> +# If a long-running operation is using the device, DeviceInUse > >>>>>> +# > >>>>>> +# Since: 1.2 > >>>>>> +## > >>>>>> +{ 'command': 'commit', 'data': { 'device': 'str' }} > >>>>> Should we use this as an opportunity to make the command more powerful? > >>>>> For example, integrating this with the 'transaction' command or a block > >>>>> job queried by 'query-block-jobs' to track its progress would be useful. > >>>>> Also, suppose I have A<- B<- C. Does 'commit' only do one layer (C > >>>>> into B), or all layers (B and C into A)? That argues that we need an > >>>>> optional parameter that says how deep to commit (committing C into B > >>>>> only to repeat and commit B into A is more time-consuming than directly > >>>>> committing both B and C into A to start with). When a commit is > >>>>> complete, which file is backing the device - is it still C (which > >>>>> continues to diverge, but now from the point of the commit) or does qemu > >>>>> pivot things to have the device now backed by B (and C can be discarded, > >>>>> particularly true if changes are now going into B which invalidate C). > >>>> What i find out is that 'commit' will commit changes only from C to B > >>>> and qemu continues with C from the new commit point. I couldn't find a > >>>> way to commit changes and go back to backing file. This should be > >>>> supported by parameter and also as you mention that commit all changes > >>>> through all snapshots should be supported by another parameter. > >>>> The 'transaction' feature would be nice to have too. > >>> Which makes it sound like we're starting to overlap with Jeff's work on > >>> 'block-commit'. > >>> > >>> If 'block-commit' proves to be better all around at doing what we want, > >>> do we even need to keep 'commit' in QMP, or would it be okay for HMP only? > >> If the 'block-commit' will be better I think that we could drop the > >> 'commit' completely. And have only 'block-commit' for both QMP and HMP. > > > > I completely agree about the QMP part, but for HMP it's a good idea to > > maintain the commit command. To achieve this, we can implement hmp_commit() > > in terms of block-commit. > > > > Jeff, can you answer us here? Does block-commit supersedes the commit command > > we have today? > > The block-commit will supercede in functionality the commit command in > place today, but it is a live operation - as such, it will take longer > to complete, but it won't pause the guest. This is very nice, is this being targeted for 1.2?
On 06/15/2012 10:44 AM, Luiz Capitulino wrote: > On Fri, 15 Jun 2012 10:11:46 -0400 > Jeff Cody <jcody@redhat.com> wrote: > >> On 06/15/2012 10:02 AM, Luiz Capitulino wrote: >>> On Thu, 14 Jun 2012 17:21:44 +0200 >>> Pavel Hrdina <phrdina@redhat.com> wrote: >>> >>>> On 06/14/2012 05:04 PM, Eric Blake wrote: >>>>> On 06/14/2012 08:56 AM, Pavel Hrdina wrote: >>>>>> On 06/14/2012 02:18 PM, Eric Blake wrote: >>>>>>> On 06/14/2012 01:35 AM, Pavel Hrdina wrote: >>>>>>>> Signed-off-by: Pavel Hrdina<phrdina@redhat.com> >>>>>>>> --- >>>>>>>> +++ b/qapi-schema.json >>>>>>>> @@ -1169,6 +1169,21 @@ >>>>>>>> { 'command': 'block_resize', 'data': { 'device': 'str', 'size': >>>>>>>> 'int' }} >>>>>>>> >>>>>>>> ## >>>>>>>> +# @commit >>>>>>>> +# >>>>>>>> +# Commit changes to the disk images (if -snapshot is used) or >>>>>>>> backing files. >>>>>>>> +# >>>>>>>> +# @device: the name of the device or the "all" to commit all devices >>>>>>>> +# >>>>>>>> +# Returns: nothing on success >>>>>>>> +# If @device is not a valid block device, DeviceNotFound >>>>>>>> +# If a long-running operation is using the device, DeviceInUse >>>>>>>> +# >>>>>>>> +# Since: 1.2 >>>>>>>> +## >>>>>>>> +{ 'command': 'commit', 'data': { 'device': 'str' }} >>>>>>> Should we use this as an opportunity to make the command more powerful? >>>>>>> For example, integrating this with the 'transaction' command or a block >>>>>>> job queried by 'query-block-jobs' to track its progress would be useful. >>>>>>> Also, suppose I have A<- B<- C. Does 'commit' only do one layer (C >>>>>>> into B), or all layers (B and C into A)? That argues that we need an >>>>>>> optional parameter that says how deep to commit (committing C into B >>>>>>> only to repeat and commit B into A is more time-consuming than directly >>>>>>> committing both B and C into A to start with). When a commit is >>>>>>> complete, which file is backing the device - is it still C (which >>>>>>> continues to diverge, but now from the point of the commit) or does qemu >>>>>>> pivot things to have the device now backed by B (and C can be discarded, >>>>>>> particularly true if changes are now going into B which invalidate C). >>>>>> What i find out is that 'commit' will commit changes only from C to B >>>>>> and qemu continues with C from the new commit point. I couldn't find a >>>>>> way to commit changes and go back to backing file. This should be >>>>>> supported by parameter and also as you mention that commit all changes >>>>>> through all snapshots should be supported by another parameter. >>>>>> The 'transaction' feature would be nice to have too. >>>>> Which makes it sound like we're starting to overlap with Jeff's work on >>>>> 'block-commit'. >>>>> >>>>> If 'block-commit' proves to be better all around at doing what we want, >>>>> do we even need to keep 'commit' in QMP, or would it be okay for HMP only? >>>> If the 'block-commit' will be better I think that we could drop the >>>> 'commit' completely. And have only 'block-commit' for both QMP and HMP. >>> >>> I completely agree about the QMP part, but for HMP it's a good idea to >>> maintain the commit command. To achieve this, we can implement hmp_commit() >>> in terms of block-commit. >>> >>> Jeff, can you answer us here? Does block-commit supersedes the commit command >>> we have today? >> >> The block-commit will supercede in functionality the commit command in >> place today, but it is a live operation - as such, it will take longer >> to complete, but it won't pause the guest. > > This is very nice, is this being targeted for 1.2? Yes, I'd like to see this in 1.2, so that is my goal.
diff --git a/blockdev.c b/blockdev.c index 622ecba..d78af31 100644 --- a/blockdev.c +++ b/blockdev.c @@ -631,27 +631,26 @@ err: return NULL; } -void do_commit(Monitor *mon, const QDict *qdict) +void qmp_commit(const char *device, Error **errp) { - const char *device = qdict_get_str(qdict, "device"); BlockDriverState *bs; int ret; if (!strcmp(device, "all")) { ret = bdrv_commit_all(); if (ret == -EBUSY) { - qerror_report(QERR_DEVICE_IN_USE, device); + error_set(errp, QERR_DEVICE_IN_USE, device); return; } } else { bs = bdrv_find(device); if (!bs) { - qerror_report(QERR_DEVICE_NOT_FOUND, device); + error_set(errp, QERR_DEVICE_NOT_FOUND, device); return; } ret = bdrv_commit(bs); if (ret == -EBUSY) { - qerror_report(QERR_DEVICE_IN_USE, device); + error_set(errp, QERR_DEVICE_IN_USE, device); return; } } diff --git a/blockdev.h b/blockdev.h index 260e16b..c2e52bb 100644 --- a/blockdev.h +++ b/blockdev.h @@ -60,6 +60,5 @@ DriveInfo *add_init_drive(const char *opts); void qmp_change_blockdev(const char *device, const char *filename, bool has_format, const char *format, Error **errp); -void do_commit(Monitor *mon, const QDict *qdict); int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data); #endif diff --git a/hmp-commands.hx b/hmp-commands.hx index f5d9d91..53d2c34 100644 --- a/hmp-commands.hx +++ b/hmp-commands.hx @@ -28,7 +28,7 @@ ETEXI .args_type = "device:B", .params = "device|all", .help = "commit changes to the disk images (if -snapshot is used) or backing files", - .mhandler.cmd = do_commit, + .mhandler.cmd = hmp_commit, }, STEXI diff --git a/hmp.c b/hmp.c index 2ce8cb9..176defa 100644 --- a/hmp.c +++ b/hmp.c @@ -999,3 +999,12 @@ void hmp_netdev_del(Monitor *mon, const QDict *qdict) qmp_netdev_del(id, &err); hmp_handle_error(mon, &err); } + +void hmp_commit(Monitor *mon, const QDict *qdict) +{ + const char *device = qdict_get_str(qdict, "device"); + Error *err = NULL; + + qmp_commit(device, &err); + hmp_handle_error(mon, &err); +} diff --git a/hmp.h b/hmp.h index 79d138d..99d57c0 100644 --- a/hmp.h +++ b/hmp.h @@ -64,5 +64,6 @@ void hmp_device_del(Monitor *mon, const QDict *qdict); void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict); void hmp_netdev_add(Monitor *mon, const QDict *qdict); void hmp_netdev_del(Monitor *mon, const QDict *qdict); +void hmp_commit(Monitor *mon, const QDict *qdict); #endif diff --git a/qapi-schema.json b/qapi-schema.json index 3b6e346..10cb22b 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -1169,6 +1169,21 @@ { 'command': 'block_resize', 'data': { 'device': 'str', 'size': 'int' }} ## +# @commit +# +# Commit changes to the disk images (if -snapshot is used) or backing files. +# +# @device: the name of the device or the "all" to commit all devices +# +# Returns: nothing on success +# If @device is not a valid block device, DeviceNotFound +# If a long-running operation is using the device, DeviceInUse +# +# Since: 1.1 +## +{ 'command': 'commit', 'data': { 'device': 'str' }} + +## # @NewImageMode # # An enumeration that tells QEMU how to set the backing file path in diff --git a/qmp-commands.hx b/qmp-commands.hx index 2e1a38e..8b6bfbc 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -714,6 +714,30 @@ Example: -> { "execute": "block_resize", "arguments": { "device": "scratch", "size": 1073741824 } } <- { "return": {} } + +EQMP + + { + .name = "commit", + .args_type = "device:B", + .mhandler.cmd_new = qmp_marshal_input_commit, + }, + +SQMP +commit +------ + +Commit changes to the disk images (if -snapshot is used) or backing files. + +Arguments: + +- "device": the device's ID, or all (json-string) + +Example: + +-> { "execute": "commit", "arguments": { "device": "all" } } +<- { "return": {} } + EQMP {
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- blockdev.c | 9 ++++----- blockdev.h | 1 - hmp-commands.hx | 2 +- hmp.c | 9 +++++++++ hmp.h | 1 + qapi-schema.json | 15 +++++++++++++++ qmp-commands.hx | 24 ++++++++++++++++++++++++ 7 files changed, 54 insertions(+), 7 deletions(-)