1. Patchew
  2. Libvirt
  3. View series

[libvirt RFC PATCH] util: vireventglibwatch: watch for G_IO_HUP and G_IO_ERR

Ján Tomko posted 1 patch 4 years, 2 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/7f8f601f366643745879d72471217935ef339f39.1582072163.git.jtomko@redhat.com
src/util/vireventglibwatch.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
[libvirt RFC PATCH] util: vireventglibwatch: watch for G_IO_HUP and G_IO_ERR
Posted by Ján Tomko 4 years, 2 months ago 
To more closely match the previous usage in virEventPollDispatchHandles,
where called the handle callback for any revents returned by poll.

This should fix the virtlogd error on subsequent domain startup:
  error: can't connect to virtlogd: Cannot open log file:
  '/var/log/libvirt/qemu/f28live.log': Device or resource busy
as well as virtlogd spinning caused by virLogHandlerDomainLogFileEvent
never being called on hangup.

Signed-off-by: Ján Tomko <jtomko@redhat.com>
Fixes: f8ab47cb4491dd72d866c1a96a9d94b8c3341de9
Fixes: 946a25274c46ffff46323c62f567ae7e753aa921
---
 src/util/vireventglibwatch.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/util/vireventglibwatch.c b/src/util/vireventglibwatch.c
index 7694e74f23..178707f6b7 100644
--- a/src/util/vireventglibwatch.c
+++ b/src/util/vireventglibwatch.c
@@ -89,11 +89,11 @@ GSource *virEventGLibCreateSocketWatch(int fd,
                           sizeof(virEventGLibFDSource));
     ssource = (virEventGLibFDSource *)source;
 
-    ssource->condition = condition;
+    ssource->condition = condition | G_IO_HUP | G_IO_ERR;
     ssource->fd = fd;
 
     ssource->pollfd.fd = fd;
-    ssource->pollfd.events = condition;
+    ssource->pollfd.events = condition | G_IO_HUP | G_IO_ERR;
 
     g_source_add_poll(source, &ssource->pollfd);
 
-- 
2.24.1
Re: [libvirt RFC PATCH] util: vireventglibwatch: watch for G_IO_HUP and G_IO_ERR
Posted by Daniel P. Berrangé 4 years, 1 month ago 
On Wed, Feb 19, 2020 at 01:31:22AM +0100, Ján Tomko wrote:
> To more closely match the previous usage in virEventPollDispatchHandles,
> where called the handle callback for any revents returned by poll.
> 
> This should fix the virtlogd error on subsequent domain startup:
>   error: can't connect to virtlogd: Cannot open log file:
>   '/var/log/libvirt/qemu/f28live.log': Device or resource busy
> as well as virtlogd spinning caused by virLogHandlerDomainLogFileEvent
> never being called on hangup.
> 
> Signed-off-by: Ján Tomko <jtomko@redhat.com>
> Fixes: f8ab47cb4491dd72d866c1a96a9d94b8c3341de9
> Fixes: 946a25274c46ffff46323c62f567ae7e753aa921
> ---
>  src/util/vireventglibwatch.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

> diff --git a/src/util/vireventglibwatch.c b/src/util/vireventglibwatch.c
> index 7694e74f23..178707f6b7 100644
> --- a/src/util/vireventglibwatch.c
> +++ b/src/util/vireventglibwatch.c
> @@ -89,11 +89,11 @@ GSource *virEventGLibCreateSocketWatch(int fd,
>                            sizeof(virEventGLibFDSource));
>      ssource = (virEventGLibFDSource *)source;
>  
> -    ssource->condition = condition;
> +    ssource->condition = condition | G_IO_HUP | G_IO_ERR;
>      ssource->fd = fd;
>  
>      ssource->pollfd.fd = fd;
> -    ssource->pollfd.events = condition;
> +    ssource->pollfd.events = condition | G_IO_HUP | G_IO_ERR;
>  
>      g_source_add_poll(source, &ssource->pollfd);

