core: add IODeviceLatencyTargetSec by htejun · Pull Request #9247 · systemd/systemd

htejun · 2018-06-09T21:28:47Z

This adds support for the following proposed latency based IO control
mechanism.

https://lkml.org/lkml/2018/6/5/428

This should be merged after the kernel feature is merged. Submitting
now for early review.

yuwata · 2018-06-10T07:26:14Z

src/core/cgroup.h

+struct CGroupIODeviceLatency {
+        LIST_FIELDS(CGroupIODeviceLatency, device_latencies);
+        char *path;
+        uint64_t target_usec;


I think using usec_t is better.

poettering

looks pretty good, just some minor comments

poettering · 2018-06-11T08:41:45Z

man/systemd.resource-control.xml

+        <listitem>
+          <para>Set the per-device average target I/O latency for the executed processes, if the unified control group
+          hierarchy is used on the system. Takes a space-separated pair of a file path and a timespan value to specify
+          the device specific latency target. (Example: "/dev/sda 25ms"). The file path may be specified


maybe use <literal>…</literal> here, rather than ""

poettering · 2018-06-11T08:42:56Z

src/core/cgroup.c

        return n;
 }

+static unsigned cgroup_apply_io_device_latency(Unit *u, const char *dev_path, uint64_t target) {


uint64_t target → usec_t target?

poettering · 2018-06-11T09:24:24Z

src/core/cgroup.c

+
+                                LIST_FOREACH_SAFE(device_latencies, l, next, c->io_device_latencies) {
+                                        if (!cgroup_apply_io_device_latency(u, l->path, l->target_usec))
+                                                cgroup_context_free_io_device_latency(c, l);


hmm, so applying these might modify the loaded unit file state?

I am seeing the existing code already did it like that, but I wonder if this is a good idea. I am pretty sure we should leave configuration unmodified by reality, if you so will. I mean, devices come and go, and I think our policy should be here that we apply what we can apply, and if we cant' apply it now, then we should handle it gracefully, but if we maybe can apply it safely later, for example because a device appeared, we should do it then. i.e. truly graceful best-effort behaviour.

but dunno, mayb it's ok to get in like this for now, but I am pretty sure we should clean that up sooner or later

Hmm... yeah maybe. I have no idea why all the device io configs try to ditch configs if the device lookup fails or the config is the default. Maybe it's to conserve memory? Given that they're all tied to a cgroup's lifetime, just keeping them around no matter what should be fine, I think.

someone called Tejun Heo added that bit in 979d031. Maybe we should ask him ;-)

I prepped #9275 now, which drops this bit. PTAL. (and rebase on top of it after that's merged)

I completely forgot that it was me. Gees. Anyways, yeah, I can't think of downsides of just keeping the configs around.

poettering · 2018-06-11T10:51:09Z

Hmm, I made some changes to the block device resolving and validation code in #9255. I figure this PR should follow the same logic of those changes as soon as it gets merged.

keszybz

There is a mismatch between the docs and the checks for valid paths, but @poettering already handled that in #9255.

keszybz · 2018-06-11T08:45:20Z

man/systemd.resource-control.xml

+        <listitem>
+          <para>Set the per-device average target I/O latency for the executed processes, if the unified control group
+          hierarchy is used on the system. Takes a space-separated pair of a file path and a timespan value to specify
+          the device specific latency target. (Example: "/dev/sda 25ms"). The file path may be specified


Takes a file path and a timespan separated by a space …

(I know this just copies existing text.)

keszybz · 2018-06-11T08:50:31Z

src/core/cgroup.c

+        r = cg_set_attribute("io", u->cgroup_path, "io.latency", buf);
+        if (r < 0)
+                log_unit_full(u, IN_SET(r, -ENOENT, -EROFS, -EACCES) ? LOG_DEBUG : LOG_WARNING, r,
+                              "Failed to set io.latency: %m");


"Failed to set io.latency on cgroup %s: %", u->cgroup_path) ?

keszybz · 2018-06-11T08:57:30Z

src/shared/bus-unit-util.c

+                        target = e+1;
+
+                        r = parse_sec(target, &usec);
+


Spurious newline.

htejun · 2018-06-11T15:37:26Z

Applied changes for all comments except for #9255. Will rebase on top once it's merged.

keszybz · 2018-06-12T10:54:05Z

Please rebase.

htejun · 2018-06-12T18:43:46Z

@keszybz, will wait for #9275 and then rebase. Thanks.

poettering · 2018-06-14T15:49:07Z

looks good to me, but let's wait until the upstream kernel feature is merged and until after the upcoming v239 release.

Summary: New upstream release, plus backports of systemd/systemd#9244 systemd/systemd#9247 and systemd/systemd#9410 Reviewed By: jaymzh Differential Revision: D8609874 fbshipit-source-id: a620516cb7bc4305b144c811aa5eac5d19dc8274

This adds support for the following proposed latency based IO control mechanism. https://lkml.org/lkml/2018/6/5/428

evverx · 2018-07-03T21:22:47Z

This pull request fixes 4 alerts when merging b874a67 into 98b0b11 - view on LGTM.com

fixed alerts:

4 for FIXME comment

Comment posted by LGTM.com

htejun · 2018-07-11T15:45:16Z

blk-iolatency got merged into block/for-next and will be pulled into Linus's tree when v4.19-rc1 window opens.

keszybz · 2018-08-22T14:46:03Z

blk-iolatency is available in 4.19.0-0.rc0.git6.1.fc30.x86_64, let's merge.

evverx · 2018-08-22T15:16:29Z

This pull request fixes 4 alerts when merging b874a67 into 71d1700 - view on LGTM.com

fixed alerts:

4 for FIXME comment

Comment posted by LGTM.com

yuwata added cgroups pid1 labels Jun 10, 2018

yuwata reviewed Jun 10, 2018

View reviewed changes

htejun force-pushed the io_latency branch from 42f5cc9 to b971458 Compare June 10, 2018 12:21

keszybz added the do-not-merge 💣 label Jun 11, 2018

poettering reviewed Jun 11, 2018

View reviewed changes

poettering mentioned this pull request Jun 11, 2018

some block device handling fixes #9255

Merged

keszybz reviewed Jun 11, 2018

View reviewed changes

htejun force-pushed the io_latency branch from b971458 to 63e617f Compare June 11, 2018 15:35

htejun force-pushed the io_latency branch from 63e617f to f4ca091 Compare June 13, 2018 21:17

poettering added the postponed label Jun 14, 2018

core: add IODeviceLatencyTargetSec

b874a67

This adds support for the following proposed latency based IO control mechanism. https://lkml.org/lkml/2018/6/5/428

htejun force-pushed the io_latency branch from f4ca091 to b874a67 Compare July 3, 2018 20:43

keszybz removed do-not-merge 💣 postponed labels Aug 22, 2018

keszybz merged commit 6ae4283 into systemd:master Aug 22, 2018

Uh oh!

Conversation

htejun commented Jun 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

poettering left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

poettering commented Jun 11, 2018

Uh oh!

keszybz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htejun commented Jun 11, 2018

Uh oh!

keszybz commented Jun 12, 2018

Uh oh!

htejun commented Jun 12, 2018

Uh oh!

poettering commented Jun 14, 2018

Uh oh!

evverx commented Jul 3, 2018

Uh oh!

htejun commented Jul 11, 2018

Uh oh!

keszybz commented Aug 22, 2018

Uh oh!

evverx commented Aug 22, 2018

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants