From 07b3e716696b52c218506116edb2f44c22ad291c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Mon, 21 Nov 2022 23:07:36 +0100 Subject: [PATCH] Make change tracking error-resilient MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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č --- containers.go | 15 ++++++++----- images.go | 15 ++++++++----- layers.go | 62 ++++++++++++++++++++++++++++----------------------- 3 files changed, 52 insertions(+), 40 deletions(-) diff --git a/containers.go b/containers.go index 8d5cf7c29d..872618e682 100644 --- a/containers.go +++ b/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 -// call load(), not reloadIfChanged() (because the “if changed” state will not -// be detected again). +// reloadIfChanged() with lockedForWriting could succeed. 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. // diff --git a/images.go b/images.go index 07f20404de..1de0fe59f9 100644 --- a/images.go +++ b/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 -// call load(), not reloadIfChanged() (because the “if changed” state will not -// be detected again). +// reloadIfChanged() with lockedForWriting could succeed. 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. // diff --git a/layers.go b/layers.go index 597edd6927..e7fe52fd24 100644 --- a/layers.go +++ b/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. -func (r *layerStore) layersModified() (bool, error) { +// last recorded writer. If so, it returns a lockfile.LastWrite value to record on a successful +// 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 -// call load(), not reloadIfChanged() (because the “if changed” state will not -// be detected again). +// reloadIfChanged() with lockedForWriting could succeed. 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 - return r.load(lockedForWriting) + // r.load also reloads mounts data; so, on this path, we don’t need to call reloadMountsIfChanged. + 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.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 - } + // rlstore.mountsLastWrite is initialized inside rlstore.load(). if _, err := rlstore.load(true); err != nil { return nil, err }