Merge ~slyon/ubuntu/+source/systemd:slyon/lp1949089-fail-mount-uc18 into ~ubuntu-core-dev/ubuntu/+source/systemd:ubuntu-bionic

[Lukas Märdian]

[Dan Streetman (ddstreet)]

Sorry, this is a NAK from me - the patches fix an important bug that we don't want to reintroduce. Instead let's figure out what the new problem is with snapd's mount units and fix that properly.

review: Disapprove

Unmerged commits

cbf7a31... by Lukas Märdian on 2021-11-08

update changelog

35bfac7... by Lukas Märdian on 2021-11-05

Revert "Catch up on cgroup empty inotify after reexec/reload"

This reverts commit 5ef61bd930612a90ce3ed9105cbadc5ff97b6ffc.

diff --git a/debian/changelog b/debian/changelog
index 5e2e2aa..4ca8597 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,16 @@
+systemd (237-3ubuntu10.53) bionic; urgency=medium
+
+  * Fix activation of mount units in UC18 (LP: #1949089)
+    Revert "Catch up on cgroup empty inotify after reexec/reload"
+    This reverts commit 5ef61bd930612a90ce3ed9105cbadc5ff97b6ffc.
+    Files:
+    - debian/patches/lp1934147/0001-core-add-a-new-unit-method-catchup.patch
+    - debian/patches/lp1934147/0002-cgroup-do-catchup-for-unit-cgroup-inotify-watch-file.patch
+    - debian/patches/lp1934147/0003-core-Make-sure-cgroup_oom_queue-is-flushed-on-manage.patch
+    https://git.launchpad.net/~ubuntu-core-dev/ubuntu/+source/systemd/commit/?id=35bfac79163f39979267724644d3f725f8e9f12c
+
+ -- Lukas Märdian <[e-mail]>  Mon, 08 Nov 2021 16:28:03 +0100
+
 systemd (237-3ubuntu10.52) bionic; urgency=medium
 
   * d/extra/dhclient-enter-resolved-hook:
diff --git a/debian/patches/lp1934147/0001-core-add-a-new-unit-method-catchup.patch b/debian/patches/lp1934147/0001-core-add-a-new-unit-method-catchup.patch
deleted file mode 100644
index 487588d..0000000
--- a/debian/patches/lp1934147/0001-core-add-a-new-unit-method-catchup.patch
+++ /dev/null
@@ -1,158 +0,0 @@
-From f0831ed2a03fcef582660be1c3b1a9f3e267e656 Mon Sep 17 00:00:00 2001
-From: Lennart Poettering <[e-mail]>
-Date: Tue, 5 Jun 2018 16:53:22 +0200
-Subject: [PATCH] core: add a new unit method "catchup()"
-Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/systemd/+bug/1934147
-Origin: upstream, https://github.com/systemd/systemd/commit/f0831ed2a03fcef582660be1c3b1a9f3e267e656
-
-This is very similar to the existing unit method coldplug() but is
-called a bit later. The idea is that that coldplug() restores the unit
-state from before any prior reload/restart, i.e. puts the deserialized
-state in effect. The catchup() call is then called a bit later, to
-catch up with the system state for which we missed notifications while
-we were reloading. This is only really useful for mount, swap and device
-mount points were we should be careful to generate all missing unit
-state change events (i.e. call unit_notify() appropriately) for
-everything that happened while we were reloading.
----
- src/core/manager.c | 30 +++++++++++++++++++++++++++++-
- src/core/unit.c    | 11 ++++++++---
- src/core/unit.h    | 17 ++++++++++-------
- 3 files changed, 47 insertions(+), 11 deletions(-)
-
---- a/src/core/manager.c
-+++ b/src/core/manager.c
-@@ -1290,7 +1290,9 @@ static void manager_coldplug(Manager *m)
- 
-         assert(m);
- 
--        /* Then, let's set up their initial state. */
-+        log_debug("Invoking unit coldplug() handlers…");
-+
-+        /* Let's place the units back into their deserialized state */
-         HASHMAP_FOREACH_KEY(u, k, m->units, i) {
- 
-                 /* ignore aliases */
-@@ -1303,6 +1305,26 @@ static void manager_coldplug(Manager *m)
-         }
- }
- 
-+static void manager_catchup(Manager *m) {
-+        Iterator i;
-+        Unit *u;
-+        char *k;
-+
-+        assert(m);
-+
-+        log_debug("Invoking unit catchup() handlers…");
-+
-+        /* Let's catch up on any state changes that happened while we were reloading/reexecing */
-+        HASHMAP_FOREACH_KEY(u, k, m->units, i) {
-+
-+                /* ignore aliases */
-+                if (u->id != k)
-+                        continue;
-+
-+                unit_catchup(u);
-+        }
-+}
-+
- static void manager_build_unit_path_cache(Manager *m) {
-         char **i;
-         int r;
-@@ -1477,6 +1499,9 @@ int manager_startup(Manager *m, FILE *se
-                 m->send_reloading_done = true;
-         }
- 
-+        /* Let's finally catch up with any changes that took place while we were reloading/reexecing */
-+        manager_catchup(m);
-+
-         return 0;
- }
- 
-@@ -3139,6 +3164,9 @@ int manager_reload(Manager *m) {
-         /* It might be safe to log to the journal now. */
-         manager_recheck_journal(m);
- 
-+        /* Let's finally catch up with any changes that took place while we were reloading/reexecing */
-+        manager_catchup(m);
-+
-         /* Sync current state of bus names with our set of listening units */
-         if (m->api_bus)
-                 manager_sync_bus_names(m, m->api_bus);
---- a/src/core/unit.c
-+++ b/src/core/unit.c
-@@ -2331,7 +2331,6 @@ static void unit_update_on_console(Unit
-                 manager_ref_console(u->manager);
-         else
-                 manager_unref_console(u->manager);
--
- }
- 
- static bool unit_process_job(Job *j, UnitActiveState ns, UnitNotifyFlags flags) {
-@@ -3788,8 +3787,7 @@ int unit_coldplug(Unit *u) {
- 
-         assert(u);
- 
--        /* Make sure we don't enter a loop, when coldplugging
--         * recursively. */
-+        /* Make sure we don't enter a loop, when coldplugging recursively. */
-         if (u->coldplugged)
-                 return 0;
- 
-@@ -3817,6 +3815,13 @@ int unit_coldplug(Unit *u) {
-         return r;
- }
- 
-+void unit_catchup(Unit *u) {
-+        assert(u);
-+
-+        if (UNIT_VTABLE(u)->catchup)
-+                UNIT_VTABLE(u)->catchup(u);
-+}
-+
- static bool fragment_mtime_newer(const char *path, usec_t mtime, bool path_masked) {
-         struct stat st;
- 
---- a/src/core/unit.h
-+++ b/src/core/unit.h
-@@ -454,10 +454,14 @@ struct UnitVTable {
-          * UNIT_STUB if no configuration could be found. */
-         int (*load)(Unit *u);
- 
--        /* If a lot of units got created via enumerate(), this is
--         * where to actually set the state and call unit_notify(). */
-+        /* During deserialization we only record the intended state to return to. With coldplug() we actually put the
-+         * deserialized state in effect. This is where unit_notify() should be called to start things up. */
-         int (*coldplug)(Unit *u);
- 
-+        /* This is called shortly after all units' coldplug() call was invoked. It's supposed to catch up state changes
-+         * we missed so far (for example because they took place while we were reloading/reexecing) */
-+        void (*catchup)(Unit *u);
-+
-         void (*dump)(Unit *u, FILE *f, const char *prefix);
- 
-         int (*start)(Unit *u);
-@@ -546,11 +550,9 @@ struct UnitVTable {
-         /* Returns true if the unit currently needs access to the console */
-         bool (*needs_console)(Unit *u);
- 
--        /* This is called for each unit type and should be used to
--         * enumerate existing devices and load them. However,
--         * everything that is loaded here should still stay in
--         * inactive state. It is the job of the coldplug() call above
--         * to put the units into the initial state.  */
-+        /* This is called for each unit type and should be used to enumerate units already existing in the system
-+         * internally and load them. However, everything that is loaded here should still stay in inactive state. It is
-+         * the job of the coldplug() call above to put the units into the initial state.  */
-         void (*enumerate)(Manager *m);
- 
-         /* Type specific cleanups. */
-@@ -705,6 +707,7 @@ void unit_serialize_item_format(Unit *u,
- int unit_add_node_dependency(Unit *u, const char *what, bool wants, UnitDependency d, UnitDependencyMask mask);
- 
- int unit_coldplug(Unit *u);
-+void unit_catchup(Unit *u);
- 
- void unit_status_printf(Unit *u, const char *status, const char *unit_status_msg_format) _printf_(3, 0);
- void unit_status_emit_starting_stopping_reloading(Unit *u, JobType t);
diff --git a/debian/patches/lp1934147/0002-cgroup-do-catchup-for-unit-cgroup-inotify-watch-file.patch b/debian/patches/lp1934147/0002-cgroup-do-catchup-for-unit-cgroup-inotify-watch-file.patch
deleted file mode 100644
index b9e4c43..0000000
--- a/debian/patches/lp1934147/0002-cgroup-do-catchup-for-unit-cgroup-inotify-watch-file.patch
+++ /dev/null
@@ -1,59 +0,0 @@
-From 869f52f21831b611160c4937bef822ca94c802ba Mon Sep 17 00:00:00 2001
-From: Dan Streetman <[e-mail]>
-Date: Sun, 11 Jul 2021 16:59:27 -0400
-Subject: [PATCH 1/2] cgroup: do 'catchup' for unit cgroup inotify watch files
-Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/systemd/+bug/1934147
-Origin: upstream, https://github.com/systemd/systemd/pull/20199
-
-While reexec/reload, we drop the inotify watch on cgroup file(s), so
-we need to re-check them in case they changed and we missed the event.
-
-Fixes: #20198
----
- src/core/cgroup.c | 18 ++++++++++++++++++
- src/core/cgroup.h |  2 ++
- src/core/unit.c   |  2 ++
- 3 files changed, 22 insertions(+)
-
---- a/src/core/cgroup.c
-+++ b/src/core/cgroup.c
-@@ -2563,6 +2563,20 @@ void unit_invalidate_cgroup_bpf(Unit *u)
-         }
- }
- 
-+void unit_cgroup_catchup(Unit *u) {
-+        assert(u);
-+
-+        if (!UNIT_HAS_CGROUP_CONTEXT(u))
-+                return;
-+
-+        /* We dropped the inotify watch during reexec/reload, so we need to
-+         * check these as they may have changed.
-+         * Note that (currently) the kernel doesn't actually update cgroup
-+         * file modification times, so we can't just serialize and then check
-+         * the mtime for file(s) we are interested in. */
-+        unit_add_to_cgroup_empty_queue(u);
-+}
-+
- void manager_invalidate_startup_units(Manager *m) {
-         Iterator i;
-         Unit *u;
---- a/src/core/cgroup.h
-+++ b/src/core/cgroup.h
-@@ -219,3 +219,5 @@ void manager_invalidate_startup_units(Ma
- 
- const char* cgroup_device_policy_to_string(CGroupDevicePolicy i) _const_;
- CGroupDevicePolicy cgroup_device_policy_from_string(const char *s) _pure_;
-+
-+void unit_cgroup_catchup(Unit *u);
---- a/src/core/unit.c
-+++ b/src/core/unit.c
-@@ -3820,6 +3820,8 @@ void unit_catchup(Unit *u) {
- 
-         if (UNIT_VTABLE(u)->catchup)
-                 UNIT_VTABLE(u)->catchup(u);
-+
-+        unit_cgroup_catchup(u);
- }
- 
- static bool fragment_mtime_newer(const char *path, usec_t mtime, bool path_masked) {
diff --git a/debian/patches/lp1934147/0003-core-Make-sure-cgroup_oom_queue-is-flushed-on-manage.patch b/debian/patches/lp1934147/0003-core-Make-sure-cgroup_oom_queue-is-flushed-on-manage.patch
deleted file mode 100644
index 17c2cfa..0000000
--- a/debian/patches/lp1934147/0003-core-Make-sure-cgroup_oom_queue-is-flushed-on-manage.patch
+++ /dev/null
@@ -1,32 +0,0 @@
-From 13e721036bf4ba15eb255d8f0a14800f969ac0d7 Mon Sep 17 00:00:00 2001
-From: =?UTF-8?q?Michal=20Koutn=C3=BD?= <[e-mail]>
-Date: Wed, 4 Aug 2021 18:59:35 +0200
-Subject: [PATCH 2/2] core: Make sure cgroup_oom_queue is flushed on manager
- exit
-Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/systemd/+bug/1934147
-Origin: upstream, https://github.com/systemd/systemd/pull/20199
-
-The unit queues are not serialized/deserialized (they are recreated
-after reexec/reload instead). The destroyed units are not removed from
-the cgroup_oom_queue. That means the queue may contain possibly invalid
-pointers to released units.
-
-Fix this by removing the units from cgroup_oom_queue as we do for
-others. When at it, sync assert checks with currently existing queues
-and put them in order in the manager cleanup code.
----
- src/core/manager.c | 4 ++++
- src/core/unit.c    | 7 +++++--
- 2 files changed, 9 insertions(+), 2 deletions(-)
-
---- a/src/core/manager.c
-+++ b/src/core/manager.c
-@@ -1171,6 +1171,8 @@ static void manager_clear_jobs_and_units
-         assert(!m->cleanup_queue);
-         assert(!m->gc_unit_queue);
-         assert(!m->gc_job_queue);
-+        assert(!m->cgroup_realize_queue);
-+        assert(!m->cgroup_empty_queue);
- 
-         assert(hashmap_isempty(m->jobs));
-         assert(hashmap_isempty(m->units));
diff --git a/debian/patches/series b/debian/patches/series
index 093231f..e4a9aec 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -259,7 +259,4 @@ lp1815101-0004-networkd-stop-clients-when-networkd-shuts-down.patch
 lp1815101-0005-network-add-KeepConfiguration-dhcp-on-stop.patch
 lp1815101-0006-network-make-KeepConfiguration-static-drop-DHCP-addr.patch
 lp1815101-0007-man-add-documentation-about-KeepConfiguration.patch
-lp1934147/0001-core-add-a-new-unit-method-catchup.patch
-lp1934147/0002-cgroup-do-catchup-for-unit-cgroup-inotify-watch-file.patch
-lp1934147/0003-core-Make-sure-cgroup_oom_queue-is-flushed-on-manage.patch
 lp1934981-correct-suspend-then-sleep-string.patch