From: Lennart Poettering Date: Thu, 23 Mar 2023 18:05:30 +0000 (+0100) Subject: mount: check right before invoking /bin/umount if it makes sense X-Git-Tag: v254-rc1~385^2~1 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=1483892a421ca34bc841a8e8b1f385744c0407ed;p=thirdparty%2Fsystemd.git mount: check right before invoking /bin/umount if it makes sense Notifications from /proc/self/mountinfo are async, so if we stop a service (and while doing so get rid of the credentials mount point of it), then it will take a while until the notification reaches us and we actually scan the table again. In particular as we nowadays ratelimit notifications on the table, since it's so inefficient. And as I learnt the ratelimiting is actually quite regularly hit during shutdown, where a flurry of umount events are genreated. Hence, let's check if a mount point is actually a mountpoint before trying to unmount it. And if it isn't let's wait for the notification to come in. (This race might be triggred not just by us on ourselves btw: there are other daemons that unmount stuff when stopping where the race also exists, but might simply be harder to trigger: if during service shutdown these services remove some mount then they might collide with us doing the same. After all, we have the rule to unmount everything mounted automatically for you during shutdown.) In the long run we should also start making us of this when it becomes available: https://github.com/util-linux/util-linux/issues/2132 With that we can make issues like this go away entirely from our side of things at least. Fixes: #25527 --- diff --git a/src/basic/unit-def.c b/src/basic/unit-def.c index 86b66e2be0c..bfd748ada4a 100644 --- a/src/basic/unit-def.c +++ b/src/basic/unit-def.c @@ -152,6 +152,7 @@ static const char* const mount_state_table[_MOUNT_STATE_MAX] = { [MOUNT_REMOUNTING_SIGKILL] = "remounting-sigkill", [MOUNT_UNMOUNTING_SIGTERM] = "unmounting-sigterm", [MOUNT_UNMOUNTING_SIGKILL] = "unmounting-sigkill", + [MOUNT_UNMOUNTING_CATCHUP] = "unmounting-catchup", [MOUNT_FAILED] = "failed", [MOUNT_CLEANING] = "cleaning", }; diff --git a/src/basic/unit-def.h b/src/basic/unit-def.h index 169e1f719ec..7bd86cb1d2f 100644 --- a/src/basic/unit-def.h +++ b/src/basic/unit-def.h @@ -97,6 +97,7 @@ typedef enum MountState { MOUNT_REMOUNTING_SIGKILL, MOUNT_UNMOUNTING_SIGTERM, MOUNT_UNMOUNTING_SIGKILL, + MOUNT_UNMOUNTING_CATCHUP, MOUNT_FAILED, MOUNT_CLEANING, _MOUNT_STATE_MAX, diff --git a/src/core/mount.c b/src/core/mount.c index 4a0dc7ebacf..295e4c66c79 100644 --- a/src/core/mount.c +++ b/src/core/mount.c @@ -48,6 +48,7 @@ static const UnitActiveState state_translation_table[_MOUNT_STATE_MAX] = { [MOUNT_REMOUNTING_SIGKILL] = UNIT_RELOADING, [MOUNT_UNMOUNTING_SIGTERM] = UNIT_DEACTIVATING, [MOUNT_UNMOUNTING_SIGKILL] = UNIT_DEACTIVATING, + [MOUNT_UNMOUNTING_CATCHUP] = UNIT_DEACTIVATING, [MOUNT_FAILED] = UNIT_FAILED, [MOUNT_CLEANING] = UNIT_MAINTENANCE, }; @@ -1070,11 +1071,37 @@ static void mount_enter_unmounting(Mount *m) { assert(m); + r = path_is_mount_point(m->where, NULL, 0); + if (IN_SET(r, 0, -ENOENT)) { + /* Not a mount point anymore ? Then we raced against something else which unmounted it? Let's + * handle this gracefully, and wait until we get the kernel notification about it. */ + + log_unit_debug(UNIT(m), "Path '%s' is not a mount point anymore, waiting for mount table refresh.", m->where); + + /* Apparently our idea of the kernel mount table is out of date. Make sure we re-read it + * again, soon, so that we don't delay mount handling unnecessarily. */ + (void) sd_event_source_leave_ratelimit(UNIT(m)->manager->mount_event_source); + + m->control_command_id = _MOUNT_EXEC_COMMAND_INVALID; + mount_unwatch_control_pid(m); + + r = mount_arm_timer(m, usec_add(now(CLOCK_MONOTONIC), m->timeout_usec)); + if (r < 0) + goto fail; + + /* Let's enter a distinct state where we just wait for the mount table to catch up */ + mount_set_state(m, MOUNT_UNMOUNTING_CATCHUP); + return; + } + if (r < 0) + log_unit_debug_errno(UNIT(m), r, "Unable to determine if '%s' is a mount point, ignoring: %m", m->where); + /* Start counting our attempts */ if (!IN_SET(m->state, MOUNT_UNMOUNTING, MOUNT_UNMOUNTING_SIGTERM, - MOUNT_UNMOUNTING_SIGKILL)) + MOUNT_UNMOUNTING_SIGKILL, + MOUNT_UNMOUNTING_CATCHUP)) m->n_retry_umount = 0; m->control_command_id = MOUNT_EXEC_UNMOUNT; @@ -1095,7 +1122,6 @@ static void mount_enter_unmounting(Mount *m) { goto fail; mount_set_state(m, MOUNT_UNMOUNTING); - return; fail: @@ -1268,6 +1294,7 @@ static int mount_start(Unit *u) { MOUNT_UNMOUNTING, MOUNT_UNMOUNTING_SIGTERM, MOUNT_UNMOUNTING_SIGKILL, + MOUNT_UNMOUNTING_CATCHUP, MOUNT_CLEANING)) return -EAGAIN; @@ -1297,6 +1324,7 @@ static int mount_stop(Unit *u) { case MOUNT_UNMOUNTING: case MOUNT_UNMOUNTING_SIGKILL: case MOUNT_UNMOUNTING_SIGTERM: + case MOUNT_UNMOUNTING_CATCHUP: /* Already on it */ return 0; @@ -1562,6 +1590,10 @@ static void mount_sigchld_event(Unit *u, pid_t pid, int code, int status) { mount_enter_dead_or_mounted(m, f); break; + case MOUNT_UNMOUNTING_CATCHUP: + log_unit_debug(u, "Was notified about control process death, but wasn't expecting it. Ignoring."); + break; + case MOUNT_CLEANING: if (m->clean_result == MOUNT_SUCCESS) m->clean_result = f; @@ -1636,6 +1668,11 @@ static int mount_dispatch_timer(sd_event_source *source, usec_t usec, void *user mount_enter_dead_or_mounted(m, MOUNT_FAILURE_TIMEOUT); break; + case MOUNT_UNMOUNTING_CATCHUP: + log_unit_warning(UNIT(m), "Waiting for unmount notification timed out. Giving up."); + mount_enter_dead_or_mounted(m, MOUNT_FAILURE_TIMEOUT); + break; + case MOUNT_CLEANING: log_unit_warning(UNIT(m), "Cleaning timed out. killing."); @@ -2077,8 +2114,7 @@ static int mount_process_proc_self_mountinfo(Manager *m) { if (!mount_is_mounted(mount)) { - /* A mount point is not around right now. It - * might be gone, or might never have + /* A mount point is not around right now. It might be gone, or might never have * existed. */ if (mount->from_proc_self_mountinfo && @@ -2093,6 +2129,7 @@ static int mount_process_proc_self_mountinfo(Manager *m) { switch (mount->state) { case MOUNT_MOUNTED: + case MOUNT_UNMOUNTING_CATCHUP: /* This has just been unmounted by somebody else, follow the state change. */ mount_enter_dead(mount, MOUNT_SUCCESS); break; @@ -2131,11 +2168,9 @@ static int mount_process_proc_self_mountinfo(Manager *m) { break; default: - /* Nothing really changed, but let's - * issue an notification call - * nonetheless, in case somebody is - * waiting for this. (e.g. file system - * ro/rw remounts.) */ + /* Nothing really changed, but let's issue an notification call nonetheless, + * in case somebody is waiting for this. (e.g. file system ro/rw + * remounts.) */ mount_set_state(mount, mount->state); break; }