-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
libct/cg/fs2: set per-device io weight if available #3022
Conversation
8271ed2
to
1222ce7
Compare
@askervin PTAL |
1222ce7
to
462301a
Compare
This is a feature, not a bug (as I see it), so adding to 1.1 milestone |
8a2746e
to
dfdab37
Compare
The test case for cgroup v2 reveal an issue with v1 blkio, so I am adding a bug fix for it, and changing the milestone to 1.0.0 (as it is now a bug fix). I can split this PR into two -- a bug fix (for cgroup v1) and a new feature (for cgroup v2), but as the test case is the same I'd rather keep this together. |
This comment has been minimized.
This comment has been minimized.
487f620
to
c3bec6e
Compare
d186a46
to
6a7517a
Compare
6a7517a
to
09607a8
Compare
For per-device weight, you can set weight and/or leaf weight. The problem is, with the recent fix to use BFQ on cgroup v1, if per-device weights are set, the code tries to set device weight to blkio.bfq.weight, and the leaf weight to blkio.leaf_weight_device. The latter file does not exist on kernels v5.0, meaning one can not set any per-device weights at all. The fix is to only set weights if they are non-zero (i.e. set). The test case will come in a following commit. Fixes: 6339d8a Signed-off-by: Kir Kolyshkin <[email protected]>
09607a8
to
81530bc
Compare
Rebased on top of merged #3024. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kolyshkin , great test & finding that leaf_device_weight
was being written in cgroups v1 every time when setting device weights!
I did some manual testing with this cgroups v2 code, just to make sure that sequences like
- open-write-seek-read-write
- open-seek-read-write
onio.bfq.weight
indeed do what we expect. And it does.
This looks good and works fine. Only a super-nit: a typo in a comment.
81530bc
to
0ac70e4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Special thanks for fixing the issue in PR #3010! :)
libcontainer/cgroups/fs2/io.go
Outdated
return false | ||
} | ||
_, _ = bfq.Seek(0, 0) | ||
exp := []byte("default ") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this really guaranteed to be in the first line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes; see bfq_io_show_weight
in block/bfq-cgroup.c
, e.g.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And before torvalds/linux@795fe54 it was a number in there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Future version of the kernel may have another line before "default 100".
So I guess we have to scan all the lines?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory that would be an ABI breakage, but yeah it seems a bit risky to depend on the exact order of lines in a kernel file when the alternative is pretty inexpensive. (And in the past cgroup ABIs haven't exactly been completely free of ABI breakages.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree (that the relaxed check is to look for a line starting with default
anywhere in the file).
So far we have to distinguish between two versions of the bfq.io.weight
API. Let's call them version 1 and version 2. Version 1 only allows to get/set the default weight. Version 2 also allows to get/set per-device weights.
Now, if the first line of bfq.io.weight
does read back as default XXX
, we do have version 2. Otherwise it's something else. Either version 1 which does not support setting per-device weights, or perhaps a version 3 we don't know anything about.
But let's assume that write format for version 3 is backward-compatible with version 2 (same as version 2 write is backward-compatible with version 1, although the read is not). In this case we need to check not for version 2, but for anything other than version 1.
Such a check would be to read the entire file and see that it's not a version 1 file, i.e. not a single number followed by a newline.
I will update the commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated. The new check specifically checks for v1 file, not v2 file, and is thus more future-proof (assuming that in v3 file format will expand and not go back to same as it was in v1).
0ac70e4
to
2916817
Compare
Per-device weight is supported since kernel v5.4 (kernel commit 795fe54c2a8), so let's set those if supplied. [v2: implement a more relaxed check in bfqDeviceWeightSupported] Signed-off-by: Kir Kolyshkin <[email protected]>
This works for both cgroup v1 and v2. Signed-off-by: Kir Kolyshkin <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. With this I'll prep a release.
I accidentally found out that per-device weight is supported
in BFQ scheduler since kernel v5.4 (kernel commit 795fe54c2a8),
so let's set those if supplied.
Next, I added a test case for it and decided to not make is v2-specific,
as the test for cgroup v1 would look very much the same. This test found a bug
in v1 (see #3010 (comment)), so I fixed it as well.
This closes a small gap in cgroup v2 feature parity with cgroup v1.
Proposed changelog entry