From c83efd0f0721dcc3cf3ceb35c77d8fd8713b7f77 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Mon, 21 Nov 2022 23:24:04 +0100 Subject: [PATCH] Update c/storage after https://github.com/containers/storage/pull/1436 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ... and update to remove the now-deprecated Locker interface. Signed-off-by: Miloslav Trmač --- go.mod | 2 +- go.sum | 4 +- libpod/container_internal.go | 2 +- libpod/container_internal_common.go | 2 +- libpod/events/logfile.go | 2 +- libpod/lock/file/file_lock.go | 6 +- libpod/networking_common.go | 4 +- libpod/networking_freebsd.go | 2 +- libpod/networking_linux.go | 4 +- libpod/networking_unsupported.go | 2 +- libpod/reset.go | 3 +- libpod/runtime.go | 3 +- pkg/rootless/rootless.go | 2 +- test/e2e/common_test.go | 6 +- .../containers/storage/containers.go | 33 ++- .../github.com/containers/storage/images.go | 53 +++-- .../github.com/containers/storage/layers.go | 147 ++++++++---- .../containers/storage/lockfile_compat.go | 5 +- .../storage/pkg/lockfile/lockfile.go | 61 +++-- .../storage/pkg/lockfile/lockfile_unix.go | 219 ++++++++++++++---- .../storage/pkg/lockfile/lockfile_windows.go | 96 ++++++-- vendor/github.com/containers/storage/store.go | 81 ++++--- vendor/modules.txt | 2 +- 23 files changed, 531 insertions(+), 210 deletions(-) diff --git a/go.mod b/go.mod index 2346154545..dd95c586f8 100644 --- a/go.mod +++ b/go.mod @@ -17,7 +17,7 @@ require ( github.com/containers/image/v5 v5.23.1-0.20221130170538-333c50e3eac8 github.com/containers/ocicrypt v1.1.6 github.com/containers/psgo v1.8.0 - github.com/containers/storage v1.44.1-0.20221121144727-71fd3e87df7a + github.com/containers/storage v1.44.1-0.20221201083122-c5a80ad65f42 github.com/coreos/go-systemd/v22 v22.5.0 github.com/coreos/stream-metadata-go v0.0.0-20210225230131-70edb9eb47b3 github.com/cyphar/filepath-securejoin v0.2.3 diff --git a/go.sum b/go.sum index 208ce4f29c..c9eebef20e 100644 --- a/go.sum +++ b/go.sum @@ -281,8 +281,8 @@ github.com/containers/psgo v1.8.0 h1:2loGekmGAxM9ir5OsXWEfGwFxorMPYnc6gEDsGFQvhY github.com/containers/psgo v1.8.0/go.mod h1:T8ZxnX3Ur4RvnhxFJ7t8xJ1F48RhiZB4rSrOaR/qGHc= github.com/containers/storage v1.37.0/go.mod h1:kqeJeS0b7DO2ZT1nVWs0XufrmPFbgV3c+Q/45RlH6r4= github.com/containers/storage v1.43.0/go.mod h1:uZ147thiIFGdVTjMmIw19knttQnUCl3y9zjreHrg11s= -github.com/containers/storage v1.44.1-0.20221121144727-71fd3e87df7a h1:Kds8yAenoKQ7d95T+2oOfnLJpxPAwG9grUf0lIA4JJs= -github.com/containers/storage v1.44.1-0.20221121144727-71fd3e87df7a/go.mod h1:pYkSXaKIGAuEQmIf/melI5wbS/JBM++6Xp4JuVTqY7U= +github.com/containers/storage v1.44.1-0.20221201083122-c5a80ad65f42 h1:lba+h0VcMGvO/C4Q+oMhGxpgajzyQifbcedOYQNVRx8= +github.com/containers/storage v1.44.1-0.20221201083122-c5a80ad65f42/go.mod h1:pYkSXaKIGAuEQmIf/melI5wbS/JBM++6Xp4JuVTqY7U= github.com/coreos/bbolt v1.3.2/go.mod h1:iRUV2dpdMOn7Bo10OQBFzIJO9kkE559Wcmn+qkEiiKk= github.com/coreos/etcd v3.3.10+incompatible/go.mod h1:uF7uidLiAD3TWHmW31ZFd/JWoc32PjwdhPthX9715RE= github.com/coreos/go-iptables v0.4.5/go.mod h1:/mVI274lEDI2ns62jHCDnCyBF9Iwsmekav8Dbxlm1MU= diff --git a/libpod/container_internal.go b/libpod/container_internal.go index 9926bdfa27..dcc3e1ae34 100644 --- a/libpod/container_internal.go +++ b/libpod/container_internal.go @@ -1884,7 +1884,7 @@ func (c *Container) cleanup(ctx context.Context) error { if hoststFile, ok := c.state.BindMounts[config.DefaultHostsFile]; ok { if _, err := os.Stat(hoststFile); err == nil { // we cannot use the dependency container lock due ABBA deadlocks - if lock, err := lockfile.GetLockfile(hoststFile); err == nil { + if lock, err := lockfile.GetLockFile(hoststFile); err == nil { lock.Lock() // make sure to ignore ENOENT error in case the netns container was cleaned up before this one if err := etchosts.Remove(hoststFile, getLocalhostHostEntry(c)); err != nil && !errors.Is(err, os.ErrNotExist) { diff --git a/libpod/container_internal_common.go b/libpod/container_internal_common.go index 065868638f..bfb5d233e4 100644 --- a/libpod/container_internal_common.go +++ b/libpod/container_internal_common.go @@ -1685,7 +1685,7 @@ func (c *Container) makeBindMounts() error { hostsPath, exists := bindMounts[config.DefaultHostsFile] if !c.config.UseImageHosts && exists { // we cannot use the dependency container lock due ABBA deadlocks in cleanup() - lock, err := lockfile.GetLockfile(hostsPath) + lock, err := lockfile.GetLockFile(hostsPath) if err != nil { return fmt.Errorf("failed to lock hosts file: %w", err) } diff --git a/libpod/events/logfile.go b/libpod/events/logfile.go index bb0f461e3f..b0872bf4cf 100644 --- a/libpod/events/logfile.go +++ b/libpod/events/logfile.go @@ -45,7 +45,7 @@ func newLogFileEventer(options EventerOptions) (*EventLogFile, error) { // Writes to the log file func (e EventLogFile) Write(ee Event) error { // We need to lock events file - lock, err := lockfile.GetLockfile(e.options.LogFilePath + ".lock") + lock, err := lockfile.GetLockFile(e.options.LogFilePath + ".lock") if err != nil { return err } diff --git a/libpod/lock/file/file_lock.go b/libpod/lock/file/file_lock.go index bcbaea5e6e..e26ca24af9 100644 --- a/libpod/lock/file/file_lock.go +++ b/libpod/lock/file/file_lock.go @@ -7,7 +7,7 @@ import ( "strconv" "syscall" - "github.com/containers/storage" + "github.com/containers/storage/pkg/lockfile" "github.com/sirupsen/logrus" ) @@ -150,7 +150,7 @@ func (locks *FileLocks) LockFileLock(lck uint32) error { return fmt.Errorf("locks have already been closed: %w", syscall.EINVAL) } - l, err := storage.GetLockfile(locks.getLockPath(lck)) + l, err := lockfile.GetLockFile(locks.getLockPath(lck)) if err != nil { return fmt.Errorf("acquiring lock: %w", err) } @@ -164,7 +164,7 @@ func (locks *FileLocks) UnlockFileLock(lck uint32) error { if !locks.valid { return fmt.Errorf("locks have already been closed: %w", syscall.EINVAL) } - l, err := storage.GetLockfile(locks.getLockPath(lck)) + l, err := lockfile.GetLockFile(locks.getLockPath(lck)) if err != nil { return fmt.Errorf("acquiring lock: %w", err) } diff --git a/libpod/networking_common.go b/libpod/networking_common.go index 4068e4e0a2..101435e141 100644 --- a/libpod/networking_common.go +++ b/libpod/networking_common.go @@ -451,7 +451,7 @@ func (c *Container) NetworkDisconnect(nameOrID, netName string, force bool) erro if len(rm) > 0 { // make sure to lock this file to prevent concurrent writes when // this is used a net dependency container - lock, err := lockfile.GetLockfile(file) + lock, err := lockfile.GetLockFile(file) if err != nil { return fmt.Errorf("failed to lock hosts file: %w", err) } @@ -591,7 +591,7 @@ func (c *Container) NetworkConnect(nameOrID, netName string, netOpts types.PerNe if file, ok := c.state.BindMounts[config.DefaultHostsFile]; ok { // make sure to lock this file to prevent concurrent writes when // this is used a net dependency container - lock, err := lockfile.GetLockfile(file) + lock, err := lockfile.GetLockFile(file) if err != nil { return fmt.Errorf("failed to lock hosts file: %w", err) } diff --git a/libpod/networking_freebsd.go b/libpod/networking_freebsd.go index 0c2168c96a..9f5c2e6b6a 100644 --- a/libpod/networking_freebsd.go +++ b/libpod/networking_freebsd.go @@ -75,7 +75,7 @@ type LinkStatistics64 struct { type RootlessNetNS struct { dir string - Lock lockfile.Locker + Lock *lockfile.LockFile } // getPath will join the given path to the rootless netns dir diff --git a/libpod/networking_linux.go b/libpod/networking_linux.go index 300324b3b2..d5827839bb 100644 --- a/libpod/networking_linux.go +++ b/libpod/networking_linux.go @@ -53,7 +53,7 @@ const ( type RootlessNetNS struct { ns ns.NetNS dir string - Lock lockfile.Locker + Lock *lockfile.LockFile } // getPath will join the given path to the rootless netns dir @@ -333,7 +333,7 @@ func (r *Runtime) GetRootlessNetNs(new bool) (*RootlessNetNS, error) { runDir := r.config.Engine.TmpDir lfile := filepath.Join(runDir, "rootless-netns.lock") - lock, err := lockfile.GetLockfile(lfile) + lock, err := lockfile.GetLockFile(lfile) if err != nil { return nil, fmt.Errorf("failed to get rootless-netns lockfile: %w", err) } diff --git a/libpod/networking_unsupported.go b/libpod/networking_unsupported.go index ab4c8b08f1..7163a02dd0 100644 --- a/libpod/networking_unsupported.go +++ b/libpod/networking_unsupported.go @@ -15,7 +15,7 @@ import ( type RootlessNetNS struct { dir string - Lock lockfile.Locker + Lock *lockfile.LockFile } // ocicniPortsToNetTypesPorts convert the old port format to the new one diff --git a/libpod/reset.go b/libpod/reset.go index e9f4cf94e6..fb157c4c09 100644 --- a/libpod/reset.go +++ b/libpod/reset.go @@ -14,6 +14,7 @@ import ( "github.com/containers/podman/v4/pkg/rootless" "github.com/containers/podman/v4/pkg/util" "github.com/containers/storage" + "github.com/containers/storage/pkg/lockfile" stypes "github.com/containers/storage/types" "github.com/sirupsen/logrus" ) @@ -35,7 +36,7 @@ func (r *Runtime) removeAllDirs() error { // TODO: maybe want a helper for getting the path? This is duped from // runtime.go runtimeAliveLock := filepath.Join(r.config.Engine.TmpDir, "alive.lck") - aliveLock, err := storage.GetLockfile(runtimeAliveLock) + aliveLock, err := lockfile.GetLockFile(runtimeAliveLock) if err != nil { logrus.Errorf("Lock runtime alive lock %s: %v", runtimeAliveLock, err) } else { diff --git a/libpod/runtime.go b/libpod/runtime.go index d80add032b..16010874be 100644 --- a/libpod/runtime.go +++ b/libpod/runtime.go @@ -35,6 +35,7 @@ import ( "github.com/containers/podman/v4/pkg/util" "github.com/containers/podman/v4/utils" "github.com/containers/storage" + "github.com/containers/storage/pkg/lockfile" "github.com/containers/storage/pkg/unshare" "github.com/docker/docker/pkg/namesgenerator" spec "github.com/opencontainers/runtime-spec/specs-go" @@ -539,7 +540,7 @@ func makeRuntime(runtime *Runtime) (retErr error) { // This check must be locked to prevent races runtimeAliveLock := filepath.Join(runtime.config.Engine.TmpDir, "alive.lck") runtimeAliveFile := filepath.Join(runtime.config.Engine.TmpDir, "alive") - aliveLock, err := storage.GetLockfile(runtimeAliveLock) + aliveLock, err := lockfile.GetLockFile(runtimeAliveLock) if err != nil { return fmt.Errorf("acquiring runtime init lock: %w", err) } diff --git a/pkg/rootless/rootless.go b/pkg/rootless/rootless.go index a32ff697d9..5dc5a535e2 100644 --- a/pkg/rootless/rootless.go +++ b/pkg/rootless/rootless.go @@ -29,7 +29,7 @@ func TryJoinPauseProcess(pausePidPath string) (bool, int, error) { } // It could not join the pause process, let's lock the file before trying to delete it. - pidFileLock, err := lockfile.GetLockfile(pausePidPath) + pidFileLock, err := lockfile.GetLockFile(pausePidPath) if err != nil { // The file was deleted by another process. if os.IsNotExist(err) { diff --git a/test/e2e/common_test.go b/test/e2e/common_test.go index 31b121d077..65b957751a 100644 --- a/test/e2e/common_test.go +++ b/test/e2e/common_test.go @@ -23,7 +23,7 @@ import ( "github.com/containers/podman/v4/pkg/rootless" "github.com/containers/podman/v4/pkg/util" . "github.com/containers/podman/v4/test/utils" - "github.com/containers/storage" + "github.com/containers/storage/pkg/lockfile" "github.com/containers/storage/pkg/reexec" "github.com/containers/storage/pkg/stringid" jsoniter "github.com/json-iterator/go" @@ -402,9 +402,9 @@ func processTestResult(f GinkgoTestDescription) { testResultsMutex.Unlock() } -func GetPortLock(port string) storage.Locker { +func GetPortLock(port string) *lockfile.LockFile { lockFile := filepath.Join(LockTmpDir, port) - lock, err := storage.GetLockfile(lockFile) + lock, err := lockfile.GetLockFile(lockFile) if err != nil { fmt.Println(err) os.Exit(1) diff --git a/vendor/github.com/containers/storage/containers.go b/vendor/github.com/containers/storage/containers.go index 7db03d37c3..872618e682 100644 --- a/vendor/github.com/containers/storage/containers.go +++ b/vendor/github.com/containers/storage/containers.go @@ -10,6 +10,7 @@ import ( "github.com/containers/storage/pkg/idtools" "github.com/containers/storage/pkg/ioutils" + "github.com/containers/storage/pkg/lockfile" "github.com/containers/storage/pkg/stringid" "github.com/containers/storage/pkg/truncindex" digest "github.com/opencontainers/go-digest" @@ -140,9 +141,10 @@ type rwContainerStore interface { } type containerStore struct { - lockfile Locker + lockfile *lockfile.LockFile dir string jsonPath [numContainerLocationIndex]string + lastWrite lockfile.LastWrite containers []*Container idindex *truncindex.TruncIndex byid map[string]*Container @@ -262,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() @@ -295,19 +297,20 @@ 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() - modified, err := r.lockfile.Modified() + lastWrite, modified, err := r.lockfile.ModifiedSince(r.lastWrite) if err != nil { return false, err } 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 } @@ -363,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. // @@ -469,7 +475,12 @@ func (r *containerStore) save(saveLocations containerLocations) error { return err } } - return r.lockfile.Touch() + lw, err := r.lockfile.RecordWrite() + if err != nil { + return err + } + r.lastWrite = lw + return nil } func (r *containerStore) saveFor(modifiedContainer *Container) error { @@ -487,7 +498,7 @@ func newContainerStore(dir string, runDir string, transient bool) (rwContainerSt } volatileDir = runDir } - lockfile, err := GetLockfile(filepath.Join(volatileDir, "containers.lock")) + lockfile, err := lockfile.GetLockFile(filepath.Join(volatileDir, "containers.lock")) if err != nil { return nil, err } @@ -507,6 +518,10 @@ func newContainerStore(dir string, runDir string, transient bool) (rwContainerSt if err := cstore.startWritingWithReload(false); err != nil { return nil, err } + cstore.lastWrite, err = cstore.lockfile.GetLastWrite() + if err != nil { + return nil, err + } defer cstore.stopWriting() if _, err := cstore.load(true); err != nil { return nil, err diff --git a/vendor/github.com/containers/storage/images.go b/vendor/github.com/containers/storage/images.go index 5d467daf74..1de0fe59f9 100644 --- a/vendor/github.com/containers/storage/images.go +++ b/vendor/github.com/containers/storage/images.go @@ -9,6 +9,7 @@ import ( "time" "github.com/containers/storage/pkg/ioutils" + "github.com/containers/storage/pkg/lockfile" "github.com/containers/storage/pkg/stringid" "github.com/containers/storage/pkg/stringutils" "github.com/containers/storage/pkg/truncindex" @@ -156,14 +157,15 @@ type rwImageStore interface { } type imageStore struct { - lockfile Locker // lockfile.IsReadWrite can be used to distinguish between read-write and read-only image stores. - dir string - images []*Image - idindex *truncindex.TruncIndex - byid map[string]*Image - byname map[string]*Image - bydigest map[digest.Digest][]*Image - loadMut sync.Mutex + lockfile *lockfile.LockFile // lockfile.IsReadWrite can be used to distinguish between read-write and read-only image stores. + dir string + lastWrite lockfile.LastWrite + images []*Image + idindex *truncindex.TruncIndex + byid map[string]*Image + byname map[string]*Image + bydigest map[digest.Digest][]*Image + loadMut sync.Mutex } func copyImage(i *Image) *Image { @@ -255,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() @@ -295,19 +297,20 @@ 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() - modified, err := r.lockfile.Modified() + lastWrite, modified, err := r.lockfile.ModifiedSince(r.lastWrite) if err != nil { return false, err } 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 } @@ -374,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. // @@ -457,14 +463,19 @@ func (r *imageStore) Save() error { if err := ioutils.AtomicWriteFile(rpath, jdata, 0600); err != nil { return err } - return r.lockfile.Touch() + lw, err := r.lockfile.RecordWrite() + if err != nil { + return err + } + r.lastWrite = lw + return nil } func newImageStore(dir string) (rwImageStore, error) { if err := os.MkdirAll(dir, 0700); err != nil { return nil, err } - lockfile, err := GetLockfile(filepath.Join(dir, "images.lock")) + lockfile, err := lockfile.GetLockFile(filepath.Join(dir, "images.lock")) if err != nil { return nil, err } @@ -480,6 +491,10 @@ func newImageStore(dir string) (rwImageStore, error) { return nil, err } defer istore.stopWriting() + istore.lastWrite, err = istore.lockfile.GetLastWrite() + if err != nil { + return nil, err + } if _, err := istore.load(true); err != nil { return nil, err } @@ -487,7 +502,7 @@ func newImageStore(dir string) (rwImageStore, error) { } func newROImageStore(dir string) (roImageStore, error) { - lockfile, err := GetROLockfile(filepath.Join(dir, "images.lock")) + lockfile, err := lockfile.GetROLockFile(filepath.Join(dir, "images.lock")) if err != nil { return nil, err } @@ -503,6 +518,10 @@ func newROImageStore(dir string) (roImageStore, error) { return nil, err } defer istore.stopReading() + istore.lastWrite, err = istore.lockfile.GetLastWrite() + if err != nil { + return nil, err + } if _, err := istore.load(false); err != nil { return nil, err } diff --git a/vendor/github.com/containers/storage/layers.go b/vendor/github.com/containers/storage/layers.go index 3b580e2009..e7fe52fd24 100644 --- a/vendor/github.com/containers/storage/layers.go +++ b/vendor/github.com/containers/storage/layers.go @@ -18,6 +18,7 @@ import ( "github.com/containers/storage/pkg/archive" "github.com/containers/storage/pkg/idtools" "github.com/containers/storage/pkg/ioutils" + "github.com/containers/storage/pkg/lockfile" "github.com/containers/storage/pkg/mount" "github.com/containers/storage/pkg/stringid" "github.com/containers/storage/pkg/system" @@ -301,12 +302,14 @@ type rwLayerStore interface { } type layerStore struct { - lockfile Locker - mountsLockfile Locker + lockfile *lockfile.LockFile + mountsLockfile *lockfile.LockFile rundir string jsonPath [numLayerLocationIndex]string driver drivers.Driver layerdir string + lastWrite lockfile.LastWrite + mountsLastWrite lockfile.LastWrite // Only valid if lockfile.IsReadWrite() layers []*Layer idindex *truncindex.TruncIndex byid map[string]*Layer @@ -418,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() @@ -447,25 +450,17 @@ func (r *layerStore) stopReading() { r.lockfile.Unlock() } -// Modified() checks if the most recent writer was a party other than the -// last recorded writer. It should only be called with the lock held. -func (r *layerStore) Modified() (bool, error) { - var mmodified bool - lmodified, err := r.lockfile.Modified() +// layersModified() checks if the most recent writer to r.jsonPath[] was a party other than the +// 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 lmodified, err + return lockfile.LastWrite{}, modified, err } - if r.lockfile.IsReadWrite() { - r.mountsLockfile.RLock() - defer r.mountsLockfile.Unlock() - mmodified, err = r.mountsLockfile.Modified() - if err != nil { - return lmodified, err - } - } - - if lmodified || mmodified { - return true, nil + if modified { + return lastWrite, true, nil } // If the layers.json file or container-layers.json has been @@ -474,14 +469,15 @@ func (r *layerStore) Modified() (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. @@ -490,23 +486,50 @@ func (r *layerStore) Modified() (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() - modified, err := r.Modified() + lastWrite, layersModified, err := r.layersModified() if err != nil { return false, err } - if modified { - return r.load(lockedForWriting) + if layersModified { + // 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() + defer r.mountsLockfile.Unlock() + if err := r.reloadMountsIfChanged(); err != nil { + return false, err + } } return false, nil } +// reloadMountsIfChanged reloads the contents of mountsPath from disk if it is changed. +// +// The caller must hold r.mountsLockFile for reading or writing. +func (r *layerStore) reloadMountsIfChanged() error { + lastWrite, modified, err := r.mountsLockfile.ModifiedSince(r.mountsLastWrite) + if err != nil { + return err + } + if modified { + if err = r.loadMounts(); err != nil { + return err + } + r.mountsLastWrite = lastWrite + } + return nil +} + func (r *layerStore) Layers() ([]Layer, error) { layers := make([]Layer, len(r.layers)) for i := range r.layers { @@ -548,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. // @@ -652,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 { @@ -779,8 +815,12 @@ func (r *layerStore) saveLayers(saveLocations layerLocations) error { if err := ioutils.AtomicWriteFileWithOpts(rpath, jldata, 0600, opts); err != nil { return err } - return r.lockfile.Touch() } + lw, err := r.lockfile.RecordWrite() + if err != nil { + return err + } + r.lastWrite = lw return nil } @@ -810,9 +850,11 @@ func (r *layerStore) saveMounts() error { if err = ioutils.AtomicWriteFile(mpath, jmdata, 0600); err != nil { return err } - if err := r.mountsLockfile.Touch(); err != nil { + lw, err := r.mountsLockfile.RecordWrite() + if err != nil { return err } + r.mountsLastWrite = lw return r.loadMounts() } @@ -828,11 +870,11 @@ func (s *store) newLayerStore(rundir string, layerdir string, driver drivers.Dri // layers.json might be used externally as a read-only layer (using e.g. // additionalimagestores), and that would look for the lockfile in the // same directory - lockfile, err := GetLockfile(filepath.Join(layerdir, "layers.lock")) + lockFile, err := lockfile.GetLockFile(filepath.Join(layerdir, "layers.lock")) if err != nil { return nil, err } - mountsLockfile, err := GetLockfile(filepath.Join(rundir, "mountpoints.lock")) + mountsLockfile, err := lockfile.GetLockFile(filepath.Join(rundir, "mountpoints.lock")) if err != nil { return nil, err } @@ -841,7 +883,7 @@ func (s *store) newLayerStore(rundir string, layerdir string, driver drivers.Dri volatileDir = rundir } rlstore := layerStore{ - lockfile: lockfile, + lockfile: lockFile, mountsLockfile: mountsLockfile, driver: driver, rundir: rundir, @@ -858,6 +900,12 @@ func (s *store) newLayerStore(rundir string, layerdir string, driver drivers.Dri return nil, err } defer rlstore.stopWriting() + lw, err := rlstore.lockfile.GetLastWrite() + if err != nil { + return nil, err + } + rlstore.lastWrite = lw + // rlstore.mountsLastWrite is initialized inside rlstore.load(). if _, err := rlstore.load(true); err != nil { return nil, err } @@ -865,7 +913,7 @@ func (s *store) newLayerStore(rundir string, layerdir string, driver drivers.Dri } func newROLayerStore(rundir string, layerdir string, driver drivers.Driver) (roLayerStore, error) { - lockfile, err := GetROLockfile(filepath.Join(layerdir, "layers.lock")) + lockfile, err := lockfile.GetROLockFile(filepath.Join(layerdir, "layers.lock")) if err != nil { return nil, err } @@ -887,6 +935,11 @@ func newROLayerStore(rundir string, layerdir string, driver drivers.Driver) (roL return nil, err } defer rlstore.stopReading() + lw, err := rlstore.lockfile.GetLastWrite() + if err != nil { + return nil, err + } + rlstore.lastWrite = lw if _, err := rlstore.load(false); err != nil { return nil, err } @@ -1218,10 +1271,8 @@ func (r *layerStore) Mounted(id string) (int, error) { } r.mountsLockfile.RLock() defer r.mountsLockfile.Unlock() - if modified, err := r.mountsLockfile.Modified(); modified || err != nil { - if err = r.loadMounts(); err != nil { - return 0, err - } + if err := r.reloadMountsIfChanged(); err != nil { + return 0, err } layer, ok := r.lookup(id) if !ok { @@ -1248,10 +1299,8 @@ func (r *layerStore) Mount(id string, options drivers.MountOpts) (string, error) } r.mountsLockfile.Lock() defer r.mountsLockfile.Unlock() - if modified, err := r.mountsLockfile.Modified(); modified || err != nil { - if err = r.loadMounts(); err != nil { - return "", err - } + if err := r.reloadMountsIfChanged(); err != nil { + return "", err } layer, ok := r.lookup(id) if !ok { @@ -1298,10 +1347,8 @@ func (r *layerStore) Unmount(id string, force bool) (bool, error) { } r.mountsLockfile.Lock() defer r.mountsLockfile.Unlock() - if modified, err := r.mountsLockfile.Modified(); modified || err != nil { - if err = r.loadMounts(); err != nil { - return false, err - } + if err := r.reloadMountsIfChanged(); err != nil { + return false, err } layer, ok := r.lookup(id) if !ok { @@ -1336,10 +1383,8 @@ func (r *layerStore) ParentOwners(id string) (uids, gids []int, err error) { } r.mountsLockfile.RLock() defer r.mountsLockfile.Unlock() - if modified, err := r.mountsLockfile.Modified(); modified || err != nil { - if err = r.loadMounts(); err != nil { - return nil, nil, err - } + if err := r.reloadMountsIfChanged(); err != nil { + return nil, nil, err } layer, ok := r.lookup(id) if !ok { diff --git a/vendor/github.com/containers/storage/lockfile_compat.go b/vendor/github.com/containers/storage/lockfile_compat.go index 6fac2ebac6..640203881a 100644 --- a/vendor/github.com/containers/storage/lockfile_compat.go +++ b/vendor/github.com/containers/storage/lockfile_compat.go @@ -4,12 +4,15 @@ import ( "github.com/containers/storage/pkg/lockfile" ) -type Locker = lockfile.Locker +// Deprecated: Use lockfile.*LockFile. +type Locker = lockfile.Locker //lint:ignore SA1019 // lockfile.Locker is deprecated +// Deprecated: Use lockfile.GetLockFile. func GetLockfile(path string) (lockfile.Locker, error) { return lockfile.GetLockfile(path) } +// Deprecated: Use lockfile.GetROLockFile. func GetROLockfile(path string) (lockfile.Locker, error) { return lockfile.GetROLockfile(path) } diff --git a/vendor/github.com/containers/storage/pkg/lockfile/lockfile.go b/vendor/github.com/containers/storage/pkg/lockfile/lockfile.go index f639357f4a..b654699b95 100644 --- a/vendor/github.com/containers/storage/pkg/lockfile/lockfile.go +++ b/vendor/github.com/containers/storage/pkg/lockfile/lockfile.go @@ -10,6 +10,8 @@ import ( // A Locker represents a file lock where the file is used to cache an // identifier of the last party that made changes to whatever's being protected // by the lock. +// +// Deprecated: Refer directly to *LockFile, the provided implementation, instead. type Locker interface { // Acquire a writer lock. // The default unix implementation panics if: @@ -28,10 +30,13 @@ type Locker interface { // Touch records, for others sharing the lock, that the caller was the // last writer. It should only be called with the lock held. + // + // Deprecated: Use *LockFile.RecordWrite. Touch() error // Modified() checks if the most recent writer was a party other than the // last recorded writer. It should only be called with the lock held. + // Deprecated: Use *LockFile.ModifiedSince. Modified() (bool, error) // TouchedSince() checks if the most recent writer modified the file (likely using Touch()) after the specified time. @@ -50,58 +55,76 @@ type Locker interface { } var ( - lockfiles map[string]Locker - lockfilesLock sync.Mutex + lockFiles map[string]*LockFile + lockFilesLock sync.Mutex ) +// GetLockFile opens a read-write lock file, creating it if necessary. The +// *LockFile object may already be locked if the path has already been requested +// by the current process. +func GetLockFile(path string) (*LockFile, error) { + return getLockfile(path, false) +} + // GetLockfile opens a read-write lock file, creating it if necessary. The // Locker object may already be locked if the path has already been requested // by the current process. +// +// Deprecated: Use GetLockFile func GetLockfile(path string) (Locker, error) { - return getLockfile(path, false) + return GetLockFile(path) +} + +// GetROLockFile opens a read-only lock file, creating it if necessary. The +// *LockFile object may already be locked if the path has already been requested +// by the current process. +func GetROLockFile(path string) (*LockFile, error) { + return getLockfile(path, true) } // GetROLockfile opens a read-only lock file, creating it if necessary. The // Locker object may already be locked if the path has already been requested // by the current process. +// +// Deprecated: Use GetROLockFile func GetROLockfile(path string) (Locker, error) { - return getLockfile(path, true) + return GetROLockFile(path) } -// getLockfile returns a Locker object, possibly (depending on the platform) +// getLockFile returns a *LockFile object, possibly (depending on the platform) // working inter-process, and associated with the specified path. // -// If ro, the lock is a read-write lock and the returned Locker should correspond to the +// If ro, the lock is a read-write lock and the returned *LockFile should correspond to the // “lock for reading” (shared) operation; otherwise, the lock is either an exclusive lock, -// or a read-write lock and Locker should correspond to the “lock for writing” (exclusive) operation. +// or a read-write lock and *LockFile should correspond to the “lock for writing” (exclusive) operation. // // WARNING: // - The lock may or MAY NOT be inter-process. // - There may or MAY NOT be an actual object on the filesystem created for the specified path. // - Even if ro, the lock MAY be exclusive. -func getLockfile(path string, ro bool) (Locker, error) { - lockfilesLock.Lock() - defer lockfilesLock.Unlock() - if lockfiles == nil { - lockfiles = make(map[string]Locker) +func getLockfile(path string, ro bool) (*LockFile, error) { + lockFilesLock.Lock() + defer lockFilesLock.Unlock() + if lockFiles == nil { + lockFiles = make(map[string]*LockFile) } cleanPath, err := filepath.Abs(path) if err != nil { return nil, fmt.Errorf("ensuring that path %q is an absolute path: %w", path, err) } - if locker, ok := lockfiles[cleanPath]; ok { - if ro && locker.IsReadWrite() { + if lockFile, ok := lockFiles[cleanPath]; ok { + if ro && lockFile.IsReadWrite() { return nil, fmt.Errorf("lock %q is not a read-only lock", cleanPath) } - if !ro && !locker.IsReadWrite() { + if !ro && !lockFile.IsReadWrite() { return nil, fmt.Errorf("lock %q is not a read-write lock", cleanPath) } - return locker, nil + return lockFile, nil } - locker, err := createLockerForPath(cleanPath, ro) // platform-dependent locker + lockFile, err := createLockFileForPath(cleanPath, ro) // platform-dependent LockFile if err != nil { return nil, err } - lockfiles[cleanPath] = locker - return locker, nil + lockFiles[cleanPath] = lockFile + return lockFile, nil } diff --git a/vendor/github.com/containers/storage/pkg/lockfile/lockfile_unix.go b/vendor/github.com/containers/storage/pkg/lockfile/lockfile_unix.go index 21378e9420..335980914b 100644 --- a/vendor/github.com/containers/storage/pkg/lockfile/lockfile_unix.go +++ b/vendor/github.com/containers/storage/pkg/lockfile/lockfile_unix.go @@ -18,27 +18,48 @@ import ( "golang.org/x/sys/unix" ) -type lockfile struct { +// *LockFile represents a file lock where the file is used to cache an +// identifier of the last party that made changes to whatever's being protected +// by the lock. +// +// It MUST NOT be created manually. Use GetLockFile or GetROLockFile instead. +type LockFile struct { + // The following fields are only set when constructing *LockFile, and must never be modified afterwards. + // They are safe to access without any other locking. + file string + ro bool + // rwMutex serializes concurrent reader-writer acquisitions in the same process space rwMutex *sync.RWMutex // stateMutex is used to synchronize concurrent accesses to the state below stateMutex *sync.Mutex counter int64 - file string - fd uintptr - lw []byte // "last writer"-unique value valid as of the last .Touch() or .Modified(), generated by newLastWriterID() + lw LastWrite // A global value valid as of the last .Touch() or .Modified() locktype int16 locked bool - ro bool + // The following fields are only modified on transitions between counter == 0 / counter != 0. + // Thus, they can be safely accessed by users _that currently hold the LockFile_ without locking. + // In other cases, they need to be protected using stateMutex. + fd uintptr +} + +// LastWrite is an opaque identifier of the last write to some *LockFile. +// It can be used by users of a *LockFile to determine if the lock indicates changes +// since the last check. +// +// Never construct a LastWrite manually; only accept it from *LockFile methods, and pass it back. +type LastWrite struct { + // Never modify fields of a LastWrite object; it has value semantics. + state []byte // Contents of the lock file. } const lastWriterIDSize = 64 // This must be the same as len(stringid.GenerateRandomID) var lastWriterIDCounter uint64 // Private state for newLastWriterID -// newLastWriterID returns a new "last writer" ID. +// newLastWrite returns a new "last write" ID. // The value must be different on every call, and also differ from values // generated by other processes. -func newLastWriterID() []byte { +func newLastWrite() LastWrite { // The ID is (PID, time, per-process counter, random) // PID + time represents both a unique process across reboots, // and a specific time within the process; the per-process counter @@ -60,7 +81,38 @@ func newLastWriterID() []byte { panic(err) // This shouldn't happen } - return res + return LastWrite{ + state: res, + } +} + +// newLastWriteFromData returns a LastWrite corresponding to data that came from a previous LastWrite.serialize +func newLastWriteFromData(serialized []byte) LastWrite { + if serialized == nil { + panic("newLastWriteFromData with nil data") + } + return LastWrite{ + state: serialized, + } +} + +// serialize returns bytes to write to the lock file to represent the specified write. +func (lw LastWrite) serialize() []byte { + if lw.state == nil { + panic("LastWrite.serialize on an uninitialized object") + } + return lw.state +} + +// Equals returns true if lw matches other +func (lw LastWrite) equals(other LastWrite) bool { + if lw.state == nil { + panic("LastWrite.equals on an uninitialized object") + } + if other.state == nil { + panic("LastWrite.equals with an uninitialized counterparty") + } + return bytes.Equal(lw.state, other.state) } // openLock opens the file at path and returns the corresponding file @@ -84,7 +136,7 @@ func openLock(path string, ro bool) (fd int, err error) { // the directory of the lockfile seems to be removed, try to create it if os.IsNotExist(err) { if err := os.MkdirAll(filepath.Dir(path), 0700); err != nil { - return fd, fmt.Errorf("creating locker directory: %w", err) + return fd, fmt.Errorf("creating lock file directory: %w", err) } return openLock(path, ro) @@ -93,20 +145,20 @@ func openLock(path string, ro bool) (fd int, err error) { return fd, &os.PathError{Op: "open", Path: path, Err: err} } -// createLockerForPath returns a Locker object, possibly (depending on the platform) +// createLockFileForPath returns new *LockFile object, possibly (depending on the platform) // working inter-process and associated with the specified path. // // This function will be called at most once for each path value within a single process. // -// If ro, the lock is a read-write lock and the returned Locker should correspond to the +// If ro, the lock is a read-write lock and the returned *LockFile should correspond to the // “lock for reading” (shared) operation; otherwise, the lock is either an exclusive lock, -// or a read-write lock and Locker should correspond to the “lock for writing” (exclusive) operation. +// or a read-write lock and *LockFile should correspond to the “lock for writing” (exclusive) operation. // // WARNING: // - The lock may or MAY NOT be inter-process. // - There may or MAY NOT be an actual object on the filesystem created for the specified path. // - Even if ro, the lock MAY be exclusive. -func createLockerForPath(path string, ro bool) (Locker, error) { +func createLockFileForPath(path string, ro bool) (*LockFile, error) { // Check if we can open the lock. fd, err := openLock(path, ro) if err != nil { @@ -118,19 +170,21 @@ func createLockerForPath(path string, ro bool) (Locker, error) { if ro { locktype = unix.F_RDLCK } - return &lockfile{ - stateMutex: &sync.Mutex{}, + return &LockFile{ + file: path, + ro: ro, + rwMutex: &sync.RWMutex{}, - file: path, - lw: newLastWriterID(), + stateMutex: &sync.Mutex{}, + lw: newLastWrite(), // For compatibility, the first call of .Modified() will always report a change. locktype: int16(locktype), locked: false, - ro: ro}, nil + }, nil } // lock locks the lockfile via FCTNL(2) based on the specified type and // command. -func (l *lockfile) lock(lType int16) { +func (l *LockFile) lock(lType int16) { lk := unix.Flock_t{ Type: lType, Whence: int16(unix.SEEK_SET), @@ -168,7 +222,7 @@ func (l *lockfile) lock(lType int16) { } // Lock locks the lockfile as a writer. Panic if the lock is a read-only one. -func (l *lockfile) Lock() { +func (l *LockFile) Lock() { if l.ro { panic("can't take write lock on read-only lock file") } else { @@ -177,12 +231,12 @@ func (l *lockfile) Lock() { } // LockRead locks the lockfile as a reader. -func (l *lockfile) RLock() { +func (l *LockFile) RLock() { l.lock(unix.F_RDLCK) } // Unlock unlocks the lockfile. -func (l *lockfile) Unlock() { +func (l *LockFile) Unlock() { l.stateMutex.Lock() if !l.locked { // Panic when unlocking an unlocked lock. That's a violation @@ -213,7 +267,7 @@ func (l *lockfile) Unlock() { l.stateMutex.Unlock() } -func (l *lockfile) AssertLocked() { +func (l *LockFile) AssertLocked() { // DO NOT provide a variant that returns the value of l.locked. // // If the caller does not hold the lock, l.locked might nevertheless be true because another goroutine does hold it, and @@ -230,7 +284,7 @@ func (l *lockfile) AssertLocked() { } } -func (l *lockfile) AssertLockedForWriting() { +func (l *LockFile) AssertLockedForWriting() { // DO NOT provide a variant that returns the current lock state. // // The same caveats as for AssertLocked apply equally. @@ -242,53 +296,128 @@ func (l *lockfile) AssertLockedForWriting() { } } -// Touch updates the lock file with the UID of the user. -func (l *lockfile) Touch() error { +// GetLastWrite returns a LastWrite value corresponding to current state of the lock. +// This is typically called before (_not after_) loading the state when initializing a consumer +// of the data protected by the lock. +// During the lifetime of the consumer, the consumer should usually call ModifiedSince instead. +// +// The caller must hold the lock (for reading or writing). +func (l *LockFile) GetLastWrite() (LastWrite, error) { + l.AssertLocked() + contents := make([]byte, lastWriterIDSize) + n, err := unix.Pread(int(l.fd), contents, 0) + if err != nil { + return LastWrite{}, err + } + // It is important to handle the partial read case, because + // the initial size of the lock file is zero, which is a valid + // state (no writes yet) + contents = contents[:n] + return newLastWriteFromData(contents), nil +} + +// RecordWrite updates the lock with a new LastWrite value, and returns the new value. +// +// If this function fails, the LastWriter value of the lock is indeterminate; +// the caller should keep using the previously-recorded LastWrite value, +// and possibly detecting its own modification as an external one: +// +// lw, err := state.lock.RecordWrite() +// if err != nil { /* fail */ } +// state.lastWrite = lw +// +// The caller must hold the lock for writing. +func (l *LockFile) RecordWrite() (LastWrite, error) { + l.AssertLockedForWriting() + lw := newLastWrite() + lockContents := lw.serialize() + n, err := unix.Pwrite(int(l.fd), lockContents, 0) + if err != nil { + return LastWrite{}, err + } + if n != len(lockContents) { + return LastWrite{}, unix.ENOSPC + } + return lw, nil +} + +// ModifiedSince checks if the lock has been changed since a provided LastWrite value, +// and returns the one to record instead. +// +// If ModifiedSince reports no modification, the previous LastWrite value +// is still valid and can continue to be used. +// +// If this function fails, the LastWriter value of the lock is indeterminate; +// the caller should fail and keep using the previously-recorded LastWrite value, +// so that it continues failing until the situation is resolved. Similarly, +// it should only update the recorded LastWrite value after processing the update: +// +// lw2, modified, err := state.lock.ModifiedSince(state.lastWrite) +// if err != nil { /* fail */ } +// state.lastWrite = lw2 +// if modified { +// if err := reload(); err != nil { /* fail */ } +// state.lastWrite = lw2 +// } +// +// The caller must hold the lock (for reading or writing). +func (l *LockFile) ModifiedSince(previous LastWrite) (LastWrite, bool, error) { + l.AssertLocked() + currentLW, err := l.GetLastWrite() + if err != nil { + return LastWrite{}, false, err + } + modified := !previous.equals(currentLW) + return currentLW, modified, nil +} + +// Touch updates the lock file with to record that the current lock holder has modified the lock-protected data. +// +// Deprecated: Use *LockFile.RecordWrite. +func (l *LockFile) Touch() error { + lw, err := l.RecordWrite() + if err != nil { + return err + } l.stateMutex.Lock() if !l.locked || (l.locktype != unix.F_WRLCK) { panic("attempted to update last-writer in lockfile without the write lock") } defer l.stateMutex.Unlock() - l.lw = newLastWriterID() - n, err := unix.Pwrite(int(l.fd), l.lw, 0) - if err != nil { - return err - } - if n != len(l.lw) { - return unix.ENOSPC - } + l.lw = lw return nil } // Modified indicates if the lockfile has been updated since the last time it // was loaded. -func (l *lockfile) Modified() (bool, error) { +// NOTE: Unlike ModifiedSince, this returns true the first time it is called on a *LockFile. +// Callers cannot, in general, rely on this, because that might have happened for some other +// owner of the same *LockFile who created it previously. +// +// Deprecated: Use *LockFile.ModifiedSince. +func (l *LockFile) Modified() (bool, error) { l.stateMutex.Lock() if !l.locked { panic("attempted to check last-writer in lockfile without locking it first") } defer l.stateMutex.Unlock() - currentLW := make([]byte, lastWriterIDSize) - n, err := unix.Pread(int(l.fd), currentLW, 0) + oldLW := l.lw + // Note that this is called with stateMutex held; that’s fine because ModifiedSince doesn’t need to lock it. + currentLW, modified, err := l.ModifiedSince(oldLW) if err != nil { return true, err } - // It is important to handle the partial read case, because - // the initial size of the lock file is zero, which is a valid - // state (no writes yet) - currentLW = currentLW[:n] - oldLW := l.lw l.lw = currentLW - return !bytes.Equal(currentLW, oldLW), nil + return modified, nil } // IsReadWriteLock indicates if the lock file is a read-write lock. -func (l *lockfile) IsReadWrite() bool { +func (l *LockFile) IsReadWrite() bool { return !l.ro } // TouchedSince indicates if the lock file has been touched since the specified time -func (l *lockfile) TouchedSince(when time.Time) bool { +func (l *LockFile) TouchedSince(when time.Time) bool { st, err := system.Fstat(int(l.fd)) if err != nil { return true diff --git a/vendor/github.com/containers/storage/pkg/lockfile/lockfile_windows.go b/vendor/github.com/containers/storage/pkg/lockfile/lockfile_windows.go index cc898d82b5..09f2aca5cc 100644 --- a/vendor/github.com/containers/storage/pkg/lockfile/lockfile_windows.go +++ b/vendor/github.com/containers/storage/pkg/lockfile/lockfile_windows.go @@ -9,45 +9,58 @@ import ( "time" ) -// createLockerForPath returns a Locker object, possibly (depending on the platform) +// createLockFileForPath returns a *LockFile object, possibly (depending on the platform) // working inter-process and associated with the specified path. // // This function will be called at most once for each path value within a single process. // -// If ro, the lock is a read-write lock and the returned Locker should correspond to the +// If ro, the lock is a read-write lock and the returned *LockFile should correspond to the // “lock for reading” (shared) operation; otherwise, the lock is either an exclusive lock, -// or a read-write lock and Locker should correspond to the “lock for writing” (exclusive) operation. +// or a read-write lock and *LockFile should correspond to the “lock for writing” (exclusive) operation. // // WARNING: // - The lock may or MAY NOT be inter-process. // - There may or MAY NOT be an actual object on the filesystem created for the specified path. // - Even if ro, the lock MAY be exclusive. -func createLockerForPath(path string, ro bool) (Locker, error) { - return &lockfile{locked: false}, nil +func createLockFileForPath(path string, ro bool) (*LockFile, error) { + return &LockFile{locked: false}, nil } -type lockfile struct { +// *LockFile represents a file lock where the file is used to cache an +// identifier of the last party that made changes to whatever's being protected +// by the lock. +// +// It MUST NOT be created manually. Use GetLockFile or GetROLockFile instead. +type LockFile struct { mu sync.Mutex file string locked bool } -func (l *lockfile) Lock() { +// LastWrite is an opaque identifier of the last write to some *LockFile. +// It can be used by users of a *LockFile to determine if the lock indicates changes +// since the last check. +// A default-initialized LastWrite never matches any last write, i.e. it always indicates changes. +type LastWrite struct { + // Nothing: The Windows “implementation” does not actually track writes. +} + +func (l *LockFile) Lock() { l.mu.Lock() l.locked = true } -func (l *lockfile) RLock() { +func (l *LockFile) RLock() { l.mu.Lock() l.locked = true } -func (l *lockfile) Unlock() { +func (l *LockFile) Unlock() { l.locked = false l.mu.Unlock() } -func (l *lockfile) AssertLocked() { +func (l *LockFile) AssertLocked() { // DO NOT provide a variant that returns the value of l.locked. // // If the caller does not hold the lock, l.locked might nevertheless be true because another goroutine does hold it, and @@ -59,24 +72,77 @@ func (l *lockfile) AssertLocked() { } } -func (l *lockfile) AssertLockedForWriting() { +func (l *LockFile) AssertLockedForWriting() { // DO NOT provide a variant that returns the current lock state. // // The same caveats as for AssertLocked apply equally. l.AssertLocked() // The current implementation does not distinguish between read and write locks. } -func (l *lockfile) Modified() (bool, error) { +// GetLastWrite() returns a LastWrite value corresponding to current state of the lock. +// This is typically called before (_not after_) loading the state when initializing a consumer +// of the data protected by the lock. +// During the lifetime of the consumer, the consumer should usually call ModifiedSince instead. +// +// The caller must hold the lock (for reading or writing) before this function is called. +func (l *LockFile) GetLastWrite() (LastWrite, error) { + l.AssertLocked() + return LastWrite{}, nil +} + +// RecordWrite updates the lock with a new LastWrite value, and returns the new value. +// +// If this function fails, the LastWriter value of the lock is indeterminate; +// the caller should keep using the previously-recorded LastWrite value, +// and possibly detecting its own modification as an external one: +// +// lw, err := state.lock.RecordWrite() +// if err != nil { /* fail */ } +// state.lastWrite = lw +// +// The caller must hold the lock for writing. +func (l *LockFile) RecordWrite() (LastWrite, error) { + return LastWrite{}, nil +} + +// ModifiedSince checks if the lock has been changed since a provided LastWrite value, +// and returns the one to record instead. +// +// If ModifiedSince reports no modification, the previous LastWrite value +// is still valid and can continue to be used. +// +// If this function fails, the LastWriter value of the lock is indeterminate; +// the caller should fail and keep using the previously-recorded LastWrite value, +// so that it continues failing until the situation is resolved. Similarly, +// it should only update the recorded LastWrite value after processing the update: +// +// lw2, modified, err := state.lock.ModifiedSince(state.lastWrite) +// if err != nil { /* fail */ } +// state.lastWrite = lw2 +// if modified { +// if err := reload(); err != nil { /* fail */ } +// state.lastWrite = lw2 +// } +// +// The caller must hold the lock (for reading or writing). +func (l *LockFile) ModifiedSince(previous LastWrite) (LastWrite, bool, error) { + return LastWrite{}, false, nil +} + +// Deprecated: Use *LockFile.ModifiedSince. +func (l *LockFile) Modified() (bool, error) { return false, nil } -func (l *lockfile) Touch() error { + +// Deprecated: Use *LockFile.RecordWrite. +func (l *LockFile) Touch() error { return nil } -func (l *lockfile) IsReadWrite() bool { +func (l *LockFile) IsReadWrite() bool { return false } -func (l *lockfile) TouchedSince(when time.Time) bool { +func (l *LockFile) TouchedSince(when time.Time) bool { stat, err := os.Stat(l.file) if err != nil { return true diff --git a/vendor/github.com/containers/storage/store.go b/vendor/github.com/containers/storage/store.go index 9cb62d9b3d..f5c2e73ac8 100644 --- a/vendor/github.com/containers/storage/store.go +++ b/vendor/github.com/containers/storage/store.go @@ -20,6 +20,7 @@ import ( "github.com/containers/storage/pkg/directory" "github.com/containers/storage/pkg/idtools" "github.com/containers/storage/pkg/ioutils" + "github.com/containers/storage/pkg/lockfile" "github.com/containers/storage/pkg/parsers" "github.com/containers/storage/pkg/stringutils" "github.com/containers/storage/pkg/system" @@ -579,30 +580,31 @@ type ContainerOptions struct { } type store struct { - lastLoaded time.Time - runRoot string - graphLock Locker - usernsLock Locker - graphRoot string - graphDriverName string - graphOptions []string - pullOptions map[string]string - uidMap []idtools.IDMap - gidMap []idtools.IDMap - autoUsernsUser string - additionalUIDs *idSet // Set by getAvailableIDs() - additionalGIDs *idSet // Set by getAvailableIDs() - autoNsMinSize uint32 - autoNsMaxSize uint32 - graphDriver drivers.Driver - layerStore rwLayerStore - roLayerStores []roLayerStore - imageStore rwImageStore - roImageStores []roImageStore - containerStore rwContainerStore - digestLockRoot string - disableVolatile bool - transientStore bool + lastLoaded time.Time + runRoot string + graphLock *lockfile.LockFile + usernsLock *lockfile.LockFile + graphRoot string + graphDriverName string + graphOptions []string + pullOptions map[string]string + uidMap []idtools.IDMap + gidMap []idtools.IDMap + autoUsernsUser string + additionalUIDs *idSet // Set by getAvailableIDs() + additionalGIDs *idSet // Set by getAvailableIDs() + autoNsMinSize uint32 + autoNsMaxSize uint32 + graphLockLastWrite lockfile.LastWrite + graphDriver drivers.Driver + layerStore rwLayerStore + roLayerStores []roLayerStore + imageStore rwImageStore + roImageStores []roImageStore + containerStore rwContainerStore + digestLockRoot string + disableVolatile bool + transientStore bool } // GetStore attempts to find an already-created Store object matching the @@ -677,12 +679,12 @@ func GetStore(options types.StoreOptions) (Store, error) { return nil, err } - graphLock, err := GetLockfile(filepath.Join(options.GraphRoot, "storage.lock")) + graphLock, err := lockfile.GetLockFile(filepath.Join(options.GraphRoot, "storage.lock")) if err != nil { return nil, err } - usernsLock, err := GetLockfile(filepath.Join(options.GraphRoot, "userns.lock")) + usernsLock, err := lockfile.GetLockFile(filepath.Join(options.GraphRoot, "userns.lock")) if err != nil { return nil, err } @@ -713,6 +715,18 @@ func GetStore(options types.StoreOptions) (Store, error) { transientStore: options.TransientStore, pullOptions: options.PullOptions, } + if err := func() error { // A scope for defer + s.graphLock.Lock() + defer s.graphLock.Unlock() + lastWrite, err := s.graphLock.GetLastWrite() + if err != nil { + return err + } + s.graphLockLastWrite = lastWrite + return nil + }(); err != nil { + return nil, err + } if err := s.load(); err != nil { return nil, err } @@ -839,7 +853,7 @@ func (s *store) load() error { // GetDigestLock returns a digest-specific Locker. func (s *store) GetDigestLock(d digest.Digest) (Locker, error) { - return GetLockfile(filepath.Join(s.digestLockRoot, d.String())) + return lockfile.GetLockFile(filepath.Join(s.digestLockRoot, d.String())) } func (s *store) getGraphDriver() (drivers.Driver, error) { @@ -2510,10 +2524,11 @@ func (s *store) mount(id string, options drivers.MountOpts) (string, error) { } defer rlstore.stopWriting() - modified, err := s.graphLock.Modified() + lastWrite, modified, err := s.graphLock.ModifiedSince(s.graphLockLastWrite) if err != nil { return "", err } + s.graphLockLastWrite = lastWrite /* We need to make sure the home mount is present when the Mount is done. */ if modified { @@ -2657,10 +2672,11 @@ func (s *store) Diff(from, to string, options *DiffOptions) (io.ReadCloser, erro s.graphLock.Lock() defer s.graphLock.Unlock() - modified, err := s.graphLock.Modified() + lastWrite, modified, err := s.graphLock.ModifiedSince(s.graphLockLastWrite) if err != nil { return nil, err } + s.graphLockLastWrite = lastWrite // We need to make sure the home mount is present when the Mount is done. if modified { @@ -3219,11 +3235,14 @@ func (s *store) Shutdown(force bool) ([]string, error) { } if err == nil { err = s.graphDriver.Cleanup() - if err2 := s.graphLock.Touch(); err2 != nil { + // We don’t retain the lastWrite value, and treat this update as if someone else did the .Cleanup(), + // so that we reload after a .Shutdown() the same way other processes would. + // Shutdown() is basically an error path, so reliability is more important than performance. + if _, err2 := s.graphLock.RecordWrite(); err2 != nil { if err == nil { err = err2 } else { - err = fmt.Errorf("(graphLock.Touch failed: %v) %w", err2, err) + err = fmt.Errorf("(graphLock.RecordWrite failed: %v) %w", err2, err) } } } diff --git a/vendor/modules.txt b/vendor/modules.txt index 27fb3253ca..de6333d953 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -264,7 +264,7 @@ github.com/containers/psgo/internal/dev github.com/containers/psgo/internal/host github.com/containers/psgo/internal/proc github.com/containers/psgo/internal/process -# github.com/containers/storage v1.44.1-0.20221121144727-71fd3e87df7a +# github.com/containers/storage v1.44.1-0.20221201083122-c5a80ad65f42 ## explicit; go 1.17 github.com/containers/storage github.com/containers/storage/drivers