diff --git a/layers.go b/layers.go index f16e32727e..4bdd750b0a 100644 --- a/layers.go +++ b/layers.go @@ -285,11 +285,15 @@ type rwLayerStore interface { } type layerStore struct { - lockfile Locker - mountsLockfile Locker - rundir string - driver drivers.Driver - layerdir string + // The following fields are only set when constructing layerStore, and must never be modified afterwards. They are safe to access without any other locking. + lockfile Locker + mountsLockfile Locker + rundir string + layerdir string + + // The following fields can only be read/written with read/write ownership of inProcessLock. Almost all callers should use startReading() or startWriting(). + // FIXME: Validate that the rules are actually like that. + inProcessLock sync.RWMutex // inProcessLock can ONLY be held with lockfile held. layers []*Layer idindex *truncindex.TruncIndex byid map[string]*Layer @@ -297,8 +301,10 @@ type layerStore struct { bymount map[string]*Layer bycompressedsum map[digest.Digest][]string byuncompressedsum map[digest.Digest][]string - loadMut sync.Mutex layerspathModified time.Time + + // FIXME: These fields have undocumented locking rules + driver drivers.Driver } func copyLayer(l *Layer) *Layer { @@ -333,9 +339,11 @@ func copyLayer(l *Layer) *Layer { // should use startWriting() instead. func (r *layerStore) startWritingWithReload(canReload bool) error { r.lockfile.Lock() + r.inProcessLock.Lock() succeeded := false defer func() { if !succeeded { + r.inProcessLock.Unlock() r.lockfile.Unlock() } }() @@ -358,6 +366,7 @@ func (r *layerStore) startWriting() error { // stopWriting releases locks obtained by startWriting. func (r *layerStore) stopWriting() { + r.inProcessLock.Unlock() r.lockfile.Unlock() } @@ -376,12 +385,24 @@ func (r *layerStore) startReadingWithReload(canReload bool) error { }() if canReload { - if err := r.reloadIfChanged(); err != nil { + if err := func() error { // A scope for defer + r.inProcessLock.Lock() + defer r.inProcessLock.Unlock() + return r.reloadIfChanged() + }(); err != nil { return err } } + // NOTE that we hold neither a read nor write inProcessLock at this point. That’s fine in ordinary operation, because + // the on-filesystem lock should protect us against (cooperating) writers, and any use of inProcessLock + // protects us against in-process writers modifying data. + // In presence of non-cooperating writers, we just ensure that 1) the in-memory data is not clearly out-of-date + // and 2) access to the in-memory data is not racy; + // but we can’t protect against those out-of-process writers modifying _files_ while we are assuming they are in a consistent state. + succeeded = true + r.inProcessLock.RLock() return nil } @@ -393,6 +414,7 @@ func (r *layerStore) startReading() error { // stopReading releases locks obtained by startReading. func (r *layerStore) stopReading() { + r.inProcessLock.RUnlock() r.lockfile.Unlock() } @@ -430,11 +452,9 @@ func (r *layerStore) modified() (bool, error) { return tmodified, nil } -// reloadIfChanged reloads the contents of the store from disk if it is changed. +// reloadIfChanged reloads the contents of the store from disk if it is changed. inProcessLock must be held locked for writing. +// FIXME: Document why a _write_ token is necessary (see the history of layerStore.loadMut) func (r *layerStore) reloadIfChanged() error { - r.loadMut.Lock() - defer r.loadMut.Unlock() - modified, err := r.modified() if err == nil && modified { return r.load()