From 17b43e213bcf09024e56f649b57932567785f27c Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sat, 26 Apr 2025 15:53:48 +0900 Subject: [PATCH 1/3] udev: try again to create /run/udev/queue when queueing the next event This is mostly a paranoia, but if we failed to create /run/udev/queue for some reasons on queueing an event, previously we would never create the file until once the queue became empty. This makes in such case we try to create the file again when queueing the next event. --- src/udev/udev-manager.c | 22 +++++++++++----------- src/udev/udev-manager.h | 1 + 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/src/udev/udev-manager.c b/src/udev/udev-manager.c index 8f96cad659..0c59c172e0 100644 --- a/src/udev/udev-manager.c +++ b/src/udev/udev-manager.c @@ -852,16 +852,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; } @@ -1155,13 +1156,12 @@ static int manager_unlink_queue_file(Manager *manager) { /* 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; + 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."); - return log_warning_errno(errno, "Failed to unlink /run/udev/queue: %m"); - } - - log_debug("No events are queued, removed /run/udev/queue."); + manager->queue_file_created = false; return 0; } diff --git a/src/udev/udev-manager.h b/src/udev/udev-manager.h index 9526265154..875f6f955a 100644 --- a/src/udev/udev-manager.h +++ b/src/udev/udev-manager.h @@ -61,6 +61,7 @@ typedef struct Manager { UdevConfig config_by_control; UdevConfig config; + bool queue_file_created; bool stop_exec_queue; bool exit; } Manager; From 5de236bd0771088cc5958559267a5b59ec99ed77 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Thu, 24 Apr 2025 12:53:29 +0900 Subject: [PATCH 2/3] udev: do not remove /run/udev/queue file when we are synthesizing events Note, it should be safe even if we synthesize no event, e.g. when the device has been already removed. In such case, the post event after SIGCHLD will remove the file. --- src/udev/udev-manager.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/udev/udev-manager.c b/src/udev/udev-manager.c index 0c59c172e0..7748a073b7 100644 --- a/src/udev/udev-manager.c +++ b/src/udev/udev-manager.c @@ -1154,6 +1154,9 @@ 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) From 905a89a2199d68fbdf2933a724c0c3aba3ae0f57 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Thu, 24 Apr 2025 13:59:07 +0900 Subject: [PATCH 3/3] udev: readjust priorities of event sources Follow-up for 511619087b66baa52907d1f6c25e28ccb9590a5f. Notable changes are - SIGTERM is the highest among others, to make not udevd queue too many events, as we need to serialize them anyway. - device monitor has the second highest priority, to make 'remove' uevents received earlier than IN_IGNORED inotify events. Otherwise, after IN_IGNORED is received, if there is no queued event, /run/udev/queue file will be removed by the post-event source of the inotify event, and 'udevadm settle' or friends may wrongly finish, even we will soon queue 'remove' uevents for the device. This change should fix the recent instability of TEST-64-UDEV-STORAGE. For other changes, see the comments in the code. --- src/udev/udev-manager-ctrl.c | 2 +- src/udev/udev-manager.c | 81 +++++++++++++++++++++++++++++------- src/udev/udev-manager.h | 29 +++++++++---- src/udev/udev-varlink.c | 2 +- 4 files changed, 91 insertions(+), 23 deletions(-) diff --git a/src/udev/udev-manager-ctrl.c b/src/udev/udev-manager-ctrl.c index d568549cf0..397ed03410 100644 --- a/src/udev/udev-manager-ctrl.c +++ b/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"); diff --git a/src/udev/udev-manager.c b/src/udev/udev-manager.c index 7748a073b7..2026e24f5d 100644 --- a/src/udev/udev-manager.c +++ b/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) 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) 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_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, @@ -1216,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; @@ -1229,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"); diff --git a/src/udev/udev-manager.h b/src/udev/udev-manager.h index 875f6f955a..4f6d116857 100644 --- a/src/udev/udev-manager.h +++ b/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; diff --git a/src/udev/udev-varlink.c b/src/udev/udev-varlink.c index 1e95a6a970..d087a33b5d 100644 --- a/src/udev/udev-varlink.c +++ b/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");