[Qemu-devel] virDomainBlockJobAbort and block_job_cancel

[Qemu-devel] virDomainBlockJobAbort and block_job_cancel

[Stefan Hajnoczi]

Block job cancellation waits until the job has been cancelled before
returning.  This allows clients to know that the operation has been
cancelled successfully.  Unfortunately, these semantics are not really
possible with today's QEMU and libvirt code.

A command that waits for block I/O completion may wait for many
minutes.  During this time the monitor is unavailable.  While the QMP
protocol may in theory support multiple in-flight commands, both QEMU
and libvirt's implemenations are geared towards one command at a time.
 So in practice a hung cancellation command would make the monitor
unavailable - we need to avoid this.

This means block_job_cancel cannot wait until the job is cancelled or
it risks hanging the monitor if there is a block I/O timeout.  We need
a solution that reflects this in QEMU and libvirt, here is what I
propose:

block_job_cancel returns immediately upon marking the job cancelled.
The job may still be finishing block I/O but will cancel itself at
some point in the future.  When the job actually completes it raises
the new BLOCK_JOB_CANCELLED event.

This means that virDomainBlockJobAbort() returns to the client without
a guarantee that the job has completed.  If the client enumerates jobs
it may still see a job that has not finished cancelling.  The client
must register a handler for the BLOCK_JOB_CANCELLED event if it wants
to know when the job really goes away.  The BLOCK_JOB_CANCELLED event
has the same fields as the BLOCK_JOB_COMPLETED event, except it lacks
the optional "error" message field.

The impact on clients is that they need to add a BLOCK_JOB_CANCELLED
handler if they really want to wait.  Most clients today (not many
exist) will be fine without waiting for cancellation.

Any objections or thoughts on this?

Stefan

Re: [Qemu-devel] [libvirt] virDomainBlockJobAbort and block_job_cancel

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1751 bytes --]

On 11/23/2011 07:48 AM, Stefan Hajnoczi wrote:
> This means that virDomainBlockJobAbort() returns to the client without
> a guarantee that the job has completed.  If the client enumerates jobs
> it may still see a job that has not finished cancelling.  The client
> must register a handler for the BLOCK_JOB_CANCELLED event if it wants
> to know when the job really goes away.  The BLOCK_JOB_CANCELLED event
> has the same fields as the BLOCK_JOB_COMPLETED event, except it lacks
> the optional "error" message field.
> 
> The impact on clients is that they need to add a BLOCK_JOB_CANCELLED
> handler if they really want to wait.  Most clients today (not many
> exist) will be fine without waiting for cancellation.
> 
> Any objections or thoughts on this?

virDomainBlockJobAbort() thankfully has an 'unsigned int flags'
argument.  For backwards-compatibility, I suggest we use it:

calling virDomainBlockJobAbort(,0) maintains old blocking behavior, and
we document that blocking until things abort may render the rest of
interactions with the domain unresponsive.

The new virDomainBlockJobAbort(,VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC) would
then implement your new proposed semantics of returning immediately once
the cancellation has been requested, even if it hasn't been acted on yet.

Maybe you could convince me to swap the flags: have 0 change semantics
to non-blocking, and a new flag to request blocking, where callers that
care have to try the flag, and if the flag is unsupported then they know
they are talking to older libvirtd where the behavior is blocking by
default, but that's a bit riskier.

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

Re: [Qemu-devel] [libvirt] virDomainBlockJobAbort and block_job_cancel

[Daniel Veillard]

