From 066b753dc5fefd0bfef92e28d3492667e2ffe22b Mon Sep 17 00:00:00 2001 From: Alan Jenkins Date: Thu, 19 Oct 2017 15:00:57 +0100 Subject: [PATCH 1/2] core: systemd-shutdown: add missing check for umount_changed The assumption was that nothing changes in the final attempt. This would be confusing if a filesystem with a process in uninterruptible sleep suddenly became un-stuck for the final attempt, but we still give up and don't try to e.g. unmount any parent mounts. I don't know how possible that is. But the code will be easier to read without an assumption that it does not attempt to justify. --- src/core/umount.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/core/umount.c b/src/core/umount.c index 813d2571392..d8a9c8ca4f2 100644 --- a/src/core/umount.c +++ b/src/core/umount.c @@ -572,6 +572,8 @@ int umount_all(bool *changed) { /* umount one more time with logging enabled */ r = mount_points_list_umount(&mp_list_head, &umount_changed, true); + if (umount_changed) + *changed = true; end: mount_points_list_free(&mp_list_head); From 116e6d964380213bb52b49d5404c3b9280911e17 Mon Sep 17 00:00:00 2001 From: Alan Jenkins Date: Thu, 19 Oct 2017 15:02:12 +0100 Subject: [PATCH 2/2] core: systemd-shutdown: avoid confusingly redundant messages After previous output from systemd-shutdown indicated a bug, my attention was drawn to redundant output lines. Did they indicate an anomaly? It turns out to be an expected, harmless result of the current code. But we don't have much justification to run such redundant operations. Let's remove the confusing redundant message. We can stop trying to remount a directory read-only once its mount entry has successfully been changed to "ro". We can simply let the kernel keep track of this for us. I don't bother to try and avoid re-parsing the mountinfo. I appreciate snappy shutdowns, but this code is already intricate and buggy enough (see issue 7131). (Disclaimer: At least for the moment, you can't _rely_ on always seeing suspicious output from systemd-shutdown. By default, you can expect the kernel to truncate the log output of systemd-shutdown. Ick ick ick! Because /dev/kmsg is rate-limited by default. Normally it prints a message "X lines supressed", but we tend to shut down before the timer expires in this case). Before: systemd-shutdown[1]: Remounting '/' read-only with options 'seclabel... EXT4-fs (vda3): re-mounted. Opts: data=ordered systemd-shutdown[1]: Remounting '/' read-only with options 'seclabel, ... EXT4-fs (vda3): re-mounted. Opts: data=ordered After: systemd-shutdown[1]: Remounting '/' read-only with options 'seclabel, ... EXT4-fs (vda3): re-mounted. Opts: data=ordered I also tested with `systemctl reboot --force`, plus a loopback mount to cause one of the umounts to fail initially. In this case another 2 lines of output are removed (out of a larger number of lines). --- src/core/umount.c | 38 +++++++++++++++++++++++++------------- 1 file changed, 25 insertions(+), 13 deletions(-) diff --git a/src/core/umount.c b/src/core/umount.c index d8a9c8ca4f2..8f6a0fe4f3f 100644 --- a/src/core/umount.c +++ b/src/core/umount.c @@ -381,13 +381,19 @@ static bool nonunmountable_path(const char *path) { || path_startswith(path, "/run/initramfs"); } +/* 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) { - MountPoint *m, *n; + MountPoint *m; int n_failed = 0; assert(head); - LIST_FOREACH_SAFE(mount_point, m, n, *head) { + LIST_FOREACH(mount_point, m, *head) { + bool mount_is_readonly; + + mount_is_readonly = fstab_test_yes_no_option(m->options, "ro\0rw\0"); /* If we are in a container, don't attempt to read-only mount anything as that brings no real @@ -397,7 +403,8 @@ static int mount_points_list_umount(MountPoint **head, bool *changed, bool log_e remount. It brings no value (we cannot leave a "dirty fs") and could hang if the network is down. */ if (detect_container() <= 0 && - !fstype_is_network(m->type)) { + !fstype_is_network(m->type) && + !mount_is_readonly) { _cleanup_free_ char *options = NULL; /* MS_REMOUNT requires that the data parameter * should be the same from the original mount @@ -444,8 +451,6 @@ static int mount_points_list_umount(MountPoint **head, bool *changed, bool log_e if (umount2(m->path, 0) == 0) { if (changed) *changed = true; - - mount_point_free(head, m); } else { if (log_error) log_warning_errno(errno, "Could not unmount %s: %m", m->path); @@ -550,9 +555,8 @@ static int dm_points_list_detach(MountPoint **head, bool *changed) { return n_failed; } -int umount_all(bool *changed) { +static int umount_all_once(bool *changed, bool log_error) { int r; - bool umount_changed; LIST_HEAD(MountPoint, mp_list_head); LIST_HEAD_INIT(mp_list_head); @@ -560,24 +564,32 @@ int umount_all(bool *changed) { if (r < 0) goto end; + r = mount_points_list_umount(&mp_list_head, changed, log_error); + + end: + mount_points_list_free(&mp_list_head); + + return r; +} + +int umount_all(bool *changed) { + bool umount_changed; + int r; + /* retry umount, until nothing can be umounted anymore */ do { umount_changed = false; - mount_points_list_umount(&mp_list_head, &umount_changed, false); + umount_all_once(&umount_changed, false); if (umount_changed) *changed = true; - } while (umount_changed); /* umount one more time with logging enabled */ - r = mount_points_list_umount(&mp_list_head, &umount_changed, true); + r = umount_all_once(&umount_changed, true); if (umount_changed) *changed = true; - end: - mount_points_list_free(&mp_list_head); - return r; }