diff --git a/cmd/containers-storage/diff.go b/cmd/containers-storage/diff.go index 2811764d52..09f8b61f04 100644 --- a/cmd/containers-storage/diff.go +++ b/cmd/containers-storage/diff.go @@ -185,8 +185,14 @@ func applyDiffUsingStagingDirectory(flags *mflag.FlagSet, action string, m stora if err != nil { return 1, err } - if err := m.ApplyDiffFromStagingDirectory(layer, out.Target, out, &options); err != nil { - m.CleanupStagingDirectory(out.Target) + + applyStagedLayerArgs := storage.ApplyStagedLayerOptions{ + ID: layer, + DiffOutput: out, + DiffOptions: &options, + } + if _, err := m.ApplyStagedLayer(applyStagedLayerArgs); err != nil { + m.CleanupStagedLayer(out) return 1, err } return 0, nil diff --git a/deprecated.go b/deprecated.go index 04972d8388..76ae6328bf 100644 --- a/deprecated.go +++ b/deprecated.go @@ -208,8 +208,6 @@ type LayerStore interface { ParentOwners(id string) (uids, gids []int, err error) ApplyDiff(to string, diff io.Reader) (int64, error) ApplyDiffWithDiffer(to string, options *drivers.ApplyDiffOpts, differ drivers.Differ) (*drivers.DriverWithDifferOutput, error) - CleanupStagingDirectory(stagingDirectory string) error - ApplyDiffFromStagingDirectory(id, stagingDirectory string, diffOutput *drivers.DriverWithDifferOutput, options *drivers.ApplyDiffOpts) error DifferTarget(id string) (string, error) LoadLocked() error PutAdditionalLayer(id string, parentLayer *Layer, names []string, aLayer drivers.AdditionalLayer) (layer *Layer, err error) diff --git a/drivers/overlay/overlay.go b/drivers/overlay/overlay.go index f26cf695bd..91f457ce20 100644 --- a/drivers/overlay/overlay.go +++ b/drivers/overlay/overlay.go @@ -28,6 +28,7 @@ import ( "github.com/containers/storage/pkg/fsutils" "github.com/containers/storage/pkg/idmap" "github.com/containers/storage/pkg/idtools" + "github.com/containers/storage/pkg/lockfile" "github.com/containers/storage/pkg/mount" "github.com/containers/storage/pkg/parsers" "github.com/containers/storage/pkg/system" @@ -83,6 +84,8 @@ const ( lowerFile = "lower" maxDepth = 500 + stagingLockFile = "staging.lock" + tocArtifact = "toc" fsVerityDigestsArtifact = "fs-verity-digests" @@ -127,6 +130,8 @@ type Driver struct { usingMetacopy bool usingComposefs bool + stagingDirsLocks map[string]*lockfile.LockFile + supportsIDMappedMounts *bool } @@ -460,6 +465,7 @@ func Init(home string, options graphdriver.Options) (graphdriver.Driver, error) supportsVolatile: supportsVolatile, usingComposefs: opts.useComposefs, options: *opts, + stagingDirsLocks: make(map[string]*lockfile.LockFile), } d.naiveDiff = graphdriver.NewNaiveDiffDriver(d, graphdriver.NewNaiveLayerIDMapUpdater(d)) @@ -876,14 +882,48 @@ func (d *Driver) Metadata(id string) (map[string]string, error) { return metadata, nil } -// Cleanup any state created by overlay which should be cleaned when daemon -// is being shutdown. For now, we just have to unmount the bind mounted -// we had created. +// Cleanup any state created by overlay which should be cleaned when +// the storage is being shutdown. The only state created by the driver +// is the bind mount on the home directory. func (d *Driver) Cleanup() error { - _ = os.RemoveAll(filepath.Join(d.home, stagingDir)) + anyPresent := d.pruneStagingDirectories() + if anyPresent { + return nil + } return mount.Unmount(d.home) } +// pruneStagingDirectories cleans up any staging directory that was leaked. +// It returns whether any staging directory is still present. +func (d *Driver) pruneStagingDirectories() bool { + for _, lock := range d.stagingDirsLocks { + lock.Unlock() + } + d.stagingDirsLocks = make(map[string]*lockfile.LockFile) + + anyPresent := false + + homeStagingDir := filepath.Join(d.home, stagingDir) + dirs, err := os.ReadDir(homeStagingDir) + if err == nil { + for _, dir := range dirs { + stagingDirToRemove := filepath.Join(homeStagingDir, dir.Name()) + lock, err := lockfile.GetLockFile(filepath.Join(stagingDirToRemove, stagingLockFile)) + if err != nil { + anyPresent = true + continue + } + if err := lock.TryLock(); err != nil { + anyPresent = true + continue + } + _ = os.RemoveAll(stagingDirToRemove) + lock.Unlock() + } + } + return anyPresent +} + // LookupAdditionalLayer looks up additional layer store by the specified // digest and ref and returns an object representing that layer. // This API is experimental and can be changed without bumping the major version number. @@ -2029,7 +2069,14 @@ func (d *Driver) DiffGetter(id string) (graphdriver.FileGetCloser, error) { // CleanupStagingDirectory cleanups the staging directory. func (d *Driver) CleanupStagingDirectory(stagingDirectory string) error { - return os.RemoveAll(stagingDirectory) + parentStagingDir := filepath.Dir(stagingDirectory) + + if lock, ok := d.stagingDirsLocks[parentStagingDir]; ok { + delete(d.stagingDirsLocks, parentStagingDir) + lock.Unlock() + } + + return os.RemoveAll(parentStagingDir) } func supportsDataOnlyLayersCached(home, runhome string) (bool, error) { @@ -2050,8 +2097,8 @@ func supportsDataOnlyLayersCached(home, runhome string) (bool, error) { return supportsDataOnly, err } -// ApplyDiff applies the changes in the new layer using the specified function -func (d *Driver) ApplyDiffWithDiffer(id, parent string, options *graphdriver.ApplyDiffWithDifferOpts, differ graphdriver.Differ) (output graphdriver.DriverWithDifferOutput, err error) { +// ApplyDiffWithDiffer applies the changes in the new layer using the specified function +func (d *Driver) ApplyDiffWithDiffer(id, parent string, options *graphdriver.ApplyDiffWithDifferOpts, differ graphdriver.Differ) (output graphdriver.DriverWithDifferOutput, errRet error) { var idMappings *idtools.IDMappings if options != nil { idMappings = options.Mappings @@ -2068,7 +2115,7 @@ func (d *Driver) ApplyDiffWithDiffer(id, parent string, options *graphdriver.App if err != nil && !os.IsExist(err) { return graphdriver.DriverWithDifferOutput{}, err } - applyDir, err = os.MkdirTemp(stagingDir, "") + layerDir, err := os.MkdirTemp(stagingDir, "") if err != nil { return graphdriver.DriverWithDifferOutput{}, err } @@ -2076,9 +2123,23 @@ func (d *Driver) ApplyDiffWithDiffer(id, parent string, options *graphdriver.App if d.options.forceMask != nil { perms = *d.options.forceMask } - if err := os.Chmod(applyDir, perms); err != nil { + applyDir = filepath.Join(layerDir, "dir") + if err := os.Mkdir(applyDir, perms); err != nil { return graphdriver.DriverWithDifferOutput{}, err } + + lock, err := lockfile.GetLockFile(filepath.Join(layerDir, stagingLockFile)) + if err != nil { + return graphdriver.DriverWithDifferOutput{}, err + } + defer func() { + if errRet != nil { + delete(d.stagingDirsLocks, layerDir) + lock.Unlock() + } + }() + d.stagingDirsLocks[layerDir] = lock + lock.Lock() } else { var err error applyDir, err = d.getDiffPath(id) @@ -2112,9 +2173,19 @@ func (d *Driver) ApplyDiffWithDiffer(id, parent string, options *graphdriver.App // ApplyDiffFromStagingDirectory applies the changes using the specified staging directory. func (d *Driver) ApplyDiffFromStagingDirectory(id, parent string, diffOutput *graphdriver.DriverWithDifferOutput, options *graphdriver.ApplyDiffWithDifferOpts) error { stagingDirectory := diffOutput.Target - if filepath.Dir(stagingDirectory) != d.getStagingDir(id) { + parentStagingDir := filepath.Dir(stagingDirectory) + + defer func() { + if lock, ok := d.stagingDirsLocks[parentStagingDir]; ok { + delete(d.stagingDirsLocks, parentStagingDir) + lock.Unlock() + } + }() + + if filepath.Dir(parentStagingDir) != d.getStagingDir(id) { return fmt.Errorf("%q is not a staging directory", stagingDirectory) } + diffPath, err := d.getDiffPath(id) if err != nil { return err diff --git a/pkg/lockfile/lockfile.go b/pkg/lockfile/lockfile.go index 5dd6741086..5101475786 100644 --- a/pkg/lockfile/lockfile.go +++ b/pkg/lockfile/lockfile.go @@ -133,11 +133,25 @@ func (l *LockFile) Lock() { } } -// LockRead locks the lockfile as a reader. +// RLock locks the lockfile as a reader. func (l *LockFile) RLock() { l.lock(readLock) } +// TryLock attempts to lock the lockfile as a writer. Panic if the lock is a read-only one. +func (l *LockFile) TryLock() error { + if l.ro { + panic("can't take write lock on read-only lock file") + } else { + return l.tryLock(writeLock) + } +} + +// TryRLock attempts to lock the lockfile as a reader. +func (l *LockFile) TryRLock() error { + return l.tryLock(readLock) +} + // Unlock unlocks the lockfile. func (l *LockFile) Unlock() { l.stateMutex.Lock() @@ -401,9 +415,47 @@ func (l *LockFile) lock(lType lockType) { // Optimization: only use the (expensive) syscall when // the counter is 0. In this case, we're either the first // reader lock or a writer lock. - lockHandle(l.fd, lType) + lockHandle(l.fd, lType, false) + } + l.lockType = lType + l.locked = true + l.counter++ +} + +// lock locks the lockfile via syscall based on the specified type and +// command. +func (l *LockFile) tryLock(lType lockType) error { + var success bool + if lType == readLock { + success = l.rwMutex.TryRLock() + } else { + success = l.rwMutex.TryLock() + } + if !success { + return fmt.Errorf("resource temporarily unavailable") + } + l.stateMutex.Lock() + defer l.stateMutex.Unlock() + if l.counter == 0 { + // If we're the first reference on the lock, we need to open the file again. + fd, err := openLock(l.file, l.ro) + if err != nil { + l.rwMutex.Unlock() + return err + } + l.fd = fd + + // Optimization: only use the (expensive) syscall when + // the counter is 0. In this case, we're either the first + // reader lock or a writer lock. + if err = lockHandle(l.fd, lType, true); err != nil { + closeHandle(fd) + l.rwMutex.Unlock() + return err + } } l.lockType = lType l.locked = true l.counter++ + return nil } diff --git a/pkg/lockfile/lockfile_test.go b/pkg/lockfile/lockfile_test.go index 9512fbf74e..824bb32ae3 100644 --- a/pkg/lockfile/lockfile_test.go +++ b/pkg/lockfile/lockfile_test.go @@ -5,6 +5,7 @@ import ( "os" "os/exec" "path/filepath" + "runtime" "sync" "sync/atomic" "testing" @@ -221,6 +222,62 @@ func TestLockfileName(t *testing.T) { assert.NotEmpty(t, l.name, "lockfile name should be recorded correctly") } +func TestTryWriteLockFile(t *testing.T) { + runtime.LockOSThread() + defer runtime.UnlockOSThread() + + l, err := getTempLockfile() + require.Nil(t, err, "error getting temporary lock file") + defer os.Remove(l.name) + + err = l.TryLock() + assert.Nil(t, err) + + l.AssertLocked() + + errChan := make(chan error) + go func() { + errChan <- l.TryRLock() + errChan <- l.TryLock() + }() + assert.NotNil(t, <-errChan) + assert.NotNil(t, <-errChan) + + l.Unlock() +} + +func TestTryReadLockFile(t *testing.T) { + runtime.LockOSThread() + defer runtime.UnlockOSThread() + + l, err := getTempLockfile() + require.Nil(t, err, "error getting temporary lock file") + defer os.Remove(l.name) + + err = l.TryRLock() + assert.Nil(t, err) + + l.AssertLocked() + + errChan := make(chan error) + go func() { + errChan <- l.TryRLock() + l.Unlock() + + errChan <- l.TryLock() + }() + assert.Nil(t, <-errChan) + assert.NotNil(t, <-errChan) + + l.Unlock() + + go func() { + errChan <- l.TryLock() + l.Unlock() + }() + assert.Nil(t, <-errChan) +} + func TestLockfileRead(t *testing.T) { l, err := getTempLockfile() require.Nil(t, err, "error getting temporary lock file") diff --git a/pkg/lockfile/lockfile_unix.go b/pkg/lockfile/lockfile_unix.go index 38e737e265..0eff003bcd 100644 --- a/pkg/lockfile/lockfile_unix.go +++ b/pkg/lockfile/lockfile_unix.go @@ -74,7 +74,7 @@ func openHandle(path string, mode int) (fileHandle, error) { return fileHandle(fd), err } -func lockHandle(fd fileHandle, lType lockType) { +func lockHandle(fd fileHandle, lType lockType, nonblocking bool) error { fType := unix.F_RDLCK if lType != readLock { fType = unix.F_WRLCK @@ -85,7 +85,15 @@ func lockHandle(fd fileHandle, lType lockType) { Start: 0, Len: 0, } - for unix.FcntlFlock(uintptr(fd), unix.F_SETLKW, &lk) != nil { + cmd := unix.F_SETLKW + if nonblocking { + cmd = unix.F_SETLK + } + for { + err := unix.FcntlFlock(uintptr(fd), cmd, &lk) + if err == nil || nonblocking { + return err + } time.Sleep(10 * time.Millisecond) } } @@ -93,3 +101,7 @@ func lockHandle(fd fileHandle, lType lockType) { func unlockAndCloseHandle(fd fileHandle) { unix.Close(int(fd)) } + +func closeHandle(fd fileHandle) { + unix.Close(int(fd)) +} diff --git a/pkg/lockfile/lockfile_windows.go b/pkg/lockfile/lockfile_windows.go index 304c92b158..6482529b3e 100644 --- a/pkg/lockfile/lockfile_windows.go +++ b/pkg/lockfile/lockfile_windows.go @@ -81,19 +81,30 @@ func openHandle(path string, mode int) (fileHandle, error) { return fileHandle(fd), err } -func lockHandle(fd fileHandle, lType lockType) { +func lockHandle(fd fileHandle, lType lockType, nonblocking bool) error { flags := 0 if lType != readLock { flags = windows.LOCKFILE_EXCLUSIVE_LOCK } + if nonblocking { + flags |= windows.LOCKFILE_FAIL_IMMEDIATELY + } ol := new(windows.Overlapped) if err := windows.LockFileEx(windows.Handle(fd), uint32(flags), reserved, allBytes, allBytes, ol); err != nil { + if nonblocking { + return err + } panic(err) } + return nil } func unlockAndCloseHandle(fd fileHandle) { ol := new(windows.Overlapped) windows.UnlockFileEx(windows.Handle(fd), reserved, allBytes, allBytes, ol) + closeHandle(fd) +} + +func closeHandle(fd fileHandle) { windows.Close(windows.Handle(fd)) } diff --git a/store.go b/store.go index 253218e5ad..3c593c4009 100644 --- a/store.go +++ b/store.go @@ -330,17 +330,9 @@ type Store interface { // successfully applied with ApplyDiffFromStagingDirectory. ApplyDiffWithDiffer(to string, options *drivers.ApplyDiffWithDifferOpts, differ drivers.Differ) (*drivers.DriverWithDifferOutput, error) - // ApplyDiffFromStagingDirectory uses stagingDirectory to create the diff. - // Deprecated: it will be removed soon. Use ApplyStagedLayer instead. - ApplyDiffFromStagingDirectory(to, stagingDirectory string, diffOutput *drivers.DriverWithDifferOutput, options *drivers.ApplyDiffWithDifferOpts) error - - // CleanupStagingDirectory cleanups the staging directory. It can be used to cleanup the staging directory on errors - // Deprecated: it will be removed soon. Use CleanupStagedLayer instead. - CleanupStagingDirectory(stagingDirectory string) error - - // ApplyStagedLayer combines the functions of CreateLayer and ApplyDiffFromStagingDirectory, - // marking the layer for automatic removal if applying the diff fails - // for any reason. + // ApplyStagedLayer combines the functions of creating a layer and using the staging + // directory to populate it. + // It marks the layer for automatic removal if applying the diff fails for any reason. ApplyStagedLayer(args ApplyStagedLayerOptions) (*Layer, error) // CleanupStagedLayer cleanups the staging directory. It can be used to cleanup the staging directory on errors @@ -1445,20 +1437,8 @@ func (s *store) canUseShifting(uidmap, gidmap []idtools.IDMap) bool { return true } -func (s *store) putLayer(id, parent string, names []string, mountLabel string, writeable bool, lOptions *LayerOptions, diff io.Reader, slo *stagedLayerOptions) (*Layer, int64, error) { - rlstore, rlstores, err := s.bothLayerStoreKinds() - if err != nil { - return nil, -1, err - } - if err := rlstore.startWriting(); err != nil { - return nil, -1, err - } - defer rlstore.stopWriting() - if err := s.containerStore.startWriting(); err != nil { - return nil, -1, err - } - defer s.containerStore.stopWriting() - +// putLayer requires the rlstore, rlstores, as well as s.containerStore (even if not an argument to this function) to be locked for write. +func (s *store) putLayer(rlstore rwLayerStore, rlstores []roLayerStore, id, parent string, names []string, mountLabel string, writeable bool, lOptions *LayerOptions, diff io.Reader, slo *stagedLayerOptions) (*Layer, int64, error) { var parentLayer *Layer var options LayerOptions if lOptions != nil { @@ -1537,7 +1517,19 @@ func (s *store) putLayer(id, parent string, names []string, mountLabel string, w } func (s *store) PutLayer(id, parent string, names []string, mountLabel string, writeable bool, lOptions *LayerOptions, diff io.Reader) (*Layer, int64, error) { - return s.putLayer(id, parent, names, mountLabel, writeable, lOptions, diff, nil) + rlstore, rlstores, err := s.bothLayerStoreKinds() + if err != nil { + return nil, -1, err + } + if err := rlstore.startWriting(); err != nil { + return nil, -1, err + } + defer rlstore.stopWriting() + if err := s.containerStore.startWriting(); err != nil { + return nil, -1, err + } + defer s.containerStore.stopWriting() + return s.putLayer(rlstore, rlstores, id, parent, names, mountLabel, writeable, lOptions, diff, nil) } func (s *store) CreateLayer(id, parent string, names []string, mountLabel string, writeable bool, options *LayerOptions) (*Layer, error) { @@ -3002,36 +2994,39 @@ func (s *store) Diff(from, to string, options *DiffOptions) (io.ReadCloser, erro return nil, ErrLayerUnknown } -func (s *store) ApplyDiffFromStagingDirectory(to, stagingDirectory string, diffOutput *drivers.DriverWithDifferOutput, options *drivers.ApplyDiffWithDifferOpts) error { - if stagingDirectory != diffOutput.Target { - return fmt.Errorf("invalid value for staging directory, it must be the same as the differ target directory") +func (s *store) ApplyStagedLayer(args ApplyStagedLayerOptions) (*Layer, error) { + rlstore, rlstores, err := s.bothLayerStoreKinds() + if err != nil { + return nil, err } - _, err := writeToLayerStore(s, func(rlstore rwLayerStore) (struct{}, error) { - if !rlstore.Exists(to) { - return struct{}{}, ErrLayerUnknown - } - return struct{}{}, rlstore.applyDiffFromStagingDirectory(to, diffOutput, options) - }) - return err -} + if err := rlstore.startWriting(); err != nil { + return nil, err + } + defer rlstore.stopWriting() + + layer, err := rlstore.Get(args.ID) + if err != nil && !errors.Is(err, ErrLayerUnknown) { + return layer, err + } + if err == nil { + return layer, rlstore.applyDiffFromStagingDirectory(args.ID, args.DiffOutput, args.DiffOptions) + } + + // if the layer doesn't exist yet, try to create it. + + if err := s.containerStore.startWriting(); err != nil { + return nil, err + } + defer s.containerStore.stopWriting() -func (s *store) ApplyStagedLayer(args ApplyStagedLayerOptions) (*Layer, error) { slo := stagedLayerOptions{ DiffOutput: args.DiffOutput, DiffOptions: args.DiffOptions, } - - layer, _, err := s.putLayer(args.ID, args.ParentLayer, args.Names, args.MountLabel, args.Writeable, args.LayerOptions, nil, &slo) + layer, _, err = s.putLayer(rlstore, rlstores, args.ID, args.ParentLayer, args.Names, args.MountLabel, args.Writeable, args.LayerOptions, nil, &slo) return layer, err } -func (s *store) CleanupStagingDirectory(stagingDirectory string) error { - _, err := writeToLayerStore(s, func(rlstore rwLayerStore) (struct{}, error) { - return struct{}{}, rlstore.CleanupStagingDirectory(stagingDirectory) - }) - return err -} - func (s *store) CleanupStagedLayer(diffOutput *drivers.DriverWithDifferOutput) error { _, err := writeToLayerStore(s, func(rlstore rwLayerStore) (struct{}, error) { return struct{}{}, rlstore.CleanupStagingDirectory(diffOutput.Target)