chunked: use mmap to load cache files#1857
chunked: use mmap to load cache files#1857openshift-merge-bot[bot] merged 2 commits intocontainers:mainfrom
Conversation
7460370 to
36da862
Compare
|
Is this something that will need to be back ported into podman 5.0 or wait for 5.1? |
|
it is fine for 5.1 |
07be00f to
ec1aba0
Compare
|
LGTM |
mtrmac
left a comment
There was a problem hiding this comment.
For now just a note on the public API, I didn’t read the actual cache part.
It’s possible that these things are not much a concern for the cache, I didn’t check.
43b4236 to
99049ca
Compare
|
I've pushed a new version that doesn't require any API change, and try to cast the output from |
|
Oh, that’s clever. Maybe worth a comment in the |
|
@kolyshkin PTAL |
99049ca to
fde18e6
Compare
|
rebased, please take a look |
|
I'm still looking at it, hope to finish tomorrow. |
kolyshkin
left a comment
There was a problem hiding this comment.
I will continue tomorrow. For now, I've split this into two patches for easier review:
- rename manifest to cacheFile
- the rest of your changes
I might split it further as there are some code cleanups in here that may benefit from being in a separate commit.
pkg/chunked/cache_linux.go
Outdated
| _ = unix.Madvise(buf, unix.MADV_RANDOM) | ||
|
|
||
| var lcd chunkedLayerData | ||
| return buf, buf, err |
There was a problem hiding this comment.
nit: for readability, I'd write this as return buf, buf, nil since err is always nil here.
f911427 to
d2801ec
Compare
kolyshkin
left a comment
There was a problem hiding this comment.
Added some more comments.
pkg/chunked/cache_linux.go
Outdated
| cacheFile, err := readCacheFileFromReader(bigData) | ||
| if err == nil { | ||
| c.addLayer(r.ID, cacheFile) | ||
| // the layer is present in the store and it is already loaded. Attempt to use |
There was a problem hiding this comment.
| // the layer is present in the store and it is already loaded. Attempt to use | |
| // The layer is present in the store and it is already loaded. Attempt to |
d2801ec to
5259c9b
Compare
pkg/chunked/cache_linux.go
Outdated
| // The layer is present in the store and it is already loaded. Attempt to | ||
| // re-use it if mmap'ed. | ||
| if l, found := loadedLayers[r.ID]; found { | ||
| if l.mmapBuffer != nil { |
There was a problem hiding this comment.
Does that mean if a layer is loaded via io.ReadAll (rather than Mmap), we are re-reading it?
There was a problem hiding this comment.
this was meant to be an optimization to use only when the file is first created, so we already have its content in memory and to avoid reloading it. I've fixed it to not reload the cache when the file was initially loaded using io.ReadAll.
Applied the following fixup patch and pushed the new version:
diff --git a/pkg/chunked/cache_linux.go b/pkg/chunked/cache_linux.go
index 386bda515..01bfc8d92 100644
--- a/pkg/chunked/cache_linux.go
+++ b/pkg/chunked/cache_linux.go
@@ -46,6 +46,12 @@ type layer struct {
// mmapBuffer is nil when the cache file is fully loaded in memory.
// Otherwise it points to a mmap'ed buffer that is referenced by cacheFile.vdata.
mmapBuffer []byte
+
+ // reloadWithMmap is set when the current process generates the cache file,
+ // and cacheFile reuses the memory buffer used by the generation function.
+ // Next time the layer cache is used, attempt to reload the file using
+ // mmap.
+ reloadWithMmap bool
}
type layersCache struct {
@@ -201,7 +207,12 @@ func (c *layersCache) createCacheFileFromTOC(layerID string) (*layer, error) {
if err != nil {
return nil, err
}
- return c.createLayer(layerID, cacheFile, nil)
+ l, err := c.createLayer(layerID, cacheFile, nil)
+ if err != nil {
+ return nil, err
+ }
+ l.reloadWithMmap = true
+ return l, nil
}
func (c *layersCache) load() error {
@@ -222,8 +233,8 @@ func (c *layersCache) load() error {
// The layer is present in the store and it is already loaded. Attempt to
// re-use it if mmap'ed.
if l, found := loadedLayers[r.ID]; found {
- if l.mmapBuffer != nil {
- // It is loaded through mmap. Re-use it.
+ // If the layer is not marked for re-load, move it to newLayers.
+ if !l.reloadWithMmap {
delete(loadedLayers, r.ID)
newLayers = append(newLayers, l)
continue5259c9b to
af646a8
Compare
|
@kolyshkin are you fine with the last version? |
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
af646a8 to
2a4e4b3
Compare
|
rebased |
mtrmac
left a comment
There was a problem hiding this comment.
Please add a comment around layerStore.BigData noting that pkg/chunked relies on the returned type being exactly *os.File.
LGTM otherwise.
reduce memory usage for the process by not loading entirely in memory any cache file for the layers. The memory mapped files can be shared among multiple instances of Podman, as well as not being fully loaded in memory. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
2a4e4b3 to
080dbaf
Compare
|
@mtrmac addressed your comments and pushed a new version |
mtrmac
left a comment
There was a problem hiding this comment.
Thanks! LGTM. Up to @kolyshkin now.
|
@kolyshkin @rhatdan PTAL |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: giuseppe, kolyshkin The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/lgtm |
|
@giuseppe: you cannot LGTM your own PR. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/lgtm |
reduce memory usage for the process by not loading entirely in memory any cache file for the layers.
The memory mapped files can be shared among multiple instances of Podman, as well as not being fully loaded in memory.