From: Alan Jenkins <alan.christopher.jenkins@gmail.com>
Date: Thu, 19 Oct 2017 14:02:12 +0000 (+0100)
Subject: core: systemd-shutdown: avoid confusingly redundant messages
X-Git-Tag: v236~292^2
X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=refs%2Fpull%2F7142%2Fhead;p=thirdparty%2Fsystemd.git

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).
---

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;
 }