udev: Make Storage Tests Stable Again ! (systemd#37262)

Author: bluca

src/udev/udev-manager-ctrl.c

@@ -125,7 +125,7 @@ int manager_start_ctrl(Manager *manager) {
         /* This needs to be after the inotify and uevent handling, to make sure that the ping is send back
          * after fully processing the pending uevents (including the synthetic ones we may create due to
          * inotify events). */
-        r = sd_event_source_set_priority(udev_ctrl_get_event_source(manager->ctrl), SD_EVENT_PRIORITY_IDLE);
+        r = sd_event_source_set_priority(udev_ctrl_get_event_source(manager->ctrl), EVENT_PRIORITY_CONTROL);
         if (r < 0)
                 return log_error_errno(r, "Failed to set IDLE event priority for udev control event source: %m");
 

src/udev/udev-manager.c

@@ -219,7 +219,7 @@ static int manager_reset_kill_workers_timer(Manager *manager) {
                                 USEC_PER_SEC,
                                 on_kill_workers_event,
                                 manager,
-                                SD_EVENT_PRIORITY_NORMAL,
+                                EVENT_PRIORITY_WORKER_TIMER,
                                 "kill-workers-event",
                                 /* force_reset = */ false);
                 if (r < 0)
@@ -450,13 +450,28 @@ static void worker_attach_event(Worker *worker, Event *event) {
         event->state = EVENT_RUNNING;
         event->worker = worker;
 
-        (void) sd_event_add_time_relative(e, &event->timeout_warning_event, CLOCK_MONOTONIC,
-                                          udev_warn_timeout(manager->config.timeout_usec), USEC_PER_SEC,
-                                          on_event_timeout_warning, event);
-
-        (void) sd_event_add_time_relative(e, &event->timeout_event, CLOCK_MONOTONIC,
-                                          manager_kill_worker_timeout(manager), USEC_PER_SEC,
-                                          on_event_timeout, event);
+        (void) event_reset_time_relative(
+                        e,
+                        &event->timeout_warning_event,
+                        CLOCK_MONOTONIC,
+                        udev_warn_timeout(manager->config.timeout_usec),
+                        USEC_PER_SEC,
+                        on_event_timeout_warning,
+                        event,
+                        EVENT_PRIORITY_WORKER_TIMER,
+                        "event-timeout-warn",
+                        /* force_reset = */ true);
+
+        (void) event_reset_time_relative(
+                        e,
+                        &event->timeout_event, CLOCK_MONOTONIC,
+                        manager_kill_worker_timeout(manager),
+                        USEC_PER_SEC,
+                        on_event_timeout,
+                        event,
+                        EVENT_PRIORITY_WORKER_TIMER,
+                        "event-timeout-kill",
+                        /* force_reset = */ true);
 }
 
 static int worker_spawn(Manager *manager, Event *event) {
@@ -740,10 +755,17 @@ static void event_requeue(Event *event) {
         if (event->retry_again_timeout_usec == 0)
                 event->retry_again_timeout_usec = usec_add(now_usec, EVENT_RETRY_TIMEOUT_USEC);
 
-        r = event_reset_time_relative(event->manager->event, &event->retry_event_source,
-                                      CLOCK_MONOTONIC, EVENT_RETRY_INTERVAL_USEC, 0,
-                                      on_event_retry, NULL,
-                                      0, "retry-event", true);
+        r = event_reset_time_relative(
+                        event->manager->event,
+                        &event->retry_event_source,
+                        CLOCK_MONOTONIC,
+                        EVENT_RETRY_INTERVAL_USEC,
+                        /* accuracy = */ 0,
+                        on_event_retry,
+                        /* userdata = */ NULL,
+                        EVENT_PRIORITY_RETRY_EVENT,
+                        "retry-event",
+                        /* force_reset = */ true);
         if (r < 0) {
                 log_device_warning_errno(
                                 dev, r,
@@ -852,16 +874,17 @@ static int event_queue_insert(Manager *manager, sd_device *dev) {
                 .state = EVENT_QUEUED,
         };
 
-        if (!manager->events) {
+        LIST_APPEND(event, manager->events, event);
+        log_device_uevent(dev, "Device is queued");
+
+        if (!manager->queue_file_created) {
                 r = touch("/run/udev/queue");
                 if (r < 0)
                         log_warning_errno(r, "Failed to touch /run/udev/queue, ignoring: %m");
+                else
+                        manager->queue_file_created = true;
         }
 
-        LIST_APPEND(event, manager->events, event);
-
-        log_device_uevent(dev, "Device is queued");
-
         return 0;
 }
 
@@ -1153,15 +1176,17 @@ static int manager_unlink_queue_file(Manager *manager) {
         if (manager->events)
                 return 0; /* There are queued events. */
 
+        if (!set_isempty(manager->synthesize_change_child_event_sources))
+                return 0; /* There are child processes that should trigger synthetic events. */
+
         /* There are no queued events. Let's remove /run/udev/queue and clean up the idle processes. */
         if (unlink("/run/udev/queue") < 0) {
-                if (errno == ENOENT)
-                        return 0;
-
-                return log_warning_errno(errno, "Failed to unlink /run/udev/queue: %m");
-        }
+                if (errno != ENOENT)
+                        return log_warning_errno(errno, "Failed to unlink /run/udev/queue: %m");
+        } else
+                log_debug("No events are queued, removed /run/udev/queue.");
 
-        log_debug("No events are queued, removed /run/udev/queue.");
+        manager->queue_file_created = false;
         return 0;
 }
 
@@ -1213,6 +1238,37 @@ static int on_post(sd_event_source *s, void *userdata) {
         return 0;
 }
 
+static int manager_setup_signal(
+                Manager *manager,
+                sd_event *event,
+                int signal,
+                sd_event_signal_handler_t handler,
+                int64_t priority,
+                const char *description) {
+
+        _cleanup_(sd_event_source_unrefp) sd_event_source *s = NULL;
+        int r;
+
+        assert(manager);
+        assert(event);
+
+        r = sd_event_add_signal(event, &s, signal | SD_EVENT_SIGNAL_PROCMASK, handler, manager);
+        if (r < 0)
+                return r;
+
+        r = sd_event_source_set_priority(s, priority);
+        if (r < 0)
+                return r;
+
+        (void) sd_event_source_set_description(s, description);
+
+        r = sd_event_source_set_floating(s, true);
+        if (r < 0)
+                return r;
+
+        return 0;
+}
+
 static int manager_setup_event(Manager *manager) {
         _cleanup_(sd_event_unrefp) sd_event *e = NULL;
         int r;
@@ -1226,15 +1282,15 @@ static int manager_setup_event(Manager *manager) {
         if (r < 0)
                 return log_error_errno(r, "Failed to allocate event loop: %m");
 
-        r = sd_event_add_signal(e, /* ret_event_source = */ NULL, SIGINT | SD_EVENT_SIGNAL_PROCMASK, on_sigterm, manager);
+        r = manager_setup_signal(manager, e, SIGINT, on_sigterm, EVENT_PRIORITY_SIGTERM, "sigint-event-source");
         if (r < 0)
                 return log_error_errno(r, "Failed to create SIGINT event source: %m");
 
-        r = sd_event_add_signal(e, /* ret_event_source = */ NULL, SIGTERM | SD_EVENT_SIGNAL_PROCMASK, on_sigterm, manager);
+        r = manager_setup_signal(manager, e, SIGTERM, on_sigterm, EVENT_PRIORITY_SIGTERM, "sigterm-event-source");
         if (r < 0)
                 return log_error_errno(r, "Failed to create SIGTERM event source: %m");
 
-        r = sd_event_add_signal(e, /* ret_event_source = */ NULL, SIGHUP | SD_EVENT_SIGNAL_PROCMASK, on_sighup, manager);
+        r = manager_setup_signal(manager, e, SIGHUP, on_sighup, EVENT_PRIORITY_SIGHUP, "sighup-event-source");
         if (r < 0)
                 return log_error_errno(r, "Failed to create SIGHUP event source: %m");
 

src/udev/udev-manager.h

@@ -15,17 +15,32 @@
 #include "udev-ctrl.h"
 #include "udev-def.h"
 
+/* This should have a higher priority than the device monitor and inotify watch, to make device monitor and
+ * inotify event source stopped as soon as possible when the signal is received. Otherwise, we may continue
+ * receive events that needs to be serialized anyway. */
+#define EVENT_PRIORITY_SIGTERM        (SD_EVENT_PRIORITY_NORMAL - 6)
+/* This must have a higher priority than the inotify event source, to make 'remove' uevent received earlier
+ * than IN_IGNORED inotify event. */
+#define EVENT_PRIORITY_DEVICE_MONITOR (SD_EVENT_PRIORITY_NORMAL - 5)
 /* This must have a higher priority than the worker notification, to make IN_IGNORED event received earlier
  * than notifications about requests of adding/removing inotify watches. */
-#define EVENT_PRIORITY_INOTIFY_WATCH  (SD_EVENT_PRIORITY_NORMAL - 30)
+#define EVENT_PRIORITY_INOTIFY_WATCH  (SD_EVENT_PRIORITY_NORMAL - 4)
 /* This must have a higher priority than the worker SIGCHLD event, to make notifications about completions of
  * processing events received before SIGCHLD. */
-#define EVENT_PRIORITY_WORKER_NOTIFY  (SD_EVENT_PRIORITY_NORMAL - 20)
-/* This should have a higher priority than other events, especially timer events about killing long running
- * worker processes or idle worker processes. */
-#define EVENT_PRIORITY_WORKER_SIGCHLD (SD_EVENT_PRIORITY_NORMAL - 10)
-/* This should have a lower priority to make signal and timer event sources processed earlier. */
-#define EVENT_PRIORITY_DEVICE_MONITOR (SD_EVENT_PRIORITY_NORMAL + 10)
+#define EVENT_PRIORITY_WORKER_NOTIFY  (SD_EVENT_PRIORITY_NORMAL - 3)
+/* This should have a higher priority than timer events about killing long running worker processes or idle
+ * worker processes. */
+#define EVENT_PRIORITY_WORKER_SIGCHLD (SD_EVENT_PRIORITY_NORMAL - 2)
+/* As said in the above, this should have a lower proority than the SIGCHLD event source. */
+#define EVENT_PRIORITY_WORKER_TIMER   (SD_EVENT_PRIORITY_NORMAL - 1)
+/* This should have a lower priority than most event sources, but let's process earlier than varlink and the
+ * legacy control socket. */
+#define EVENT_PRIORITY_SIGHUP         (SD_EVENT_PRIORITY_NORMAL + 1)
+/* Let's not interrupt the service by any user process, even that requires privileges. */
+#define EVENT_PRIORITY_VARLINK        (SD_EVENT_PRIORITY_NORMAL + 2)
+#define EVENT_PRIORITY_CONTROL        (SD_EVENT_PRIORITY_NORMAL + 2)
+/* The event is intended to trigger the post-event source, hence can be the lowest priority. */
+#define EVENT_PRIORITY_RETRY_EVENT    (SD_EVENT_PRIORITY_NORMAL + 3)
 
 typedef struct Event Event;
 typedef struct UdevRules UdevRules;
@@ -61,6 +76,7 @@ typedef struct Manager {
         UdevConfig config_by_control;
         UdevConfig config;
 
+        bool queue_file_created;
         bool stop_exec_queue;
         bool exit;
 } Manager;

src/udev/udev-varlink.c

@@ -170,7 +170,7 @@ int manager_start_varlink_server(Manager *manager) {
         /* This needs to be after the inotify and uevent handling, to make sure that the ping is send back
          * after fully processing the pending uevents (including the synthetic ones we may create due to
          * inotify events). */
-        r = sd_varlink_server_attach_event(v, manager->event, SD_EVENT_PRIORITY_IDLE);
+        r = sd_varlink_server_attach_event(v, manager->event, EVENT_PRIORITY_VARLINK);
         if (r < 0)
                 return log_error_errno(r, "Failed to attach Varlink connection to event loop: %m");