Remove locking from layer store creation #493
Conversation
|
Let's see if this works... |
|
Ready for review, PTAL |
|
Thanks a lot, @saschagrunert! Would you mind opening test PRs for Skopeo, Buildah, Podman and CRI-O vendoring this branch? This will help us gain confidence in the change and make sure we didn't miss any potential side effect. |
Yep sure, see the linked PRs. Let's see what the tests say. |
|
@saschagrunert Looks like something went wrong with your rebase push? |
|
Yes, thanks for the hint. I'll have to check if the podman test failures in containers/podman#4696 are caused by this PR. 🤔 |
|
Looks like we have something which finally works. I will check it out with libpod |
|
Ready for review PTAL @vrothberg @rhatdan @nalind |
|
@saschagrunert could you rebase the other project just to double check those too? |
Yes, I updated those as well: |
layers.go
Outdated
| if conflict, ok := names[name]; ok { | ||
| r.removeName(conflict, name) | ||
| shouldSave = true | ||
| if r.Locked() { |
There was a problem hiding this comment.
Please add a comment why this only applies when the store is locked.
There was a problem hiding this comment.
Probably not needed, I will give a removal of this part another try.
There was a problem hiding this comment.
Looks like it is working, I'm now vendoring it into the related PRs.
|
Worked on Buildah, skopeo, libpod, CRI-O. This looks good. |
| @@ -2788,6 +2788,9 @@ func (s *store) Layers() ([]Layer, error) { | |||
| if err != nil { | |||
| return nil, err | |||
| } | |||
| if err := lstore.LoadLocked(); err != nil { | |||
| return nil, err | |||
| } | |||
There was a problem hiding this comment.
This unconditionally reloads the store contents using a write lock here, in addition to where it's conditionally done under a read lock on line 2805. We'll probably want to drop lstore from the initial value of the slice that's constructed on line 2800 to avoid that extra bit of work.
There was a problem hiding this comment.
Alright I put another commit on top of my changes to see if the new logic breaks anything.
|
I think there's a bit of work we can drop from |
The `layerstore.Load()` on `new[RO]LayerStore` required a held lock on store initialization. Since the consumers of the layer storage already call `Load()`, it should not be necessary to lock on initialization of the layer store. Signed-off-by: Sascha Grunert <[email protected]>
|
Seems to work, squashing commits and re-vendoring to: |
|
LGTM |
The
layerstore.Load()onnew[RO]LayerStorerequired a held lock onstore initialization. Since the consumers of the layer storage already
call
Load(), it should not be necessary to lock on initialization ofthe layer store.