Lock the mounts list with its own lockfile

Separate loading and saving the mountpoints.json table out of the main
layer load/save paths so that they can be called independently, so that
we can mount and unmount layers (which requires that we update that
information) when the layer list itself may only be held with a read
lock.

The new loadMounts() and saveMounts() methods need to be called only for
read-write layer stores.  Callers that just refer to the mount
information can take a read lock on the mounts information, but callers
that modify the mount information need to acquire a write lock.

Break the unwritten "stores don't manage their own locks" rule and have
the layer store handle managing the lock for the mountpoints list, with
the understanding that the layer store's lock will always have been
acquired before we try to take the mounts lock.

Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>

layers.go

@@ -229,6 +229,7 @@ type LayerStore interface {
 
 type layerStore struct {
 	lockfile          Locker
+	mountsLockfile    Locker
 	rundir            string
 	driver            drivers.Driver
 	layerdir          string
@@ -291,7 +292,6 @@ func (r *layerStore) Load() error {
 	idlist := []string{}
 	ids := make(map[string]*Layer)
 	names := make(map[string]*Layer)
-	mounts := make(map[string]*Layer)
 	compressedsums := make(map[digest.Digest][]string)
 	uncompressedsums := make(map[digest.Digest][]string)
 	if r.lockfile.IsReadWrite() {
@@ -319,35 +319,25 @@ func (r *layerStore) Load() error {
 				label.ReserveLabel(layer.MountLabel)
 			}
 		}
+		err = nil
 	}
 	if shouldSave && (!r.IsReadWrite() || !r.Locked()) {
 		return ErrDuplicateLayerNames
 	}
-	mpath := r.mountspath()
-	data, err = ioutil.ReadFile(mpath)
-	if err != nil && !os.IsNotExist(err) {
-		return err
-	}
-	layerMounts := []layerMountPoint{}
-	if err = json.Unmarshal(data, &layerMounts); len(data) == 0 || err == nil {
-		for _, mount := range layerMounts {
-			if mount.MountPoint != "" {
-				if layer, ok := ids[mount.ID]; ok {
-					mounts[mount.MountPoint] = layer
-					layer.MountPoint = mount.MountPoint
-					layer.MountCount = mount.MountCount
-				}
-			}
-		}
-	}
 	r.layers = layers
 	r.idindex = truncindex.NewTruncIndex(idlist)
 	r.byid = ids
 	r.byname = names
-	r.bymount = mounts
 	r.bycompressedsum = compressedsums
 	r.byuncompressedsum = uncompressedsums
-	err = nil
+	// Load and merge information about which layers are mounted, and where.
+	if r.IsReadWrite() {
+		r.mountsLockfile.RLock()
+		defer r.mountsLockfile.Unlock()
+		if err = r.loadMounts(); err != nil {
+			return err
+		}
+	}
 	// Last step: if we're writable, try to remove anything that a previous
 	// user of this storage area marked for deletion but didn't manage to
 	// actually delete.
@@ -373,6 +363,30 @@ func (r *layerStore) Load() error {
 	return err
 }
 
+func (r *layerStore) loadMounts() error {
+	mounts := make(map[string]*Layer)
+	mpath := r.mountspath()
+	data, err := ioutil.ReadFile(mpath)
+	if err != nil && !os.IsNotExist(err) {
+		return err
+	}
+	layerMounts := []layerMountPoint{}
+	if err = json.Unmarshal(data, &layerMounts); len(data) == 0 || err == nil {
+		for _, mount := range layerMounts {
+			if mount.MountPoint != "" {
+				if layer, ok := r.lookup(mount.ID); ok {
+					mounts[mount.MountPoint] = layer
+					layer.MountPoint = mount.MountPoint
+					layer.MountCount = mount.MountCount
+				}
+			}
+		}
+		err = nil
+	}
+	r.bymount = mounts
+	return err
+}
+
 func (r *layerStore) Save() error {
 	if !r.IsReadWrite() {
 		return errors.Wrapf(ErrStoreIsReadOnly, "not allowed to modify the layer store at %q", r.layerspath())
@@ -388,6 +402,25 @@ func (r *layerStore) Save() error {
 	if err != nil {
 		return err
 	}
+	if err := ioutils.AtomicWriteFile(rpath, jldata, 0600); err != nil {
+		return err
+	}
+	if !r.IsReadWrite() {
+		return nil
+	}
+	r.mountsLockfile.Lock()
+	defer r.mountsLockfile.Unlock()
+	defer r.mountsLockfile.Touch()
+	return r.saveMounts()
+}
+
+func (r *layerStore) saveMounts() error {
+	if !r.IsReadWrite() {
+		return errors.Wrapf(ErrStoreIsReadOnly, "not allowed to modify the layer store at %q", r.layerspath())
+	}
+	if !r.mountsLockfile.Locked() {
+		return errors.New("layer store mount information is not locked for writing")
+	}
 	mpath := r.mountspath()
 	if err := os.MkdirAll(filepath.Dir(mpath), 0700); err != nil {
 		return err
@@ -406,11 +439,10 @@ func (r *layerStore) Save() error {
 	if err != nil {
 		return err
 	}
-	if err := ioutils.AtomicWriteFile(rpath, jldata, 0600); err != nil {
+	if err = ioutils.AtomicWriteFile(mpath, jmdata, 0600); err != nil {
 		return err
 	}
-	defer r.Touch()
+	return r.loadMounts()
-	return ioutils.AtomicWriteFile(mpath, jmdata, 0600)
 }
 
 func newLayerStore(rundir string, layerdir string, driver drivers.Driver, uidMap, gidMap []idtools.IDMap) (LayerStore, error) {
@@ -426,8 +458,13 @@ func newLayerStore(rundir string, layerdir string, driver drivers.Driver, uidMap
 	}
 	lockfile.Lock()
 	defer lockfile.Unlock()
+	mountsLockfile, err := GetLockfile(filepath.Join(rundir, "mountpoints.lock"))
+	if err != nil {
+		return nil, err
+	}
 	rlstore := layerStore{
 		lockfile:       lockfile,
+		mountsLockfile: mountsLockfile,
 		driver:         driver,
 		rundir:         rundir,
 		layerdir:       layerdir,
@@ -452,6 +489,7 @@ func newROLayerStore(rundir string, layerdir string, driver drivers.Driver) (ROL
 	defer lockfile.Unlock()
 	rlstore := layerStore{
 		lockfile:       lockfile,
+		mountsLockfile: nil,
 		driver:         driver,
 		rundir:         rundir,
 		layerdir:       layerdir,
@@ -673,6 +711,16 @@ func (r *layerStore) Create(id string, parent *Layer, names []string, mountLabel
 }
 
 func (r *layerStore) Mounted(id string) (int, error) {
+	if !r.IsReadWrite() {
+		return 0, errors.Wrapf(ErrStoreIsReadOnly, "no mount information for layers at %q", r.mountspath())
+	}
+	r.mountsLockfile.RLock()
+	defer r.mountsLockfile.Unlock()
+	if modified, err := r.mountsLockfile.Modified(); modified || err != nil {
+		if err = r.loadMounts(); err != nil {
+			return 0, err
+		}
+	}
 	layer, ok := r.lookup(id)
 	if !ok {
 		return 0, ErrLayerUnknown
@@ -684,13 +732,21 @@ func (r *layerStore) Mount(id string, options drivers.MountOpts) (string, error)
 	if !r.IsReadWrite() {
 		return "", errors.Wrapf(ErrStoreIsReadOnly, "not allowed to update mount locations for layers at %q", r.mountspath())
 	}
+	r.mountsLockfile.Lock()
+	defer r.mountsLockfile.Unlock()
+	if modified, err := r.mountsLockfile.Modified(); modified || err != nil {
+		if err = r.loadMounts(); err != nil {
+			return "", err
+		}
+	}
+	defer r.mountsLockfile.Touch()
 	layer, ok := r.lookup(id)
 	if !ok {
 		return "", ErrLayerUnknown
 	}
 	if layer.MountCount > 0 {
 		layer.MountCount++
-		return layer.MountPoint, r.Save()
+		return layer.MountPoint, r.saveMounts()
 	}
 	if options.MountLabel == "" {
 		options.MountLabel = layer.MountLabel
@@ -709,7 +765,7 @@ func (r *layerStore) Mount(id string, options drivers.MountOpts) (string, error)
 		layer.MountPoint = filepath.Clean(mountpoint)
 		layer.MountCount++
 		r.bymount[layer.MountPoint] = layer
-		err = r.Save()
+		err = r.saveMounts()
 	}
 	return mountpoint, err
 }
@@ -718,6 +774,14 @@ func (r *layerStore) Unmount(id string, force bool) (bool, error) {
 	if !r.IsReadWrite() {
 		return false, errors.Wrapf(ErrStoreIsReadOnly, "not allowed to update mount locations for layers at %q", r.mountspath())
 	}
+	r.mountsLockfile.Lock()
+	defer r.mountsLockfile.Unlock()
+	if modified, err := r.mountsLockfile.Modified(); modified || err != nil {
+		if err = r.loadMounts(); err != nil {
+			return false, err
+		}
+	}
+	defer r.mountsLockfile.Touch()
 	layer, ok := r.lookup(id)
 	if !ok {
 		layerByMount, ok := r.bymount[filepath.Clean(id)]
@@ -731,7 +795,7 @@ func (r *layerStore) Unmount(id string, force bool) (bool, error) {
 	}
 	if layer.MountCount > 1 {
 		layer.MountCount--
-		return true, r.Save()
+		return true, r.saveMounts()
 	}
 	err := r.driver.Put(id)
 	if err == nil || os.IsNotExist(err) {
@@ -740,12 +804,22 @@ func (r *layerStore) Unmount(id string, force bool) (bool, error) {
 		}
 		layer.MountCount--
 		layer.MountPoint = ""
-		return false, r.Save()
+		return false, r.saveMounts()
 	}
 	return true, err
 }
 
 func (r *layerStore) ParentOwners(id string) (uids, gids []int, err error) {
+	if !r.IsReadWrite() {
+		return nil, nil, errors.Wrapf(ErrStoreIsReadOnly, "no mount information for layers at %q", r.mountspath())
+	}
+	r.mountsLockfile.RLock()
+	defer r.mountsLockfile.Unlock()
+	if modified, err := r.mountsLockfile.Modified(); modified || err != nil {
+		if err = r.loadMounts(); err != nil {
+			return nil, nil, err
+		}
+	}
 	layer, ok := r.lookup(id)
 	if !ok {
 		return nil, nil, ErrLayerUnknown
@@ -865,12 +939,20 @@ func (r *layerStore) Delete(id string) error {
 	// The layer may already have been explicitly unmounted, but if not, we
 	// should try to clean that up before we start deleting anything at the
 	// driver level.
-	for layer.MountCount > 0 {
+	mountCount, err := r.Mounted(id)
+	if err != nil {
+		return errors.Wrapf(err, "error checking if layer %q is still mounted", id)
+	}
+	for mountCount > 0 {
 		if _, err := r.Unmount(id, false); err != nil {
 			return err
 		}
+		mountCount, err = r.Mounted(id)
+		if err != nil {
+			return errors.Wrapf(err, "error checking if layer %q is still mounted", id)
 		}
-	err := r.driver.Remove(id)
+	}
+	err = r.driver.Remove(id)
 	if err == nil {
 		os.Remove(r.tspath(id))
 		delete(r.byid, id)
@@ -1236,7 +1318,20 @@ func (r *layerStore) Touch() error {
 }
 
 func (r *layerStore) Modified() (bool, error) {
-	return r.lockfile.Modified()
+	var mmodified bool
+	lmodified, err := r.lockfile.Modified()
+	if err != nil {
+		return lmodified, err
+	}
+	if r.IsReadWrite() {
+		r.mountsLockfile.RLock()
+		defer r.mountsLockfile.Unlock()
+		mmodified, err = r.mountsLockfile.Modified()
+		if err != nil {
+			return lmodified, err
+		}
+	}
+	return lmodified || mmodified, nil
 }
 
 func (r *layerStore) IsReadWrite() bool {