Skip to content

Commit

Permalink
Make change tracking error-resilient
Browse files Browse the repository at this point in the history 
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>
  • Loading branch information
@mtrmac
mtrmac committed Nov 22, 2022
1 parent 2c40de9 commit 07b3e71
Show file tree
Hide file tree
Showing 3 changed files with 52 additions and 40 deletions.
15 changes: 9 additions & 6 deletions containers.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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()
Expand All @@ -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
}
Expand Down Expand Up @@ -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.
//
Expand Down
15 changes: 9 additions & 6 deletions images.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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()
Expand All @@ -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
}
Expand Down Expand Up @@ -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.
//
Expand Down
62 changes: 34 additions & 28 deletions layers.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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
Expand All @@ -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.
Expand All @@ -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()
Expand All @@ -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
}
Expand Down Expand Up @@ -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.
//
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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
}
Expand Down

0 comments on commit 07b3e71

Please sign in to comment.