From a2df3ea4ae058693bc7bf203d144e8af3c9493d2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Koutn=C3=BD?= Date: Fri, 17 Feb 2017 17:47:20 +0100 Subject: [PATCH 1/3] job: add JobRunningTimeoutSec for JOB_RUNNING state Unit.JobTimeoutSec starts when a job is enqueued in a transaction. The introduced distinct Unit.JobRunningTimeoutSec starts only when the job starts running (e.g. it groups all Exec* commands of a service or spans waiting for a device period.) Unit.JobRunningTimeoutSec is intended to be used by default instead of Unit.JobTimeoutSec for device units where such behavior causes less confusion (consider a job for a _netdev mount device, with this change the timeout will start ticking only after the network is ready). --- man/systemd.unit.xml | 19 ++++++++------ src/core/dbus-unit.c | 1 + src/core/job.c | 38 ++++++++++++++++++++++----- src/core/job.h | 2 +- src/core/load-fragment-gperf.gperf.m4 | 1 + src/core/transaction.c | 2 +- src/core/unit.c | 4 +++ src/core/unit.h | 1 + 8 files changed, 51 insertions(+), 17 deletions(-) diff --git a/man/systemd.unit.xml b/man/systemd.unit.xml index 44841ac7dd..23f62542d0 100644 --- a/man/systemd.unit.xml +++ b/man/systemd.unit.xml @@ -718,17 +718,20 @@ JobTimeoutSec= + JobRunningTimeoutSec= JobTimeoutAction= JobTimeoutRebootArgument= - When a job for this unit is queued, a time-out may be configured. If this time limit is - reached, the job will be cancelled, the unit however will not change state or even enter the - failed mode. This value defaults to infinity (job timeouts disabled), - except for device units. NB: this timeout is independent from any unit-specific timeout (for example, the - timeout set with TimeoutStartSec= in service units) as the job timeout has no effect on the - unit itself, only on the job that might be pending for it. Or in other words: unit-specific timeouts are useful - to abort unit state changes, and revert them. The job timeout set with this option however is useful to abort - only the job waiting for the unit state to change. + When a job for this unit is queued, a time-out JobTimeoutSec= may be + configured. Similarly, JobRunningTimeoutSec= starts counting when the queued job is actually + started. If either time limit is reached, the job will be cancelled, the unit however will not change state or + even enter the failed mode. This value defaults to infinity (job timeouts + disabled), except for device units (JobRunningTimeoutSec= defaults to + DefaultTimeoutStartSec=). NB: this timeout is independent from any unit-specific timeout + (for example, the timeout set with TimeoutStartSec= in service units) as the job timeout has + no effect on the unit itself, only on the job that might be pending for it. Or in other words: unit-specific + timeouts are useful to abort unit state changes, and revert them. The job timeout set with this option however + is useful to abort only the job waiting for the unit state to change. JobTimeoutAction= optionally configures an additional diff --git a/src/core/dbus-unit.c b/src/core/dbus-unit.c index f15bb2196c..b0645ce294 100644 --- a/src/core/dbus-unit.c +++ b/src/core/dbus-unit.c @@ -748,6 +748,7 @@ const sd_bus_vtable bus_unit_vtable[] = { SD_BUS_PROPERTY("IgnoreOnIsolate", "b", bus_property_get_bool, offsetof(Unit, ignore_on_isolate), SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("NeedDaemonReload", "b", property_get_need_daemon_reload, 0, SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("JobTimeoutUSec", "t", bus_property_get_usec, offsetof(Unit, job_timeout), SD_BUS_VTABLE_PROPERTY_CONST), + SD_BUS_PROPERTY("JobRunningTimeoutUSec", "t", bus_property_get_usec, offsetof(Unit, job_running_timeout), SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("JobTimeoutAction", "s", property_get_emergency_action, offsetof(Unit, job_timeout_action), SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("JobTimeoutRebootArgument", "s", NULL, offsetof(Unit, job_timeout_reboot_arg), SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("ConditionResult", "b", bus_property_get_bool, offsetof(Unit, condition_result), SD_BUS_VTABLE_PROPERTY_EMITS_CHANGE), diff --git a/src/core/job.c b/src/core/job.c index e2349830a8..2b43cf6126 100644 --- a/src/core/job.c +++ b/src/core/job.c @@ -576,6 +576,7 @@ int job_run_and_invalidate(Job *j) { if (!job_is_runnable(j)) return -EAGAIN; + job_start_timer(j, true); job_set_state(j, JOB_RUNNING); job_add_to_dbus_queue(j); @@ -949,22 +950,45 @@ static int job_dispatch_timer(sd_event_source *s, uint64_t monotonic, void *user return 0; } -int job_start_timer(Job *j) { +int job_start_timer(Job *j, bool job_running) { int r; + usec_t run_begin, timeout_time, old_timeout_time; - if (j->timer_event_source) - return 0; + if (job_running) { + if (j->unit->job_running_timeout == USEC_INFINITY) + return 0; - j->begin_usec = now(CLOCK_MONOTONIC); + run_begin = now(CLOCK_MONOTONIC); + timeout_time = usec_add(run_begin, j->unit->job_running_timeout); - if (j->unit->job_timeout == USEC_INFINITY) - return 0; + if (j->timer_event_source) { + /* Update only if JobRunningTimeoutSec= results in earlier timeout */ + r = sd_event_source_get_time(j->timer_event_source, &old_timeout_time); + if (r < 0) + return r; + + if (old_timeout_time <= timeout_time) + return 0; + + return sd_event_source_set_time(j->timer_event_source, timeout_time); + } + } else { + if (j->timer_event_source) + return 0; + + j->begin_usec = now(CLOCK_MONOTONIC); + + if (j->unit->job_timeout == USEC_INFINITY) + return 0; + + timeout_time = usec_add(j->begin_usec, j->unit->job_timeout); + } r = sd_event_add_time( j->manager->event, &j->timer_event_source, CLOCK_MONOTONIC, - usec_add(j->begin_usec, j->unit->job_timeout), 0, + timeout_time, 0, job_dispatch_timer, j); if (r < 0) return r; diff --git a/src/core/job.h b/src/core/job.h index bea743f462..8b3d38fc60 100644 --- a/src/core/job.h +++ b/src/core/job.h @@ -220,7 +220,7 @@ int job_type_merge_and_collapse(JobType *a, JobType b, Unit *u); void job_add_to_run_queue(Job *j); void job_add_to_dbus_queue(Job *j); -int job_start_timer(Job *j); +int job_start_timer(Job *j, bool job_running); int job_run_and_invalidate(Job *j); int job_finish_and_invalidate(Job *j, JobResult result, bool recursive, bool already); diff --git a/src/core/load-fragment-gperf.gperf.m4 b/src/core/load-fragment-gperf.gperf.m4 index cb9e6fea27..97adbdd7bb 100644 --- a/src/core/load-fragment-gperf.gperf.m4 +++ b/src/core/load-fragment-gperf.gperf.m4 @@ -194,6 +194,7 @@ Unit.OnFailureIsolate, config_parse_job_mode_isolate, 0, Unit.IgnoreOnIsolate, config_parse_bool, 0, offsetof(Unit, ignore_on_isolate) Unit.IgnoreOnSnapshot, config_parse_warn_compat, DISABLED_LEGACY, 0 Unit.JobTimeoutSec, config_parse_sec_fix_0, 0, offsetof(Unit, job_timeout) +Unit.JobRunningTimeoutSec, config_parse_sec, 0, offsetof(Unit, job_running_timeout) Unit.JobTimeoutAction, config_parse_emergency_action, 0, offsetof(Unit, job_timeout_action) Unit.JobTimeoutRebootArgument, config_parse_unit_string_printf, 0, offsetof(Unit, job_timeout_reboot_arg) Unit.StartLimitIntervalSec, config_parse_sec, 0, offsetof(Unit, start_limit.interval) diff --git a/src/core/transaction.c b/src/core/transaction.c index b6d1062414..a2dfd8ae90 100644 --- a/src/core/transaction.c +++ b/src/core/transaction.c @@ -632,7 +632,7 @@ static int transaction_apply(Transaction *tr, Manager *m, JobMode mode) { job_add_to_run_queue(j); job_add_to_dbus_queue(j); - job_start_timer(j); + job_start_timer(j, false); job_shutdown_magic(j); } diff --git a/src/core/unit.c b/src/core/unit.c index f76b6c30a8..bd866774a2 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -99,6 +99,7 @@ Unit *unit_new(Manager *m, size_t size) { u->on_failure_job_mode = JOB_REPLACE; u->cgroup_inotify_wd = -1; u->job_timeout = USEC_INFINITY; + u->job_running_timeout = USEC_INFINITY; u->ref_uid = UID_INVALID; u->ref_gid = GID_INVALID; u->cpu_usage_last = NSEC_INFINITY; @@ -1336,6 +1337,9 @@ int unit_load(Unit *u) { goto fail; } + if (u->job_running_timeout != USEC_INFINITY && u->job_running_timeout > u->job_timeout) + log_unit_warning(u, "JobRunningTimeoutSec= is greater than JobTimeoutSec=, it has no effect."); + unit_update_cgroup_members_masks(u); } diff --git a/src/core/unit.h b/src/core/unit.h index 8052c234fd..cf21b37e22 100644 --- a/src/core/unit.h +++ b/src/core/unit.h @@ -114,6 +114,7 @@ struct Unit { /* Job timeout and action to take */ usec_t job_timeout; + usec_t job_running_timeout; EmergencyAction job_timeout_action; char *job_timeout_reboot_arg; From 4195077ab4c823c385cdc3c868a56178f8d93723 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Koutn=C3=BD?= Date: Tue, 24 Jan 2017 17:04:32 +0100 Subject: [PATCH 2/3] fstab-generator: Apply _netdev option also to device units In case the device field of fstab record is an actual device (not an address) apply same dependencies to the device unit as to the mount unit, i.e. > After=network-online.target network.target > Wants=network-online.targe It makes sense to start the device expecting job only when network is actually ready (consider e.g. iSCSI devices) since it is device's implicit dependency. The eventual implementation should better obtain network flag from udev database and would also take into account device hierarchy (see [1]). This patch approximates that by taking the `_netdev` option as a hint from the user both about the filesystem and underlying device. (For local devices with network filesystems (e.g. ocfs2), this hint leads to unused dependencies.) [1] https://lists.freedesktop.org/archives/systemd-devel/2014-October/024718.html --- src/fstab-generator/fstab-generator.c | 4 +++ src/shared/generator.c | 39 +++++++++++++++++++++++++++ src/shared/generator.h | 6 +++++ 3 files changed, 49 insertions(+) diff --git a/src/fstab-generator/fstab-generator.c b/src/fstab-generator/fstab-generator.c index 2677a3fb32..43d3308e95 100644 --- a/src/fstab-generator/fstab-generator.c +++ b/src/fstab-generator/fstab-generator.c @@ -399,6 +399,10 @@ static int add_mount( if (r < 0) return r; + r = generator_write_device_deps(dest, what, where, opts); + if (r < 0) + return r; + r = write_mount_timeout(f, where, opts); if (r < 0) return r; diff --git a/src/shared/generator.c b/src/shared/generator.c index 9a069b2f97..5f6b1de811 100644 --- a/src/shared/generator.c +++ b/src/shared/generator.c @@ -192,6 +192,45 @@ int generator_write_timeouts( program_invocation_short_name, timeout); } +int generator_write_device_deps( + const char *dir, + const char *what, + const char *where, + const char *opts) { + + /* fstab records that specify _netdev option should apply the network + * ordering on the actual device depending on network connection. If we + * are not mounting real device (NFS, CIFS), we rely on _netdev effect + * on the mount unit itself. */ + + _cleanup_free_ char *node = NULL, *unit = NULL; + int r; + + if (!fstab_test_option(opts, "_netdev\0")) + return 0; + + node = fstab_node_to_udev_node(what); + if (!node) + return log_oom(); + + /* Nothing to apply dependencies to. */ + if (!is_device_path(node)) + return 0; + + r = unit_name_from_path(node, ".device", &unit); + if (r < 0) + return log_error_errno(r, "Failed to make unit name from path: %m"); + + /* See mount_add_default_dependencies for explanation why we create such + * dependencies. */ + return write_drop_in_format(dir, unit, 50, "netdev-dependencies", + "# Automatically generated by %s\n\n" + "[Unit]\n" + "After=" SPECIAL_NETWORK_ONLINE_TARGET " " SPECIAL_NETWORK_TARGET "\n" + "Wants=" SPECIAL_NETWORK_ONLINE_TARGET "\n", + program_invocation_short_name); +} + int generator_write_initrd_root_device_deps(const char *dir, const char *what) { _cleanup_free_ char *unit = NULL; int r; diff --git a/src/shared/generator.h b/src/shared/generator.h index a6017c1b76..825d934c8e 100644 --- a/src/shared/generator.h +++ b/src/shared/generator.h @@ -35,6 +35,12 @@ int generator_write_timeouts( const char *opts, char **filtered); +int generator_write_device_deps( + const char *dir, + const char *what, + const char *where, + const char *opts); + int generator_write_initrd_root_device_deps( const char *dir, const char *what); From d9732d78034cd01a75f2cda6ebaddb4c3a0929ea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Koutn=C3=BD?= Date: Wed, 5 Apr 2017 14:11:12 +0200 Subject: [PATCH 3/3] core/device: Use JobRunningTimeoutSec= for device units Device job timeouts should respect possible device job dependencies so we set JobRunningTimeoutSec= by default. --- src/core/device.c | 2 +- src/shared/generator.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/core/device.c b/src/core/device.c index 0e67c96552..da008f6041 100644 --- a/src/core/device.c +++ b/src/core/device.c @@ -112,7 +112,7 @@ static void device_init(Unit *u) { * indefinitely for plugged in devices, something which cannot * happen for the other units since their operations time out * anyway. */ - u->job_timeout = u->manager->default_timeout_start_usec; + u->job_running_timeout = u->manager->default_timeout_start_usec; u->ignore_on_isolate = true; } diff --git a/src/shared/generator.c b/src/shared/generator.c index 5f6b1de811..19ec133772 100644 --- a/src/shared/generator.c +++ b/src/shared/generator.c @@ -188,7 +188,7 @@ int generator_write_timeouts( return write_drop_in_format(dir, unit, 50, "device-timeout", "# Automatically generated by %s\n\n" - "[Unit]\nJobTimeoutSec=%s", + "[Unit]\nJobRunningTimeoutSec=%s", program_invocation_short_name, timeout); }