chunked: use mmap to load cache files

[giuseppe]

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.

[rhatdan]

Is this something that will need to be back ported into podman 5.0 or wait for 5.1?

[giuseppe]

it is fine for 5.1

[rhatdan]

LGTM
@mtrmac PTAL

[mtrmac]

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.

[giuseppe]

I've pushed a new version that doesn't require any API change, and try to cast the output from LayerBigData to a os.File

[mtrmac]

Oh, that’s clever. Maybe worth a comment in the layers.go implementation, pointing at this assumption, so that it isn’t randomly broken by some wrapping helper.

[giuseppe]

@kolyshkin PTAL

[giuseppe]

rebased, please take a look

[kolyshkin]

I'm still looking at it, hope to finish tomorrow.

[kolyshkin]

I will continue tomorrow. For now, I've split this into two patches for easier review:

1. rename manifest to cacheFile
2. 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.

[kolyshkin]

nit: for readability, I'd write this as return buf, buf, nil since err is always nil here.

[kolyshkin]

Added some more comments.

[kolyshkin]

Suggested change

	// 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

[giuseppe]

fixed

[kolyshkin]

Does that mean if a layer is loaded via io.ReadAll (rather than Mmap), we are re-reading it?

[giuseppe]

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)
 				continue

[giuseppe]

@kolyshkin are you fine with the last version?

[giuseppe]

rebased

[mtrmac]

Please add a comment around layerStore.BigData noting that pkg/chunked relies on the returned type being exactly *os.File.

LGTM otherwise.

[giuseppe]

@mtrmac addressed your comments and pushed a new version

[mtrmac]

Thanks! LGTM. Up to @kolyshkin now.

[giuseppe]

@kolyshkin @rhatdan PTAL

[kolyshkin]

lgtm

[openshift-ci]

[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

Needs approval from an approver in each of these files:

• OWNERS [giuseppe]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[giuseppe]

/lgtm

[openshift-ci]

@giuseppe: you cannot LGTM your own PR.

In response to this:

/lgtm

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.

[rhatdan]

/lgtm