From: Yu Watanabe Date: Thu, 10 Aug 2023 02:26:43 +0000 (+0900) Subject: Revert "mount: check right before invoking /bin/umount if it makes sense" X-Git-Tag: v255-rc1~700^2~1 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=e323d2e463270ef556aeb753455bdf01a22af46d;p=thirdparty%2Fsystemd.git Revert "mount: check right before invoking /bin/umount if it makes sense" This reverts commit 1483892a421ca34bc841a8e8b1f385744c0407ed. As the commit says, it does not solve the race. Moreover, it introduces an regression #28410. Also, checking by `path_is_mount_point()` may trigger automount. From statx(2), > AT_NO_AUTOMOUNT > Don't automount the terminal ("basename") component of pathname > if it is a directory that is an automount point. Similar statements can be found in fstatat(2), which is used in the fallback call for statx() in glibc, and name_to_handle_at(2), which is used as the fallback when statx() failed. So, `path_is_mount_point()` may _do_ trigger automount for parent paths. That should be avoided especially on shutdown. The original issue #25527 that is 'fixed' by the commit is not serious, and should be fixed by making umount command handle path gracefully: https://github.com/util-linux/util-linux/issues/2132 Fixes #28410. --- diff --git a/src/basic/unit-def.c b/src/basic/unit-def.c index 595e4414c7e..23611329368 100644 --- a/src/basic/unit-def.c +++ b/src/basic/unit-def.c @@ -152,7 +152,6 @@ 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 f67fc1d620d..345ca533a30 100644 --- a/src/basic/unit-def.h +++ b/src/basic/unit-def.h @@ -97,7 +97,6 @@ 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 5bd72a0b656..b773ce5fcae 100644 --- a/src/core/mount.c +++ b/src/core/mount.c @@ -48,7 +48,6 @@ 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, }; @@ -1063,37 +1062,11 @@ 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_CATCHUP)) + MOUNT_UNMOUNTING_SIGKILL)) m->n_retry_umount = 0; m->control_command_id = MOUNT_EXEC_UNMOUNT; @@ -1114,6 +1087,7 @@ static void mount_enter_unmounting(Mount *m) { goto fail; mount_set_state(m, MOUNT_UNMOUNTING); + return; fail: @@ -1286,7 +1260,6 @@ static int mount_start(Unit *u) { MOUNT_UNMOUNTING, MOUNT_UNMOUNTING_SIGTERM, MOUNT_UNMOUNTING_SIGKILL, - MOUNT_UNMOUNTING_CATCHUP, MOUNT_CLEANING)) return -EAGAIN; @@ -1316,7 +1289,6 @@ 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; @@ -1582,10 +1554,6 @@ 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; @@ -1660,11 +1628,6 @@ 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."); @@ -2121,7 +2084,6 @@ 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;