[4.8] fsmanager.GetStats performance regression#48
Closed
kolyshkin wants to merge 2 commits into
Closed
Conversation
In case openat2() is not available, it does not make sense to calculate relpath (and check if path has /sys/fs/cgroup prefix). Reverse the order of checks to not do that in case openat2 is not available. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com> (cherry picked from commit 272d34f5e82b7a0dd354fa828e65e218241080ad) Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Commit 88e8350, among the other things, replaced filepath.Join with securejoin.SecureJoin for both reads and writes to cgroupfs. Commits e76ac1c and 31f0f5b switched more code to use fscommon.ReadFile (and thus securejoin). Commit 0228226 introduced fscommon.OpenFile (which uses securejoin as the fallback if openat2(2) is not available, which is the case for older kernels), and commit c95e690 switched most of cgroup/fs[2] code to use it. As a result, fs.GetStats() method became noticeable slower, mostly due to securejoin calling os.Lstat and filepath.Clean. Using securejoin as a security measure while reading cgroupfs files is not well justified, as cgroupfs do not contain symlinks, and none of the GetStats code have uncleaned paths. This patch modifies the code to not use securejoin in case the file is to be opened read-only. Using BenchmarkGetStats on a CentOS 8 VM, I see the following improvement: Before: > BenchmarkGetStats-8 8376 625135 ns/op After: > BenchmarkGetStats-8 13162 452281 ns/op Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com> (cherry picked from commit 474a605c79101fad941da5ecc3b7a817ff049bf3) Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Collaborator
Author
|
Draft until opencontainers/runc#2921 is merged |
Collaborator
Author
|
wrong repo 🤦🏻 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a backport of opencontainers/runc#2921 (modulo benchmark code). Original description follows.
Commit 88e8350 (PR #2169), among the other things, replaced filepath.Join with
securejoin.SecureJoin for both reads and writes to cgroupfs.
Commits e76ac1c and 31f0f5b (PR #2604) switched more code to use
fscommon.ReadFile(and thussecurejoin). Commit 0228226 (PR #2635) introducedfscommon.OpenFile(which usessecurejoinas a fallback ifopenat2(2)is not available,which is the case for older kernels), and commit c95e690 (PR #2635) switched
most of cgroup/fs[2] code to use it.
As a result,
fs.GetStats()method became noticeable slower, mostly dueto securejoin calling
os.Lstatandfilepath.Clean.Using securejoin as a security measure while reading cgroupfs files is
not very well justified, as cgroupfs do not contain symlinks, and none of the
GetStats code have uncleaned paths.
This PR modifies the code to not use securejoin in case the file is
to be opened read-only, and introduces a benchmark to measure
the different.
Using the added
BenchmarkGetStatson a CentOS 8 VM, I see the followingimprovement:
Before (the last commit):
After: