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

@@ -301,20 +301,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 layerReadAccess() or layerWriteAccess().
+	// 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 {
@@ -358,6 +364,7 @@ func (r *layerStore) layerspath() string {
 	return filepath.Join(r.layerdir, "layers.json")
 }
 
+// FIXME: Use layerWriteToken
 func (r *layerStore) Load() error {
 	shouldSave := false
 	rpath := r.layerspath()
@@ -613,7 +620,8 @@ func newROLayerStore(rundir string, layerdir string, driver drivers.Driver) (roL
 // DO NOT CALL THIS from outside of layerStore.
 // Almost all users should call layerReadAccess() instead.
 func (r *layerStore) privateWithLayerReadToken(fn func(token layerReadToken)) {
-	// TO DO: Actually make this safe against concurrent writers
+	r.inProcessLock.RLock()
+	defer r.inProcessLock.RUnlock()
 	token := layerReadToken{}
 	fn(token)
 }
@@ -622,7 +630,8 @@ func (r *layerStore) privateWithLayerReadToken(fn func(token layerReadToken)) {
 // DO NOT CALL THIS from outside of layerStore.
 // Almost all users should call layerWriteAccess() instead.
 func (r *layerStore) privateWithLayerWriteToken(fn func(token layerWriteToken)) {
-	// TO DO: Actually make this safe against concurrent writers
+	r.inProcessLock.Lock()
+	defer r.inProcessLock.Unlock()
 	token := layerWriteToken{}
 	fn(token)
 }
@@ -651,14 +660,26 @@ func layerReadAccess[T any](r roLayerStore, fn func(token layerReadToken) (T, er
 	if !ok {
 		return res, errors.New("internal error: roLayerStore is not a *layerStore"), true
 	}
+	// CAREFUL: We can now technically call all methods of layerStore, including the read-write ones,
+	// on layerStores that were created as read-only.
+	// FIXME: Is there a way to protect that better?
 
 	r.RLock()
 	defer r.Unlock()
-	if err := r.ReloadIfChanged(); err != nil {
+	var err error
+	store.privateWithLayerWriteToken(func(token layerWriteToken) {
+		err = store.reloadIfChanged(token)
+	})
+	if err != nil {
 		return res, err, true
 	}
+	// NOTE that we hold neither a read nor write token 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.
 	var done bool
-	var err error
 	store.privateWithLayerReadToken(func(token layerReadToken) {
 		res, err, done = fn(token)
 	})
@@ -679,11 +700,11 @@ func layerWriteAccess(r rwLayerStore, fn func(token layerWriteToken) error) erro
 
 	r.Lock()
 	defer r.Unlock()
-	if err := r.ReloadIfChanged(); err != nil {
-		return err
-	}
 	var err error
 	store.privateWithLayerWriteToken(func(token layerWriteToken) {
+		if err = store.reloadIfChanged(token); err != nil {
+			return
+		}
 		err = fn(token)
 	})
 	return err
@@ -2033,6 +2054,8 @@ func (r *layerStore) Touch() error {
 	return r.lockfile.Touch()
 }
 
+// FIXME: Use layerWriteToken
+// FIXME: Document why a _write_ token is necessary (see the history of layerStore.loadMut)
 func (r *layerStore) Modified() (bool, error) {
 	var mmodified, tmodified bool
 	lmodified, err := r.lockfile.Modified()
@@ -2077,10 +2100,16 @@ func (r *layerStore) Locked() bool {
 	return r.lockfile.Locked()
 }
 
+// Deprecated: Use reloadIfChanged, or almost always higher-level interfaces.
 func (r *layerStore) ReloadIfChanged() error {
-	r.loadMut.Lock()
-	defer r.loadMut.Unlock()
+	var err error
+	r.privateWithLayerWriteToken(func(token layerWriteToken) {
+		err = r.reloadIfChanged(token)
+	})
+	return err
+}
 
+func (r *layerStore) reloadIfChanged(token layerWriteToken) error {
 	modified, err := r.Modified()
 	if err == nil && modified {
 		return r.Load()

store.go

@@ -62,6 +62,8 @@ type roFileBasedStore interface {
 	Load() error
 
 	// ReloadIfChanged reloads the contents of the store from disk if it is changed.
+	//
+	// Deprecated: Use reloadIfChanged, or almost always higher-level interfaces.
 	ReloadIfChanged() error
 }