From c2f7777603039b0e9b7e8fcdf517b1486dc14781 Mon Sep 17 00:00:00 2001 From: Brian Goff Date: Sat, 27 Feb 2016 07:54:17 -0500 Subject: [PATCH 1/2] Revert "Add finer-grained locking for aufs" This reverts commit f31014197cbe9438cc956ed12c47093a0324c82d. Signed-off-by: Brian Goff --- daemon/graphdriver/aufs/aufs.go | 94 ++++++++++---------------------- daemon/graphdriver/aufs/mount.go | 2 +- 2 files changed, 29 insertions(+), 67 deletions(-) diff --git a/daemon/graphdriver/aufs/aufs.go b/daemon/graphdriver/aufs/aufs.go index 529d44c265..e03576aa8a 100644 --- a/daemon/graphdriver/aufs/aufs.go +++ b/daemon/graphdriver/aufs/aufs.go @@ -66,7 +66,6 @@ func init() { type data struct { referenceCount int path string - sync.Mutex } // Driver contains information about the filesystem mounted. @@ -77,7 +76,7 @@ type Driver struct { root string uidMaps []idtools.IDMap gidMaps []idtools.IDMap - globalLock sync.Mutex // Protects concurrent modification to active + sync.Mutex // Protects concurrent modification to active active map[string]*data } @@ -203,20 +202,7 @@ func (a *Driver) Exists(id string) bool { // Create three folders for each id // mnt, layers, and diff func (a *Driver) Create(id, parent, mountLabel string) error { - m := a.getActive(id) - m.Lock() - - var err error - defer func() { - a.globalLock.Lock() - if err != nil { - delete(a.active, id) - } - a.globalLock.Unlock() - m.Unlock() - }() - - if err = a.createDirsFor(id); err != nil { + if err := a.createDirsFor(id); err != nil { return err } // Write the layers metadata @@ -227,22 +213,23 @@ func (a *Driver) Create(id, parent, mountLabel string) error { defer f.Close() if parent != "" { - var ids []string - ids, err = getParentIds(a.rootPath(), parent) + ids, err := getParentIds(a.rootPath(), parent) if err != nil { return err } - if _, err = fmt.Fprintln(f, parent); err != nil { + if _, err := fmt.Fprintln(f, parent); err != nil { return err } for _, i := range ids { - if _, err = fmt.Fprintln(f, i); err != nil { + if _, err := fmt.Fprintln(f, i); err != nil { return err } } } - + a.Lock() + a.active[id] = &data{} + a.Unlock() return nil } @@ -266,10 +253,11 @@ func (a *Driver) createDirsFor(id string) error { // Remove will unmount and remove the given id. func (a *Driver) Remove(id string) error { - m := a.getActive(id) - m.Lock() - defer m.Unlock() + // Protect the a.active from concurrent access + a.Lock() + defer a.Unlock() + m := a.active[id] if m != nil { if m.referenceCount > 0 { return nil @@ -300,9 +288,9 @@ func (a *Driver) Remove(id string) error { return err } if m != nil { - a.globalLock.Lock() + a.Lock() delete(a.active, id) - a.globalLock.Unlock() + a.Unlock() } return nil } @@ -310,36 +298,21 @@ func (a *Driver) Remove(id string) error { // Get returns the rootfs path for the id. // This will mount the dir at it's given path func (a *Driver) Get(id, mountLabel string) (string, error) { - m := a.getActive(id) - m.Lock() - defer m.Unlock() + // Protect the a.active from concurrent access + a.Lock() + defer a.Unlock() + + m := a.active[id] + if m == nil { + m = &data{} + a.active[id] = m + } parents, err := a.getParentLayerPaths(id) if err != nil && !os.IsNotExist(err) { return "", err } - var parentLocks []*data - a.globalLock.Lock() - for _, p := range parents { - parentM, exists := a.active[p] - if !exists { - parentM = &data{} - a.active[p] = parentM - } - parentLocks = append(parentLocks, parentM) - } - a.globalLock.Unlock() - - for _, l := range parentLocks { - l.Lock() - } - defer func() { - for _, l := range parentLocks { - l.Unlock() - } - }() - // If a dir does not have a parent ( no layers )do not try to mount // just return the diff path to the data m.path = path.Join(a.rootPath(), "diff", id) @@ -355,24 +328,13 @@ func (a *Driver) Get(id, mountLabel string) (string, error) { return m.path, nil } -func (a *Driver) getActive(id string) *data { - // Protect the a.active from concurrent access - a.globalLock.Lock() - m, exists := a.active[id] - if !exists { - m = &data{} - a.active[id] = m - } - a.globalLock.Unlock() - return m -} - // Put unmounts and updates list of active mounts. func (a *Driver) Put(id string) error { - m := a.getActive(id) - m.Lock() - defer m.Unlock() + // Protect the a.active from concurrent access + a.Lock() + defer a.Unlock() + m := a.active[id] if m == nil { // but it might be still here if a.Exists(id) { @@ -384,7 +346,6 @@ func (a *Driver) Put(id string) error { } return nil } - if count := m.referenceCount; count > 1 { m.referenceCount = count - 1 } else { @@ -393,6 +354,7 @@ func (a *Driver) Put(id string) error { if ids != nil && len(ids) > 0 { a.unmount(m) } + delete(a.active, id) } return nil } diff --git a/daemon/graphdriver/aufs/mount.go b/daemon/graphdriver/aufs/mount.go index 36fa62e41b..d7e9bf9fd7 100644 --- a/daemon/graphdriver/aufs/mount.go +++ b/daemon/graphdriver/aufs/mount.go @@ -12,7 +12,7 @@ import ( // Unmount the target specified. func Unmount(target string) error { if err := exec.Command("auplink", target, "flush").Run(); err != nil { - logrus.Errorf("Couldn't run auplink before unmount %s: %s", target, err) + logrus.Errorf("Couldn't run auplink before unmount: %s", err) } if err := syscall.Unmount(target, 0); err != nil { return err From e386dfc33fc1fd5ed06496bd19f01a37c3c46341 Mon Sep 17 00:00:00 2001 From: Brian Goff Date: Sat, 27 Feb 2016 07:55:20 -0500 Subject: [PATCH 2/2] fix double-lock Signed-off-by: Brian Goff --- daemon/graphdriver/aufs/aufs.go | 2 -- daemon/graphdriver/aufs/mount.go | 2 +- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/daemon/graphdriver/aufs/aufs.go b/daemon/graphdriver/aufs/aufs.go index e03576aa8a..2d73d282fc 100644 --- a/daemon/graphdriver/aufs/aufs.go +++ b/daemon/graphdriver/aufs/aufs.go @@ -288,9 +288,7 @@ func (a *Driver) Remove(id string) error { return err } if m != nil { - a.Lock() delete(a.active, id) - a.Unlock() } return nil } diff --git a/daemon/graphdriver/aufs/mount.go b/daemon/graphdriver/aufs/mount.go index d7e9bf9fd7..36fa62e41b 100644 --- a/daemon/graphdriver/aufs/mount.go +++ b/daemon/graphdriver/aufs/mount.go @@ -12,7 +12,7 @@ import ( // Unmount the target specified. func Unmount(target string) error { if err := exec.Command("auplink", target, "flush").Run(); err != nil { - logrus.Errorf("Couldn't run auplink before unmount: %s", err) + logrus.Errorf("Couldn't run auplink before unmount %s: %s", target, err) } if err := syscall.Unmount(target, 0); err != nil { return err