Merge pull request #17185 from cpuguy83/use_finer_locking_for_volume_store

Fix potential races in the volume store

locker/README.md

@@ -0,0 +1,65 @@
+Locker
+=====
+
+locker provides a mechanism for creating finer-grained locking to help
+free up more global locks to handle other tasks.
+
+The implementation looks close to a sync.Mutex, however the user must provide a
+reference to use to refer to the underlying lock when locking and unlocking,
+and unlock may generate an error.
+
+If a lock with a given name does not exist when `Lock` is called, one is
+created.
+Lock references are automatically cleaned up on `Unlock` if nothing else is
+waiting for the lock.
+
+
+## Usage
+
+```go
+package important
+
+import (
+	"sync"
+	"time"
+
+	"github.com/docker/docker/pkg/locker"
+)
+
+type important struct {
+	locks *locker.Locker
+	data  map[string]interface{}
+	mu    sync.Mutex
+}
+
+func (i *important) Get(name string) interface{} {
+	i.locks.Lock(name)
+	defer i.locks.Unlock(name)
+	return data[name]
+}
+
+func (i *important) Create(name string, data interface{}) {
+	i.locks.Lock(name)
+	defer i.locks.Unlock(name)
+
+	i.createImporatant(data)
+
+	s.mu.Lock()
+	i.data[name] = data
+	s.mu.Unlock()
+}
+
+func (i *important) createImportant(data interface{}) {
+	time.Sleep(10 * time.Second)
+}
+```
+
+For functions dealing with a given name, always lock at the beginning of the
+function (or before doing anything with the underlying state), this ensures any
+other function that is dealing with the same name will block.
+
+When needing to modify the underlying data, use the global lock to ensure nothing
+else is modfying it at the same time.
+Since name lock is already in place, no reads will occur while the modification
+is being performed.
+

locker/locker.go

@@ -0,0 +1,111 @@
+/*
+Package locker provides a mechanism for creating finer-grained locking to help
+free up more global locks to handle other tasks.
+
+The implementation looks close to a sync.Mutex, however the user must provide a
+reference to use to refer to the underlying lock when locking and unlocking,
+and unlock may generate an error.
+
+If a lock with a given name does not exist when `Lock` is called, one is
+created.
+Lock references are automatically cleaned up on `Unlock` if nothing else is
+waiting for the lock.
+*/
+package locker
+
+import (
+	"errors"
+	"sync"
+	"sync/atomic"
+)
+
+// ErrNoSuchLock is returned when the requested lock does not exist
+var ErrNoSuchLock = errors.New("no such lock")
+
+// Locker provides a locking mechanism based on the passed in reference name
+type Locker struct {
+	mu    sync.Mutex
+	locks map[string]*lockCtr
+}
+
+// lockCtr is used by Locker to represent a lock with a given name.
+type lockCtr struct {
+	mu sync.Mutex
+	// waiters is the number of waiters waiting to acquire the lock
+	waiters uint32
+}
+
+// inc increments the number of waiters waiting for the lock
+func (l *lockCtr) inc() {
+	atomic.AddUint32(&l.waiters, 1)
+}
+
+// dec decrements the number of waiters wating on the lock
+func (l *lockCtr) dec() {
+	atomic.AddUint32(&l.waiters, ^uint32(l.waiters-1))
+}
+
+// count gets the current number of waiters
+func (l *lockCtr) count() uint32 {
+	return atomic.LoadUint32(&l.waiters)
+}
+
+// Lock locks the mutex
+func (l *lockCtr) Lock() {
+	l.mu.Lock()
+}
+
+// Unlock unlocks the mutex
+func (l *lockCtr) Unlock() {
+	l.mu.Unlock()
+}
+
+// New creates a new Locker
+func New() *Locker {
+	return &Locker{
+		locks: make(map[string]*lockCtr),
+	}
+}
+
+// Lock locks a mutex with the given name. If it doesn't exist, one is created
+func (l *Locker) Lock(name string) {
+	l.mu.Lock()
+	if l.locks == nil {
+		l.locks = make(map[string]*lockCtr)
+	}
+
+	nameLock, exists := l.locks[name]
+	if !exists {
+		nameLock = &lockCtr{}
+		l.locks[name] = nameLock
+	}
+
+	// increment the nameLock waiters while inside the main mutex
+	// this makes sure that the lock isn't deleted if `Lock` and `Unlock` are called concurrently
+	nameLock.inc()
+	l.mu.Unlock()
+
+	// Lock the nameLock outside the main mutex so we don't block other operations
+	// once locked then we can decrement the number of waiters for this lock
+	nameLock.Lock()
+	nameLock.dec()
+}
+
+// Unlock unlocks the mutex with the given name
+// If the given lock is not being waited on by any other callers, it is deleted
+func (l *Locker) Unlock(name string) error {
+	l.mu.Lock()
+	nameLock, exists := l.locks[name]
+	if !exists {
+		l.mu.Unlock()
+		return ErrNoSuchLock
+	}
+
+	if nameLock.count() == 0 {
+		delete(l.locks, name)
+	}
+	nameLock.Unlock()
+
+	l.mu.Unlock()
+	return nil
+}

locker/locker_test.go

@@ -0,0 +1,90 @@
+package locker
+
+import (
+	"runtime"
+	"testing"
+)
+
+func TestLockCounter(t *testing.T) {
+	l := &lockCtr{}
+	l.inc()
+
+	if l.waiters != 1 {
+		t.Fatal("counter inc failed")
+	}
+
+	l.dec()
+	if l.waiters != 0 {
+		t.Fatal("counter dec failed")
+	}
+}
+
+func TestLockerLock(t *testing.T) {
+	l := New()
+	l.Lock("test")
+	ctr := l.locks["test"]
+
+	if ctr.count() != 0 {
+		t.Fatalf("expected waiters to be 0, got :%d", ctr.waiters)
+	}
+
+	chDone := make(chan struct{})
+	go func() {
+		l.Lock("test")
+		close(chDone)
+	}()
+
+	runtime.Gosched()
+
+	select {
+	case <-chDone:
+		t.Fatal("lock should not have returned while it was still held")
+	default:
+	}
+
+	if ctr.count() != 1 {
+		t.Fatalf("expected waiters to be 1, got: %d", ctr.count())
+	}
+
+	if err := l.Unlock("test"); err != nil {
+		t.Fatal(err)
+	}
+	runtime.Gosched()
+
+	select {
+	case <-chDone:
+	default:
+		// one more time just to be sure
+		runtime.Gosched()
+		select {
+		case <-chDone:
+		default:
+			t.Fatalf("lock should have completed")
+		}
+	}
+
+	if ctr.count() != 0 {
+		t.Fatalf("expected waiters to be 0, got: %d", ctr.count())
+	}
+}
+
+func TestLockerUnlock(t *testing.T) {
+	l := New()
+
+	l.Lock("test")
+	l.Unlock("test")
+
+	chDone := make(chan struct{})
+	go func() {
+		l.Lock("test")
+		close(chDone)
+	}()
+
+	runtime.Gosched()
+
+	select {
+	case <-chDone:
+	default:
+		t.Fatalf("lock should not be blocked")
+	}
+}