On 22.08.2011 15:30, Eric Blake wrote: > On 08/22/2011 07:22 AM, Eric Blake wrote: >> On 08/22/2011 04:49 AM, Michal Privoznik wrote: >>> >>> I don't think that's problem. High priority calls must be guaranteed to >>> end in reasonably short time. And although we talk to monitor here, we >>> are guaranteed to end. Therefore no need to change my previous patch. >> >> No, we are not guaranteed to end in a reasonably short time. Monitor >> calls are, by nature, indefinite time commands, because they require >> response from an external process. >> >> Rather, we should add virDomainShutdownFlags() (which would be low >> priority, to match virDomainShutdown() being low priority), and make >> this a flag an option to virDomainShutdownFlags(). That is, you get a >> choice between shutdown via ACPI signal, or shutdown via monitor >> command, but both choices involve interaction with the monitor, and are >> therefore inherently liable to blocking (although shutdown via the >> monitor is much more likely to succeed in a short amount of time if the >> monitor is available, because it does not depend on the guest's reaction >> to ACPI). >> >>> >>> 'Accessing monitor' is a bit unfortunate formulation, because 99% calls >>> which do access monitor can block indefinitely. And this is the real >>> problem. Stuck API. It doesn't really matter if it is because of >>> monitor, NFS, etc. I just wanted to provide guide for developers if they >>> are in doubt whether to mark API as high or low priority. >> >> The point is that virDomainDestroy() can't make a monitor command, or it >> will get stuck. I don't think it makes sense to give >> virDomainDestroyFlags() a flag that it can only honor in cases where the >> monitor is not stuck, but which gets skipped otherwise. I think that if >> we support a flag, then the use of the monitor has to be unconditional >> with that flag, which in turn implies that we need >> virDomainShutdownFlags(). > > I wrote the above before even glancing at your patch. But now, looking > at the patch, it looks like you tried to take this into consideration - > you are trying to add condition variables to guarantee that the monitor > command will be attempted if possible, but that the command will give up > on an unresponsive monitor and resort to the more drastic process kill, > so that the command will always succeed even in the face of a stuck > monitor. > > Maybe your approach has merit after all. But in that case, I think we > need two ways to expose the monitor command: > > virDomainDestroyFlags(, VIR_DOMAIN_DESTROY_FLUSH_CACHE) - best effort > try to flush cache, but may still lose data because the destroy is > guaranteed to happen after x seconds even if monitor could not be obtained > > virDomainShutdownFlags(, VIR_DOMAIN_SHUTDOWN_FLUSH_CACHE) - guarantee > that the flush cache happens, or that the command fails due to a timeout > in obtaining the monitor > Agree. I've sent v2 for destroy (DV suggested some corrections). And for shutdown - I will sent follow up patch. -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list