From 9c3349e23b14db27e7ba45f82cf647899c563ea9 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 5 Jan 2015 17:22:10 +0100 Subject: [PATCH] core: rework counting of running jobs Let's unify the code that counts the running jobs a bit, in order to make sure we are less likely to miss one. This is related to this bug: https://bugs.freedesktop.org/show_bug.cgi?id=87349 However, it probably won't fix it fully, and I cannot reproduce the issue. The change also adds an explicit assert change when the counter is off. --- src/core/job.c | 61 +++++++++++++++++++++++++++++++++------------- src/core/manager.c | 8 ++---- src/core/unit.c | 11 +++++---- 3 files changed, 52 insertions(+), 28 deletions(-) diff --git a/src/core/job.c b/src/core/job.c index a3ba7cf70..2129773ed 100644 --- a/src/core/job.c +++ b/src/core/job.c @@ -96,11 +96,39 @@ void job_free(Job *j) { free(j); } +static void job_set_state(Job *j, JobState state) { + assert(j); + assert(state >= 0); + assert(state < _JOB_STATE_MAX); + + if (j->state == state) + return; + + j->state = state; + + if (!j->installed) + return; + + if (j->state == JOB_RUNNING) + j->unit->manager->n_running_jobs++; + else { + assert(j->state == JOB_WAITING); + assert(j->unit->manager->n_running_jobs > 0); + + j->unit->manager->n_running_jobs--; + + if (j->unit->manager->n_running_jobs <= 0) + j->unit->manager->jobs_in_progress_event_source = sd_event_source_unref(j->unit->manager->jobs_in_progress_event_source); + } +} + void job_uninstall(Job *j) { Job **pj; assert(j->installed); + job_set_state(j, JOB_WAITING); + pj = (j->type == JOB_NOP) ? &j->unit->nop_job : &j->unit->job; assert(*pj == j); @@ -155,6 +183,7 @@ Job* job_install(Job *j) { assert(!j->installed); assert(j->type < _JOB_TYPE_MAX_IN_TRANSACTION); + assert(j->state == JOB_WAITING); pj = (j->type == JOB_NOP) ? &j->unit->nop_job : &j->unit->job; uj = *pj; @@ -181,8 +210,8 @@ Job* job_install(Job *j) { log_unit_debug(uj->unit->id, "Merged into running job, re-running: %s/%s as %u", uj->unit->id, job_type_to_string(uj->type), (unsigned) uj->id); - uj->state = JOB_WAITING; - uj->manager->n_running_jobs--; + + job_set_state(uj, JOB_WAITING); return uj; } } @@ -191,6 +220,7 @@ Job* job_install(Job *j) { /* Install the job */ *pj = j; j->installed = true; + j->manager->n_installed_jobs ++; log_unit_debug(j->unit->id, "Installed new job %s/%s as %u", @@ -209,15 +239,17 @@ int job_install_deserialized(Job *j) { } pj = (j->type == JOB_NOP) ? &j->unit->nop_job : &j->unit->job; - if (*pj) { - log_unit_debug(j->unit->id, - "Unit %s already has a job installed. Not installing deserialized job.", - j->unit->id); + log_unit_debug(j->unit->id, "Unit %s already has a job installed. Not installing deserialized job.", j->unit->id); return -EEXIST; } + *pj = j; j->installed = true; + + if (j->state == JOB_RUNNING) + j->unit->manager->n_running_jobs++; + log_unit_debug(j->unit->id, "Reinstalled deserialized job %s/%s as %u", j->unit->id, job_type_to_string(j->type), (unsigned) j->id); @@ -484,8 +516,7 @@ int job_run_and_invalidate(Job *j) { if (!job_is_runnable(j)) return -EAGAIN; - j->state = JOB_RUNNING; - m->n_running_jobs++; + job_set_state(j, JOB_RUNNING); job_add_to_dbus_queue(j); /* While we execute this operation the job might go away (for @@ -549,10 +580,9 @@ int job_run_and_invalidate(Job *j) { r = job_finish_and_invalidate(j, JOB_ASSERT, true); else if (r == -ENOTSUP) r = job_finish_and_invalidate(j, JOB_UNSUPPORTED, true); - else if (r == -EAGAIN) { - j->state = JOB_WAITING; - m->n_running_jobs--; - } else if (r < 0) + else if (r == -EAGAIN) + job_set_state(j, JOB_WAITING); + else if (r < 0) r = job_finish_and_invalidate(j, JOB_FAILED, true); } @@ -780,9 +810,6 @@ int job_finish_and_invalidate(Job *j, JobResult result, bool recursive) { j->result = result; - if (j->state == JOB_RUNNING) - j->manager->n_running_jobs--; - log_unit_debug(u->id, "Job %s/%s finished, result=%s", u->id, job_type_to_string(t), job_result_to_string(result)); @@ -795,7 +822,7 @@ int job_finish_and_invalidate(Job *j, JobResult result, bool recursive) { if (result == JOB_DONE && t == JOB_RESTART) { job_change_type(j, JOB_START); - j->state = JOB_WAITING; + job_set_state(j, JOB_WAITING); job_add_to_run_queue(j); @@ -1027,7 +1054,7 @@ int job_deserialize(Job *j, FILE *f, FDSet *fds) { if (s < 0) log_debug("Failed to parse job state %s", v); else - j->state = s; + job_set_state(j, s); } else if (streq(l, "job-override")) { int b; diff --git a/src/core/manager.c b/src/core/manager.c index 9705e64da..519b37438 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -162,6 +162,7 @@ static void manager_print_jobs_in_progress(Manager *m) { uint64_t x; assert(m); + assert(m->n_running_jobs > 0); manager_flip_auto_status(m, true); @@ -184,8 +185,7 @@ static void manager_print_jobs_in_progress(Manager *m) { m->jobs_in_progress_iteration++; if (m->n_running_jobs > 1) - if (asprintf(&job_of_n, "(%u of %u) ", counter, m->n_running_jobs) < 0) - job_of_n = NULL; + asprintf(&job_of_n, "(%u of %u) ", counter, m->n_running_jobs); format_timespan(time, sizeof(time), now(CLOCK_MONOTONIC) - j->begin_usec, 1*USEC_PER_SEC); if (job_get_timeout(j, &x) > 0) @@ -197,7 +197,6 @@ static void manager_print_jobs_in_progress(Manager *m) { job_type_to_string(j->type), unit_description(j->unit), time, limit); - } static int have_ask_password(void) { @@ -2635,9 +2634,6 @@ void manager_check_finished(Manager *m) { assert(m); - if (m->n_running_jobs == 0) - m->jobs_in_progress_event_source = sd_event_source_unref(m->jobs_in_progress_event_source); - if (hashmap_size(m->jobs) > 0) { if (m->jobs_in_progress_event_source) diff --git a/src/core/unit.c b/src/core/unit.c index 9f7ba9227..229bd0f73 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -2674,7 +2674,9 @@ int unit_deserialize(Unit *u, FILE *f, FDSet *fds) { if (streq(l, "job")) { if (v[0] == '\0') { /* new-style serialized job */ - Job *j = job_new_raw(u); + Job *j; + + j = job_new_raw(u); if (!j) return -ENOMEM; @@ -2696,12 +2698,11 @@ int unit_deserialize(Unit *u, FILE *f, FDSet *fds) { job_free(j); return r; } - - if (j->state == JOB_RUNNING) - u->manager->n_running_jobs++; } else { /* legacy */ - JobType type = job_type_from_string(v); + JobType type; + + type = job_type_from_string(v); if (type < 0) log_debug("Failed to parse job type value %s", v); else -- 2.30.2