nixos/stc: Improve mount unit handling

We should sometimes restart the units rather than reloading them so the
changes are actually applied. / and /nix are explicitly excluded because
there was some very old issue where these were unmounted. I don't think
this will affect many people since most people use fstab mounts instead
but I plan to adapt this behavior for fstab mounts as well in the future
(once I wrote a test for the fstab thingies).

3 files changed, 51 insertions, 9 deletions

diff --git a/nixos/doc/manual/development/unit-handling.section.md b/nixos/doc/manual/development/unit-handling.section.md
index a7ccb3dbd042d..32d44dbfff054 100644
--- a/nixos/doc/manual/development/unit-handling.section.md
+++ b/nixos/doc/manual/development/unit-handling.section.md
@@ -25,8 +25,11 @@ checks:
     since changes in their values are applied by systemd when systemd is
     reloaded.
 
-  - `.mount` units are **reload**ed. These mostly come from the `/etc/fstab`
-    parser.
+  - `.mount` units are **reload**ed if only their `Options` changed. If anything
+    else changed (like `What`), they are **restart**ed unless they are the mount
+    unit for `/` or `/nix` in which case they are reloaded to prevent the system
+    from crashing. Note that this is the case for `.mount` units and not for
+    mounts from `/etc/fstab`. These are explained in [](#sec-switching-systems).
 
   - `.socket` units are currently ignored. This is to be fixed at a later
     point.
diff --git a/nixos/modules/system/activation/switch-to-configuration.pl b/nixos/modules/system/activation/switch-to-configuration.pl
index 04d90968c4c12..8bd450d7343b2 100755
--- a/nixos/modules/system/activation/switch-to-configuration.pl
+++ b/nixos/modules/system/activation/switch-to-configuration.pl
@@ -313,7 +313,8 @@ sub unrecord_unit {
 # needs to be restarted or reloaded. If the units differ, the service
 # is restarted unless the only difference is `X-Reload-Triggers` in the
 # `Unit` section. If this is the only modification, the unit is reloaded
-# instead of restarted.
+# instead of restarted. If the only difference is `Options` in the
+# `[Mount]` section, the unit is reloaded rather than restarted.
 # Returns:
 # - 0 if the units are equal
 # - 1 if the units are different and a restart action is required
@@ -390,6 +391,11 @@ sub compare_units { ## no critic(Subroutines::ProhibitExcessComplexity)
                         next;
                     }
                 }
+                # If this is a mount unit, check if it was only `Options`
+                if ($section_name eq "Mount" and $ini_key eq "Options") {
+                    $ret = 2;
+                    next;
+                }
                 return 1;
             }
         }
@@ -440,10 +446,18 @@ sub handle_modified_unit { ## no critic(Subroutines::ProhibitManyArgs, Subroutin
         # properties (resource limits and inotify watches)
         # seem to get applied on daemon-reload.
     } elsif ($unit =~ /\.mount$/msx) {
-        # Reload the changed mount unit to force a remount.
-        # FIXME: only reload when Options= changed, restart otherwise
-        $units_to_reload->{$unit} = 1;
-        record_unit($reload_list_file, $unit);
+        # Just restart the unit. We wouldn't have gotten into this subroutine
+        # if only `Options` was changed, in which case the unit would be reloaded.
+        # The only exception is / and /nix because it's very unlikely we can safely
+        # unmount them so we reload them instead. This means that we may not get
+        # all changes into the running system but it's better than crashing it.
+        if ($unit eq "-.mount" or $unit eq "nix.mount") {
+            $units_to_reload->{$unit} = 1;
+            record_unit($reload_list_file, $unit);
+        } else {
+            $units_to_restart->{$unit} = 1;
+            record_unit($restart_list_file, $unit);
+        }
     } elsif ($unit =~ /\.socket$/msx) {
         # FIXME: do something?
         # Attempt to fix this: https://github.com/NixOS/nixpkgs/pull/141192
diff --git a/nixos/tests/switch-test.nix b/nixos/tests/switch-test.nix
index 53595ae7d3e29..529a20864206d 100644
--- a/nixos/tests/switch-test.nix
+++ b/nixos/tests/switch-test.nix
@@ -450,7 +450,7 @@ in {
           ];
         };
 
-        mountModified.configuration = {
+        mountOptionsModified.configuration = {
           systemd.mounts = [
             {
               description = "Testmount";
@@ -463,6 +463,19 @@ in {
           ];
         };
 
+        mountModified.configuration = {
+          systemd.mounts = [
+            {
+              description = "Testmount";
+              what = "ramfs";
+              type = "ramfs";
+              where = "/testmount";
+              options = "size=10M";
+              wantedBy = [ "local-fs.target" ];
+            }
+          ];
+        };
+
         timer.configuration = {
           systemd.timers.test-timer = {
             wantedBy = [ "timers.target" ];
@@ -1137,7 +1150,8 @@ in {
         switch_to_specialisation("${machine}", "mount")
         out = machine.succeed("mount | grep 'on /testmount'")
         assert_contains(out, "size=1024k")
-        out = switch_to_specialisation("${machine}", "mountModified")
+        # Changing options reloads the unit
+        out = switch_to_specialisation("${machine}", "mountOptionsModified")
         assert_lacks(out, "stopping the following units:")
         assert_lacks(out, "NOT restarting the following changed units:")
         assert_contains(out, "reloading the following units: testmount.mount\n")
@@ -1147,6 +1161,17 @@ in {
         # It changed
         out = machine.succeed("mount | grep 'on /testmount'")
         assert_contains(out, "size=10240k")
+        # Changing anything but `Options=` restarts the unit
+        out = switch_to_specialisation("${machine}", "mountModified")
+        assert_lacks(out, "stopping the following units:")
+        assert_lacks(out, "NOT restarting the following changed units:")
+        assert_lacks(out, "reloading the following units:")
+        assert_contains(out, "\nrestarting the following units: testmount.mount\n")
+        assert_lacks(out, "\nstarting the following units:")
+        assert_lacks(out, "the following new units were started:")
+        # It changed
+        out = machine.succeed("mount | grep 'on /testmount'")
+        assert_contains(out, "ramfs")
 
     with subtest("timers"):
         switch_to_specialisation("${machine}", "timer")