From 73d9ede12c9328c44e38699dbe3a04479d3926e6 Mon Sep 17 00:00:00 2001 From: Alexander Larsson Date: Wed, 23 Apr 2014 13:50:53 +0200 Subject: [PATCH] devicemapper: Don't mount in Create() We used to mount in Create() to be able to create a few files that needs to be in each device. However, this mount is problematic for selinux, as we need to set the mount label at mount-time, and it is not known at the time of Create(). This change just moves the file creation to first Get() call and drops the mount from Create(). Additionally, this lets us remove some complexities we had to avoid an extra unmount+mount cycle. Docker-DCO-1.1-Signed-off-by: Alexander Larsson (github: alexlarsson) --- daemon/graphdriver/devmapper/deviceset.go | 44 +------------- daemon/graphdriver/devmapper/driver.go | 64 +++++++++------------ daemon/graphdriver/devmapper/driver_test.go | 11 ---- 3 files changed, 30 insertions(+), 89 deletions(-) diff --git a/daemon/graphdriver/devmapper/deviceset.go b/daemon/graphdriver/devmapper/deviceset.go index 97d670a3d9..640bebd32b 100644 --- a/daemon/graphdriver/devmapper/deviceset.go +++ b/daemon/graphdriver/devmapper/deviceset.go @@ -35,12 +35,6 @@ type DevInfo struct { 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:"-"` // The global DeviceSet lock guarantees that we serialize all // the calls to libdevmapper (which is not threadsafe), but we @@ -94,14 +88,6 @@ type DevStatus struct { HighestMappedSector uint64 } -type UnmountMode int - -const ( - UnmountRegular UnmountMode = iota - UnmountFloat - UnmountSink -) - func getDevName(name string) string { return "/dev/mapper/" + name } @@ -876,12 +862,7 @@ func (devices *DeviceSet) MountDevice(hash, path string, mountLabel string) erro 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++ - } + info.mountCount++ return nil } @@ -903,13 +884,12 @@ func (devices *DeviceSet) MountDevice(hash, path string, mountLabel string) erro info.mountCount = 1 info.mountPath = path - info.floating = false return devices.setInitialized(info) } -func (devices *DeviceSet) UnmountDevice(hash string, mode UnmountMode) error { - utils.Debugf("[devmapper] UnmountDevice(hash=%s, mode=%d)", hash, mode) +func (devices *DeviceSet) UnmountDevice(hash string) error { + utils.Debugf("[devmapper] UnmountDevice(hash=%s)", hash) defer utils.Debugf("[devmapper] UnmountDevice END") info, err := devices.lookupDevice(hash) @@ -923,24 +903,6 @@ func (devices *DeviceSet) UnmountDevice(hash string, mode UnmountMode) error { devices.Lock() defer devices.Unlock() - 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) } diff --git a/daemon/graphdriver/devmapper/driver.go b/daemon/graphdriver/devmapper/driver.go index e958ef3e59..66c4cb0767 100644 --- a/daemon/graphdriver/devmapper/driver.go +++ b/daemon/graphdriver/devmapper/driver.go @@ -64,26 +64,6 @@ func (d *Driver) Create(id, parent string, mountLabel string) error { if err := d.DeviceSet.AddDevice(id, parent); err != nil { return err } - mp := path.Join(d.home, "mnt", id) - if err := d.mount(id, mp); err != nil { - return err - } - - if err := osMkdirAll(path.Join(mp, "rootfs"), 0755); err != nil && !osIsExist(err) { - return err - } - - // Create an "id" file with the container/image id in it to help reconscruct this in case - // of later problems - if err := ioutil.WriteFile(path.Join(mp, "id"), []byte(id), 0600); err != nil { - 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 } @@ -96,10 +76,6 @@ func (d *Driver) Remove(id string) error { return 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 if err := d.DeviceSet.DeleteDevice(id); err != nil { return err @@ -115,28 +91,42 @@ func (d *Driver) Remove(id string) error { func (d *Driver) Get(id string) (string, error) { mp := path.Join(d.home, "mnt", id) - if err := d.mount(id, mp); err != nil { + + // Create the target directories if they don't exist + if err := osMkdirAll(mp, 0755); err != nil && !osIsExist(err) { return "", err } - return path.Join(mp, "rootfs"), nil + // Mount the device + if err := d.DeviceSet.MountDevice(id, mp, ""); err != nil { + return "", err + } + + rootFs := path.Join(mp, "rootfs") + if err := osMkdirAll(rootFs, 0755); err != nil && !osIsExist(err) { + d.DeviceSet.UnmountDevice(id) + return "", err + } + + idFile := path.Join(mp, "id") + if _, err := osStat(idFile); err != nil && osIsNotExist(err) { + // Create an "id" file with the container/image id in it to help reconscruct this in case + // of later problems + if err := ioutil.WriteFile(idFile, []byte(id), 0600); err != nil { + d.DeviceSet.UnmountDevice(id) + return "", err + } + } + + return rootFs, nil } func (d *Driver) Put(id string) { - if err := d.DeviceSet.UnmountDevice(id, UnmountRegular); err != nil { + if err := d.DeviceSet.UnmountDevice(id); err != nil { utils.Errorf("Warning: error unmounting device %s: %s\n", id, err) } } -func (d *Driver) mount(id, mountPoint string) error { - // Create the target directories if they don't exist - if err := osMkdirAll(mountPoint, 0755); err != nil && !osIsExist(err) { - return err - } - // Mount the device - return d.DeviceSet.MountDevice(id, mountPoint, "") -} - func (d *Driver) Exists(id string) bool { - return d.Devices[id] != nil + return d.DeviceSet.HasDevice(id) } diff --git a/daemon/graphdriver/devmapper/driver_test.go b/daemon/graphdriver/devmapper/driver_test.go index d431f942aa..77e8a6013a 100644 --- a/daemon/graphdriver/devmapper/driver_test.go +++ b/daemon/graphdriver/devmapper/driver_test.go @@ -500,15 +500,10 @@ func TestDriverCreate(t *testing.T) { calls.Assert(t, "DmTaskCreate", "DmTaskGetInfo", - "sysMount", "DmTaskRun", - "DmTaskSetTarget", "DmTaskSetSector", - "DmTaskSetCookie", - "DmUdevWait", "DmTaskSetName", "DmTaskSetMessage", - "DmTaskSetAddNode", ) }() @@ -619,15 +614,10 @@ func TestDriverRemove(t *testing.T) { calls.Assert(t, "DmTaskCreate", "DmTaskGetInfo", - "sysMount", "DmTaskRun", - "DmTaskSetTarget", "DmTaskSetSector", - "DmTaskSetCookie", - "DmUdevWait", "DmTaskSetName", "DmTaskSetMessage", - "DmTaskSetAddNode", ) Mounted = func(mnt string) (bool, error) { @@ -650,7 +640,6 @@ func TestDriverRemove(t *testing.T) { "DmTaskSetTarget", "DmTaskSetAddNode", "DmUdevWait", - "sysUnmount", ) }() runtime.GC()