From d5641e0d7e8f55937fbc3a7ecd667e42c5836d80 Mon Sep 17 00:00:00 2001 From: Kyle Walker Date: Wed, 13 Dec 2017 12:49:26 -0500 Subject: [PATCH 1/2] core: Implement timeout based umount/remount limit Remount, and subsequent umount, attempts can hang for inaccessible network based mount points. This can leave a system in a hard hang state that requires a hard reset in order to recover. This change moves the remount, and umount attempts into separate child processes. The remount and umount operations will block for up to 90 seconds (DEFAULT_TIMEOUT_USEC). Should those waits fail, the parent will issue a SIGKILL to the child and continue with the shutdown efforts. In addition, instead of only reporting some additional errors on the final attempt, failures are reported as they occur. --- src/basic/process-util.c | 61 +++++++++++++++++++ src/basic/process-util.h | 2 + src/core/umount.c | 124 ++++++++++++++++++++++++++++++--------- 3 files changed, 159 insertions(+), 28 deletions(-) diff --git a/src/basic/process-util.c b/src/basic/process-util.c index 5f001494f0..0b2eb07abc 100644 --- a/src/basic/process-util.c +++ b/src/basic/process-util.c @@ -699,6 +699,67 @@ int wait_for_terminate_and_warn(const char *name, pid_t pid, bool check_exit_cod return -EPROTO; } +/* + * Return values: + * < 0 : wait_for_terminate_with_timeout() failed to get the state of the + * process, the process timed out, the process was terminated by a + * signal, or failed for an unknown reason. + * >=0 : The process terminated normally with no failures. + * + * Success is indicated by a return value of zero, a timeout is indicated + * by ETIMEDOUT, and all other child failure states are indicated by error + * is indicated by a non-zero value. + */ +int wait_for_terminate_with_timeout(pid_t pid, usec_t timeout) { + sigset_t mask; + int r; + usec_t until; + + assert_se(sigemptyset(&mask) == 0); + assert_se(sigaddset(&mask, SIGCHLD) == 0); + + /* Drop into a sigtimewait-based timeout. Waiting for the + * pid to exit. */ + until = now(CLOCK_MONOTONIC) + timeout; + for (;;) { + usec_t n; + siginfo_t status = {}; + struct timespec ts; + + n = now(CLOCK_MONOTONIC); + if (n >= until) + break; + + r = sigtimedwait(&mask, NULL, timespec_store(&ts, until - n)) < 0 ? -errno : 0; + /* Assuming we woke due to the child exiting. */ + if (waitid(P_PID, pid, &status, WEXITED|WNOHANG) == 0) { + if (status.si_pid == pid) { + /* This is the correct child.*/ + if (status.si_code == CLD_EXITED) + return (status.si_status == 0) ? 0 : -EPROTO; + else + return -EPROTO; + } + } + /* Not the child, check for errors and proceed appropriately */ + if (r < 0) { + switch (r) { + case -EAGAIN: + /* Timed out, child is likely hung. */ + return -ETIMEDOUT; + case -EINTR: + /* Received a different signal and should retry */ + continue; + default: + /* Return any unexpected errors */ + return r; + } + } + } + + return -EPROTO; +} + void sigkill_wait(pid_t pid) { assert(pid > 1); diff --git a/src/basic/process-util.h b/src/basic/process-util.h index 39c194df0d..57955e1e89 100644 --- a/src/basic/process-util.h +++ b/src/basic/process-util.h @@ -33,6 +33,7 @@ #include "format-util.h" #include "ioprio.h" #include "macro.h" +#include "time-util.h" #define procfs_file_alloca(pid, field) \ ({ \ @@ -61,6 +62,7 @@ int get_process_ppid(pid_t pid, pid_t *ppid); int wait_for_terminate(pid_t pid, siginfo_t *status); int wait_for_terminate_and_warn(const char *name, pid_t pid, bool check_exit_code); +int wait_for_terminate_with_timeout(pid_t pid, usec_t timeout); void sigkill_wait(pid_t pid); void sigkill_waitp(pid_t *pid); diff --git a/src/core/umount.c b/src/core/umount.c index cd58a9cf6d..7f8ddb99ee 100644 --- a/src/core/umount.c +++ b/src/core/umount.c @@ -28,6 +28,7 @@ #include "libudev.h" #include "alloc-util.h" +#include "def.h" #include "escape.h" #include "fd-util.h" #include "fstab-util.h" @@ -35,6 +36,7 @@ #include "list.h" #include "mount-setup.h" #include "path-util.h" +#include "signal-util.h" #include "string-util.h" #include "udev-util.h" #include "umount.h" @@ -376,10 +378,86 @@ static bool nonunmountable_path(const char *path) { || path_startswith(path, "/run/initramfs"); } +static int remount_with_timeout(MountPoint *m, char *options, int *n_failed) { + pid_t pid; + int r; + + BLOCK_SIGNALS(SIGCHLD); + + /* Due to the possiblity of a remount operation hanging, we + * fork a child process and set a timeout. If the timeout + * lapses, the assumption is that that particular remount + * failed. */ + pid = fork(); + if (pid < 0) + return log_error_errno(errno, "Failed to fork: %m"); + + if (pid == 0) { + log_info("Remounting '%s' read-only in with options '%s'.", m->path, options); + + /* Start the mount operation here in the child */ + r = mount(NULL, m->path, NULL, MS_REMOUNT|MS_RDONLY, options); + if (r < 0) + log_error_errno(errno, "Failed to remount '%s' read-only: %m", m->path); + + _exit(r < 0 ? EXIT_FAILURE : EXIT_SUCCESS); + } + + r = wait_for_terminate_with_timeout(pid, DEFAULT_TIMEOUT_USEC); + if (r == -ETIMEDOUT) { + log_error_errno(errno, "Remounting '%s' - timed out, issuing SIGKILL to PID "PID_FMT".", m->path, pid); + (void) kill(pid, SIGKILL); + } else if (r < 0) + log_error_errno(r, "Failed to wait for process: %m"); + + return r; +} + +static int umount_with_timeout(MountPoint *m, bool *changed) { + pid_t pid; + int r; + + BLOCK_SIGNALS(SIGCHLD); + + /* Due to the possiblity of a umount operation hanging, we + * fork a child process and set a timeout. If the timeout + * lapses, the assumption is that that particular umount + * failed. */ + pid = fork(); + if (pid < 0) + return log_error_errno(errno, "Failed to fork: %m"); + + if (pid == 0) { + log_info("Unmounting '%s'.", m->path); + + /* Start the mount operation here in the child Using MNT_FORCE + * causes some filesystems (e.g. FUSE and NFS and other network + * filesystems) to abort any pending requests and return -EIO + * rather than blocking indefinitely. If the filesysten is + * "busy", this may allow processes to die, thus making the + * filesystem less busy so the unmount might succeed (rather + * then return EBUSY).*/ + r = umount2(m->path, MNT_FORCE); + if (r < 0) + log_error_errno(errno, "Failed to unmount %s: %m", m->path); + + _exit(r < 0 ? EXIT_FAILURE : EXIT_SUCCESS); + } + + r = wait_for_terminate_with_timeout(pid, DEFAULT_TIMEOUT_USEC); + if (r == -ETIMEDOUT) { + log_error_errno(errno, "Unmounting '%s' - timed out, issuing SIGKILL to PID "PID_FMT".", m->path, pid); + (void) kill(pid, SIGKILL); + } else if (r < 0) + log_error_errno(r, "Failed to wait for process: %m"); + + return r; +} + /* This includes remounting readonly, which changes the kernel mount options. * Therefore the list passed to this function is invalidated, and should not be reused. */ -static int mount_points_list_umount(MountPoint **head, bool *changed, bool log_error) { +static int mount_points_list_umount(MountPoint **head, bool *changed) { MountPoint *m; int n_failed = 0; @@ -425,13 +503,15 @@ static int mount_points_list_umount(MountPoint **head, bool *changed, bool log_e * explicitly remount the super block of that * alias read-only we hence should be * relatively safe regarding keeping dirty an fs - * we cannot otherwise see. */ - log_info("Remounting '%s' read-only with options '%s'.", m->path, options); - if (mount(NULL, m->path, NULL, MS_REMOUNT|MS_RDONLY, options) < 0) { - if (log_error) - log_notice_errno(errno, "Failed to remount '%s' read-only: %m", m->path); + * we cannot otherwise see. + * + * Since the remount can hang in the instance of + * remote filesystems, we remount asynchronously + * and skip the subsequent umount if it fails */ + if (remount_with_timeout(m, options, &n_failed) < 0) { if (nonunmountable_path(m->path)) n_failed++; + continue; } } @@ -441,21 +521,12 @@ static int mount_points_list_umount(MountPoint **head, bool *changed, bool log_e if (nonunmountable_path(m->path)) continue; - /* Trying to umount. Using MNT_FORCE causes some - * filesystems (e.g. FUSE and NFS and other network - * filesystems) to abort any pending requests and - * return -EIO rather than blocking indefinitely. - * If the filesysten is "busy", this may allow processes - * to die, thus making the filesystem less busy so - * the unmount might succeed (rather then return EBUSY).*/ - log_info("Unmounting %s.", m->path); - if (umount2(m->path, MNT_FORCE) == 0) { + /* Trying to umount */ + if (umount_with_timeout(m, changed) < 0) + n_failed++; + else { if (changed) *changed = true; - } else { - if (log_error) - log_warning_errno(errno, "Could not unmount %s: %m", m->path); - n_failed++; } } @@ -556,7 +627,7 @@ static int dm_points_list_detach(MountPoint **head, bool *changed) { return n_failed; } -static int umount_all_once(bool *changed, bool log_error) { +static int umount_all_once(bool *changed) { int r; LIST_HEAD(MountPoint, mp_list_head); @@ -565,7 +636,7 @@ static int umount_all_once(bool *changed, bool log_error) { if (r < 0) goto end; - r = mount_points_list_umount(&mp_list_head, changed, log_error); + r = mount_points_list_umount(&mp_list_head, changed); end: mount_points_list_free(&mp_list_head); @@ -577,20 +648,17 @@ int umount_all(bool *changed) { bool umount_changed; int r; - /* retry umount, until nothing can be umounted anymore */ + /* Retry umount, until nothing can be umounted anymore. Mounts are + * processed in order, newest first. The retries are needed when + * an old mount has been moved, to a path inside a newer mount. */ do { umount_changed = false; - umount_all_once(&umount_changed, false); + r = umount_all_once(&umount_changed); if (umount_changed) *changed = true; } while (umount_changed); - /* umount one more time with logging enabled */ - r = umount_all_once(&umount_changed, true); - if (umount_changed) - *changed = true; - return r; } From 73ad712fcfea5d8ba475044698d31d2c15d4180d Mon Sep 17 00:00:00 2001 From: Kyle Walker Date: Thu, 14 Dec 2017 11:46:03 -0500 Subject: [PATCH 2/2] core: Implement sync_with_progress() In similar fashion to the previous change, sync() operations can stall endlessly if cache is unable to be written out. In order to avoid an unbounded hang, the sync takes place within a child process. Every 10 seconds (SYNC_TIMEOUT_USEC), the value of /proc/meminfo "Dirty" is checked to verify it is smaller than the last iteration. If the sync is not making progress for 3 successive iterations (SYNC_PROGRESS_ATTEMPTS), a SIGKILL is sent to the sync process and the shutdown continues. --- src/core/shutdown.c | 111 ++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 107 insertions(+), 4 deletions(-) diff --git a/src/core/shutdown.c b/src/core/shutdown.c index d09ef190a2..aca89d13d1 100644 --- a/src/core/shutdown.c +++ b/src/core/shutdown.c @@ -32,6 +32,7 @@ #include "alloc-util.h" #include "cgroup-util.h" +#include "fd-util.h" #include "def.h" #include "exec-util.h" #include "fileio.h" @@ -40,6 +41,7 @@ #include "missing.h" #include "parse-util.h" #include "process-util.h" +#include "signal-util.h" #include "string-util.h" #include "switch-root.h" #include "terminal-util.h" @@ -50,6 +52,9 @@ #define FINALIZE_ATTEMPTS 50 +#define SYNC_PROGRESS_ATTEMPTS 3 +#define SYNC_TIMEOUT_USEC (10*USEC_PER_SEC) + static char* arg_verb; static uint8_t arg_exit_code; @@ -159,6 +164,103 @@ static int switch_root_initramfs(void) { return switch_root("/run/initramfs", "/oldroot", false, MS_BIND); } +/* Read the following fields from /proc/meminfo: + * + * NFS_Unstable + * Writeback + * Dirty + * + * Return true if the sum of these fields is greater than the previous + * value input. For all other issues, report the failure and indicate that + * the sync is not making progress. + */ +static bool sync_making_progress(unsigned long long *prev_dirty) { + _cleanup_fclose_ FILE *f = NULL; + char line[LINE_MAX]; + bool r = false; + unsigned long long val = 0; + + f = fopen("/proc/meminfo", "re"); + if (!f) + return log_warning_errno(errno, "Failed to open /proc/meminfo: %m"); + + FOREACH_LINE(line, f, log_warning_errno(errno, "Failed to parse /proc/meminfo: %m")) { + unsigned long long ull = 0; + + if (!first_word(line, "NFS_Unstable:") && !first_word(line, "Writeback:") && !first_word(line, "Dirty:")) + continue; + + errno = 0; + if (sscanf(line, "%*s %llu %*s", &ull) != 1) { + if (errno != 0) + log_warning_errno(errno, "Failed to parse /proc/meminfo: %m"); + else + log_warning("Failed to parse /proc/meminfo"); + + return false; + } + + val += ull; + } + + r = *prev_dirty > val; + + *prev_dirty = val; + + return r; +} + +static void sync_with_progress(void) { + unsigned checks; + pid_t pid; + int r; + unsigned long long dirty = ULONG_LONG_MAX; + + BLOCK_SIGNALS(SIGCHLD); + + /* Due to the possiblity of the sync operation hanging, we fork + * a child process and monitor the progress. If the timeout + * lapses, the assumption is that that particular sync stalled. */ + pid = fork(); + if (pid < 0) { + log_error_errno(errno, "Failed to fork: %m"); + return; + } + + if (pid == 0) { + /* Start the sync operation here in the child */ + sync(); + _exit(EXIT_SUCCESS); + } + + log_info("Syncing filesystems and block devices."); + + /* Start monitoring the sync operation. If more than + * SYNC_PROGRESS_ATTEMPTS lapse without progress being made, + * we assume that the sync is stalled */ + for (checks = 0; checks < SYNC_PROGRESS_ATTEMPTS; checks++) { + r = wait_for_terminate_with_timeout(pid, SYNC_TIMEOUT_USEC); + if (r == 0) + /* Sync finished without error. + * (The sync itself does not return an error code) */ + return; + else if (r == -ETIMEDOUT) { + /* Reset the check counter if the "Dirty" value is + * decreasing */ + if (sync_making_progress(&dirty)) + checks = 0; + } else { + log_error_errno(r, "Failed to sync filesystems and block devices: %m"); + return; + } + } + + /* Only reached in the event of a timeout. We should issue a kill + * to the stray process. */ + log_error("Syncing filesystems and block devices - timed out, issuing SIGKILL to PID "PID_FMT".", pid); + (void) kill(pid, SIGKILL); +} + int main(int argc, char *argv[]) { bool need_umount, need_swapoff, need_loop_detach, need_dm_detach; bool in_container, use_watchdog = false; @@ -221,9 +323,10 @@ int main(int argc, char *argv[]) { /* Synchronize everything that is not written to disk yet at this point already. This is a good idea so that * slow IO is processed here already and the final process killing spree is not impacted by processes - * desperately trying to sync IO to disk within their timeout. */ + * desperately trying to sync IO to disk within their timeout. Do not remove this sync, data corruption will + * result. */ if (!in_container) - sync(); + sync_with_progress(); log_info("Sending SIGTERM to remaining processes..."); broadcast_signal(SIGTERM, true, true); @@ -365,9 +468,9 @@ int main(int argc, char *argv[]) { /* The kernel will automatically flush ATA disks and suchlike on reboot(), but the file systems need to be * sync'ed explicitly in advance. So let's do this here, but not needlessly slow down containers. Note that we * sync'ed things already once above, but we did some more work since then which might have caused IO, hence - * let's doit once more. */ + * let's do it once more. Do not remove this sync, data corruption will result. */ if (!in_container) - sync(); + sync_with_progress(); if (streq(arg_verb, "exit")) { if (in_container)