PROTOTYPE: Turn loadMut into inProcessLock

- Clearly separate layerStore fields by how concurrent
  access is governed (might not be actually accureate at this point)

- Then turn loadMut (which currently only ensures exclusion
  inside ReloadIfChanged) into a RWMutex that protects all readers
  against changes by ReloadIfChanged.

- This adds a new synchronization on the _bodies_ of layerReadAccess,
  potentially slowing them down _in presence of external writers_ only.

- OTOH in layerWriteAccess, we use a single lock scope for both
  reloadIfChanged and the body, which makes it clear that the lock
  scope is actually exactly the same as the scope of r.Lock(),
  and the total overhead should be almost the same as well.

  (Possibly investigate only having one of inProcessLock
  and layerStore.Locker.rwMutex. We might not need both.)

Signed-off-by: Miloslav Trmač <mitr@redhat.com>

layers.go

@@ -285,20 +285,26 @@ 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
 	byname             map[string]*Layer
 	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()