lvmlockd: fix locking for thin

lvremove of a thin lv while the pool is inactive would
leave the pool locked but inactive.

lvcreate of a thin snapshot while the pool is inactive
would leave the pool locked but inactive.

lvcreate of a thin lv could activate the pool to check
a threshold before the pool lock was acquired in lvmlockd.

lib/locking/lvmlockd.c

@@ -676,6 +676,10 @@ int handle_sanlock_lv(struct cmd_context *cmd, struct volume_group *vg)
 	 * Another host may have extended the lvmlock LV already.
 	 * Refresh so that we'll find the new space they added
 	 * when we search for new space.
+	 *
+	 * FIXME: we should be able to check if the lvmlock size
+	 * in VG metadata is smaller than lvmlock size reported
+	 * by the kernel, and avoid refresh if they match.
 	 */
 	if (!_refresh_sanlock_lv(cmd, vg))
 		return 0;
@@ -2575,7 +2579,7 @@ int lockd_lv_name(struct cmd_context *cmd, struct volume_group *vg,
 	}
 
  retry:
-	log_debug("lockd LV %s/%s mode %s uuid %s", vg->name, lv_name, mode, lv_uuid);
+	log_debug("lockd LV %s/%s mode %s uuid %s %s", vg->name, lv_name, mode, lv_uuid, opts ?: "");
 
 	/* Pass PV list for IDM lock type */
 	if (!strcmp(vg->lock_type, "idm")) {
@@ -3178,7 +3182,7 @@ int lockd_init_lv(struct cmd_context *cmd, struct volume_group *vg, struct logic
 
 	} else if (seg_is_thin(lp)) {
 		if ((seg_is_thin_volume(lp) && !lp->create_pool) ||
-		    (!seg_is_thin_volume(lp) && lp->snapshot)) {
+		    (!seg_is_thin_volume(lp) && lp->origin_name)) {
 			struct lv_list *lvl;
 
 			/*
@@ -3186,12 +3190,13 @@ int lockd_init_lv(struct cmd_context *cmd, struct volume_group *vg, struct logic
 			 * their own lock but use the pool lock.  If an lv does not
 			 * use its own lock, its lock_args is set to NULL.
 			 */
+			log_debug("lockd_init_lv thin %s locking thin pool", display_lvname(lv));
 
 			if (!(lvl = find_lv_in_vg(vg, lp->pool_name))) {
 				log_error("Failed to find thin pool %s/%s", vg->name, lp->pool_name);
 				return 0;
 			}
-			if (!lockd_lv(cmd, lvl->lv, "ex", LDLV_PERSISTENT)) {
+			if (!lockd_lv(cmd, lvl->lv, "ex", 0)) {
 				log_error("Failed to lock thin pool %s/%s", vg->name, lp->pool_name);
 				return 0;
 			}

lib/metadata/lv_manip.c

@@ -6887,6 +6887,10 @@ int lv_resize(struct cmd_context *cmd, struct logical_volume *lv,
 	/*
 	 * If the LV is locked due to being active, this lock call is a no-op.
 	 * Otherwise, this acquires a transient lock on the lv (not PERSISTENT)
+	 * FIXME: should probably use a persistent lock in case the command
+	 * crashes while the lv is active, in which case we'd want the active
+	 * lv to remain locked. This means then adding lockd_lv("un") at the
+	 * end.
 	 */
 	if (!lockd_lv_resize(cmd, lv_top, "ex", 0, lp))
 		return_0;
@@ -7549,6 +7553,7 @@ int lv_remove_single(struct cmd_context *cmd, struct logical_volume *lv,
 	int visible, historical;
 	struct logical_volume *pool_lv = NULL;
 	struct logical_volume *lock_lv = lv;
+	struct logical_volume *lockd_pool = NULL;
 	struct lv_segment *cache_seg = NULL;
 	struct seg_list *sl;
 	struct lv_segment *seg = first_seg(lv);
@@ -7613,8 +7618,21 @@ int lv_remove_single(struct cmd_context *cmd, struct logical_volume *lv,
 		return 0;
 	}
 
-	if (!lockd_lv(cmd, lock_lv, "ex", LDLV_PERSISTENT))
+	if (vg_is_shared(vg)) {
-		return_0;
+		if (lv_is_thin_type(lv)) {
+			/* FIXME: is this also needed for other types? */
+			/* Thin is special because it needs to be active and locked to remove. */
+			if (lv_is_thin_volume(lv))
+				lockd_pool = first_seg(lv)->pool_lv;
+			else if (lv_is_thin_pool(lv))
+				lockd_pool = lv;
+			if (!lockd_lv(cmd, lock_lv, "ex", LDLV_PERSISTENT))
+				return_0;
+		} else {
+			if (!lockd_lv(cmd, lock_lv, "ex", 0))
+				return_0;
+		}
+	}
 
 	if (!lv_is_cache_vol(lv)) {
 		if (!_lv_remove_check_in_use(lv, force))
@@ -7763,8 +7781,10 @@ int lv_remove_single(struct cmd_context *cmd, struct logical_volume *lv,
 					display_lvname(pool_lv));
 	}
 
-	if (!lockd_lv(cmd, lv, "un", LDLV_PERSISTENT))
+	if (lockd_pool && !thin_pool_is_active(lockd_pool)) {
-		log_warn("WARNING: Failed to unlock %s.", display_lvname(lv));
+		if (!lockd_lv_name(cmd, vg, lockd_pool->name, &lockd_pool->lvid.id[1], lockd_pool->lock_args, "un", LDLV_PERSISTENT))
+			log_warn("WARNING: Failed to unlock %s.", display_lvname(lockd_pool));
+	}
 	lockd_free_lv(cmd, vg, lv->name, &lv->lvid.id[1], lv->lock_args);
 
 	if (!suppress_remove_message && (visible || historical)) {
@@ -9243,6 +9263,11 @@ static struct logical_volume *_lv_create_an_lv(struct volume_group *vg,
 					return_NULL;
 				/* New pool is now inactive */
 			} else {
+				if (!lockd_lv(cmd, pool_lv, "ex", LDLV_PERSISTENT)) {
+					log_error("Failed to lock thin pool.");
+					return NULL;
+				}
+
 				if (!activate_lv(cmd, pool_lv)) {
 					log_error("Aborting. Failed to locally activate thin pool %s.",
 						  display_lvname(pool_lv));
@@ -9624,6 +9649,10 @@ static struct logical_volume *_lv_create_an_lv(struct volume_group *vg,
 				/* Avoid multiple thin-pool activations in this case */
 				if (thin_pool_was_active < 0)
 					thin_pool_was_active = 0;
+				if (!lockd_lv(cmd, pool_lv, "ex", LDLV_PERSISTENT)) {
+					log_error("Failed to lock thin pool.");
+					return NULL;
+				}
 				if (!activate_lv(cmd, pool_lv)) {
 					log_error("Failed to activate thin pool %s.",
 						  display_lvname(pool_lv));
@@ -9648,11 +9677,15 @@ static struct logical_volume *_lv_create_an_lv(struct volume_group *vg,
 		}
 
 		/* Restore inactive state if needed */
-		if (!thin_pool_was_active &&
+		if (!thin_pool_was_active) {
-		    !deactivate_lv(cmd, pool_lv)) {
+			if (!deactivate_lv(cmd, pool_lv)) {
-			log_error("Failed to deactivate thin pool %s.",
+				log_error("Failed to deactivate thin pool %s.", display_lvname(pool_lv));
-				  display_lvname(pool_lv));
+				return NULL;
-			return NULL;
+			}
+			if (!lockd_lv(cmd, pool_lv, "un", LDLV_PERSISTENT)) {
+				log_error("Failed to unlock thin pool.");
+				return NULL;
+			}
 		}
 	} else if (lp->snapshot) {
 		lv->status |= LV_TEMPORARY;