On Wed, Nov 23, 2011 at 09:04:50AM -0700, Eric Blake wrote:
> On 11/23/2011 07:48 AM, Stefan Hajnoczi wrote:
> > This means that virDomainBlockJobAbort() returns to the client without
> > a guarantee that the job has completed.  If the client enumerates jobs
> > it may still see a job that has not finished cancelling.  The client
> > must register a handler for the BLOCK_JOB_CANCELLED event if it wants
> > to know when the job really goes away.  The BLOCK_JOB_CANCELLED event
> > has the same fields as the BLOCK_JOB_COMPLETED event, except it lacks
> > the optional "error" message field.
> > 
> > The impact on clients is that they need to add a BLOCK_JOB_CANCELLED
> > handler if they really want to wait.  Most clients today (not many
> > exist) will be fine without waiting for cancellation.
> > 
> > Any objections or thoughts on this?
> 
> virDomainBlockJobAbort() thankfully has an 'unsigned int flags'
> argument.  For backwards-compatibility, I suggest we use it:
> 
> calling virDomainBlockJobAbort(,0) maintains old blocking behavior, and
> we document that blocking until things abort may render the rest of
> interactions with the domain unresponsive.
> 
> The new virDomainBlockJobAbort(,VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC) would
> then implement your new proposed semantics of returning immediately once
> the cancellation has been requested, even if it hasn't been acted on yet.
> 
> Maybe you could convince me to swap the flags: have 0 change semantics
> to non-blocking, and a new flag to request blocking, where callers that
> care have to try the flag, and if the flag is unsupported then they know
> they are talking to older libvirtd where the behavior is blocking by
> default, but that's a bit riskier.

  Agreed, I would rather not change the current call semantic,
but an ASYNC flag would be a really good addition. We can document
the risk of not using it in the function description and suggest
new applications use ASYNC flag.

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel@veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/

Re: [Qemu-devel] [libvirt] virDomainBlockJobAbort and block_job_cancel

[Stefan Hajnoczi]

On Thu, Nov 24, 2011 at 5:31 AM, Daniel Veillard <veillard@redhat.com> wrote:
> On Wed, Nov 23, 2011 at 09:04:50AM -0700, Eric Blake wrote:
>> On 11/23/2011 07:48 AM, Stefan Hajnoczi wrote:
>> > This means that virDomainBlockJobAbort() returns to the client without
>> > a guarantee that the job has completed.  If the client enumerates jobs
>> > it may still see a job that has not finished cancelling.  The client
>> > must register a handler for the BLOCK_JOB_CANCELLED event if it wants
>> > to know when the job really goes away.  The BLOCK_JOB_CANCELLED event
>> > has the same fields as the BLOCK_JOB_COMPLETED event, except it lacks
>> > the optional "error" message field.
>> >
>> > The impact on clients is that they need to add a BLOCK_JOB_CANCELLED
>> > handler if they really want to wait.  Most clients today (not many
>> > exist) will be fine without waiting for cancellation.
>> >
>> > Any objections or thoughts on this?
>>
>> virDomainBlockJobAbort() thankfully has an 'unsigned int flags'
>> argument.  For backwards-compatibility, I suggest we use it:
>>
>> calling virDomainBlockJobAbort(,0) maintains old blocking behavior, and
>> we document that blocking until things abort may render the rest of
>> interactions with the domain unresponsive.
>>
>> The new virDomainBlockJobAbort(,VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC) would
>> then implement your new proposed semantics of returning immediately once
>> the cancellation has been requested, even if it hasn't been acted on yet.
>>
>> Maybe you could convince me to swap the flags: have 0 change semantics
>> to non-blocking, and a new flag to request blocking, where callers that
>> care have to try the flag, and if the flag is unsupported then they know
>> they are talking to older libvirtd where the behavior is blocking by
>> default, but that's a bit riskier.
>
>  Agreed, I would rather not change the current call semantic,
> but an ASYNC flag would be a really good addition. We can document
> the risk of not using it in the function description and suggest
> new applications use ASYNC flag.

Yep, that's a nice suggestion and solves the problem.

Stefan

Re: [Qemu-devel] [libvirt] virDomainBlockJobAbort and block_job_cancel

[Adam Litke]

On Thu, Nov 24, 2011 at 09:21:42AM +0000, Stefan Hajnoczi wrote:
> On Thu, Nov 24, 2011 at 5:31 AM, Daniel Veillard <veillard@redhat.com> wrote:
> > On Wed, Nov 23, 2011 at 09:04:50AM -0700, Eric Blake wrote:
> >> On 11/23/2011 07:48 AM, Stefan Hajnoczi wrote:
> >> > This means that virDomainBlockJobAbort() returns to the client without
> >> > a guarantee that the job has completed.  If the client enumerates jobs
> >> > it may still see a job that has not finished cancelling.  The client
> >> > must register a handler for the BLOCK_JOB_CANCELLED event if it wants
> >> > to know when the job really goes away.  The BLOCK_JOB_CANCELLED event
> >> > has the same fields as the BLOCK_JOB_COMPLETED event, except it lacks
> >> > the optional "error" message field.
> >> >
> >> > The impact on clients is that they need to add a BLOCK_JOB_CANCELLED
> >> > handler if they really want to wait.  Most clients today (not many
> >> > exist) will be fine without waiting for cancellation.
> >> >
> >> > Any objections or thoughts on this?
> >>
> >> virDomainBlockJobAbort() thankfully has an 'unsigned int flags'
> >> argument.  For backwards-compatibility, I suggest we use it:
> >>
> >> calling virDomainBlockJobAbort(,0) maintains old blocking behavior, and
> >> we document that blocking until things abort may render the rest of
> >> interactions with the domain unresponsive.
> >>
> >> The new virDomainBlockJobAbort(,VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC) would
> >> then implement your new proposed semantics of returning immediately once
> >> the cancellation has been requested, even if it hasn't been acted on yet.
> >>
> >> Maybe you could convince me to swap the flags: have 0 change semantics
> >> to non-blocking, and a new flag to request blocking, where callers that
> >> care have to try the flag, and if the flag is unsupported then they know
> >> they are talking to older libvirtd where the behavior is blocking by
> >> default, but that's a bit riskier.
> >
> >  Agreed, I would rather not change the current call semantic,
> > but an ASYNC flag would be a really good addition. We can document
> > the risk of not using it in the function description and suggest
> > new applications use ASYNC flag.
> 
> Yep, that's a nice suggestion and solves the problem.

I am almost ready to post the code that makes the above change, but before I do,
I need to ask a question about implementing synchronous behavior.

Stefan's qemu tree has a block_job_cancel command that always acts
asynchronously.  In order to provide the synchronous behavior in libvirt (when
flags is 0), I need to wait for the block job to go away.  I see two options:

1) Use the event:
To implement this I would create an internal event callback function and
register it to receive the block job events.  When the cancel event comes in, I
can exit the qemu job context.  One problem I see with this is that events are
only available when using the json monitor mode.

2) Poll the qemu monitor
To do it this way, I would write a function that repeatedly calls
virDomainGetBlockJobInfo() against the disk in question.  Once the job
disappears from the list I can return with confidence that the job is gone.
This is obviously sub-optimal because I need to poll and sleep.

3) Ask Stefan to provide a synchronous mode for the qemu monitor command
This one is the nicest from a libvirt perspective, but I doubt qemu wants to add
such an interface given that it basically has broken semantics as far as qemu is
concerned.

If this is all too nasty, we could probably just change the behavior of
blockJobAbort and make it always synchronous with a 'cancelled' event.

Thoughts?

-- 
Adam Litke <agl@us.ibm.com>
IBM Linux Technology Center

Re: [Qemu-devel] [libvirt] virDomainBlockJobAbort and block_job_cancel

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 3083 bytes --]

On 12/07/2011 03:35 PM, Adam Litke wrote:
> Stefan's qemu tree has a block_job_cancel command that always acts
> asynchronously.  In order to provide the synchronous behavior in libvirt (when
> flags is 0), I need to wait for the block job to go away.  I see two options:
> 
> 1) Use the event:
> To implement this I would create an internal event callback function and
> register it to receive the block job events.  When the cancel event comes in, I
> can exit the qemu job context.  One problem I see with this is that events are
> only available when using the json monitor mode.

I like this idea.  We have internally handled events before, and limited
it to just JSON if that made life easier: for example, virDomainReboot
on qemu is rejected if you only have the HMP monitor, and if you have
the JSON monitor, the implementation internally handles the event to
change the domain state.

Can we reliably detect whether qemu is new enough to provide the event,
and if qemu was older and did not provide the event, do we reliably know
that abort was blocking in that case?

It's okay to make things work that used to fail, but it is a regression
to make blocking job cancel fail where it used to work, so rejecting
blocking job cancel with HMP monitor is not a good idea.  If we can
guarantee that all qemu new enough to have async cancel also support the
event, while older qemu was always blocking, and that we can always use
the JSON monitor to newer qemu, then we're set - merely ensure that we
use only the JSON monitor and the event.  But if we can't make the
guarantees, and insist on supporting newer qemu via HMP monitor, then we
may need a hybrid combination of options 1 and 2, or for less code
maintenance, just option 2.

