[systemd-devel] [PATCHv2] core: do not spawn jobs or touch other units during coldplugging

[Ivan Shapovalov]

On 2015-04-25 at 04:00 +0300, Ivan Shapovalov wrote:
> On 2015-04-24 at 16:04 +0200, Lennart Poettering wrote:
> > [...]
> > 
> > Actually, it really is about the UNIT_TRIGGERS dependencies only,
> > since we don't do the retroactive deps stuff at all when we are
> > coldplugging, it's conditionalized in m->n_reloading <= 0.
> 
> So, I think I understand the problem. We should do this not only for
> UNIT_TRIGGERS, but also for any dependencies which may matter
> when activating that unit. That is, anything which is referenced by
> transaction_add_job_and_dependencies()... recursively.

Here is what I have in mind. Don't know whether this is correct, but
it fixes the problem for me.

From 515d878e526e52fc154874e93a4c97555ebd8cff Mon Sep 17 00:00:00 2001
From: Ivan Shapovalov <intelfx100 at gmail.com>
Date: Sat, 25 Apr 2015 04:57:59 +0300
Subject: [PATCH] core: coldplug all units which participate in jobs

This is yet another attempt to fix coldplugging order (more especially,
the problem which happens when one creates a job during coldplugging, and
it references a not-yet-coldplugged unit).

Now we forcibly coldplug all units which participate in jobs. This
is a superset of previously implemented handling of the UNIT_TRIGGERS
dependencies, so that handling is removed.
---
 src/core/transaction.c | 6 ++++++
 src/core/unit.c        | 8 --------
 2 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/src/core/transaction.c b/src/core/transaction.c
index 5974b1e..a02c02c 100644
--- a/src/core/transaction.c
+++ b/src/core/transaction.c
@@ -848,6 +848,12 @@ int transaction_add_job_and_dependencies(
         assert(type < _JOB_TYPE_MAX_IN_TRANSACTION);
         assert(unit);
 
+        /* Before adding jobs for this unit, let's ensure that its state has been loaded.
+         * This matters when jobs are spawned as part of coldplugging itself (see. e. g. path_coldplug().
+         * This way, we "recursively" coldplug units, ensuring that we do not look at state of
+         * not-yet-coldplugged units. */
+        unit_coldplug(unit);
+
         /* log_debug("Pulling in %s/%s from %s/%s", */
         /*           unit->id, job_type_to_string(type), */
         /*           by ? by->unit->id : "NA", */
diff --git a/src/core/unit.c b/src/core/unit.c
index 2b356e2..996b648 100644
--- a/src/core/unit.c
+++ b/src/core/unit.c
@@ -2889,14 +2889,6 @@ int unit_coldplug(Unit *u) {
 
         u->coldplugged = true;
 
-        /* Make sure everything that we might pull in through
-         * triggering is coldplugged before us */
-        SET_FOREACH(other, u->dependencies[UNIT_TRIGGERS], i) {
-                r = unit_coldplug(other);
-                if (r < 0)
-                        return r;
-        }
-
         if (UNIT_VTABLE(u)->coldplug) {
                 r = UNIT_VTABLE(u)->coldplug(u);
                 if (r < 0)
-- 
2.3.6

-- 
Ivan Shapovalov / intelfx /
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 213 bytes
Desc: This is a digitally signed message part
URL: <http://lists.freedesktop.org/archives/systemd-devel/attachments/20150425/8cc21567/attachment-0001.sig>