Faster Chown #557
Merged
Faster Chown #557
Conversation
This file contains 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
|
Forgot to |
This is mostly to get pkg/pwalk in. See https://github.com/opencontainers/selinux/releases/tag/v1.4.0 for release notes Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This is a parallel Walk implementation and we're expecting some speedup thanks to that. Note that pwalk.Walk error handling is primitive; for one thing, it never sends an error to the callpack, thus the error checking code is no longer needed. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This is purely aesthetical -- in case we can't get struct stat_t, return early. This improves readability and decreases the indentation. No functional change. Please review this with --ignore-space-change. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This function is already called with the result from os.Lstat(), so there's no need to do it again. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
kolyshkin
force-pushed
the
chown-use-pwalk
branch
from
4d87903
to
be28a55
Compare
|
LGTM |
|
Can anyone suggest a good benchmark setup so we can compare before and after? |
|
Kolshkin, the use case is we are trying to accelerate is the time it takes to chown a container image. Create an image with a lot of files/directories in it, then create a container with a different user namespace and mount it. If you create this with a thread of 1 versus several, you would think the mount/chown would happen faster in the second version. If we are dealing with overlay and metacopy=on, then it should be very fast, since files would not be being copied up. |
|
Curious to see how much improvement does it bring. Say run fedora image in a user namespace with and without these patches and how much is the improvement. |
|
@rhvgoyal I'll run some benchmarks once time permits; for now you can take a look at opencontainers/selinux#85 and containers/common#82. I expect at least 1.5x improvement in speed for Chown(), maybe better since I also removed an extra lstat(2) call. |
|
A simple test would be something like the following: make sure you run it as without the patch: and the patch: I'll investigate what is going on here, but it seems the time keeps increasing (not related to this patch). |
|
turned out to be an issue in Podman. I've opened a PR to address it: containers/podman#5469 I've repeated the tests on top of the libpod fix. without the patch: and the patch: |
|
You should really be clearing the file system cache between each run, to remove any optimizations by the kernel. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 makes use of
pwalk.Walk()(introduced in opencontainers/selinux#89)for recursive
Chown, and also optimizesdrivers/platformLChown()a bit, following the discussion at 2df72f3#r387867583TODO:
@rhatdan @giuseppe PTAL