> 
> 2) Poll the qemu monitor
> To do it this way, I would write a function that repeatedly calls
> virDomainGetBlockJobInfo() against the disk in question.  Once the job
> disappears from the list I can return with confidence that the job is gone.
> This is obviously sub-optimal because I need to poll and sleep.

We've done this before, for both HMP and JSON - see
qemuMigrationWaitForCompletion.  I agree that an event is nicer than
polling, but we may be locked into this.

> 
> 3) Ask Stefan to provide a synchronous mode for the qemu monitor command
> This one is the nicest from a libvirt perspective, but I doubt qemu wants to add
> such an interface given that it basically has broken semantics as far as qemu is
> concerned.

Or even:

4) Ask Stefan to make the HMP monitor command synchronous, but only
expose the JSON command as asynchronous.  After all, it is only HMP
where we can't wait for an event.

> 
> If this is all too nasty, we could probably just change the behavior of
> blockJobAbort and make it always synchronous with a 'cancelled' event.

No, we can't change the behavior without breaking back-compat of
existing clients of job cancellation.

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

Re: [Qemu-devel] [libvirt] virDomainBlockJobAbort and block_job_cancel

[Stefan Hajnoczi]

On Wed, Dec 7, 2011 at 11:01 PM, Eric Blake <eblake@redhat.com> wrote:
> On 12/07/2011 03:35 PM, Adam Litke wrote:
> 4) Ask Stefan to make the HMP monitor command synchronous, but only
> expose the JSON command as asynchronous.  After all, it is only HMP
> where we can't wait for an event.

QEMU cannot do async commands, even for HMP.  My QEMU patches used to
work because there is a broken async flag for monitor commands.  I
didn't know it was broken at the time O:-).

I have CCed Luiz who has been working on moving commands off the old
MONITOR_CMD_ASYNC flag.  My current understanding is that patches
adding use of MONITOR_CMD_ASYNC will not be accepted.

>> 2) Poll the qemu monitor
>> To do it this way, I would write a function that repeatedly calls
>> virDomainGetBlockJobInfo() against the disk in question.  Once the job
>> disappears from the list I can return with confidence that the job is gone.
>> This is obviously sub-optimal because I need to poll and sleep.
>
> We've done this before, for both HMP and JSON - see
> qemuMigrationWaitForCompletion.  I agree that an event is nicer than
> polling, but we may be locked into this.

This seems like the safest option although it's ugly.

Stefan

Re: [Qemu-devel] [libvirt] virDomainBlockJobAbort and block_job_cancel

[Adam Litke]

On Wed, Dec 07, 2011 at 04:01:58PM -0700, Eric Blake wrote:
> On 12/07/2011 03:35 PM, Adam Litke wrote:
> > Stefan's qemu tree has a block_job_cancel command that always acts
> > asynchronously.  In order to provide the synchronous behavior in libvirt (when
> > flags is 0), I need to wait for the block job to go away.  I see two options:
> > 
> > 1) Use the event:
> > To implement this I would create an internal event callback function and
> > register it to receive the block job events.  When the cancel event comes in, I
> > can exit the qemu job context.  One problem I see with this is that events are
> > only available when using the json monitor mode.
> 
> I like this idea.  We have internally handled events before, and limited
> it to just JSON if that made life easier: for example, virDomainReboot
> on qemu is rejected if you only have the HMP monitor, and if you have
> the JSON monitor, the implementation internally handles the event to
> change the domain state.
> 
> Can we reliably detect whether qemu is new enough to provide the event,
> and if qemu was older and did not provide the event, do we reliably know
> that abort was blocking in that case?

I think we can say that qemu will operate in one of two modes:
a) Blocking abort AND event is not emitted
b) Non-blocking abort AND event is emitted

The difficulty is in detecting which case the current qemu supports.  I don't
believe there is a way to query qemu for a list of currently-supported events.
Therefore, we'd have to use version numbers.  If we do this, how do we avoid
breaking users of qemu git versions that fall between official qemu releases?