This is something I knew about but forgot to fix beforef pushing the
original patches. At the OS level poll()  will always return HUP/ERR
events and we relied on this historically.  GLib uses poll() and so
will also see HUP/ERR events but unless you requested G_IO_ERR/HUP,
the callback will never be invoked to dispatch the event. This is
what leads to the 100% CPU burn behaviour.

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
Re: [libvirt RFC PATCH] util: vireventglibwatch: watch for G_IO_HUP and G_IO_ERR
Posted by Michal Privoznik 4 years, 2 months ago 
On 2/19/20 1:31 AM, Ján Tomko wrote:
> To more closely match the previous usage in virEventPollDispatchHandles,
> where called the handle callback for any revents returned by poll.
> 
> This should fix the virtlogd error on subsequent domain startup:
>    error: can't connect to virtlogd: Cannot open log file:
>    '/var/log/libvirt/qemu/f28live.log': Device or resource busy
> as well as virtlogd spinning caused by virLogHandlerDomainLogFileEvent
> never being called on hangup.
> 
> Signed-off-by: Ján Tomko <jtomko@redhat.com>
> Fixes: f8ab47cb4491dd72d866c1a96a9d94b8c3341de9
> Fixes: 946a25274c46ffff46323c62f567ae7e753aa921
> ---
>   src/util/vireventglibwatch.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/util/vireventglibwatch.c b/src/util/vireventglibwatch.c
> index 7694e74f23..178707f6b7 100644
> --- a/src/util/vireventglibwatch.c
> +++ b/src/util/vireventglibwatch.c
> @@ -89,11 +89,11 @@ GSource *virEventGLibCreateSocketWatch(int fd,
>                             sizeof(virEventGLibFDSource));
>       ssource = (virEventGLibFDSource *)source;
>   
> -    ssource->condition = condition;
> +    ssource->condition = condition | G_IO_HUP | G_IO_ERR;
>       ssource->fd = fd;
>   
>       ssource->pollfd.fd = fd;
> -    ssource->pollfd.events = condition;
> +    ssource->pollfd.events = condition | G_IO_HUP | G_IO_ERR;
>   
>       g_source_add_poll(source, &ssource->pollfd);
>   
> 

Yeah, to docs says we need to put HUP and ERR explicitly on the list of 
events. From [1]:

gushort events:

a bitwise combination from GIOCondition, specifying which events should 
be polled for. Typically for reading from a file descriptor you would 
use G_IO_IN | G_IO_HUP | G_IO_ERR, and for writing you would use 
G_IO_OUT | G_IO_ERR.

1: 
https://developer.gnome.org/glib/unstable/glib-The-Main-Event-Loop.html#GPollFD

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

Michal
Re: [libvirt RFC PATCH] util: vireventglibwatch: watch for G_IO_HUP and G_IO_ERR
Posted by Andrea Bolognani 4 years, 2 months ago 
On Wed, 2020-02-19 at 01:31 +0100, Ján Tomko wrote:
> To more closely match the previous usage in virEventPollDispatchHandles,
> where called the handle callback for any revents returned by poll.
> 
> This should fix the virtlogd error on subsequent domain startup:
>   error: can't connect to virtlogd: Cannot open log file:
>   '/var/log/libvirt/qemu/f28live.log': Device or resource busy
> as well as virtlogd spinning caused by virLogHandlerDomainLogFileEvent
> never being called on hangup.
> 
> Signed-off-by: Ján Tomko <jtomko@redhat.com>
> Fixes: f8ab47cb4491dd72d866c1a96a9d94b8c3341de9
> Fixes: 946a25274c46ffff46323c62f567ae7e753aa921
> ---
>  src/util/vireventglibwatch.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Dan and Pavel should confirm this is the correct way to address the
issue, but it makes the problem go away on my machine so

  Tested-by: Andrea Bolognani <abologna@redhat.com>

-- 
Andrea Bolognani / Red Hat / Virtualization

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.