[libvirt PATCH v4 12/25] nodedev: Refresh mdev devices when changes are detected

[Jonathon Jongsma]

On Wed, 17 Feb 2021 14:35:51 +0100
Erik Skultety <eskultet at redhat.com> wrote:

> > +static void
> > +mdevctlEventHandleCallback(GFileMonitor *monitor G_GNUC_UNUSED,
> > +                           GFile *file,
> > +                           GFile *other_file G_GNUC_UNUSED,
> > +                           GFileMonitorEvent event_type,
> > +                           gpointer user_data)
> > +{
> > +    udevEventDataPtr priv = user_data;
> > +    /* if a new directory appears, monitor that directory for
> > changes */
> > +    if (event_type == G_FILE_MONITOR_EVENT_CREATED &&
> > +        g_file_query_file_type(file, G_FILE_QUERY_INFO_NONE, NULL)
> > ==
> > +        G_FILE_TYPE_DIRECTORY) {
> > +        GList *newmonitors = monitorFileRecursively(priv, file);
> > +        priv->mdevctlMonitors =
> > g_list_concat(priv->mdevctlMonitors, newmonitors);  
> 
> priv->mdevctlMonitors is shared among threads, but you only acquire
> the mutex just before exec'ing mdevctl in the mdevctlHandler thread,
> so ^this can yield unexpected results in terms of directories
> monitored.

A) It doesn't really affect the directories being monitored, it only
affects our ability to free these monitors later in the destructor.

B) Aside from the very first time this variable is set in
nodeStateInitialize(), the only place that accesses the mdevctlMonitors
variable is this glib signal callback (to add new monitors to the list)
and the destructor (to free the monitors). And I believe those should
always run in the main thread. So I don't think there's really much of
a thread-safety issue here. Perhaps there's a slight race in
nodeStateInitialize() which appears to be called from a non-main
thread. If a directory gets created immediately after connecting to the
file monitor signal but before the function returns and the GList gets
assigned to priv->mdevctlMonitors, I suppose the signal handler could
run and it could get corrupted. Would you like me to protect all
accesses with a Mutex?


> 
> > +    }
> > +
> > +    /* When mdevctl creates a device, it can result in multiple
> > notify events
> > +     * emitted for a single logical change (e.g. several CHANGED
> > events, or a
> > +     * CREATED and CHANGED event followed by CHANGES_DONE_HINT).
> > To avoid
> > +     * spawning a mdevctl thread multiple times for a single
> > logical
> > +     * configuration change, try to coalesce these changes by
> > waiting for the
> > +     * CHANGES_DONE_HINT event. As a fallback,  add a timeout to
> > trigger the
> > +     * signal if that event never comes */  
> 
> So there's no guaranteed pattern to monitor, is that so? IOW Even the
> CHANGES_DONE_HINT may not actually arrive? That's a very poor design.
> I'd like to ditch the timer as much as possible.

Maybe we can simply rely on the CHANGES_DONE_HINT. I tend to be
conservative and was trying to handle the case where the
CHANGES_DONE_HINT might not be delivered (it is called a "hint" after
all). Part of this was caused by my reading this comment in a test
case within the glib repository:

  /* Sometimes the emission of 'CHANGES_DONE_HINT' may be late because
   * it depends on the ability of file monitor implementation to report
   * 'CHANGES_DONE_HINT' itself. If the file monitor implementation
   * doesn't report 'CHANGES_DONE_HINT' itself, it may be emitted by
   * GLocalFileMonitor after a few seconds, which causes the event to
   * mix with results from different steps. Since 'CHANGES_DONE_HINT'
   * is just a hint, we don't require it to be reliable and we simply
   * ignore unexpected 'CHANGES_DONE_HINT' events here. */

So it is not reliably communicated if your file monitor backend doesn't
implement it. But the above comment seems to suggest that if the file monitor
backend doesn't support it, the event will be generated by glib itself and will
simply be delayed by a couple of seconds rather than missing altogether. I'd
have to read the glib code to tell whether it's possible for it to be omitted
altogether. I guess I don't see the timer as a horrible workaround, but I can
ditch it if you want.

Alternatively I suppose I could simply kick off a new thread for each file
change event and not bother trying to coalesce them at all. This will result in
perhaps a an extra thread or two being launched when mdevctl modifies
the mdev registry behind our backs, but that's unlikely to be a common
occurrence and so shouldn't really be a significant performance issue.

> 
> > +    if (event_type != G_FILE_MONITOR_EVENT_CHANGES_DONE_HINT) {
> > +        if (priv->mdevctlTimeout > 0)
> > +            virEventRemoveTimeout(priv->mdevctlTimeout);
> > +        priv->mdevctlTimeout = virEventAddTimeout(100,
> > scheduleMdevctlHandler,
> > +                                                  priv, NULL);
> > +        return;
> > +    }
> > +
> > +    scheduleMdevctlHandler(-1, priv);
> > +}
> > +
> > +
> >  static int
> >  nodeStateInitialize(bool privileged,
> >                      const char *root,

Jonathon