> It's okay to make things work that used to fail, but it is a regression
> to make blocking job cancel fail where it used to work, so rejecting
> blocking job cancel with HMP monitor is not a good idea.  If we can
> guarantee that all qemu new enough to have async cancel also support the
> event, while older qemu was always blocking, and that we can always use
> the JSON monitor to newer qemu, then we're set - merely ensure that we
> use only the JSON monitor and the event.  But if we can't make the
> guarantees, and insist on supporting newer qemu via HMP monitor, then we
> may need a hybrid combination of options 1 and 2, or for less code
> maintenance, just option 2.

Is there a deprecation plan for HMP with newer qemu versions?  I really hate the
idea of needing two implementations for this: one polling and one event-based.

> > 2) Poll the qemu monitor
> > To do it this way, I would write a function that repeatedly calls
> > virDomainGetBlockJobInfo() against the disk in question.  Once the job
> > disappears from the list I can return with confidence that the job is gone.
> > This is obviously sub-optimal because I need to poll and sleep.
> 
> We've done this before, for both HMP and JSON - see
> qemuMigrationWaitForCompletion.  I agree that an event is nicer than
> polling, but we may be locked into this.
> 
> > 
> > 3) Ask Stefan to provide a synchronous mode for the qemu monitor command
> > This one is the nicest from a libvirt perspective, but I doubt qemu wants to add
> > such an interface given that it basically has broken semantics as far as qemu is
> > concerned.
> 
> Or even:
> 
> 4) Ask Stefan to make the HMP monitor command synchronous, but only
> expose the JSON command as asynchronous.  After all, it is only HMP
> where we can't wait for an event.

Stefan, how 'bout it?

> > 
> > If this is all too nasty, we could probably just change the behavior of
> > blockJobAbort and make it always synchronous with a 'cancelled' event.
> 
> No, we can't change the behavior without breaking back-compat of
> existing clients of job cancellation.

-- 
Adam Litke <agl@us.ibm.com>
IBM Linux Technology Center

Re: [Qemu-devel] [libvirt] virDomainBlockJobAbort and block_job_cancel

[Stefan Hajnoczi]

On Thu, Dec 8, 2011 at 2:55 PM, Adam Litke <agl@us.ibm.com> wrote:
> On Wed, Dec 07, 2011 at 04:01:58PM -0700, Eric Blake wrote:
>> On 12/07/2011 03:35 PM, Adam Litke wrote:
>> > Stefan's qemu tree has a block_job_cancel command that always acts
>> > asynchronously.  In order to provide the synchronous behavior in libvirt (when
>> > flags is 0), I need to wait for the block job to go away.  I see two options:
>> >
>> > 1) Use the event:
>> > To implement this I would create an internal event callback function and
>> > register it to receive the block job events.  When the cancel event comes in, I
>> > can exit the qemu job context.  One problem I see with this is that events are
>> > only available when using the json monitor mode.
>>
>> I like this idea.  We have internally handled events before, and limited
>> it to just JSON if that made life easier: for example, virDomainReboot
>> on qemu is rejected if you only have the HMP monitor, and if you have
>> the JSON monitor, the implementation internally handles the event to
>> change the domain state.
>>
>> Can we reliably detect whether qemu is new enough to provide the event,
>> and if qemu was older and did not provide the event, do we reliably know
>> that abort was blocking in that case?
>
> I think we can say that qemu will operate in one of two modes:
> a) Blocking abort AND event is not emitted
> b) Non-blocking abort AND event is emitted
>
> The difficulty is in detecting which case the current qemu supports.  I don't
> believe there is a way to query qemu for a list of currently-supported events.
> Therefore, we'd have to use version numbers.  If we do this, how do we avoid
> breaking users of qemu git versions that fall between official qemu releases?

I agree.  Checking version numbers is always problematic - distros may
backport features.

This isn't pretty but how about:

1. Issue block_job_cancel
2. Issue query-block-jobs and check if the job is there.
3. Check for QMP events (if applicable).
4. If the block job was visible then we must be async and need to
expect an event if we didn't already get one.
5. If the block job was not visible then it has been stopped.

Stefan