From: Dave Reisner <dreisner@archlinux.org>
Date: Thu, 11 Jun 2020 14:34:13 +0000 (-0400)
Subject: Revert "job: Don't mark as redundant if deps are relevant"
X-Git-Tag: v246-rc1~118
X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=cc479760b4736082d26ec332f2423a9ab23d59c5;p=thirdparty%2Fsystemd.git

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

This reverts commit 097537f07a2fab3cb73aef7bc59f2a66aa93f533.

At least Fedora and Debian have already reverted this at the distro
level because it causes more problems than it solves. Arch is debating
reverting it as well [0] but would strongly prefer that this happens
upstream first. Fixes #15188.

[0] https://bugs.archlinux.org/task/66458
---

diff --git a/src/core/job.c b/src/core/job.c
index d518ac89693..ff49136d703 100644
--- a/src/core/job.c
+++ b/src/core/job.c
@@ -387,62 +387,25 @@ JobType job_type_lookup_merge(JobType a, JobType b) {
         return job_merging_table[(a - 1) * a / 2 + b];
 }
 
-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) {
+bool job_type_is_redundant(JobType a, UnitActiveState b) {
+        switch (a) {
 
         case JOB_START:
-                return IN_SET(state, UNIT_ACTIVE, UNIT_RELOADING) && !job_later_link_matters(j, JOB_START, generation);
+                return IN_SET(b, UNIT_ACTIVE, UNIT_RELOADING);
 
         case JOB_STOP:
-                return IN_SET(state, UNIT_INACTIVE, UNIT_FAILED) && !job_later_link_matters(j, JOB_STOP, generation);
+                return IN_SET(b, UNIT_INACTIVE, UNIT_FAILED);
 
         case JOB_VERIFY_ACTIVE:
-                return IN_SET(state, UNIT_ACTIVE, UNIT_RELOADING);
+                return IN_SET(b, UNIT_ACTIVE, UNIT_RELOADING);
 
         case JOB_RELOAD:
                 return
-                        state == UNIT_RELOADING;
+                        b == UNIT_RELOADING;
 
         case JOB_RESTART:
                 return
-                        state == UNIT_ACTIVATING;
+                        b == UNIT_ACTIVATING;
 
         case JOB_NOP:
                 return true;
diff --git a/src/core/job.h b/src/core/job.h
index 02b057ee064..03ad640618e 100644
--- a/src/core/job.h
+++ b/src/core/job.h
@@ -196,8 +196,7 @@ _pure_ static inline bool job_type_is_superset(JobType a, JobType b) {
         return a == job_type_lookup_merge(a, b);
 }
 
-bool job_later_link_matters(Job *j, JobType type, unsigned generation);
-bool job_is_redundant(Job *j, unsigned generation);
+bool job_type_is_redundant(JobType a, UnitActiveState b) _pure_;
 
 /* 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 0fb52372f16..ef6470656ce 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, unsigned generation) {
+static void transaction_drop_redundant(Transaction *tr) {
         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, unsigned generation) {
 
                         LIST_FOREACH(transaction, k, j)
                                 if (tr->anchor_job == k ||
-                                    !job_is_redundant(k, generation) ||
+                                    !job_type_is_redundant(k->type, unit_active_state(k->unit)) ||
                                     (k->unit->job && job_type_is_conflicting(k->type, k->unit->job->type))) {
                                         keep = true;
                                         break;
@@ -735,7 +735,7 @@ int transaction_activate(
                 transaction_minimize_impact(tr);
 
         /* Third step: Drop redundant jobs */
-        transaction_drop_redundant(tr, generation++);
+        transaction_drop_redundant(tr);
 
         for (;;) {
                 /* Fourth step: Let's remove unneeded jobs that might
@@ -777,7 +777,7 @@ int transaction_activate(
         }
 
         /* Eights step: Drop redundant jobs again, if the merging now allows us to drop more. */
-        transaction_drop_redundant(tr, generation++);
+        transaction_drop_redundant(tr);
 
         /* Ninth step: check whether we can actually apply this */
         r = transaction_is_destructive(tr, mode, e);