Re: [Qemu-devel] [RFC 0/2]: qemu-ga: drop automatic reaper

[Michal Privoznik]

On 19.04.2012 21:34, Luiz Capitulino wrote:
> On Thu, 19 Apr 2012 13:57:48 -0500
> Michael Roth <address@hidden> wrote:
> 
>> On Thu, Apr 19, 2012 at 02:36:53PM -0300, Luiz Capitulino wrote:
>>> Michael,
>>>
>>> I'm going to revive this topic one more time. I was working on v2 of my 
>>> fixes
>>> to the suspend race bugs and really thought that the real problem is that 
>>> the
>>> code shouldn't be that complex.
>>>
>>> Basically, this series drops the automatic reaper and adds waitpid() calls &
>>> related logic to the suspend and shutdown functions.
>>>
>>> My objective with this RFC is to understand why this can't be done.
>>
>> Hi Luiz,
>>
>> CC'ing Michael as some of these suggestions might affect his work on
>> libvirt integration.
> 
> I forgot to CC him and Eric.
> 
>> Thanks for taking the time to implement what this would look like
>> this. I think this is a sensible approach in general, the main issue
>> for me is the specific commands currently impacted by such a change:
>> suspend and shutdown.
>>
>> While we do document that these commands may not return a response,
>> the new behavior actually introduces a pathological assurance that
>> they *won't* return a response:
>> shutdown/pm-suspend/echo mem >/sys/power/state never return, or don't
>> return till the guest wakes up.
> 
> Yes, but that can be viewed as an implementation detail because this is
> also possible with the current code (ie. if the suspend process manages
> to suspend the guest before qemu-ga is able to emit a response).
> 
>> So *every* suspend/shutdown command will result in a timeout or
>> indefinite hang to the user. Honestly I just find the behavior
>> completely unexpected/unintuitive and would prefer to reduce it to
>> being a corner case. As the code stands currently it's actually
>> extremely rare that a response won't be returned before the commands
>> are executed.
> 
> I don't think we can assume this is a corner case. It all depends on how
> the two processes are scheduled. On an idle VM it's more likely that qemu-ga
> will emit the response first because the suspend process does more I/O, but
> that really depends on the scheduler/system load. This is also true for the
> shutdown command.
> 
> As we have discussed with Anthony on irc months ago, the client can't expect
> a response from this command and should reset the connection before sending
> new commands when the VM resumes.
> 
> If this turns out to be a problem for libvirt, I'd say it's a libvirt bug.
> 
>> So that's why we're jumping through hoops atm. It's more a
>> philosophical reason than a technical one.
>>
>> But if we were to do things as you proposed and document things as
>> "this command will *only* show a response on error", I guess maybe I
>> can live with that... in the case of shutdown older clients will
>> start seeing timeouts where they previously expected a "nothing"
>> response. The nothing response never entailed success or failure so
>> this shouldn't break anything that wasn't already broken however...
> 
> Yes, and again, depending on the VM load it's possible that the VM vanishes
> before qemu-ga emits its response.
> 
>> But, I think if we tell users we'll *only* send response on error,
>> we should do our part to *not* send the responses, rather than relying
>> on them having implemented the reset mechanism to throw them away after
>> guest wake-up. What we could do is allow a command to set a flag, after
>> it reaps it's child (in the case of suspend this would be after
>> wake-up, for shutdown it'd basically be a no-op, but worth adding
>> for readability sake), to have qemu-ga not send a response. We'd
>> implement it similarly to how we did ga_set_response_delimited().
> 
> Fine with me. Stating that "do not wait for an OK response, because none
> will be sent" sounds clearer than "an OK response may, or may not be
> emitted. Or it may be emitted when the VM resumes".


Just to make this clear: this "report-only-error" behavior concerns only
guest-suspend-* and guest-shutdown commands, right? Because otherwise,
if we enable such behavior for all commands (e.g. fsfreeze) I think we
are entering the world of pain.

>From user POV there is a huge difference between those 2 sets of
commands (suspend/shutdown on one side, the others on the other side):
- the first emits an event on qemu monitor, so libvirt can catch that
and confirm suspend/shutdown has succeeded
- while the latter emits no event, it is impossible to determine command
successfulness. Simply, because it is hard to tell if command is in
execution phase (e.g. kernel is syncing disks) or command has already
finished (and FS is frozen already).

Therefore, I can live with making commands from suspend/shutdown family
only-error-reporting as long as I can check guest has been really
suspended/shut off.

Michal