devmapper: Get rid of metaData.devicesLock

Right now we seem to have 3 locks. 

- devinfo.lock
  This is a per device lock

- metaData.devicesLock

  This is supposedely protecting map of devices.

- Global DeviceSet lock

  This is protecting map of devices as well as serializing calls to libdevmapper.

Semantics of per devices lock and global deviceset lock seem to be very clear.
Even ordering between these two locks has been defined properly.

What is not clear is the need and ordering of metaData.devicesLock. Looks like
this lock is not necessary and global DeviceSet lock should be used to
protect map of devices as it is part of DeviceSet.

This patchset gets rid of metaData.devicesLock and instead uses DeviceSet
lock to protect map of devices.

Also at couple of places during initialization takes devices.Lock(). That
is not strictly necessary as there is supposed to be one thread of execution
during initializaiton. Still it makes the code clearer.

I think this makes code more clear and easier to understand and easier to
make further changes.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>

daemon/graphdriver/devmapper/deviceset.go

@@ -76,14 +76,13 @@ type devInfo struct {
 }
 
 type metaData struct {
-	Devices     map[string]*devInfo `json:"Devices"`
+	Devices map[string]*devInfo `json:"Devices"`
-	devicesLock sync.Mutex          // Protects all read/writes to Devices map
 }
 
 // DeviceSet holds information about list of devices
 type DeviceSet struct {
 	metaData      `json:"-"`
-	sync.Mutex    `json:"-"` // Protects Devices map and serializes calls into libdevmapper
+	sync.Mutex    `json:"-"` // Protects all fields of DeviceSet and serializes calls into libdevmapper
 	root          string
 	devicePrefix  string
 	TransactionID uint64 `json:"-"`
@@ -355,9 +354,8 @@ func (devices *DeviceSet) isDeviceIDFree(deviceID int) bool {
 	return true
 }
 
+// Should be called with devices.Lock() held.
 func (devices *DeviceSet) lookupDevice(hash string) (*devInfo, error) {
-	devices.devicesLock.Lock()
-	defer devices.devicesLock.Unlock()
 	info := devices.Devices[hash]
 	if info == nil {
 		info = devices.loadMetadata(hash)
@@ -370,6 +368,13 @@ func (devices *DeviceSet) lookupDevice(hash string) (*devInfo, error) {
 	return info, nil
 }
 
+func (devices *DeviceSet) lookupDeviceWithLock(hash string) (*devInfo, error) {
+	devices.Lock()
+	defer devices.Unlock()
+	info, err := devices.lookupDevice(hash)
+	return info, err
+}
+
 func (devices *DeviceSet) deviceFileWalkFunction(path string, finfo os.FileInfo) error {
 
 	// Skip some of the meta files which are not device files.
@@ -405,10 +410,7 @@ func (devices *DeviceSet) deviceFileWalkFunction(path string, finfo os.FileInfo)
 		return fmt.Errorf("Error loading device metadata file %s", hash)
 	}
 
-	devices.Lock()
 	devices.markDeviceIDUsed(dinfo.DeviceID)
-	devices.Unlock()
-
 	logrus.Debugf("Added deviceID=%d to DeviceIDMap", dinfo.DeviceID)
 	return nil
 }
@@ -434,6 +436,7 @@ func (devices *DeviceSet) constructDeviceIDMap() error {
 	return filepath.Walk(devices.metadataDir(), scan)
 }
 
+// Should be called with devices.Lock() held.
 func (devices *DeviceSet) unregisterDevice(id int, hash string) error {
 	logrus.Debugf("unregisterDevice(%v, %v)", id, hash)
 	info := &devInfo{
@@ -441,9 +444,7 @@ func (devices *DeviceSet) unregisterDevice(id int, hash string) error {
 		DeviceID: id,
 	}
 
-	devices.devicesLock.Lock()
 	delete(devices.Devices, hash)
-	devices.devicesLock.Unlock()
 
 	if err := devices.removeMetadata(info); err != nil {
 		logrus.Debugf("Error removing metadata: %s", err)
@@ -453,6 +454,7 @@ func (devices *DeviceSet) unregisterDevice(id int, hash string) error {
 	return nil
 }
 
+// Should be called with devices.Lock() held.
 func (devices *DeviceSet) registerDevice(id int, hash string, size uint64, transactionID uint64) (*devInfo, error) {
 	logrus.Debugf("registerDevice(%v, %v)", id, hash)
 	info := &devInfo{
@@ -464,15 +466,11 @@ func (devices *DeviceSet) registerDevice(id int, hash string, size uint64, trans
 		devices:       devices,
 	}
 
-	devices.devicesLock.Lock()
 	devices.Devices[hash] = info
-	devices.devicesLock.Unlock()
 
 	if err := devices.saveMetadata(info); err != nil {
 		// Try to remove unused device
-		devices.devicesLock.Lock()
 		delete(devices.Devices, hash)
-		devices.devicesLock.Unlock()
 		return nil, err
 	}
 
@@ -556,6 +554,9 @@ func (devices *DeviceSet) migrateOldMetaData() error {
 }
 
 func (devices *DeviceSet) initMetaData() error {
+	devices.Lock()
+	defer devices.Unlock()
+
 	if err := devices.migrateOldMetaData(); err != nil {
 		return err
 	}
@@ -596,6 +597,9 @@ func (devices *DeviceSet) getNextFreeDeviceID() (int, error) {
 }
 
 func (devices *DeviceSet) createRegisterDevice(hash string) (*devInfo, error) {
+	devices.Lock()
+	defer devices.Unlock()
+
 	deviceID, err := devices.getNextFreeDeviceID()
 	if err != nil {
 		return nil, err
@@ -774,7 +778,7 @@ func (devices *DeviceSet) saveBaseDeviceUUID(baseInfo *devInfo) error {
 }
 
 func (devices *DeviceSet) setupBaseImage() error {
-	oldInfo, _ := devices.lookupDevice("")
+	oldInfo, _ := devices.lookupDeviceWithLock("")
 	if oldInfo != nil && oldInfo.Initialized {
 		// If BaseDeviceUUID is nil (upgrade case), save it and
 		// return success.
@@ -1443,7 +1447,7 @@ func (devices *DeviceSet) AddDevice(hash, baseHash string) error {
 	logrus.Debugf("[deviceset] AddDevice(hash=%s basehash=%s)", hash, baseHash)
 	defer logrus.Debugf("[deviceset] AddDevice(hash=%s basehash=%s) END", hash, baseHash)
 
-	baseInfo, err := devices.lookupDevice(baseHash)
+	baseInfo, err := devices.lookupDeviceWithLock(baseHash)
 	if err != nil {
 		return err
 	}
@@ -1465,6 +1469,7 @@ func (devices *DeviceSet) AddDevice(hash, baseHash string) error {
 	return nil
 }
 
+// Should be called with devices.Lock() held.
 func (devices *DeviceSet) deleteDevice(info *devInfo) error {
 	if devices.doBlkDiscard {
 		// This is a workaround for the kernel not discarding block so
@@ -1508,7 +1513,7 @@ func (devices *DeviceSet) deleteDevice(info *devInfo) error {
 
 // DeleteDevice deletes a device from the hash.
 func (devices *DeviceSet) DeleteDevice(hash string) error {
-	info, err := devices.lookupDevice(hash)
+	info, err := devices.lookupDeviceWithLock(hash)
 	if err != nil {
 		return err
 	}
@@ -1651,11 +1656,11 @@ func (devices *DeviceSet) Shutdown() error {
 
 	var devs []*devInfo
 
-	devices.devicesLock.Lock()
+	devices.Lock()
 	for _, info := range devices.Devices {
 		devs = append(devs, info)
 	}
-	devices.devicesLock.Unlock()
+	devices.Unlock()
 
 	for _, info := range devs {
 		info.lock.Lock()
@@ -1676,7 +1681,7 @@ func (devices *DeviceSet) Shutdown() error {
 		info.lock.Unlock()
 	}
 
-	info, _ := devices.lookupDevice("")
+	info, _ := devices.lookupDeviceWithLock("")
 	if info != nil {
 		info.lock.Lock()
 		devices.Lock()
@@ -1702,7 +1707,7 @@ func (devices *DeviceSet) Shutdown() error {
 
 // MountDevice mounts the device if not already mounted.
 func (devices *DeviceSet) MountDevice(hash, path, mountLabel string) error {
-	info, err := devices.lookupDevice(hash)
+	info, err := devices.lookupDeviceWithLock(hash)
 	if err != nil {
 		return err
 	}
@@ -1756,7 +1761,7 @@ func (devices *DeviceSet) UnmountDevice(hash string) error {
 	logrus.Debugf("[devmapper] UnmountDevice(hash=%s)", hash)
 	defer logrus.Debugf("[devmapper] UnmountDevice(hash=%s) END", hash)
 
-	info, err := devices.lookupDevice(hash)
+	info, err := devices.lookupDeviceWithLock(hash)
 	if err != nil {
 		return err
 	}
@@ -1793,10 +1798,7 @@ func (devices *DeviceSet) UnmountDevice(hash string) error {
 
 // HasDevice returns true if the device metadata exists.
 func (devices *DeviceSet) HasDevice(hash string) bool {
-	devices.Lock()
+	info, _ := devices.lookupDeviceWithLock(hash)
-	defer devices.Unlock()
-
-	info, _ := devices.lookupDevice(hash)
 	return info != nil
 }
 
@@ -1805,15 +1807,12 @@ func (devices *DeviceSet) List() []string {
 	devices.Lock()
 	defer devices.Unlock()
 
-	devices.devicesLock.Lock()
 	ids := make([]string, len(devices.Devices))
 	i := 0
 	for k := range devices.Devices {
 		ids[i] = k
 		i++
 	}
-	devices.devicesLock.Unlock()
-
 	return ids
 }
 
@@ -1831,7 +1830,7 @@ func (devices *DeviceSet) deviceStatus(devName string) (sizeInSectors, mappedSec
 
 // GetDeviceStatus provides size, mapped sectors
 func (devices *DeviceSet) GetDeviceStatus(hash string) (*DevStatus, error) {
-	info, err := devices.lookupDevice(hash)
+	info, err := devices.lookupDeviceWithLock(hash)
 	if err != nil {
 		return nil, err
 	}
@@ -1957,7 +1956,7 @@ func (devices *DeviceSet) Status() *Status {
 
 // Status returns the current status of this deviceset
 func (devices *DeviceSet) exportDeviceMetadata(hash string) (*deviceMetadata, error) {
-	info, err := devices.lookupDevice(hash)
+	info, err := devices.lookupDeviceWithLock(hash)
 	if err != nil {
 		return nil, err
 	}