From: Kevin Kuehler <keur@xcf.berkeley.edu>
Date: Tue, 19 Nov 2019 21:43:58 +0000 (-0800)
Subject: job: Don't mark as redundant if deps are relevant
X-Git-Tag: v245-rc1~196
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=097537f07a2fab3cb73aef7bc59f2a66aa93f533;p=thirdparty%2Fsystemd.git

job: Don't mark as redundant if deps are relevant

In the steps given in #13850, the resulting graph looks like:

    C (Anchor) -> B -> A

Since B is inactive, it will be flagged as redundant and removed from
the transaction, causing A to get garbage collected. The proposed fix is
to not mark nodes as redundant if doing so causes a relevant node to be
garbage collected.

Fixes #13850
---

diff --git a/src/core/job.c b/src/core/job.c
index 5048a5093e7..5982404cf0c 100644
--- a/src/core/job.c
+++ b/src/core/job.c
@@ -383,25 +383,62 @@ JobType job_type_lookup_merge(JobType a, JobType b) {
         return job_merging_table[(a - 1) * a / 2 + b];
 }
 
-bool job_type_is_redundant(JobType a, UnitActiveState b) {
-        switch (a) {
+bool job_later_link_matters(Job *j, JobType type, unsigned generation) {
+        JobDependency *l;
+
+        assert(j);
+
+        j->generation = generation;
+
+        LIST_FOREACH(subject, l, j->subject_list) {
+                UnitActiveState state = _UNIT_ACTIVE_STATE_INVALID;
+
+                /* Have we seen this before? */
+                if (l->object->generation == generation)
+                        continue;
+
+                state = unit_active_state(l->object->unit);
+                switch (type) {
+
+                case JOB_START:
+                        return IN_SET(state, UNIT_INACTIVE, UNIT_FAILED) ||
+                               job_later_link_matters(l->object, type, generation);
+
+                case JOB_STOP:
+                        return IN_SET(state, UNIT_ACTIVE, UNIT_RELOADING) ||
+                               job_later_link_matters(l->object, type, generation);
+
+                default:
+                        assert_not_reached("Invalid job type");
+                }
+        }
+
+        return false;
+}
+
+bool job_is_redundant(Job *j, unsigned generation) {
+
+        assert(j);
+
+        UnitActiveState state = unit_active_state(j->unit);
+        switch (j->type) {
 
         case JOB_START:
-                return IN_SET(b, UNIT_ACTIVE, UNIT_RELOADING);
+                return IN_SET(state, UNIT_ACTIVE, UNIT_RELOADING) && !job_later_link_matters(j, JOB_START, generation);
 
         case JOB_STOP:
-                return IN_SET(b, UNIT_INACTIVE, UNIT_FAILED);
+                return IN_SET(state, UNIT_INACTIVE, UNIT_FAILED) && !job_later_link_matters(j, JOB_STOP, generation);
 
         case JOB_VERIFY_ACTIVE:
-                return IN_SET(b, UNIT_ACTIVE, UNIT_RELOADING);
+                return IN_SET(state, UNIT_ACTIVE, UNIT_RELOADING);
 
         case JOB_RELOAD:
                 return
-                        b == UNIT_RELOADING;
+                        state == UNIT_RELOADING;
 
         case JOB_RESTART:
                 return
-                        b == UNIT_ACTIVATING;
+                        state == UNIT_ACTIVATING;
 
         case JOB_NOP:
                 return true;
diff --git a/src/core/job.h b/src/core/job.h
index 03ad640618e..02b057ee064 100644
--- a/src/core/job.h
+++ b/src/core/job.h
@@ -196,7 +196,8 @@ _pure_ static inline bool job_type_is_superset(JobType a, JobType b) {
         return a == job_type_lookup_merge(a, b);
 }
 
-bool job_type_is_redundant(JobType a, UnitActiveState b) _pure_;
+bool job_later_link_matters(Job *j, JobType type, unsigned generation);
+bool job_is_redundant(Job *j, unsigned generation);
 
 /* Collapses a state-dependent job type into a simpler type by observing
  * the state of the unit which it is going to be applied to. */
diff --git a/src/core/transaction.c b/src/core/transaction.c
index a0ea0f04897..8d67f9ce1ab 100644
--- a/src/core/transaction.c
+++ b/src/core/transaction.c
@@ -279,7 +279,7 @@ static int transaction_merge_jobs(Transaction *tr, sd_bus_error *e) {
         return 0;
 }
 
-static void transaction_drop_redundant(Transaction *tr) {
+static void transaction_drop_redundant(Transaction *tr, unsigned generation) {
         bool again;
 
         /* Goes through the transaction and removes all jobs of the units whose jobs are all noops. If not
@@ -299,7 +299,7 @@ static void transaction_drop_redundant(Transaction *tr) {
 
                         LIST_FOREACH(transaction, k, j)
                                 if (tr->anchor_job == k ||
-                                    !job_type_is_redundant(k->type, unit_active_state(k->unit)) ||
+                                    !job_is_redundant(k, generation) ||
                                     (k->unit->job && job_type_is_conflicting(k->type, k->unit->job->type))) {
                                         keep = true;
                                         break;
@@ -730,7 +730,7 @@ int transaction_activate(
                 transaction_minimize_impact(tr);
 
         /* Third step: Drop redundant jobs */
-        transaction_drop_redundant(tr);
+        transaction_drop_redundant(tr, generation++);
 
         for (;;) {
                 /* Fourth step: Let's remove unneeded jobs that might
@@ -772,7 +772,7 @@ int transaction_activate(
         }
 
         /* Eights step: Drop redundant jobs again, if the merging now allows us to drop more. */
-        transaction_drop_redundant(tr);
+        transaction_drop_redundant(tr, generation++);
 
         /* Ninth step: check whether we can actually apply this */
         r = transaction_is_destructive(tr, mode, e);