layerStore.Load(): don't try to lock the mounts list on cleanup

When cleaning up an incomplete layer, don't call regular Delete() to
handle it, since that calls Save(), which tries to lock the mountpoints
list, which we've already obtained a lock over.  Add a variation on
Delete() that skips the Save() step, which we're about to do anyway, and
call that instead.

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

layers.go

@@ -363,7 +363,7 @@ func (r *layerStore) Load() error {
 			}
 			if cleanup, ok := layer.Flags[incompleteFlag]; ok {
 				if b, ok := cleanup.(bool); ok && b {
-					err = r.Delete(layer.ID)
+					err = r.deleteInternal(layer.ID)
 					if err != nil {
 						break
 					}
@@ -958,7 +958,7 @@ func (r *layerStore) tspath(id string) string {
 	return filepath.Join(r.layerdir, id+tarSplitSuffix)
 }
 
-func (r *layerStore) Delete(id string) error {
+func (r *layerStore) deleteInternal(id string) error {
 	if !r.IsReadWrite() {
 		return errors.Wrapf(ErrStoreIsReadOnly, "not allowed to delete layers at %q", r.layerspath())
 	}
@@ -967,23 +967,7 @@ func (r *layerStore) Delete(id string) error {
 		return ErrLayerUnknown
 	}
 	id = layer.ID
-	// 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.
-	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)
@@ -1019,13 +1003,38 @@ func (r *layerStore) Delete(id string) error {
 				label.ReleaseLabel(mountLabel)
 			}
 		}
-		if err = r.Save(); err != nil {
-			return err
-		}
 	}
 	return err
 }
 
+func (r *layerStore) Delete(id string) error {
+	layer, ok := r.lookup(id)
+	if !ok {
+		return ErrLayerUnknown
+	}
+	id = layer.ID
+	// 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.
+	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)
+		}
+	}
+	if err := r.deleteInternal(id); err != nil {
+		return err
+	}
+	return r.Save()
+}
+
 func (r *layerStore) Lookup(name string) (id string, err error) {
 	if layer, ok := r.lookup(name); ok {
 		return layer.ID, nil

tests/cleanup-layer.bats

@@ -0,0 +1,17 @@
+#!/usr/bin/env bats
+
+load helpers
+
+@test "cleanup-layer" {
+	# Create a layer.
+	run storage --debug=false create-layer
+	[ "$status" -eq 0 ]
+	[ "$output" != "" ]
+	sed -i -e 's/"id":/"flags":{"incomplete":true},"id":/g' ${TESTDIR}/root/${STORAGE_DRIVER}-layers/layers.json
+
+	# Get a list of the layers, which should clean it up.
+	run storage --debug=false layers
+	[ "$status" -eq 0 ]
+	echo "$output"
+	[ "${#lines[*]}" -eq 0 ]
+}