Merge pull request #1916 from giuseppe/lock-staging-dir

overlay: lock staging directories

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

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)

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,17 +2115,31 @@ 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
 		}
 		perms := defaultPerms
 		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

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
 }

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")

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,11 +85,23 @@ 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)
 	}
 }
 
 func unlockAndCloseHandle(fd fileHandle) {
 	unix.Close(int(fd))
 }
+
+func closeHandle(fd fileHandle) {
+	unix.Close(int(fd))
+}