Make change tracking error-resilient

Only update recorded LastWrite values _after_ we succesfully reload
container/image/layer stores; so, if the reload fails, the functions
will keep failing instead of using obsolete (and possibly partially loaded
and completely invalid) data.

Also, further improve mount change tracking, so that if layerStore.load()
loads it, we don't reload it afterwards.

This does not change the store.graphLock users; they will need to be cleaned up
more comprehensively, and separately, in the future.

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

containers.go

@@ -264,7 +264,7 @@ func (r *containerStore) startReading() error {
 
 		r.lockfile.Lock()
 		unlockFn = r.lockfile.Unlock
-		if _, err := r.load(true); err != nil {
+		if _, err := r.reloadIfChanged(true); err != nil {
 			return err
 		}
 		unlockFn()
@@ -297,9 +297,7 @@ func (r *containerStore) stopReading() {
 // if it is held for writing.
 //
 // If !lockedForWriting and this function fails, the return value indicates whether
-// load() with lockedForWriting could succeed. In that case the caller MUST
+// reloadIfChanged() with lockedForWriting could succeed.
-// call load(), not reloadIfChanged() (because the “if changed” state will not
-// be detected again).
 func (r *containerStore) reloadIfChanged(lockedForWriting bool) (bool, error) {
 	r.loadMut.Lock()
 	defer r.loadMut.Unlock()
@@ -308,9 +306,11 @@ func (r *containerStore) reloadIfChanged(lockedForWriting bool) (bool, error) {
 	if err != nil {
 		return false, err
 	}
-	r.lastWrite = lastWrite
 	if modified {
-		return r.load(lockedForWriting)
+		if tryLockedForWriting, err := r.load(lockedForWriting); err != nil {
+			return tryLockedForWriting, err // r.lastWrite is unchanged, so we will load the next time again.
+		}
+		r.lastWrite = lastWrite
 	}
 	return false, nil
 }
@@ -366,6 +366,9 @@ func (r *containerStore) datapath(id, key string) string {
 
 // load reloads the contents of the store from disk.
 //
+// Most callers should call reloadIfChanged() instead, to avoid overhead and to correctly
+// manage r.lastWrite.
+//
 // The caller must hold r.lockfile for reading _or_ writing; lockedForWriting is true
 // if it is held for writing.
 //

images.go

@@ -257,7 +257,7 @@ func (r *imageStore) startReadingWithReload(canReload bool) error {
 
 			r.lockfile.Lock()
 			unlockFn = r.lockfile.Unlock
-			if _, err := r.load(true); err != nil {
+			if _, err := r.reloadIfChanged(true); err != nil {
 				return err
 			}
 			unlockFn()
@@ -297,9 +297,7 @@ func (r *imageStore) stopReading() {
 // if it is held for writing.
 //
 // If !lockedForWriting and this function fails, the return value indicates whether
-// retrying with lockedForWriting could succeed. In that case the caller MUST
+// reloadIfChanged() with lockedForWriting could succeed.
-// call load(), not reloadIfChanged() (because the “if changed” state will not
-// be detected again).
 func (r *imageStore) reloadIfChanged(lockedForWriting bool) (bool, error) {
 	r.loadMut.Lock()
 	defer r.loadMut.Unlock()
@@ -308,9 +306,11 @@ func (r *imageStore) reloadIfChanged(lockedForWriting bool) (bool, error) {
 	if err != nil {
 		return false, err
 	}
-	r.lastWrite = lastWrite
 	if modified {
-		return r.load(lockedForWriting)
+		if tryLockedForWriting, err := r.load(lockedForWriting); err != nil {
+			return tryLockedForWriting, err // r.lastWrite is unchanged, so we will load the next time again.
+		}
+		r.lastWrite = lastWrite
 	}
 	return false, nil
 }
@@ -377,6 +377,9 @@ func (i *Image) recomputeDigests() error {
 
 // load reloads the contents of the store from disk.
 //
+// Most callers should call reloadIfChanged() instead, to avoid overhead and to correctly
+// manage r.lastWrite.
+//
 // The caller must hold r.lockfile for reading _or_ writing; lockedForWriting is true
 // if it is held for writing.
 //

layers.go

@@ -421,7 +421,7 @@ func (r *layerStore) startReadingWithReload(canReload bool) error {
 
 			r.lockfile.Lock()
 			unlockFn = r.lockfile.Unlock
-			if _, err := r.load(true); err != nil {
+			if _, err := r.reloadIfChanged(true); err != nil {
 				return err
 			}
 			unlockFn()
@@ -451,15 +451,16 @@ func (r *layerStore) stopReading() {
 }
 
 // layersModified() checks if the most recent writer to r.jsonPath[] was a party other than the
-// last recorded writer.  It should only be called with the lock held.
+// last recorded writer. If so, it returns a lockfile.LastWrite value to record on a successful
-func (r *layerStore) layersModified() (bool, error) {
+// reload.
+// It should only be called with the lock held.
+func (r *layerStore) layersModified() (lockfile.LastWrite, bool, error) {
 	lastWrite, modified, err := r.lockfile.ModifiedSince(r.lastWrite)
 	if err != nil {
-		return modified, err
+		return lockfile.LastWrite{}, modified, err
 	}
-	r.lastWrite = lastWrite
 	if modified {
-		return true, nil
+		return lastWrite, true, nil
 	}
 
 	// If the layers.json file or container-layers.json has been
@@ -468,14 +469,15 @@ func (r *layerStore) layersModified() (bool, error) {
 	for locationIndex := 0; locationIndex < numLayerLocationIndex; locationIndex++ {
 		info, err := os.Stat(r.jsonPath[locationIndex])
 		if err != nil && !os.IsNotExist(err) {
-			return false, fmt.Errorf("stat layers file: %w", err)
+			return lockfile.LastWrite{}, false, fmt.Errorf("stat layers file: %w", err)
 		}
 		if info != nil && info.ModTime() != r.layerspathsModified[locationIndex] {
-			return true, nil
+			// In this case the LastWrite value is equal to r.lastWrite; writing it back doesn’t hurt.
+			return lastWrite, true, nil
 		}
 	}
 
-	return false, nil
+	return lockfile.LastWrite{}, false, nil
 }
 
 // reloadIfChanged reloads the contents of the store from disk if it is changed.
@@ -484,20 +486,22 @@ func (r *layerStore) layersModified() (bool, error) {
 // if it is held for writing.
 //
 // If !lockedForWriting and this function fails, the return value indicates whether
-// retrying with lockedForWriting could succeed. In that case the caller MUST
+// reloadIfChanged() with lockedForWriting could succeed.
-// call load(), not reloadIfChanged() (because the “if changed” state will not
-// be detected again).
 func (r *layerStore) reloadIfChanged(lockedForWriting bool) (bool, error) {
 	r.loadMut.Lock()
 	defer r.loadMut.Unlock()
 
-	layersModified, err := r.layersModified()
+	lastWrite, layersModified, err := r.layersModified()
 	if err != nil {
 		return false, err
 	}
 	if layersModified {
-		// r.load also reloads mounts data
+		// r.load also reloads mounts data; so, on this path, we don’t need to call reloadMountsIfChanged.
-		return r.load(lockedForWriting)
+		if tryLockedForWriting, err := r.load(lockedForWriting); err != nil {
+			return tryLockedForWriting, err // r.lastWrite is unchanged, so we will load the next time again.
+		}
+		r.lastWrite = lastWrite
+		return false, nil
 	}
 	if r.lockfile.IsReadWrite() {
 		r.mountsLockfile.RLock()
@@ -517,11 +521,11 @@ func (r *layerStore) reloadMountsIfChanged() error {
 	if err != nil {
 		return err
 	}
-	r.mountsLastWrite = lastWrite
 	if modified {
 		if err = r.loadMounts(); err != nil {
 			return err
 		}
+		r.mountsLastWrite = lastWrite
 	}
 	return nil
 }
@@ -567,6 +571,11 @@ func (r *layerStore) mountspath() string {
 
 // load reloads the contents of the store from disk.
 //
+// Most callers should call reloadIfChanged() instead, to avoid overhead and to correctly
+// manage r.lastWrite.
+//
+// As a side effect, this sets r.mountsLastWrite.
+//
 // The caller must hold r.lockfile for reading _or_ writing; lockedForWriting is true
 // if it is held for writing.
 //
@@ -671,9 +680,17 @@ func (r *layerStore) load(lockedForWriting bool) (bool, error) {
 	if r.lockfile.IsReadWrite() {
 		r.mountsLockfile.RLock()
 		defer r.mountsLockfile.Unlock()
+		// We need to reload mounts unconditionally, becuause by creating r.layers from scratch, we have discarded the previous
+		// information, if any. So, obtain a fresh mountsLastWrite value so that we don’t unnecessarily reload the data
+		// afterwards.
+		mountsLastWrite, err := r.mountsLockfile.GetLastWrite()
+		if err != nil {
+			return false, err
+		}
 		if err := r.loadMounts(); err != nil {
 			return false, err
 		}
+		r.mountsLastWrite = mountsLastWrite
 	}
 
 	if errorToResolveBySaving != nil {
@@ -888,18 +905,7 @@ func (s *store) newLayerStore(rundir string, layerdir string, driver drivers.Dri
 		return nil, err
 	}
 	rlstore.lastWrite = lw
-	if err := func() error { // A scope for defer
+	// rlstore.mountsLastWrite is initialized inside rlstore.load().
-		rlstore.mountsLockfile.RLock()
-		defer rlstore.mountsLockfile.Unlock()
-		lw, err = rlstore.mountsLockfile.GetLastWrite()
-		if err != nil {
-			return err
-		}
-		rlstore.mountsLastWrite = lw
-		return nil
-	}(); err != nil {
-		return nil, err
-	}
 	if _, err := rlstore.load(true); err != nil {
 		return nil, err
 	}