| Message ID | 1325858501-25741-9-git-send-email-stefanha@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On 01/06/2012 07:01 AM, Stefan Hajnoczi wrote: > Add block_job_cancel, which stops an active block streaming operation. > When the operation has been cancelled the new BLOCK_JOB_CANCELLED event > is emitted. > > Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> > +++ b/hmp-commands.hx > @@ -98,6 +98,20 @@ Set maximum speed for a background block operation. > ETEXI > > { > + .name = "block_job_cancel", > + .args_type = "device:B", > + .params = "device", > + .help = "stop an active block streaming operation", > + .mhandler.cmd = hmp_block_job_cancel, > + }, > + Looking at this from libvirt's perspective, would it be possible to give this a different name? Then libvirt would know that if block_job_cancel_async exists, we have the official semantics; and if it doesn't exist, then we can try block_job_cancel as a fallback to see if we have the old blocking semantics. But by using the same name as the old unofficial blocking command, it's difficult to tell if we should expect an event, or whether completion of the command means completion of the cancel. On the other hand, I guess we can rely on completion of the command, followed by reading block job status to see if the job is still in flight, will tell us whether we need to worry about waiting for an event - if the job is complete (whether or not this command was the blocking variant), we are done; if the job is ongoing, we have the new semantics and can expect an event; and that only leaves the race of calling the command, then the job completes, then we query and see it done, then the event comes, where we just have to be ready to ignore an unexpected event. > +## > +# @block_job_cancel: > +# > +# Stop an active block streaming operation. > +# > +# This command returns immediately after marking the active block streaming > +# operation for cancellation. It is an error to call this command if no > +# operation is in progress. > +# > +# The operation will cancel as soon as possible and then emit the > +# BLOCK_JOB_CANCELLED event. Before that happens the job is still visible when > +# enumerated using query-block-jobs. Is there any policy on _ vs - in command names? It seems awkward to have block_job_cancel but query-block-jobs.
Am 20.01.2012 01:02, schrieb Eric Blake: > On 01/06/2012 07:01 AM, Stefan Hajnoczi wrote: >> Add block_job_cancel, which stops an active block streaming operation. >> When the operation has been cancelled the new BLOCK_JOB_CANCELLED event >> is emitted. >> >> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> > >> +++ b/hmp-commands.hx >> @@ -98,6 +98,20 @@ Set maximum speed for a background block operation. >> ETEXI >> >> { >> + .name = "block_job_cancel", >> + .args_type = "device:B", >> + .params = "device", >> + .help = "stop an active block streaming operation", >> + .mhandler.cmd = hmp_block_job_cancel, >> + }, >> + > > Looking at this from libvirt's perspective, would it be possible to give > this a different name? Then libvirt would know that if > block_job_cancel_async exists, we have the official semantics; and if it > doesn't exist, then we can try block_job_cancel as a fallback to see if > we have the old blocking semantics. > > But by using the same name as the old unofficial blocking command, it's > difficult to tell if we should expect an event, or whether completion of > the command means completion of the cancel. > > On the other hand, I guess we can rely on completion of the command, > followed by reading block job status to see if the job is still in > flight, will tell us whether we need to worry about waiting for an event > - if the job is complete (whether or not this command was the blocking > variant), we are done; if the job is ongoing, we have the new semantics > and can expect an event; and that only leaves the race of calling the > command, then the job completes, then we query and see it done, then the > event comes, where we just have to be ready to ignore an unexpected event. You're quoting the HMP part, is that intentional? You shouldn't be using this at all. Anyway, are there even any qemu versions out there that implement an older interface? >> +## >> +# @block_job_cancel: >> +# >> +# Stop an active block streaming operation. >> +# >> +# This command returns immediately after marking the active block streaming >> +# operation for cancellation. It is an error to call this command if no >> +# operation is in progress. >> +# >> +# The operation will cancel as soon as possible and then emit the >> +# BLOCK_JOB_CANCELLED event. Before that happens the job is still visible when >> +# enumerated using query-block-jobs. > > Is there any policy on _ vs - in command names? It seems awkward to > have block_job_cancel but query-block-jobs. block_job_cancel is HMP, whereas query-block-jobs is a QMP command. QMP uses - consistently. Not sure if HMP is consistent, but it tends to use _. Kevin
On Fri, 20 Jan 2012 09:30:45 +0100 Kevin Wolf <kwolf@redhat.com> wrote: > Am 20.01.2012 01:02, schrieb Eric Blake: > > On 01/06/2012 07:01 AM, Stefan Hajnoczi wrote: > >> Add block_job_cancel, which stops an active block streaming operation. > >> When the operation has been cancelled the new BLOCK_JOB_CANCELLED event > >> is emitted. > >> > >> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> > > > >> +++ b/hmp-commands.hx > >> @@ -98,6 +98,20 @@ Set maximum speed for a background block operation. > >> ETEXI > >> > >> { > >> + .name = "block_job_cancel", > >> + .args_type = "device:B", > >> + .params = "device", > >> + .help = "stop an active block streaming operation", > >> + .mhandler.cmd = hmp_block_job_cancel, > >> + }, > >> + > > > > Looking at this from libvirt's perspective, would it be possible to give > > this a different name? Then libvirt would know that if > > block_job_cancel_async exists, we have the official semantics; and if it > > doesn't exist, then we can try block_job_cancel as a fallback to see if > > we have the old blocking semantics. > > > > But by using the same name as the old unofficial blocking command, it's > > difficult to tell if we should expect an event, or whether completion of > > the command means completion of the cancel. > > > > On the other hand, I guess we can rely on completion of the command, > > followed by reading block job status to see if the job is still in > > flight, will tell us whether we need to worry about waiting for an event > > - if the job is complete (whether or not this command was the blocking > > variant), we are done; if the job is ongoing, we have the new semantics > > and can expect an event; and that only leaves the race of calling the > > command, then the job completes, then we query and see it done, then the > > event comes, where we just have to be ready to ignore an unexpected event. > > You're quoting the HMP part, is that intentional? You shouldn't be using > this at all. > > Anyway, are there even any qemu versions out there that implement an > older interface? > > >> +## > >> +# @block_job_cancel: > >> +# > >> +# Stop an active block streaming operation. > >> +# > >> +# This command returns immediately after marking the active block streaming > >> +# operation for cancellation. It is an error to call this command if no > >> +# operation is in progress. > >> +# > >> +# The operation will cancel as soon as possible and then emit the > >> +# BLOCK_JOB_CANCELLED event. Before that happens the job is still visible when > >> +# enumerated using query-block-jobs. > > > > Is there any policy on _ vs - in command names? It seems awkward to > > have block_job_cancel but query-block-jobs. > > block_job_cancel is HMP, whereas query-block-jobs is a QMP command. QMP > uses - consistently. Not sure if HMP is consistent, but it tends to use _. This very series broke QMP's consistency because it was designed when we were following HMP's inconsistencies...
On 01/20/2012 01:30 AM, Kevin Wolf wrote: >>> +++ b/hmp-commands.hx >> Looking at this from libvirt's perspective, would it be possible to give >> this a different name? Then libvirt would know that if >> block_job_cancel_async exists, we have the official semantics; and if it >> doesn't exist, then we can try block_job_cancel as a fallback to see if >> we have the old blocking semantics. >> >> But by using the same name as the old unofficial blocking command, it's >> difficult to tell if we should expect an event, or whether completion of >> the command means completion of the cancel. >> > > You're quoting the HMP part, is that intentional? You shouldn't be using > this at all. Oh - I missed the .hx file name when trimming my reply. I agree that libvirt will be using the QMP, not HMP command. > > Anyway, are there even any qemu versions out there that implement an > older interface? The older interface was backported into RHEL 6.2 qemu, so libvirt has to maintain the distinction (although it can equally be argued that the distinction only exists in the RHEL build, and therefore, only the RHEL build of libvirt has to deal with the issues of an early backport of a qemu feature). >> Is there any policy on _ vs - in command names? It seems awkward to >> have block_job_cancel but query-block-jobs. > > block_job_cancel is HMP, whereas query-block-jobs is a QMP command. QMP > uses - consistently. Not sure if HMP is consistent, but it tends to use _. OK, on RHEL 6.2, 'help' on HMP includes help for block_job_cancel, and '{"execute":"query-commands"}' on QMP includes {"name":"block_job_cancel"}, with underscores in both locations. So if your new QMP command is block-job-cancel, you've met the goal of having a different spelling from the RHEL backport of the earlier semantics. And if you stuck with underscores, then it's RHEL's problem to solve :)
On Fri, Jan 20, 2012 at 7:55 PM, Eric Blake <eblake@redhat.com> wrote: > On 01/20/2012 01:30 AM, Kevin Wolf wrote: >>> Is there any policy on _ vs - in command names? It seems awkward to >>> have block_job_cancel but query-block-jobs. >> >> block_job_cancel is HMP, whereas query-block-jobs is a QMP command. QMP >> uses - consistently. Not sure if HMP is consistent, but it tends to use _. > > OK, on RHEL 6.2, 'help' on HMP includes help for block_job_cancel, and > '{"execute":"query-commands"}' on QMP includes > {"name":"block_job_cancel"}, with underscores in both locations. So if > your new QMP command is block-job-cancel, you've met the goal of having > a different spelling from the RHEL backport of the earlier semantics. > And if you stuck with underscores, then it's RHEL's problem to solve :) Hi Eric, qemu.git/master has "query-block-jobs" with all other streaming/blockjob commands using underscores. I suspect this is identical to RHEL 6.2. Stefan
On 01/20/2012 12:55 PM, Eric Blake wrote: Revisiting this thread, as I'd really like to have a solution in place before qemu 1.1 is released. >>> Looking at this from libvirt's perspective, would it be possible to give >>> this a different name? Then libvirt would know that if >>> block_job_cancel_async exists, we have the official semantics; and if it >>> doesn't exist, then we can try block_job_cancel as a fallback to see if >>> we have the old blocking semantics. >>> >>> But by using the same name as the old unofficial blocking command, it's >>> difficult to tell if we should expect an event, or whether completion of >>> the command means completion of the cancel. >>> > > The older interface was backported into RHEL 6.2 qemu, so libvirt has to > maintain the distinction (although it can equally be argued that the > distinction only exists in the RHEL build, and therefore, only the RHEL > build of libvirt has to deal with the issues of an early backport of a > qemu feature). > >>> Is there any policy on _ vs - in command names? It seems awkward to >>> have block_job_cancel but query-block-jobs. >> >> block_job_cancel is HMP, whereas query-block-jobs is a QMP command. QMP >> uses - consistently. Not sure if HMP is consistent, but it tends to use _. > > OK, on RHEL 6.2, 'help' on HMP includes help for block_job_cancel, and > '{"execute":"query-commands"}' on QMP includes > {"name":"block_job_cancel"}, with underscores in both locations. So if > your new QMP command is block-job-cancel, you've met the goal of having > a different spelling from the RHEL backport of the earlier semantics. > And if you stuck with underscores, then it's RHEL's problem to solve :) Are we settled enough on the 'drive-mirror' command to commit to that in the qapi in time for qemu 1.1, even if we don't implement it until qemu 1.2? That would be a sufficient witness of another block job command that therefore implies job cancellation is asynchronous. If we don't commit to 'drive-mirror' in time, then what else can libvirt use to distinguish between RHEL 6.2 synchronous 'block_job_cancel' and the asynchronous command introduced by this series? Is it too late to rename the QMP command to 'block-job-cancel'?
Am 10.04.2012 20:06, schrieb Eric Blake: >>>> Is there any policy on _ vs - in command names? It seems awkward to >>>> have block_job_cancel but query-block-jobs. >>> >>> block_job_cancel is HMP, whereas query-block-jobs is a QMP command. QMP >>> uses - consistently. Not sure if HMP is consistent, but it tends to use _. >> >> OK, on RHEL 6.2, 'help' on HMP includes help for block_job_cancel, and >> '{"execute":"query-commands"}' on QMP includes >> {"name":"block_job_cancel"}, with underscores in both locations. So if >> your new QMP command is block-job-cancel, you've met the goal of having >> a different spelling from the RHEL backport of the earlier semantics. >> And if you stuck with underscores, then it's RHEL's problem to solve :) > > Are we settled enough on the 'drive-mirror' command to commit to that in > the qapi in time for qemu 1.1, even if we don't implement it until qemu > 1.2? That would be a sufficient witness of another block job command > that therefore implies job cancellation is asynchronous. We did this kind of thing with image streaming, and we regretted it when patch review brought up changed that we would have liked to make, but couldn't any more because libvirt already used the initial version. So I'd rather avoid promising any details of an API before it's really implemented. > If we don't > commit to 'drive-mirror' in time, then what else can libvirt use to > distinguish between RHEL 6.2 synchronous 'block_job_cancel' and the > asynchronous command introduced by this series? query-version return 0.12.1 on RHEL 6.2, plus something completely unrelated that is added in 6.3 like the transaction command? > Is it too late to rename the QMP command to 'block-job-cancel'? Streaming hasn't been in any release yet, so in theory I guess we could rename the commands. Kevin
On Wed, 11 Apr 2012 16:01:26 +0200 Kevin Wolf <kwolf@redhat.com> wrote: > > Is it too late to rename the QMP command to 'block-job-cancel'? > > Streaming hasn't been in any release yet, so in theory I guess we could > rename the commands. Yes, we've just discussed this on irc and all involved parties are ok with this.
On Wed, Apr 11, 2012 at 3:01 PM, Kevin Wolf <kwolf@redhat.com> wrote: > Am 10.04.2012 20:06, schrieb Eric Blake: >>>>> Is there any policy on _ vs - in command names? It seems awkward to >>>>> have block_job_cancel but query-block-jobs. >>>> >>>> block_job_cancel is HMP, whereas query-block-jobs is a QMP command. QMP >>>> uses - consistently. Not sure if HMP is consistent, but it tends to use _. >>> >>> OK, on RHEL 6.2, 'help' on HMP includes help for block_job_cancel, and >>> '{"execute":"query-commands"}' on QMP includes >>> {"name":"block_job_cancel"}, with underscores in both locations. So if >>> your new QMP command is block-job-cancel, you've met the goal of having >>> a different spelling from the RHEL backport of the earlier semantics. >>> And if you stuck with underscores, then it's RHEL's problem to solve :) >> >> Are we settled enough on the 'drive-mirror' command to commit to that in >> the qapi in time for qemu 1.1, even if we don't implement it until qemu >> 1.2? That would be a sufficient witness of another block job command >> that therefore implies job cancellation is asynchronous. > > We did this kind of thing with image streaming, and we regretted it when > patch review brought up changed that we would have liked to make, but > couldn't any more because libvirt already used the initial version. > > So I'd rather avoid promising any details of an API before it's really > implemented. > >> If we don't >> commit to 'drive-mirror' in time, then what else can libvirt use to >> distinguish between RHEL 6.2 synchronous 'block_job_cancel' and the >> asynchronous command introduced by this series? > > query-version return 0.12.1 on RHEL 6.2, plus something completely > unrelated that is added in 6.3 like the transaction command? > >> Is it too late to rename the QMP command to 'block-job-cancel'? > > Streaming hasn't been in any release yet, so in theory I guess we could > rename the commands. I actually like Eric's suggestion to rename the commands. Libvirt needs code to handle both semantics now anyway. We might as well use this chance to make the command naming consistent with QAPI/QMP rules now. Will send the patches shortly. Stefan
diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt index a80e604..310c4c6 100644 --- a/QMP/qmp-events.txt +++ b/QMP/qmp-events.txt @@ -293,3 +293,27 @@ Example: "len": 10737418240, "offset": 10737418240, "speed": 0 }, "timestamp": { "seconds": 1267061043, "microseconds": 959568 } } + + +BLOCK_JOB_CANCELLED +------------------- + +Emitted when a block job has been cancelled. + +Data: + +- "type": Job type ("stream" for image streaming, json-string) +- "device": Device name (json-string) +- "len": Maximum progress value (json-int) +- "offset": Current progress value (json-int) + On success this is equal to len. + On failure this is less than len. +- "speed": Rate limit, bytes per second (json-int) + +Example: + +{ "event": "BLOCK_JOB_CANCELLED", + "data": { "type": "stream", "device": "virtio-disk0", + "len": 10737418240, "offset": 134217728, + "speed": 0 }, + "timestamp": { "seconds": 1267061043, "microseconds": 959568 } } diff --git a/blockdev.c b/blockdev.c index 2dfca40..35de3bc 100644 --- a/blockdev.c +++ b/blockdev.c @@ -911,7 +911,11 @@ static void block_stream_cb(void *opaque, int ret) qdict_put(dict, "error", qstring_from_str(strerror(-ret))); } - monitor_protocol_event(QEVENT_BLOCK_JOB_COMPLETED, obj); + if (block_job_is_cancelled(bs->job)) { + monitor_protocol_event(QEVENT_BLOCK_JOB_CANCELLED, obj); + } else { + monitor_protocol_event(QEVENT_BLOCK_JOB_COMPLETED, obj); + } qobject_decref(obj); } @@ -972,3 +976,16 @@ void qmp_block_job_set_speed(const char *device, int64_t value, Error **errp) error_set(errp, QERR_NOT_SUPPORTED); } } + +void qmp_block_job_cancel(const char *device, Error **errp) +{ + BlockJob *job = find_block_job(device); + + if (!job) { + error_set(errp, QERR_DEVICE_NOT_ACTIVE, device); + return; + } + + trace_qmp_block_job_cancel(job); + block_job_cancel(job); +} diff --git a/hmp-commands.hx b/hmp-commands.hx index 5dffcb2..14d6122 100644 --- a/hmp-commands.hx +++ b/hmp-commands.hx @@ -98,6 +98,20 @@ Set maximum speed for a background block operation. ETEXI { + .name = "block_job_cancel", + .args_type = "device:B", + .params = "device", + .help = "stop an active block streaming operation", + .mhandler.cmd = hmp_block_job_cancel, + }, + +STEXI +@item block_job_cancel +@findex block_job_cancel +Stop an active block streaming operation. +ETEXI + + { .name = "eject", .args_type = "force:-f,device:B", .params = "[-f] device", diff --git a/hmp.c b/hmp.c index 1144d53..851885b 100644 --- a/hmp.c +++ b/hmp.c @@ -701,3 +701,13 @@ void hmp_block_job_set_speed(Monitor *mon, const QDict *qdict) hmp_handle_error(mon, &error); } + +void hmp_block_job_cancel(Monitor *mon, const QDict *qdict) +{ + Error *error = NULL; + const char *device = qdict_get_str(qdict, "device"); + + qmp_block_job_cancel(device, &error); + + hmp_handle_error(mon, &error); +} diff --git a/hmp.h b/hmp.h index 2c871ea..0ad2004 100644 --- a/hmp.h +++ b/hmp.h @@ -51,5 +51,6 @@ void hmp_migrate_set_downtime(Monitor *mon, const QDict *qdict); void hmp_migrate_set_speed(Monitor *mon, const QDict *qdict); void hmp_block_stream(Monitor *mon, const QDict *qdict); void hmp_block_job_set_speed(Monitor *mon, const QDict *qdict); +void hmp_block_job_cancel(Monitor *mon, const QDict *qdict); #endif diff --git a/monitor.c b/monitor.c index bb42580..01850ca 100644 --- a/monitor.c +++ b/monitor.c @@ -482,6 +482,9 @@ void monitor_protocol_event(MonitorEvent event, QObject *data) case QEVENT_BLOCK_JOB_COMPLETED: event_name = "BLOCK_JOB_COMPLETED"; break; + case QEVENT_BLOCK_JOB_CANCELLED: + event_name = "BLOCK_JOB_CANCELLED"; + break; default: abort(); break; diff --git a/monitor.h b/monitor.h index 7324236..86c997d 100644 --- a/monitor.h +++ b/monitor.h @@ -36,6 +36,7 @@ typedef enum MonitorEvent { QEVENT_SPICE_INITIALIZED, QEVENT_SPICE_DISCONNECTED, QEVENT_BLOCK_JOB_COMPLETED, + QEVENT_BLOCK_JOB_CANCELLED, QEVENT_MAX, } MonitorEvent; diff --git a/qapi-schema.json b/qapi-schema.json index 7b39371..d3fd36e 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -1329,3 +1329,32 @@ ## { 'command': 'block_job_set_speed', 'data': { 'device': 'str', 'value': 'int' } } + +## +# @block_job_cancel: +# +# Stop an active block streaming operation. +# +# This command returns immediately after marking the active block streaming +# operation for cancellation. It is an error to call this command if no +# operation is in progress. +# +# The operation will cancel as soon as possible and then emit the +# BLOCK_JOB_CANCELLED event. Before that happens the job is still visible when +# enumerated using query-block-jobs. +# +# The image file retains its backing file unless the streaming operation happens +# to complete just as it is being cancelled. +# +# A new block streaming operation can be started at a later time to finish +# copying all data from the backing file. +# +# @device: the device name +# +# Returns: Nothing on success +# If streaming is not active on this device, DeviceNotActive +# If cancellation already in progress, DeviceInUse +# +# Since: 1.1 +## +{ 'command': 'block_job_cancel', 'data': { 'device': 'str' } } diff --git a/qmp-commands.hx b/qmp-commands.hx index dc6bc2e..0a0335f 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -667,6 +667,12 @@ EQMP }, { + .name = "block_job_cancel", + .args_type = "device:B", + .mhandler.cmd_new = qmp_marshal_input_block_job_cancel, + }, + + { .name = "blockdev-snapshot-sync", .args_type = "device:B,snapshot-file:s,format:s?", .mhandler.cmd_new = qmp_marshal_input_blockdev_snapshot_sync, diff --git a/trace-events b/trace-events index 6ff0d43..196a872 100644 --- a/trace-events +++ b/trace-events @@ -75,6 +75,7 @@ stream_one_iteration(void *s, int64_t sector_num, int nb_sectors, int is_allocat stream_start(void *bs, void *base, void *s, void *co, void *opaque) "bs %p base %p s %p co %p opaque %p" # blockdev.c +qmp_block_job_cancel(void *job) "job %p" block_stream_cb(void *bs, void *job, int ret) "bs %p job %p ret %d" qmp_block_stream(void *bs, void *job) "bs %p job %p"
Add block_job_cancel, which stops an active block streaming operation. When the operation has been cancelled the new BLOCK_JOB_CANCELLED event is emitted. Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> --- QMP/qmp-events.txt | 24 ++++++++++++++++++++++++ blockdev.c | 19 ++++++++++++++++++- hmp-commands.hx | 14 ++++++++++++++ hmp.c | 10 ++++++++++ hmp.h | 1 + monitor.c | 3 +++ monitor.h | 1 + qapi-schema.json | 29 +++++++++++++++++++++++++++++ qmp-commands.hx | 6 ++++++ trace-events | 1 + 10 files changed, 107 insertions(+), 1 deletions(-)