chunked: fix reuse of the layers cache #2024
Conversation
the global singleton was never updated, causing the cache to be always recreated for each layer. It is not possible to keep the layersCache mutex for the entire load() since it calls into some store APIs causing a deadlock since findDigestInternal() is already called while some store locks are held. Closes: containers#2023 Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: giuseppe 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:
Approvers can indicate their approval by writing |
|
LGTM |
giuseppe
changed the title
There was a problem hiding this comment.
Thanks for handling this!
A fairly brief skim for now…
(It would be also convenient to have the locking rules documented in detail — e.g. which fields of layersCache and layers are immutable; which are protected by a lock (and which one).
Especially the lock hierarchy of layersCache.mutex vs. the locks of store/layerStore seems fairly non-obvious to me, given how far the code locations are from each other and how dissimilar the situation seems.
But all of that is blocked on having settled on design in the first place.)
| @@ -111,7 +111,7 @@ func getLayersCacheRef(store storage.Store) *layersCache { | |||
| cache.refs++ | |||
| return cache | |||
| } | |||
| cache := &layersCache{ | |||
| cache = &layersCache{ | |||
There was a problem hiding this comment.
(I wonder about making this kind of mistake harder to do … naming the global cacheSingleton, globalCache, or something, might make it less likely to conflict with a local. OTOH that’s very likely an overreaction.)
| @@ -300,7 +300,9 @@ func (c *layersCache) load() error { | |||
| for _, l := range loadedLayers { | |||
| l.release() | |||
There was a problem hiding this comment.
AFAICS multiple load operations can now be happening in parallel; in that case this might release the same layer multiple times, concurrently.
(I have no opinion on an alternative design at this point, I didn’t look for one yet.)
There was a problem hiding this comment.
could we just drop the release() here and let the GC deal with it? We are already using runtime.SetFinalizer()
There was a problem hiding this comment.
That would probably work… intuitively I don’t like relying on the GC that way, but I don’t have much of an objective reason (at best, that the time of release, and total memory consumption, are hard to manage).
It’s not quite sufficient, though, especially for a single-operation command like podman pull. Normally, c/image would trigger a pull of up to 6 layers in parallel, all of those would ~concurrently trigger layersCache.load, and at least read/map all those cache files into memory, or in the worst case create them from scratch, up to 6 times in parallel.
So before this PR we build/load the cache for every pulled layer; with this PR as is, we build/load the cache for up to 6 concurrent layers (and after the first batch of layers, all future cache uses are only the metadata check, and/or update for the just-added layers). This PR is an improvement but not fully where we want to be.
Ideally, I think, the cache loading / update, at least in the case where we do some I/O, should be serialized.
Would two locks work? Nested “exclusive layer loading lock” → c/storage store+layer locks → “the current `layersCache.mutex”? It seems to me that that should be possible, but I didn’t try and I didn’t check very hard.
Very vaguely, the current reference count vs. per-cache mutex vs. loading state seems badly structured, or at least it’s unclear to me why it is so. getLayersCacheRef is already serialized and it knows that it needs the cache EDIT reloaded because it is old. But then getLayersCache triggers a load (and a reconciliation against store.Layers) every time. Do we even need the timer there? Do we need the concurrent existence of multiple layersCache objects, with each chunkedDiffer keeping a reference to a particular cache version?
I’m not saying it’s obviously wrong, and for all I know it might be the best design or the only possible design, but nothing in the code writes down why it is so, and it just … feels … off.
|
If this is merged on or before August 12, 2024, please cherry-pick this to the release-1.55 branch |
This is just my opinion but basically this is attempting to optimize zstd:chunked some, and introduces some open questions around concurrency. It doesn't make sense to rush it, and if it merged it's the type of change that should only be cherry picked if there was a known reason to do so. |
the global singleton was never updated, causing the cache to be always recreated for each layer.
It is not possible to keep the layersCache mutex for the entire load() since it calls into some store APIs causing a deadlock since findDigestInternal() is already called while some store locks are held.
Closes: #2023