Be clearer about the layer store locking rules

... as modified by #2036 .

Also document a LOCKING BUG I don't now care to fix.

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

drivers/overlay/overlay.go

@@ -1878,10 +1878,21 @@ func (d *Driver) get(id string, disableShifting bool, options graphdriver.MountO
 
 // getMergedDir returns the directory path that should be used as the mount point for the overlayfs.
 func (d *Driver) getMergedDir(id, dir string, inAdditionalStore bool) string {
-	// If the layer is in an additional store, the lock we might hold only a reading lock.  To prevent
-	// races with other processes, use a private directory under the main store rundir.  At this point, the
-	// current process is holding an exclusive lock on the store, and since the rundir cannot be shared for
-	// different stores, it is safe to assume the current process has exclusive access to it.
+	// Ordinarily, .Get() (layer mounting) callers are supposed to guarantee exclusion.
+	//
+	// But additional stores are initialized with RO locks and don’t support a write
+	// lock operation at all; and naiveDiff operations cause mounts/unmounts, so they might
+	// happen on code paths where we might only holding a RO lock for the additional store.
+	// To prevent races with other processes mounting or unmounting the layer,
+	// use a private directory under the main store rundir, not the "merged" directory inside the
+	// original layer store holding the layer data.
+	//
+	// To support this, contrary to the _general_ locking rules for .Diff / .Changes (which allow a RO lock),
+	// the top-level Store implementation uses an exclusive lock for the primary layer store;
+	// and since the rundir cannot be shared for different stores, it is safe to assume the
+	// current process has exclusive access to it.
+	//
+	// LOCKING BUG? the .DiffSize operation does not currently hold an exclusive lock on the primary store.
 	if inAdditionalStore {
 		return path.Join(d.runhome, id, "merged")
 	}

layers.go

@@ -2095,6 +2095,9 @@ func (r *layerStore) layerMappings(layer *Layer) *idtools.IDMappings {
 }
 
 // Requires startReading or startWriting.
+//
+// NOTE: Overlay’s implementation assumes use of an exclusive lock over the primary layer store,
+// see drivers/overlay.Driver.getMergedDir.
 func (r *layerStore) Changes(from, to string) ([]archive.Change, error) {
 	from, to, fromLayer, toLayer, err := r.findParentAndLayer(from, to)
 	if err != nil {
@@ -2161,6 +2164,9 @@ func writeCompressedDataGoroutine(pwriter *io.PipeWriter, compressor io.WriteClo
 }
 
 // Requires startReading or startWriting.
+//
+// NOTE: Overlay’s implementation assumes use of an exclusive lock over the primary layer store,
+// see drivers/overlay.Driver.getMergedDir.
 func (r *layerStore) Diff(from, to string, options *DiffOptions) (io.ReadCloser, error) {
 	var metadata storage.Unpacker
 

store.go

@@ -2963,6 +2963,10 @@ func (s *store) Changes(from, to string) ([]archive.Change, error) {
 	if err != nil {
 		return nil, err
 	}
+
+	// While the general rules require the layer store to only be locked RO (apart from known LOCKING BUGs)
+	// the overlay driver requires the primary layer store to be locked RW; see
+	// drivers/overlay.Driver.getMergedDir.
 	if err := rlstore.startWriting(); err != nil {
 		return nil, err
 	}
@@ -3019,6 +3023,9 @@ func (s *store) Diff(from, to string, options *DiffOptions) (io.ReadCloser, erro
 		return nil, err
 	}
 
+	// While the general rules require the layer store to only be locked RO (apart from known LOCKING BUGs)
+	// the overlay driver requires the primary layer store to be locked RW; see
+	// drivers/overlay.Driver.getMergedDir.
 	if err := rlstore.startWriting(); err != nil {
 		return nil, err
 	}