On Fri, Jan 13, 2012 at 04:30:56PM -0600, Adam Litke wrote:
> On Fri, Jan 13, 2012 at 04:48:30PM -0500, Dave Allan wrote:
> > On Fri, Jan 13, 2012 at 02:51:23PM -0600, Adam Litke wrote:
> > > Qemu has changed the semantics of the "block_job_cancel" API. Originally, the
> > > operation was synchronous (ie. upon command completion, the operation was
> > > guaranteed to be completely stopped). With the new semantics, a
> > > "block_job_cancel" merely requests that the operation be cancelled and an event
> > > is triggered once the cancellation request has been honored.
> > >
> > > To adopt the new semantics while preserving compatibility I propose the
> > > following updates to the virDomainBlockJob API:
> > >
> > > A new block job event type VIR_DOMAIN_BLOCK_JOB_CANCELLED will be recognized by
> > > libvirt. Regardless of the flags used with virDomainBlockJobAbort, this event
> > > will be raised whenever it is received from qemu. This event indicates that a
> > > block job has been successfully cancelled.
> > >
> > > A new extension flag VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC will be added to the
> > > virDomainBlockJobAbort API. When enabled, this function will operate
> > > asynchronously (ie, it can return before the job has actually been cancelled).
> > > When the API is used in this mode, it is the responsibility of the caller to
> > > wait for a VIR_DOMAIN_BLOCK_JOB_CANCELLED event or poll via the
> > > virDomainGetBlockJobInfo API to check the cancellation status.
> > >
> > > Without the VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC flag, libvirt will internally poll
> > > using qemu's "query-block-jobs" API and will not return until the operation has
> > > been completed. API users are advised that this operation is unbounded and
> > > further interaction with the domain during this period may block.
> > >
> > > This patch implements the new event type, the API flag, and the polling. The
> > > main outstanding issue is whether we should bound the amount of time we will
> > > wait for cancellation and return an error.
> > >
> > > Comments on this proposal?
> >
> > Hi Adam,
> >
> > Thanks for the patch. That approach seems reasonable. Re: bounding
> > the amount of time we wait for cancellation, if the time isn't
> > bounded, what happens if the cancellation never happens? IMO the most
> > important thing is to make sure that the caller has a way to return to
> > a normally functioning state even if the virDomainBlockJobAbort call
> > is unable to cancel the job.
>
> Yes, agreed. But this brings up the familiar problem with timeouts: selecting
> the right amount of time to wait. I don't have a good answer here, but it's not
> really all that bad if we bail out too early. In that case we can just return
> VIR_ERR_OPERATION_TIMEOUT to the caller and they can retry the operation again.
> Unfortunately, these semantics were not present in the initial API. Is adding a
> new error condition (timeout) to an existing API allowed?
Yes, a new error return value is fine for existing API.
> > I'll defer to the other folks on the code, since I am at this point
> > likely to do more harm than good. :)
> >
> > Dave
> >
> >
> > > Signed-off-by: Adam Litke <agl@xxxxxxxxxx>
> > > Cc: Stefan Hajnoczi <stefanha@xxxxxxxxx>
> > > Cc: Eric Blake <eblake@xxxxxxxxxx>
> > > ---
> > > include/libvirt/libvirt.h.in | 10 ++++++++++
> > > src/libvirt.c | 9 ++++++++-
> > > src/qemu/qemu_driver.c | 24 +++++++++++++++++-------
> > > src/qemu/qemu_monitor.c | 24 ++++++++++++++++++++++++
> > > src/qemu/qemu_monitor.h | 2 ++
> > > src/qemu/qemu_monitor_json.c | 36 +++++++++++++++++++++++++++++-------
> > > 6 files changed, 90 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
> > > index e436f3c..fc7f028 100644
> > > --- a/include/libvirt/libvirt.h.in
> > > +++ b/include/libvirt/libvirt.h.in
> > > @@ -1776,6 +1776,15 @@ typedef enum {
> > > VIR_DOMAIN_BLOCK_JOB_TYPE_PULL = 1,
> > > } virDomainBlockJobType;
> > >
> > > +/**
> > > + * virDomainBlockJobAbortFlags:
> > > + *
> > > + * VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC: Request only, do not wait for completion
> > > + */
> > > +typedef enum {
> > > + VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC = 1,
> > > +} virDomainBlockJobAbortFlags;
> > > +
> > > /* An iterator for monitoring block job operations */
> > > typedef unsigned long long virDomainBlockJobCursor;
> > >
> > > @@ -3293,6 +3302,7 @@ typedef void (*virConnectDomainEventGraphicsCallback)(virConnectPtr conn,
> > > typedef enum {
> > > VIR_DOMAIN_BLOCK_JOB_COMPLETED = 0,
> > > VIR_DOMAIN_BLOCK_JOB_FAILED = 1,
> > > + VIR_DOMAIN_BLOCK_JOB_CANCELLED = 2,
> > > } virConnectDomainEventBlockJobStatus;
> > >
> > > /**
> > > diff --git a/src/libvirt.c b/src/libvirt.c
> > > index a540424..0e886f5 100644
> > > --- a/src/libvirt.c
> > > +++ b/src/libvirt.c
> > > @@ -17299,7 +17299,7 @@ error:
> > > * virDomainBlockJobAbort:
> > > * @dom: pointer to domain object
> > > * @disk: path to the block device, or device shorthand
> > > - * @flags: extra flags; not used yet, so callers should always pass 0
> > > + * @flags: bitwise or of virDomainBlockJobAbortFlags
> > > *
> > > * Cancel the active block job on the given disk.
> > > *
> > > @@ -17310,6 +17310,13 @@ error:
> > > * can be found by calling virDomainGetXMLDesc() and inspecting
> > > * elements within //domain/devices/disk.
> > > *
> > > + * By default, this function performs a synchronous operation and the caller
> > > + * may assume that the operation has completed when 0 is returned. However,
> > > + * BlockJob operations may take a long time to complete, and during this time
> > > + * further domain interactions may be unresponsive. To avoid this problem, pass
> > > + * VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC in the flags argument to enable asynchronous
> > > + * behavior. When the job has been cancelled, a BlockJob event will be emitted.
> > > + *
> > > * Returns -1 in case of failure, 0 when successful.
> > > */
> > > int virDomainBlockJobAbort(virDomainPtr dom, const char *disk,
> > > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> > > index 712f1fc..d971a5f 100644
> > > --- a/src/qemu/qemu_driver.c
> > > +++ b/src/qemu/qemu_driver.c
> > > @@ -11379,7 +11379,7 @@ cleanup:
> > > static int
> > > qemuDomainBlockJobImpl(virDomainPtr dom, const char *path,
> > > unsigned long bandwidth, virDomainBlockJobInfoPtr info,
> > > - int mode)
> > > + int flags, int mode)
> > > {
> > > struct qemud_driver *driver = dom->conn->privateData;
> > > virDomainObjPtr vm = NULL;
> > > @@ -11420,6 +11420,15 @@ qemuDomainBlockJobImpl(virDomainPtr dom, const char *path,
> > > qemuDomainObjEnterMonitorWithDriver(driver, vm);
> > > priv = vm->privateData;
> > > ret = qemuMonitorBlockJob(priv->mon, device, bandwidth, info, mode);
> > > + /*
> > > + * Qemu provides asynchronous block job cancellation but without the
> > > + * VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC flag libvirt guarantees a synchronous
> > > + * operation. Provide this behavior by waiting here (with the monitor
> > > + * locked) so we don't get confused by newly scheduled block jobs.
> > > + */
> > > + if (ret == 0 && mode == BLOCK_JOB_ABORT &&
> > > + !(flags & VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC))
> > > + ret = qemuMonitorBlockJobCancelWait(priv->mon, device);
> > > qemuDomainObjExitMonitorWithDriver(driver, vm);
> > >
> > > endjob:
> > > @@ -11439,8 +11448,7 @@ cleanup:
> > > static int
> > > qemuDomainBlockJobAbort(virDomainPtr dom, const char *path, unsigned int flags)
> > > {
> > > - virCheckFlags(0, -1);
> > > - return qemuDomainBlockJobImpl(dom, path, 0, NULL, BLOCK_JOB_ABORT);
> > > + return qemuDomainBlockJobImpl(dom, path, 0, NULL, flags, BLOCK_JOB_ABORT);
> > > }
> > >
> > > static int
> > > @@ -11448,7 +11456,7 @@ qemuDomainGetBlockJobInfo(virDomainPtr dom, const char *path,
> > > virDomainBlockJobInfoPtr info, unsigned int flags)
> > > {
> > > virCheckFlags(0, -1);
> > > - return qemuDomainBlockJobImpl(dom, path, 0, info, BLOCK_JOB_INFO);
> > > + return qemuDomainBlockJobImpl(dom, path, 0, info, flags, BLOCK_JOB_INFO);
> > > }
> > >
> > > static int
> > > @@ -11456,7 +11464,8 @@ qemuDomainBlockJobSetSpeed(virDomainPtr dom, const char *path,
> > > unsigned long bandwidth, unsigned int flags)
> > > {
> > > virCheckFlags(0, -1);
> > > - return qemuDomainBlockJobImpl(dom, path, bandwidth, NULL, BLOCK_JOB_SPEED);
> > > + return qemuDomainBlockJobImpl(dom, path, bandwidth, NULL, flags,
> > > + BLOCK_JOB_SPEED);
> > > }
> > >
> > > static int
> > > @@ -11466,9 +11475,10 @@ qemuDomainBlockPull(virDomainPtr dom, const char *path, unsigned long bandwidth,
> > > int ret;
> > >
> > > virCheckFlags(0, -1);
> > > - ret = qemuDomainBlockJobImpl(dom, path, bandwidth, NULL, BLOCK_JOB_PULL);
> > > + ret = qemuDomainBlockJobImpl(dom, path, bandwidth, NULL, flags,
> > > + BLOCK_JOB_PULL);
> > > if (ret == 0 && bandwidth != 0)
> > > - ret = qemuDomainBlockJobImpl(dom, path, bandwidth, NULL,
> > > + ret = qemuDomainBlockJobImpl(dom, path, bandwidth, NULL, flags,
> > > BLOCK_JOB_SPEED);
> > > return ret;
> > > }
> > > diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> > > index ad7e2a5..5c22486 100644
> > > --- a/src/qemu/qemu_monitor.c
> > > +++ b/src/qemu/qemu_monitor.c
> > > @@ -2585,6 +2585,30 @@ int qemuMonitorScreendump(qemuMonitorPtr mon,
> > > return ret;
> > > }
> > >
> > > +/* Poll the monitor to wait for the block job on a given disk to end.
> > > + * We don't need to worry about another block job starting since we have the
> > > + * driver locked. */
> > > +int
> > > +qemuMonitorBlockJobCancelWait(qemuMonitorPtr mon, const char *device)
> > > +{
> > > + VIR_DEBUG("mon=%p, device=%p", mon, device);
> > > + /* XXX: Should we provide some sort of escape hatch for this wait? */
> > > + while (1) {
> > > + /* Poll every 100ms */
> > > + int ret;
> > > + struct timespec ts = { .tv_sec = 0, .tv_nsec = 100 * 1000 * 1000ull };
> > > + virDomainBlockJobInfo info;
> > > +
> > > + ret = qemuMonitorBlockJob(mon, device, 0, &info, BLOCK_JOB_INFO);
> > > + if (ret < 0)
> > > + return -1;
> > > + else if (ret == 0)
> > > + return 0;
> > > + else
> > > + nanosleep(&ts, NULL);
> > > + }
> > > +}
> > > +
> > > int qemuMonitorBlockJob(qemuMonitorPtr mon,
> > > const char *device,
> > > unsigned long bandwidth,
> > > diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
> > > index 15acf8b..afc081e 100644
> > > --- a/src/qemu/qemu_monitor.h
> > > +++ b/src/qemu/qemu_monitor.h
> > > @@ -510,6 +510,8 @@ typedef enum {
> > > BLOCK_JOB_PULL = 3,
> > > } BLOCK_JOB_CMD;
> > >
> > > +int qemuMonitorBlockJobCancelWait(qemuMonitorPtr mon, const char *device);
> > > +
> > > int qemuMonitorBlockJob(qemuMonitorPtr mon,
> > > const char *device,
> > > unsigned long bandwidth,
> > > diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> > > index 7eb2a92..2ca8eeb 100644
> > > --- a/src/qemu/qemu_monitor_json.c
> > > +++ b/src/qemu/qemu_monitor_json.c
> > > @@ -57,7 +57,8 @@ static void qemuMonitorJSONHandleIOError(qemuMonitorPtr mon, virJSONValuePtr dat
> > > static void qemuMonitorJSONHandleVNCConnect(qemuMonitorPtr mon, virJSONValuePtr data);
> > > static void qemuMonitorJSONHandleVNCInitialize(qemuMonitorPtr mon, virJSONValuePtr data);
> > > static void qemuMonitorJSONHandleVNCDisconnect(qemuMonitorPtr mon, virJSONValuePtr data);
> > > -static void qemuMonitorJSONHandleBlockJob(qemuMonitorPtr mon, virJSONValuePtr data);
> > > +static void qemuMonitorJSONHandleBlockJobCompleted(qemuMonitorPtr mon, virJSONValuePtr data);
> > > +static void qemuMonitorJSONHandleBlockJobCancelled(qemuMonitorPtr mon, virJSONValuePtr data);
> > >
> > > struct {
> > > const char *type;
> > > @@ -73,7 +74,8 @@ struct {
> > > { "VNC_CONNECTED", qemuMonitorJSONHandleVNCConnect, },
> > > { "VNC_INITIALIZED", qemuMonitorJSONHandleVNCInitialize, },
> > > { "VNC_DISCONNECTED", qemuMonitorJSONHandleVNCDisconnect, },
> > > - { "BLOCK_JOB_COMPLETED", qemuMonitorJSONHandleBlockJob, },
> > > + { "BLOCK_JOB_COMPLETED", qemuMonitorJSONHandleBlockJobCompleted, },
> > > + { "BLOCK_JOB_CANCELLED", qemuMonitorJSONHandleBlockJobCancelled, },
> > > };
> > >
> > >
> > > @@ -685,13 +687,14 @@ static void qemuMonitorJSONHandleVNCDisconnect(qemuMonitorPtr mon, virJSONValueP
> > > qemuMonitorJSONHandleVNC(mon, data, VIR_DOMAIN_EVENT_GRAPHICS_DISCONNECT);
> > > }
> > >
> > > -static void qemuMonitorJSONHandleBlockJob(qemuMonitorPtr mon, virJSONValuePtr data)
> > > +static void qemuMonitorJSONHandleBlockJobImpl(qemuMonitorPtr mon,
> > > + virJSONValuePtr data,
> > > + int event)
> > > {
> > > const char *device;
> > > const char *type_str;
> > > int type = VIR_DOMAIN_BLOCK_JOB_TYPE_UNKNOWN;
> > > unsigned long long offset, len;
> > > - int status = VIR_DOMAIN_BLOCK_JOB_FAILED;
> > >
> > > if ((device = virJSONValueObjectGetString(data, "device")) == NULL) {
> > > VIR_WARN("missing device in block job event");
> > > @@ -716,13 +719,32 @@ static void qemuMonitorJSONHandleBlockJob(qemuMonitorPtr mon, virJSONValuePtr da
> > > if (STREQ(type_str, "stream"))
> > > type = VIR_DOMAIN_BLOCK_JOB_TYPE_PULL;
> > >
> > > - if (offset != 0 && offset == len)
> > > - status = VIR_DOMAIN_BLOCK_JOB_COMPLETED;
> > > + switch (event) {
> > > + case VIR_DOMAIN_BLOCK_JOB_COMPLETED:
> > > + /* Make sure the whole device has been processed */
> > > + if (offset != len)
> > > + event = VIR_DOMAIN_BLOCK_JOB_FAILED;
> > > + break;
> > > + case VIR_DOMAIN_BLOCK_JOB_FAILED:
> > > + case VIR_DOMAIN_BLOCK_JOB_CANCELLED:
> > > + break;
> > > + }
> > >
> > > out:
> > > - qemuMonitorEmitBlockJob(mon, device, type, status);
> > > + qemuMonitorEmitBlockJob(mon, device, type, event);
> > > +}
> > > +
> > > +static void qemuMonitorJSONHandleBlockJobCompleted(qemuMonitorPtr mon,
> > > + virJSONValuePtr data)
> > > +{
> > > + qemuMonitorJSONHandleBlockJobImpl(mon, data, VIR_DOMAIN_BLOCK_JOB_COMPLETED);
> > > }
> > >
> > > +static void qemuMonitorJSONHandleBlockJobCancelled(qemuMonitorPtr mon,
> > > + virJSONValuePtr data)
> > > +{
> > > + qemuMonitorJSONHandleBlockJobImpl(mon, data, VIR_DOMAIN_BLOCK_JOB_CANCELLED);
> > > +}
> > >
> > > int
> > > qemuMonitorJSONHumanCommandWithFd(qemuMonitorPtr mon,
> > > --
> > > 1.7.5.rc1
> > >
> >
>
> --
> Adam Litke <agl@xxxxxxxxxx>
> IBM Linux Technology Center
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list