core/mount: if mount is gone eventually, consider it success

Currently, if unmount initiated by us fails, we record
that in result. Later, if we tried again and succeeded,
or someone else successfully unmounted it, the unit
state is still considered failed. Let's be more tolerant
instead, and forget about previous failure.

Alternative to #32002

diff --git a/src/core/mount.c b/src/core/mount.c
index 2827f32d64a3412ea0cd5fa08f033e48c6ac791d..b90b76d3ac05fd7b22d0ed43693a48bcdff43170 100644 (file)
--- a/src/core/mount.c
+++ b/src/core/mount.c
@@ -55,7 +55,7 @@ static const UnitActiveState state_translation_table[_MOUNT_STATE_MAX] = {
 
 static int mount_dispatch_timer(sd_event_source *source, usec_t usec, void *userdata);
 static int mount_dispatch_io(sd_event_source *source, int fd, uint32_t revents, void *userdata);
-static void mount_enter_dead(Mount *m, MountResult f);
+static void mount_enter_dead(Mount *m, MountResult f, bool flush_result);
 static void mount_enter_mounted(Mount *m, MountResult f);
 static void mount_cycle_clear(Mount *m);
 static int mount_process_proc_self_mountinfo(Manager *m);
@@ -800,7 +800,7 @@ static void mount_catchup(Unit *u) {
                         break;
                 case MOUNT_MOUNTED:
                         assert(!pidref_is_set(&m->control_pid));
-                        mount_enter_dead(m, MOUNT_SUCCESS);
+                        mount_enter_dead(m, MOUNT_SUCCESS, /* flush_result = */ false);
                         break;
                 default:
                         break;
@@ -899,10 +899,10 @@ static int mount_spawn(Mount *m, ExecCommand *c, PidRef *ret_pid) {
         return 0;
 }
 
-static void mount_enter_dead(Mount *m, MountResult f) {
+static void mount_enter_dead(Mount *m, MountResult f, bool flush_result) {
         assert(m);
 
-        if (m->result == MOUNT_SUCCESS)
+        if (m->result == MOUNT_SUCCESS || flush_result)
                 m->result = f;
 
         unit_log_result(UNIT(m), m->result == MOUNT_SUCCESS, mount_result_to_string(m->result));
@@ -930,17 +930,20 @@ static void mount_enter_mounted(Mount *m, MountResult f) {
         mount_set_state(m, MOUNT_MOUNTED);
 }
 
-static void mount_enter_dead_or_mounted(Mount *m, MountResult f) {
+static void mount_enter_dead_or_mounted(Mount *m, MountResult f, bool flush_result) {
         assert(m);
 
-        /* Enter DEAD or MOUNTED state, depending on what the kernel currently says about the mount point. We use this
-         * whenever we executed an operation, so that our internal state reflects what the kernel says again, after all
-         * ultimately we just mirror the kernel's internal state on this. */
+        /* Enter DEAD or MOUNTED state, depending on what the kernel currently says about the mount point.
+         * We use this whenever we executed an operation, so that our internal state reflects what
+         * the kernel says again, after all ultimately we just mirror the kernel's internal state on this.
+         *
+         * Note that flush_result only applies to mount_enter_dead(), since that's when the result gets
+         * turned into unit end state. */
 
         if (m->from_proc_self_mountinfo)
                 mount_enter_mounted(m, f);
         else
-                mount_enter_dead(m, f);
+                mount_enter_dead(m, f, flush_result);
 }
 
 static int state_to_kill_operation(MountState state) {
@@ -990,12 +993,12 @@ static void mount_enter_signal(Mount *m, MountState state, MountResult f) {
         else if (state == MOUNT_UNMOUNTING_SIGTERM && m->kill_context.send_sigkill)
                 mount_enter_signal(m, MOUNT_UNMOUNTING_SIGKILL, MOUNT_SUCCESS);
         else
-                mount_enter_dead_or_mounted(m, MOUNT_SUCCESS);
+                mount_enter_dead_or_mounted(m, MOUNT_SUCCESS, /* flush_result = */ false);
 
         return;
 
 fail:
-        mount_enter_dead_or_mounted(m, MOUNT_FAILURE_RESOURCES);
+        mount_enter_dead_or_mounted(m, MOUNT_FAILURE_RESOURCES, /* flush_result = */ false);
 }
 
 static int mount_set_umount_command(Mount *m, ExecCommand *c) {
@@ -1057,7 +1060,7 @@ static void mount_enter_unmounting(Mount *m) {
         return;
 
 fail:
-        mount_enter_dead_or_mounted(m, MOUNT_FAILURE_RESOURCES);
+        mount_enter_dead_or_mounted(m, MOUNT_FAILURE_RESOURCES, /* flush_result = */ false);
 }
 
 static int mount_set_mount_command(Mount *m, ExecCommand *c, const MountParameters *p) {
@@ -1201,7 +1204,7 @@ static void mount_enter_mounting(Mount *m) {
         return;
 
 fail:
-        mount_enter_dead_or_mounted(m, MOUNT_FAILURE_RESOURCES);
+        mount_enter_dead_or_mounted(m, MOUNT_FAILURE_RESOURCES, /* flush_result = */ false);
 }
 
 static void mount_set_reload_result(Mount *m, MountResult result) {
@@ -1267,7 +1270,7 @@ static void mount_enter_remounting(Mount *m) {
 
 fail:
         mount_set_reload_result(m, MOUNT_FAILURE_RESOURCES);
-        mount_enter_dead_or_mounted(m, MOUNT_SUCCESS);
+        mount_enter_dead_or_mounted(m, MOUNT_SUCCESS, /* flush_result = */ false);
 }
 
 static void mount_cycle_clear(Mount *m) {
@@ -1547,7 +1550,7 @@ static void mount_sigchld_event(Unit *u, pid_t pid, int code, int status) {
                         log_unit_warning(UNIT(m), "Mount process finished, but there is no mount.");
                         f = MOUNT_FAILURE_PROTOCOL;
                 }
-                mount_enter_dead(m, f);
+                mount_enter_dead(m, f, /* flush_result = */ false);
                 break;
 
         case MOUNT_MOUNTING_DONE:
@@ -1557,7 +1560,7 @@ static void mount_sigchld_event(Unit *u, pid_t pid, int code, int status) {
         case MOUNT_REMOUNTING:
         case MOUNT_REMOUNTING_SIGTERM:
         case MOUNT_REMOUNTING_SIGKILL:
-                mount_enter_dead_or_mounted(m, MOUNT_SUCCESS);
+                mount_enter_dead_or_mounted(m, MOUNT_SUCCESS, /* flush_result = */ false);
                 break;
 
         case MOUNT_UNMOUNTING:
@@ -1578,22 +1581,27 @@ static void mount_sigchld_event(Unit *u, pid_t pid, int code, int status) {
                         /* Hmm, umount process spawned by us failed, but the mount disappeared anyway?
                          * Maybe someone else is trying to unmount at the same time. */
                         log_unit_notice(u, "Mount disappeared even though umount process failed, continuing.");
-                        mount_enter_dead(m, MOUNT_SUCCESS);
+                        mount_enter_dead(m, MOUNT_SUCCESS, /* flush_result = */ true);
                 } else
-                        mount_enter_dead_or_mounted(m, f);
+                        /* At this point, either the unmount succeeded or unexpected error occurred. We usually
+                         * remember the first error in 'result', but here let's update that forcibly, since
+                         * there could previous failed attempts yet we only care about the most recent
+                         * attempt. IOW, if we eventually managed to unmount the stuff, don't enter failed
+                         * end state. */
+                        mount_enter_dead_or_mounted(m, f, /* flush_result = */ true);
 
                 break;
 
         case MOUNT_UNMOUNTING_SIGTERM:
         case MOUNT_UNMOUNTING_SIGKILL:
-                mount_enter_dead_or_mounted(m, f);
+                mount_enter_dead_or_mounted(m, f, /* flush_result = */ false);
                 break;
 
         case MOUNT_CLEANING:
                 if (m->clean_result == MOUNT_SUCCESS)
                         m->clean_result = f;
 
-                mount_enter_dead(m, MOUNT_SUCCESS);
+                mount_enter_dead(m, MOUNT_SUCCESS, /* flush_result = */ false);
                 break;
 
         default:
@@ -1631,7 +1639,7 @@ static int mount_dispatch_timer(sd_event_source *source, usec_t usec, void *user
                         mount_enter_signal(m, MOUNT_REMOUNTING_SIGKILL, MOUNT_SUCCESS);
                 } else {
                         log_unit_warning(UNIT(m), "Remounting timed out. Skipping SIGKILL. Ignoring.");
-                        mount_enter_dead_or_mounted(m, MOUNT_SUCCESS);
+                        mount_enter_dead_or_mounted(m, MOUNT_SUCCESS, /* flush_result = */ false);
                 }
                 break;
 
@@ -1639,7 +1647,7 @@ static int mount_dispatch_timer(sd_event_source *source, usec_t usec, void *user
                 mount_set_reload_result(m, MOUNT_FAILURE_TIMEOUT);
 
                 log_unit_warning(UNIT(m), "Mount process still around after SIGKILL. Ignoring.");
-                mount_enter_dead_or_mounted(m, MOUNT_SUCCESS);
+                mount_enter_dead_or_mounted(m, MOUNT_SUCCESS, /* flush_result = */ false);
                 break;
 
         case MOUNT_UNMOUNTING:
@@ -1653,13 +1661,13 @@ static int mount_dispatch_timer(sd_event_source *source, usec_t usec, void *user
                         mount_enter_signal(m, MOUNT_UNMOUNTING_SIGKILL, MOUNT_FAILURE_TIMEOUT);
                 } else {
                         log_unit_warning(UNIT(m), "Mount process timed out. Skipping SIGKILL. Ignoring.");
-                        mount_enter_dead_or_mounted(m, MOUNT_FAILURE_TIMEOUT);
+                        mount_enter_dead_or_mounted(m, MOUNT_FAILURE_TIMEOUT, /* flush_result = */ false);
                 }
                 break;
 
         case MOUNT_UNMOUNTING_SIGKILL:
                 log_unit_warning(UNIT(m), "Mount process still around after SIGKILL. Ignoring.");
-                mount_enter_dead_or_mounted(m, MOUNT_FAILURE_TIMEOUT);
+                mount_enter_dead_or_mounted(m, MOUNT_FAILURE_TIMEOUT, /* flush_result = */ false);
                 break;
 
         case MOUNT_CLEANING:
@@ -2121,8 +2129,11 @@ static int mount_process_proc_self_mountinfo(Manager *m) {
                         switch (mount->state) {
 
                         case MOUNT_MOUNTED:
-                                /* This has just been unmounted by somebody else, follow the state change. */
-                                mount_enter_dead(mount, MOUNT_SUCCESS);
+                                /* This has just been unmounted by somebody else, follow the state change.
+                                 * Also explicitly override the result (see the comment in mount_sigchld_event()),
+                                 * but more aggressively here since the state change is extrinsic. */
+                                mount_cycle_clear(mount);
+                                mount_enter_dead(mount, MOUNT_SUCCESS, /* flush_result = */ true);
                                 break;
 
                         case MOUNT_MOUNTING_DONE:
@@ -2290,7 +2301,7 @@ static int mount_can_start(Unit *u) {
 
         r = unit_test_start_limit(u);
         if (r < 0) {
-                mount_enter_dead(m, MOUNT_FAILURE_START_LIMIT_HIT);
+                mount_enter_dead(m, MOUNT_FAILURE_START_LIMIT_HIT, /* flush_result = */ false);
                 return r;
         }