From 63d4b293e7c91b8e2ed8da56f338f1bd0a4e1464 Mon Sep 17 00:00:00 2001 From: Alexander Larsson Date: Tue, 4 Feb 2014 17:13:00 +0100 Subject: [PATCH 1/7] devmapper: Rename DeviceSet.RemoveDevice to DeleteDevice "Remove" is a bit overloaded, as it is also what deactivating a device mapper device is called. Using "delete" is more clear here. Docker-DCO-1.1-Signed-off-by: Alexander Larsson (github: alexlarsson) --- graphdriver/devmapper/deviceset.go | 8 ++++---- graphdriver/devmapper/driver.go | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/graphdriver/devmapper/deviceset.go b/graphdriver/devmapper/deviceset.go index 8432d92a4..b6f21fa38 100644 --- a/graphdriver/devmapper/deviceset.go +++ b/graphdriver/devmapper/deviceset.go @@ -290,7 +290,7 @@ func (devices *DeviceSet) setupBaseImage() error { if oldInfo != nil && !oldInfo.Initialized { utils.Debugf("Removing uninitialized base image") - if err := devices.removeDevice(""); err != nil { + if err := devices.deleteDevice(""); err != nil { utils.Debugf("\n--->Err: %s\n", err) return err } @@ -562,7 +562,7 @@ func (devices *DeviceSet) AddDevice(hash, baseHash string) error { return nil } -func (devices *DeviceSet) removeDevice(hash string) error { +func (devices *DeviceSet) deleteDevice(hash string) error { info := devices.Devices[hash] if info == nil { return fmt.Errorf("hash %s doesn't exists", hash) @@ -610,11 +610,11 @@ func (devices *DeviceSet) removeDevice(hash string) error { return nil } -func (devices *DeviceSet) RemoveDevice(hash string) error { +func (devices *DeviceSet) DeleteDevice(hash string) error { devices.Lock() defer devices.Unlock() - return devices.removeDevice(hash) + return devices.deleteDevice(hash) } func (devices *DeviceSet) deactivateDevice(hash string) error { diff --git a/graphdriver/devmapper/driver.go b/graphdriver/devmapper/driver.go index 664899cfb..9b6cb109d 100644 --- a/graphdriver/devmapper/driver.go +++ b/graphdriver/devmapper/driver.go @@ -99,7 +99,7 @@ func (d *Driver) Remove(id string) error { if err := d.unmount(id, mp); err != nil { return err } - return d.DeviceSet.RemoveDevice(id) + return d.DeviceSet.DeleteDevice(id) } func (d *Driver) Get(id string) (string, error) { From dca21dfac77de2bbd69dd5a52b8a2b7816a54367 Mon Sep 17 00:00:00 2001 From: Alexander Larsson Date: Tue, 4 Feb 2014 16:06:54 +0100 Subject: [PATCH 2/7] devmapper: add and use removeDeviceAndWait helper This adds a function that calls the lowlevel removeDevice and then waits for it to finish. Docker-DCO-1.1-Signed-off-by: Alexander Larsson (github: alexlarsson) --- graphdriver/devmapper/deviceset.go | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/graphdriver/devmapper/deviceset.go b/graphdriver/devmapper/deviceset.go index b6f21fa38..e4c835a3d 100644 --- a/graphdriver/devmapper/deviceset.go +++ b/graphdriver/devmapper/deviceset.go @@ -632,28 +632,33 @@ func (devices *DeviceSet) deactivateDevice(hash string) error { return err } if devinfo.Exists != 0 { - if err := removeDevice(devname); err != nil { + if err := devices.removeDeviceAndWait(devname); err != nil { utils.Debugf("\n--->Err: %s\n", err) return err } - if err := devices.waitRemove(hash); err != nil { - return err - } } return nil } +// Issues the underlying dm remove operation and then waits +// for it to finish. +func (devices *DeviceSet) removeDeviceAndWait(devname string) error { + if err := removeDevice(devname); err != nil { + return err + } + if err := devices.waitRemove(devname); err != nil { + return err + } + return nil +} + // waitRemove blocks until either: // a) the device registered at - is removed, // or b) the 1 second timeout expires. -func (devices *DeviceSet) waitRemove(hash string) error { - utils.Debugf("[deviceset %s] waitRemove(%s)", devices.devicePrefix, hash) - defer utils.Debugf("[deviceset %s] waitRemove(%) END", devices.devicePrefix, hash) - devname, err := devices.byHash(hash) - if err != nil { - return err - } +func (devices *DeviceSet) waitRemove(devname string) error { + utils.Debugf("[deviceset %s] waitRemove(%s)", devices.devicePrefix, devname) + defer utils.Debugf("[deviceset %s] waitRemove(%s) END", devices.devicePrefix, devname) i := 0 for ; i < 1000; i += 1 { devinfo, err := getInfo(devname) From b95c560fdda4813319a2377e240592a3261e30ef Mon Sep 17 00:00:00 2001 From: Alexander Larsson Date: Tue, 4 Feb 2014 18:19:09 +0100 Subject: [PATCH 3/7] devmapper: Move refcounting to DeviceSet We already have some kind of refcounting in DeviceSet, this fleshes it out to allow it to completely subsume the refcounting in devmapper.Driver. This allows us to drop the double refcounting, and the locking inside devmapper.Driver. This, in particular the locking simplification will make it easier in the future to parallelize the device mapper. Docker-DCO-1.1-Signed-off-by: Alexander Larsson (github: alexlarsson) --- graphdriver/devmapper/deviceset.go | 108 +++++++++++++++++++-------- graphdriver/devmapper/driver.go | 70 ++++------------- graphdriver/devmapper/driver_test.go | 3 - 3 files changed, 93 insertions(+), 88 deletions(-) diff --git a/graphdriver/devmapper/deviceset.go b/graphdriver/devmapper/deviceset.go index e4c835a3d..4d708c840 100644 --- a/graphdriver/devmapper/deviceset.go +++ b/graphdriver/devmapper/deviceset.go @@ -29,6 +29,15 @@ type DevInfo struct { TransactionId uint64 `json:"transaction_id"` Initialized bool `json:"initialized"` devices *DeviceSet `json:"-"` + + mountCount int `json:"-"` + mountPath string `json:"-"` + // A floating mount means one reference is not owned and + // will be stolen by the next mount. This allows us to + // avoid unmounting directly after creation before the + // first get (since we need to mount to set up the device + // a bit first). + floating bool `json:"-"` } type MetaData struct { @@ -43,7 +52,6 @@ type DeviceSet struct { TransactionId uint64 NewTransactionId uint64 nextFreeDevice int - activeMounts map[string]int } type DiskUsage struct { @@ -69,6 +77,14 @@ type DevStatus struct { HighestMappedSector uint64 } +type UnmountMode int + +const ( + UnmountRegular UnmountMode = iota + UnmountFloat + UnmountSink +) + func getDevName(name string) string { return "/dev/mapper/" + name } @@ -733,13 +749,12 @@ func (devices *DeviceSet) Shutdown() error { utils.Debugf("[devmapper] Shutting down DeviceSet: %s", devices.root) defer utils.Debugf("[deviceset %s] shutdown END", devices.devicePrefix) - for path, count := range devices.activeMounts { - for i := count; i > 0; i-- { - if err := sysUnmount(path, 0); err != nil { - utils.Debugf("Shutdown unmounting %s, error: %s\n", path, err) + for _, info := range devices.Devices { + if info.mountCount > 0 { + if err := sysUnmount(info.mountPath, 0); err != nil { + utils.Debugf("Shutdown unmounting %s, error: %s\n", info.mountPath, err) } } - delete(devices.activeMounts, path) } for _, d := range devices.Devices { @@ -761,22 +776,32 @@ func (devices *DeviceSet) Shutdown() error { return nil } -func (devices *DeviceSet) MountDevice(hash, path string, readOnly bool) error { +func (devices *DeviceSet) MountDevice(hash, path string) error { devices.Lock() defer devices.Unlock() + info := devices.Devices[hash] + + if info.mountCount > 0 { + if path != info.mountPath { + return fmt.Errorf("Trying to mount devmapper device in multple places (%s, %s)", info.mountPath, path) + } + + if info.floating { + // Steal floating ref + info.floating = false + } else { + info.mountCount++ + } + return nil + } + if err := devices.activateDeviceIfNeeded(hash); err != nil { return fmt.Errorf("Error activating devmapper device for '%s': %s", hash, err) } - info := devices.Devices[hash] - var flags uintptr = sysMsMgcVal - if readOnly { - flags = flags | sysMsRdOnly - } - err := sysMount(info.DevName(), path, "ext4", flags, "discard") if err != nil && err == sysEInval { err = sysMount(info.DevName(), path, "ext4", flags, "") @@ -785,20 +810,50 @@ func (devices *DeviceSet) MountDevice(hash, path string, readOnly bool) error { return fmt.Errorf("Error mounting '%s' on '%s': %s", info.DevName(), path, err) } - count := devices.activeMounts[path] - devices.activeMounts[path] = count + 1 + info.mountCount = 1 + info.mountPath = path + info.floating = false return devices.setInitialized(hash) } -func (devices *DeviceSet) UnmountDevice(hash, path string, deactivate bool) error { - utils.Debugf("[devmapper] UnmountDevice(hash=%s path=%s)", hash, path) +func (devices *DeviceSet) UnmountDevice(hash string, mode UnmountMode) error { + utils.Debugf("[devmapper] UnmountDevice(hash=%s, mode=%d)", hash, mode) defer utils.Debugf("[devmapper] UnmountDevice END") devices.Lock() defer devices.Unlock() - utils.Debugf("[devmapper] Unmount(%s)", path) - if err := sysUnmount(path, 0); err != nil { + info := devices.Devices[hash] + + if mode == UnmountFloat { + if info.floating { + return fmt.Errorf("UnmountDevice: can't float floating reference %s\n", hash) + } + + // Leave this reference floating + info.floating = true + return nil + } + + if mode == UnmountSink { + if !info.floating { + // Someone already sunk this + return nil + } + // Otherwise, treat this as a regular unmount + } + + if info.mountCount == 0 { + return fmt.Errorf("UnmountDevice: device not-mounted id %s\n", hash) + } + + info.mountCount-- + if info.mountCount > 0 { + return nil + } + + utils.Debugf("[devmapper] Unmount(%s)", info.mountPath) + if err := sysUnmount(info.mountPath, 0); err != nil { utils.Debugf("\n--->Err: %s\n", err) return err } @@ -809,15 +864,9 @@ func (devices *DeviceSet) UnmountDevice(hash, path string, deactivate bool) erro return err } - if count := devices.activeMounts[path]; count > 1 { - devices.activeMounts[path] = count - 1 - } else { - delete(devices.activeMounts, path) - } + devices.deactivateDevice(hash) - if deactivate { - devices.deactivateDevice(hash) - } + info.mountPath = "" return nil } @@ -960,9 +1009,8 @@ func NewDeviceSet(root string, doInit bool) (*DeviceSet, error) { SetDevDir("/dev") devices := &DeviceSet{ - root: root, - MetaData: MetaData{Devices: make(map[string]*DevInfo)}, - activeMounts: make(map[string]int), + root: root, + MetaData: MetaData{Devices: make(map[string]*DevInfo)}, } if err := devices.initDevmapper(doInit); err != nil { diff --git a/graphdriver/devmapper/driver.go b/graphdriver/devmapper/driver.go index 9b6cb109d..25b998e59 100644 --- a/graphdriver/devmapper/driver.go +++ b/graphdriver/devmapper/driver.go @@ -8,7 +8,6 @@ import ( "github.com/dotcloud/docker/utils" "io/ioutil" "path" - "sync" ) func init() { @@ -22,9 +21,7 @@ func init() { type Driver struct { *DeviceSet - home string - sync.Mutex // Protects concurrent modification to active - active map[string]int + home string } var Init = func(home string) (graphdriver.Driver, error) { @@ -35,7 +32,6 @@ var Init = func(home string) (graphdriver.Driver, error) { d := &Driver{ DeviceSet: deviceSet, home: home, - active: make(map[string]int), } return d, nil } @@ -83,55 +79,36 @@ func (d *Driver) Create(id, parent string) error { return err } + // We float this reference so that the next Get call can + // steal it, so we don't have to unmount + if err := d.DeviceSet.UnmountDevice(id, UnmountFloat); err != nil { + return err + } + return nil } func (d *Driver) Remove(id string) error { - // Protect the d.active from concurrent access - d.Lock() - defer d.Unlock() - - if d.active[id] != 0 { - utils.Errorf("Warning: removing active id %s\n", id) - } - - mp := path.Join(d.home, "mnt", id) - if err := d.unmount(id, mp); err != nil { + // Sink the float from create in case no Get() call was made + if err := d.DeviceSet.UnmountDevice(id, UnmountSink); err != nil { return err } + // This assumes the device has been properly Get/Put:ed and thus is unmounted return d.DeviceSet.DeleteDevice(id) } func (d *Driver) Get(id string) (string, error) { - // Protect the d.active from concurrent access - d.Lock() - defer d.Unlock() - - count := d.active[id] - mp := path.Join(d.home, "mnt", id) - if count == 0 { - if err := d.mount(id, mp); err != nil { - return "", err - } + if err := d.mount(id, mp); err != nil { + return "", err } - d.active[id] = count + 1 - return path.Join(mp, "rootfs"), nil } func (d *Driver) Put(id string) { - // Protect the d.active from concurrent access - d.Lock() - defer d.Unlock() - - if count := d.active[id]; count > 1 { - d.active[id] = count - 1 - } else { - mp := path.Join(d.home, "mnt", id) - d.unmount(id, mp) - delete(d.active, id) + if err := d.DeviceSet.UnmountDevice(id, UnmountRegular); err != nil { + utils.Errorf("Warning: error unmounting device %s: %s\n", id, err) } } @@ -140,25 +117,8 @@ func (d *Driver) mount(id, mountPoint string) error { if err := osMkdirAll(mountPoint, 0755); err != nil && !osIsExist(err) { return err } - // If mountpoint is already mounted, do nothing - if mounted, err := Mounted(mountPoint); err != nil { - return fmt.Errorf("Error checking mountpoint: %s", err) - } else if mounted { - return nil - } // Mount the device - return d.DeviceSet.MountDevice(id, mountPoint, false) -} - -func (d *Driver) unmount(id, mountPoint string) error { - // If mountpoint is not mounted, do nothing - if mounted, err := Mounted(mountPoint); err != nil { - return fmt.Errorf("Error checking mountpoint: %s", err) - } else if !mounted { - return nil - } - // Unmount the device - return d.DeviceSet.UnmountDevice(id, mountPoint, true) + return d.DeviceSet.MountDevice(id, mountPoint) } func (d *Driver) Exists(id string) bool { diff --git a/graphdriver/devmapper/driver_test.go b/graphdriver/devmapper/driver_test.go index 785845ce6..ff4ec45e5 100644 --- a/graphdriver/devmapper/driver_test.go +++ b/graphdriver/devmapper/driver_test.go @@ -495,7 +495,6 @@ func TestDriverCreate(t *testing.T) { "DmTaskCreate", "DmTaskGetInfo", "sysMount", - "Mounted", "DmTaskRun", "DmTaskSetTarget", "DmTaskSetSector", @@ -614,7 +613,6 @@ func TestDriverRemove(t *testing.T) { "DmTaskCreate", "DmTaskGetInfo", "sysMount", - "Mounted", "DmTaskRun", "DmTaskSetTarget", "DmTaskSetSector", @@ -645,7 +643,6 @@ func TestDriverRemove(t *testing.T) { "DmTaskSetTarget", "DmTaskSetAddNode", "DmUdevWait", - "Mounted", "sysUnmount", ) }() From 7e25cd5891f84c2fab656c364002290e4b87f934 Mon Sep 17 00:00:00 2001 From: Alexander Larsson Date: Tue, 4 Feb 2014 16:08:45 +0100 Subject: [PATCH 4/7] devmapper: Use removeDeviceAndWait in DeviceSet.removeDevice() This makes sure the device is removed just like in deactivateDevice. Docker-DCO-1.1-Signed-off-by: Alexander Larsson (github: alexlarsson) --- graphdriver/devmapper/deviceset.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/graphdriver/devmapper/deviceset.go b/graphdriver/devmapper/deviceset.go index 4d708c840..4955fb5cf 100644 --- a/graphdriver/devmapper/deviceset.go +++ b/graphdriver/devmapper/deviceset.go @@ -595,7 +595,7 @@ func (devices *DeviceSet) deleteDevice(hash string) error { devinfo, _ := getInfo(info.Name()) if devinfo != nil && devinfo.Exists != 0 { - if err := removeDevice(info.Name()); err != nil { + if err := devices.removeDeviceAndWait(info.Name()); err != nil { utils.Debugf("Error removing device: %s\n", err) return err } From 2c82fd93d8a01cc1f53fe861378e6d2dca0486c6 Mon Sep 17 00:00:00 2001 From: Alexander Larsson Date: Wed, 5 Feb 2014 21:37:53 +0100 Subject: [PATCH 5/7] devmapper: Handle EBUSY while removing For some reason we seem to get transient EBUSY when removing thinp devices, which prohibit removing containers. When this happens we retry a few times which seems to fix the issue for me. Docker-DCO-1.1-Signed-off-by: Alexander Larsson (github: alexlarsson) --- graphdriver/devmapper/deviceset.go | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/graphdriver/devmapper/deviceset.go b/graphdriver/devmapper/deviceset.go index 4955fb5cf..3dedfd75b 100644 --- a/graphdriver/devmapper/deviceset.go +++ b/graphdriver/devmapper/deviceset.go @@ -12,6 +12,7 @@ import ( "path" "path/filepath" "strconv" + "strings" "sync" "time" ) @@ -52,6 +53,7 @@ type DeviceSet struct { TransactionId uint64 NewTransactionId uint64 nextFreeDevice int + sawBusy bool } type DiskUsage struct { @@ -371,6 +373,10 @@ func (devices *DeviceSet) log(level int, file string, line int, dmError int, mes return // Ignore _LOG_DEBUG } + if strings.Contains(message, "busy") { + devices.sawBusy = true + } + utils.Debugf("libdevmapper(%d): %s:%d (%d) %s", level, file, line, dmError, message) } @@ -660,9 +666,26 @@ func (devices *DeviceSet) deactivateDevice(hash string) error { // Issues the underlying dm remove operation and then waits // for it to finish. func (devices *DeviceSet) removeDeviceAndWait(devname string) error { - if err := removeDevice(devname); err != nil { + var err error + + for i := 0; i < 10; i++ { + devices.sawBusy = false + err = removeDevice(devname) + if err == nil { + break + } + if !devices.sawBusy { + return err + } + + // If we see EBUSY it may be a transient error, + // sleep a bit a retry a few times. + time.Sleep(5 * time.Millisecond) + } + if err != nil { return err } + if err := devices.waitRemove(devname); err != nil { return err } From 304e33a2fe2d006cc1063c8060c6e22c9ade3d59 Mon Sep 17 00:00:00 2001 From: Alexander Larsson Date: Thu, 6 Feb 2014 22:26:09 +0100 Subject: [PATCH 6/7] devmapper: Fix UnmountDevice for non-existing device Properly error out if passed an id that doesn't exist. Docker-DCO-1.1-Signed-off-by: Alexander Larsson (github: alexlarsson) --- graphdriver/devmapper/deviceset.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/graphdriver/devmapper/deviceset.go b/graphdriver/devmapper/deviceset.go index 3dedfd75b..c1f2f558a 100644 --- a/graphdriver/devmapper/deviceset.go +++ b/graphdriver/devmapper/deviceset.go @@ -847,6 +847,9 @@ func (devices *DeviceSet) UnmountDevice(hash string, mode UnmountMode) error { defer devices.Unlock() info := devices.Devices[hash] + if info == nil { + return fmt.Errorf("UnmountDevice: no such device %s\n", hash) + } if mode == UnmountFloat { if info.floating { From 693d3f8c6ab4c9fed5fe4c5ac5fcb44568fe1638 Mon Sep 17 00:00:00 2001 From: Alexander Larsson Date: Thu, 6 Feb 2014 23:08:17 +0100 Subject: [PATCH 7/7] devmapper: Fix MountDevice for non-existing Device Docker-DCO-1.1-Signed-off-by: Alexander Larsson (github: alexlarsson) --- graphdriver/devmapper/deviceset.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/graphdriver/devmapper/deviceset.go b/graphdriver/devmapper/deviceset.go index c1f2f558a..637192dbc 100644 --- a/graphdriver/devmapper/deviceset.go +++ b/graphdriver/devmapper/deviceset.go @@ -804,6 +804,9 @@ func (devices *DeviceSet) MountDevice(hash, path string) error { defer devices.Unlock() info := devices.Devices[hash] + if info == nil { + return fmt.Errorf("Unknown device %s", hash) + } if info.mountCount > 0 { if path != info.mountPath {