-
Notifications
You must be signed in to change notification settings - Fork 241
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Document and improve store locking #1438
Conversation
This might not be entirely correct, but _some_ attempt to set rules must be better than nothing. Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Don't modify store.graphDriverName on reloads, so that it is constant throughout the life of the store, and it can be accessed without locking, as it has actually been done (to this point incorrectly). That requires special-casing the initial load(); so, split the actual driver creation into a createGraphDriverLocked() method. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
At least containers#926 suggests that using timestamps "seems to fail", without elaborating further. At the very least, ModifiedSince is the more general check, because it can work across shared filesystems or on filesystems with bad timestamp granularity, it's about as costly (a single syscall, pread vs. fstat), and we can now completely eliminate tracking store.lastLoaded. The more common code shape will also help factor the common code out further. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
That way we don't lock store.graphLock twice, and we will have only one location that doesn't use the shared "lock and reload" utility for using store.graphLock. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
... to avoid the repetitive checks for store.graphLock.ModifiedSince. store.getROLayerStores now checks for graphLock.ModifiedSince, we'll optimize that away again. store.Shutdown now checks for graphLock.ModifiedSince, that seems like a good idea anyway. Also attempt to document the purpose and rules of using graphLock; it's quite likely incomplete but at least it's a starting point. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
It's now the only caller, so: - inline it, eliminating an always-false graphDriver != nil check - only update graphLockLastWrite after we successfully reload. s.graphDriver is now never nil during the lifetime of store (but it is only safe to access with graphLock held.) Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
... and use it in callers that already obtain the graphLock. This is both an optimization, and a correctness fix: if we need to reload the graph driver, we now don't do the operation on an obsolete layerStore instance. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
22bd607
to
26d820c
Compare
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Now ready for review. Downstream tests: |
Added one more commit to document the locking hierarchy within |
store.go
Outdated
@@ -982,11 +982,8 @@ func (s *store) getLayerStore() (rwLayerStore, error) { | |||
|
|||
// getROLayerStores obtains additional read/only layer store objects used by the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is repeated I believe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, One potential issue with duplicated comments.
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
... and use it to only lock graphLock once in store.allLayerStores. More improvements to come. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
... and use it in store.Diff to only lock graphLock once, and to use a fresh layerStore object instead of an obsolete one. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
This allows to get all the layer stores with a single s.graphLock access. We do it even in store.CreateContainer, where the read-only layers are only necessarily conditionally, because getting the read-only layers is almost always a simple field access, so not having to lock graphLock again is almost certainly the improvement. Stand-alone store.getROLayerStores is now unused and has been removed. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
This was using the graphDriver field without locks, and the graph driver itself, while the implementation assumed exclusivity. Luckily all callers are actually holding the layer store lock for writing, so use that for exclusion. (layerStore already seems to extensively assume that locking the layer store for writing guarantees exclusive access to the graph driver, and because we always recreate a layer store after recreating the graph driver, that is true in practice.) Signed-off-by: Miloslav Trmač <mitr@redhat.com>
... to add an ...UseGetters suffix, to warn against direct use. This is not currently necessary, but we will encourage direct use of the container and image store fields, so the asymmetry vs. layer store objects needs to be warned against. Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
store.containerStore is always initilialized, and never nil, after the store is initialized, so store.getContainerStore() amounts to a single filed access. So just do that, and eliminate the error path and the local variables all over; just use s.containerStore directly. Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
The lambda can access s.containerStore directly. Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
The lambda can access s.containerStore directly. Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
store.imageStore and store.roImageStores are always initialized, and never nil, after the store is initialized, so store.getROImageStores() amounts to a single field access. So just do that, and eliminate the error path and the local variables all over; just use s.roImageStores directly. Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
store.imageStore is always initialized, and never nil, after the store is initialized, so store.getImageStore() amounts to a single field access. So just do that, and eliminate the error path and the local variables all over; just use s.imageStore directly. Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
The lambda can access s.imageStore directly. Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
The lambda can access s.imageStore directly. Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
It is now always nil, so simplify the callers. Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Just access s.imageStore directly. Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
7071fdc
to
273e81c
Compare
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Nice work!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Motivated by #1433 seeing unexpected layer store refreshes / existence of parallel layer stores for the same directory in a single process, this:
store
fields andstore.graphLock
. I’m not at all sure it’s correct, at least I want to establish the precedent that the design is expected to be documentedstore.graphLock
to guard againstShutdown
calls tostore.startUsingGraphDriver
/store.stopUsingGraphDriver
helpers, instead of copy&pasted and code slowly deviating over time. This is similar to the CILStore.startWriting
approach.store.graphLock
and can trigger a graph driver reload to actually use the reloaded graph driver for theirlayerStore
operations, instead of using an obsoletelayerStore
(and graph driver) instance immediately after the reload.layerStore
instances for the primary and additional stores, so that every operation only locksgraphLock
once instead of up to three times.imageStore
andcontainerStore
instances; the users can just refer to a constant field, so they are modified to do that, removing many error checks and local variablesstore
.This is on top of unmerged #1436.See individual commit messages for details.@nalind @giuseppe PTAL, it’s quite likely I’m missing something about the current locking implementation
Cc: @alexlarsson : this might significantly reduce the number of locking operations on
storage.lock
. I have no idea if it is noticeably faster, but it’s probably a noticeable change to thestrace
at least, so it might be necessary to track this separately from other changes if you are quantifying other work.