[libvirt] [PATCH v3 4/6] nodedev: Disable/re-enable polling on the udev fd

[John Ferlan]

On 08/24/2017 07:23 AM, Erik Skultety wrote:
> The event loop may get scheduled earlier than the udev event handler
> thread which means that it would keep invoking the handler callback with
> "new" events, while in fact it's most likely still the same event which
> the handler thread hasn't managed to remove from the socket queue yet.
> This is due to unwanted increments of the number of events queuing on the
> socket and causes the handler thread to spam the logs with an error about
> libudev returning NULL (errno == EAGAIN).
> 
> Signed-off-by: Erik Skultety <eskultet at redhat.com>
> ---
>  src/node_device/node_device_udev.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 

And by disabling the polling couldn't we miss an event? That would be
really bad if the event after the one we miss relies on the event we
missed creating something that the subsequent one would need (if that
makes sense).

John

> diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
> index 444e5be4d..e3a647e3d 100644
> --- a/src/node_device/node_device_udev.c
> +++ b/src/node_device/node_device_udev.c
> @@ -1697,6 +1697,7 @@ udevCheckMonitorFD(struct udev_monitor *udev_monitor, int fd)
>  static void
>  udevEventHandleThread(void *opaque)
>  {
> +    udevPrivate *priv = NULL;
>      udevEventThreadDataPtr privateData = opaque;
>      struct udev_device *device = NULL;
>      struct udev_monitor *udev_monitor = NULL;
> @@ -1716,6 +1717,7 @@ udevEventHandleThread(void *opaque)
>          virMutexUnlock(&privateData->lock);
>  
>          nodeDeviceLock();
> +        priv = driver->privateData;
>          udev_monitor = DRV_STATE_UDEV_MONITOR(driver);
>  
>          if (!udevCheckMonitorFD(udev_monitor, privateData->monitor_fd)) {
> @@ -1726,6 +1728,9 @@ udevEventHandleThread(void *opaque)
>          device = udev_monitor_receive_device(udev_monitor);
>          nodeDeviceUnlock();
>  
> +        /* Re-enable polling for new events on the @udev_monitor */
> +        virEventUpdateHandle(priv->watch, VIR_EVENT_HANDLE_READABLE);
> +
>          if (!device) {
>              virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>                             _("udev_monitor_receive_device returned NULL"));
> @@ -1751,10 +1756,12 @@ udevEventHandleCallback(int watch ATTRIBUTE_UNUSED,
>                          int events ATTRIBUTE_UNUSED,
>                          void *opaque)
>  {
> +    udevPrivate *priv = NULL;
>      struct udev_monitor *udev_monitor = NULL;
>      udevEventThreadDataPtr threadData = opaque;
>  
>      nodeDeviceLock();
> +    priv = driver->privateData;
>      udev_monitor = DRV_STATE_UDEV_MONITOR(driver);
>      if (!udevCheckMonitorFD(udev_monitor, fd)) {
>          virMutexLock(&threadData->lock);
> @@ -1771,6 +1778,16 @@ udevEventHandleCallback(int watch ATTRIBUTE_UNUSED,
>      threadData->nevents++;
>      virCondSignal(&threadData->threadCond);
>      virMutexUnlock(&threadData->lock);
> +
> +    /* Due to scheduling, the eventloop might poll the udev monitor before the
> +     * handler thread actually removes the data from the socket, thus causing it
> +     * to spam errors about data not being ready yet, because
> +     * udevEventHandleCallback would be invoked multiple times incrementing the
> +     * counter of events queuing for a single event.
> +     * Therefore we need to disable polling on the fd until the thread actually
> +     * removes the data from the socket.
> +     */
> +    virEventUpdateHandle(priv->watch, 0);
>  }
>  
>  
>