lvm2/0005-lvmlockd-fix-thick-to-thin-lv-conversion.patch

From 76a4599500aef12442c8c24be8aacc89f6a27e68 Mon Sep 17 00:00:00 2001
From: David Teigland <teigland@redhat.com>
Date: Tue, 15 Aug 2023 09:53:39 -0500
Subject: [PATCH 05/24] lvmlockd: fix thick to thin lv conversion

---
 lib/locking/lvmlockd.c  |   2 +
 lib/metadata/metadata.c |   2 +
 tools/lvconvert.c       | 231 ++++++++++++++++++++++++----------------
 3 files changed, 143 insertions(+), 92 deletions(-)

diff --git a/lib/locking/lvmlockd.c b/lib/locking/lvmlockd.c
index a8db25d7a..d44b7333a 100644
--- a/lib/locking/lvmlockd.c
+++ b/lib/locking/lvmlockd.c
@@ -2908,6 +2908,8 @@ static int _free_lv(struct cmd_context *cmd, struct volume_group *vg,
 	if (!id_write_format(lv_id, lv_uuid, sizeof(lv_uuid)))
 		return_0;
 
+	log_debug("lockd free LV %s/%s %s lock_args %s", vg->name, lv_name, lv_uuid, lock_args ?: "none");
+
 	reply = _lockd_send("free_lv",
 				"pid = " FMTd64, (int64_t) getpid(),
 				"vg_name = %s", vg->name,
diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
index f56b5002d..f8a4f6279 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -2924,6 +2924,8 @@ int vg_write(struct volume_group *vg)
 	vgid[ID_LEN] = 0;
 	memcpy(vgid, &vg->id.uuid, ID_LEN);
 
+	log_debug("Writing metadata for VG %s.", vg->name);
+
 	if (vg_is_shared(vg)) {
 		dm_list_iterate_items(lvl, &vg->lvs) {
 			if (lvl->lv->lock_args && !strcmp(lvl->lv->lock_args, "pending")) {
diff --git a/tools/lvconvert.c b/tools/lvconvert.c
index d98d34ce0..7c9540712 100644
--- a/tools/lvconvert.c
+++ b/tools/lvconvert.c
@@ -3007,9 +3007,22 @@ static struct logical_volume *_lvconvert_insert_thin_layer(struct logical_volume
 	if (!(thin_segtype = get_segtype_from_string(vg->cmd, SEG_TYPE_NAME_THIN)))
 		return_NULL;
 
+	/*
+	 * input lv foo (often linear)
+	 * creates new lv foo_tpoolN (no seg)
+	 * segment from foo is moved to foo_tpoolN
+	 * new linear segment is created for foo that maps to foo_tpoolN
+	 * returns foo_tpoolN
+	 *
+	 * In spite of the "pool" variable naming, pool_lv foo_tpoolN is *not*
+	 * yet a pool type, but rather is whatever type the input lv was.
+	 */
 	if (!(pool_lv = insert_layer_for_lv(vg->cmd, lv, 0, "_tpool%d")))
 		return_NULL;
 
+	/*
+	 * change lv foo to a thin LV using foo_tpoolN
+	 */
 	lv->status |= THIN_VOLUME | VIRTUAL;
 	lv_set_visible(pool_lv);
 
@@ -3079,9 +3092,12 @@ static int _lvconvert_to_pool(struct cmd_context *cmd,
 	const char *pool_metadata_name;             /* user-specified lv name */
 	char converted_names[3*NAME_LEN];	    /* preserve names of converted lv */
 	struct segment_type *pool_segtype;          /* thinpool or cachepool */
+	const char *str_seg_type = to_cachepool ? SEG_TYPE_NAME_CACHE_POOL : SEG_TYPE_NAME_THIN_POOL;
 	struct lv_segment *seg;
 	unsigned int target_attr = ~0;
 	unsigned int activate_pool;
+	unsigned int lock_active_pool;
+	unsigned int lock_active_pool_done = 0;
 	unsigned int zero_metadata;
 	uint64_t meta_size;
 	uint32_t meta_extents;
@@ -3096,16 +3112,18 @@ static int _lvconvert_to_pool(struct cmd_context *cmd,
 	thin_discards_t discards;
 	thin_zero_t zero_new_blocks;
 	int error_when_full;
-	int r = 0;
+	int end_error = 0;
+	int is_active;
 
 	/* for handling lvmlockd cases */
 	char *lockd_data_args = NULL;
 	char *lockd_meta_args = NULL;
 	char *lockd_data_name = NULL;
 	char *lockd_meta_name = NULL;
+	uint32_t lockd_data_flags = 0;
+	uint32_t lockd_meta_flags = 0;
 	struct id lockd_data_id;
 	struct id lockd_meta_id;
-	const char *str_seg_type = to_cachepool ? SEG_TYPE_NAME_CACHE_POOL : SEG_TYPE_NAME_THIN_POOL;
 
 	if (!_raid_split_image_conversion(lv))
 		return_0;
@@ -3124,8 +3142,10 @@ static int _lvconvert_to_pool(struct cmd_context *cmd,
 		return 0;
 	}
 
-	/* Allow to have only thinpool active and restore it's active state. */
-	activate_pool = to_thinpool && lv_is_active(lv);
+	is_active = lv_is_active(lv);
+
+	activate_pool = to_thinpool && is_active;
+	lock_active_pool = (to_thinpool || to_thin) && is_active;
 
 	/* Wipe metadata_lv by default, but allow skipping this for cache pools. */
 	zero_metadata = (to_cachepool) ? arg_int_value(cmd, zero_ARG, 1) : 1;
@@ -3356,20 +3376,11 @@ static int _lvconvert_to_pool(struct cmd_context *cmd,
 		}
 	}
 
-	/*
-	 * When the LV referenced by the original function arg "lv"
-	 * is layered
-	 *
-	 * pool_name    pool name taken from lv arg
-	 * data_name    sub lv name, generated
-	 * meta_name    sub lv name, generated
-	 *
-	 * pool_lv      new lv for pool object, created here
-	 * data_lv      sub lv, was lv arg, now renamed
-	 * metadata_lv  sub lv, existing or created here
-	 */
-
 	if (to_thin) {
+		/*
+		 * pool_lv is not yet a pool, when returned, pool_lv contains
+		 * the segment that belonged to "lv".
+		 */
 		if (!(pool_lv = _lvconvert_insert_thin_layer(lv)))
 			goto_bad;
 	} else {
@@ -3383,6 +3394,17 @@ static int _lvconvert_to_pool(struct cmd_context *cmd,
 		pool_lv = lv;
 	}
 
+	/*
+	 * starts with pool_lv foo (not a pool yet)
+	 * creates new data_lv foo_tdata
+	 * segment from pool_lv foo is moved to data_lv foo_tdata
+	 * pool_lv foo linear segment is created that maps to foo_tdata
+	 * returns data_lv foo_tdata
+	 *
+	 * (In the to_thin case, the segment from the original lv is first
+	 * moved to pool_lv by _lvconvert_insert_thin_layer, and now is
+	 * moved to data_lv.)
+	 */
 	if (!(data_lv = insert_layer_for_lv(cmd, pool_lv, 0,
 					    (to_cachepool ? "_cdata" : "_tdata"))))
 		goto_bad;
@@ -3390,33 +3412,15 @@ static int _lvconvert_to_pool(struct cmd_context *cmd,
 	data_lv->status |= (to_cachepool) ? CACHE_POOL_DATA : THIN_POOL_DATA;
 	data_lv->status |= LVM_WRITE;  /* Pool data LV is writable */
 
+	/*
+	 * pool_lv now becomes a pool type.
+	 * FIXME: change variable naming to avoid this confusion.
+	 */
 	pool_lv->status |= (to_cachepool) ? CACHE_POOL : THIN_POOL;
 
 	seg = first_seg(pool_lv);
 	seg->segtype = pool_segtype;
 
-	/*
-	 * Create a new lock for a thin pool LV.  A cache pool LV has no lock.
-	 * Locks are removed from existing LVs that are being converted to
-	 * data and meta LVs (they are unlocked and deleted below.)
-	 */
-	if (vg_is_shared(vg)) {
-		lv->lock_args = NULL;
-		pool_lv->lock_args = NULL;
-		data_lv->lock_args = NULL;
-		metadata_lv->lock_args = NULL;
-
-		if (!to_cachepool) {
-			if (!strcmp(vg->lock_type, "sanlock"))
-				pool_lv->lock_args = "pending";
-			else if (!strcmp(vg->lock_type, "dlm"))
-				pool_lv->lock_args = "dlm";
-			else if (!strcmp(vg->lock_type, "idm"))
-				pool_lv->lock_args = "idm";
-			/* The lock_args will be set in vg_write(). */
-		}
-	}
-
 	/* Apply settings to the new pool seg */
 	if (to_cachepool) {
 		if (!cache_set_params(seg, chunk_size, cache_metadata_format, cache_mode, policy_name, policy_settings))
@@ -3453,87 +3457,130 @@ static int _lvconvert_to_pool(struct cmd_context *cmd,
 	if (!_lvconvert_attach_metadata_to_pool(seg, metadata_lv))
 		goto_bad;
 
-	if (!handle_pool_metadata_spare(vg,
-					metadata_lv->le_count,
-					use_pvh, pool_metadata_spare))
-		goto_bad;
+	/*
+	 * Create a new lock for a thin pool LV.  A cache pool LV has no lock.
+	 * Locks are removed from existing LVs that are being converted to
+	 * data and meta LVs (they are unlocked and deleted below.)
+	 * Acquire the new thin pool lock if the pool will remain active at
+	 * the end of the command.
+	 */
+	if (vg_is_shared(vg)) {
+		lv->lock_args = NULL;
+		pool_lv->lock_args = NULL;
+		data_lv->lock_args = NULL;
+		metadata_lv->lock_args = NULL;
 
-	if (to_thin) {
-		if (!lockd_lv(cmd, pool_lv, "ex", LDLV_PERSISTENT)) {
-			log_error("Failed to lock pool LV %s.", display_lvname(pool_lv));
-			goto out;
+		if (!to_cachepool) {
+			if (!strcmp(vg->lock_type, "sanlock")) {
+				if (!lockd_init_lv_args(cmd, vg, pool_lv,
+							vg->lock_type, &pool_lv->lock_args)) {
+					log_error("Cannot allocate lock for new pool LV.");
+					goto_bad;
+				}
+				pool_lv->new_lock_args = 1; /* tells vg_revert to lockd_free_lv */
+			} else if (!strcmp(vg->lock_type, "dlm")) {
+				pool_lv->lock_args = "dlm";
+			} else if (!strcmp(vg->lock_type, "idm")) {
+				pool_lv->lock_args = "idm";
+			}
+
+			if (lock_active_pool) {
+			       	if (!lockd_lv(cmd, pool_lv, "ex", LDLV_PERSISTENT)) {
+					log_error("Failed to lock new pool LV %s.", display_lvname(pool_lv));
+					goto_bad;
+				}
+				lock_active_pool_done = 1;
+			}
 		}
+	}
+
+	if (to_thin) {
 		if (!lv_update_and_reload(lv))
 			goto_bad;
 	} else {
 		if (!vg_write(vg) || !vg_commit(vg))
 			goto_bad;
+	}
 
-		if (activate_pool) {
-			if (!lockd_lv(cmd, pool_lv, "ex", LDLV_PERSISTENT)) {
-				log_error("Failed to lock pool LV %s.", display_lvname(pool_lv));
-				goto out;
-			}
+	/*
+	 * The main conversion is successfully committed.  If any subsequent
+	 * steps fail (creating spare, activating, unlocking), we do not
+	 * currently have the ability to undo the changes committed up to this
+	 * point.  Failures in the remaining steps can print an error and cause
+	 * the command to exit with an error, but no partial revert of the
+	 * completed steps is attempted.
+	 */
+	log_print_unless_silent("Converted %s to %s %s.", converted_names,
+				 (to_cachepool) ? "cache" : "thin",
+				 (to_thin) ? "volume" : "pool");
 
-			if (!activate_lv(cmd, pool_lv)) {
-				log_error("Failed to activate pool logical volume %s.",
-					  display_lvname(pool_lv));
-
-				/* Deactivate subvolumes */
-				if (!deactivate_lv(cmd, seg_lv(seg, 0)))
-					log_error("Failed to deactivate pool data logical volume %s.",
-						  display_lvname(seg_lv(seg, 0)));
-				if (!deactivate_lv(cmd, seg->metadata_lv))
-					log_error("Failed to deactivate pool metadata logical volume %s.",
-						  display_lvname(seg->metadata_lv));
-				goto out;
-			}
-		}
+	/*
+	 * FIXME handle_pool_metadata_spare() calls vg_write() vg_commit()
+	 * after creating a new lvolN, but then lvolN is renamed and hidden as
+	 * [lvolN_pmspare] without any further vg_write(). So, there's an extra
+	 * vg_write and vg_commit required here to cover the renaming/hiding.
+	 */
+	if (!handle_pool_metadata_spare(vg, metadata_lv->le_count, use_pvh, pool_metadata_spare) ||
+	    !vg_write(vg) || !vg_commit(vg)) {
+		log_error("Failed to set up spare metadata LV for thin pool.");
+		end_error = 1;
 	}
 
-	r = 1;
-
-out:
-	if (r)
-		log_print_unless_silent("Converted %s to %s %s.",
-					converted_names, (to_cachepool) ? "cache" : "thin",
-					(to_thin) ? "volume" : "pool");
+	if (activate_pool && !activate_lv(cmd, pool_lv)) {
+		log_error("Failed to activate pool logical volume %s.", display_lvname(pool_lv));
+		end_error = 1;
+	}
 
 	/*
 	 * Unlock and free the locks from existing LVs that became pool data
 	 * and meta LVs.
 	 */
 	if (lockd_data_name) {
-		if (!lockd_lv_name(cmd, vg, lockd_data_name, &lockd_data_id, lockd_data_args, "un", LDLV_PERSISTENT))
+		if (!lockd_lv_name(cmd, vg, lockd_data_name, &lockd_data_id, lockd_data_args, "un", lockd_data_flags)) {
 			log_error("Failed to unlock pool data LV %s/%s", vg->name, lockd_data_name);
-		lockd_free_lv(cmd, vg, lockd_data_name, &lockd_data_id, lockd_data_args);
+			end_error = 1;
+		}
+		if (!lockd_free_lv(cmd, vg, lockd_data_name, &lockd_data_id, lockd_data_args)) {
+			log_error("Failed to free lock for pool data LV %s/%s", vg->name, lockd_data_name);
+			end_error = 1;
+		}
 	}
-
 	if (lockd_meta_name) {
-		if (!lockd_lv_name(cmd, vg, lockd_meta_name, &lockd_meta_id, lockd_meta_args, "un", LDLV_PERSISTENT))
+		if (!lockd_lv_name(cmd, vg, lockd_meta_name, &lockd_meta_id, lockd_meta_args, "un", lockd_meta_flags)) {
 			log_error("Failed to unlock pool metadata LV %s/%s", vg->name, lockd_meta_name);
-		lockd_free_lv(cmd, vg, lockd_meta_name, &lockd_meta_id, lockd_meta_args);
+			end_error = 1;
+		}
+		if (!lockd_free_lv(cmd, vg, lockd_meta_name, &lockd_meta_id, lockd_meta_args)) {
+			log_error("Failed to free lock for pool metadata LV %s/%s", vg->name, lockd_meta_name);
+			end_error = 1;
+		}
 	}
-bad:
+
 	if (policy_settings)
 		dm_config_destroy(policy_settings);
 
-	return r;
-#if 0
-revert_new_lv:
-	/* TBD */
-	if (!pool_metadata_lv_name) {
-		if (!deactivate_lv(cmd, metadata_lv)) {
-			log_error("Failed to deactivate metadata lv.");
-			return 0;
-		}
-		if (!lv_remove(metadata_lv) || !vg_write(vg) || !vg_commit(vg))
-			log_error("Manual intervention may be required to remove "
-				  "abandoned LV(s) before retrying.");
+	if (end_error) {
+		log_error("Manual intervention may be required to handle reported errors.");
+		return 0;
 	}
 
+	return 1;
+
+	/*
+	 * Error exit path for failures that occur before the main conversion
+	 * is committed.  Failures that occur after the main conversion is
+	 * committed should not exit here.  There is some cleanup missing here.
+	 */
+bad:
+	if (lock_active_pool_done)
+		lockd_lv(cmd, pool_lv, "un", LDLV_PERSISTENT);
+	if (pool_lv && pool_lv->lock_args && pool_lv->new_lock_args)
+		lockd_free_lv(cmd, vg, pool_lv->name, &pool_lv->lvid.id[1], pool_lv->lock_args);
+
+	if (policy_settings)
+		dm_config_destroy(policy_settings);
+
 	return 0;
-#endif
 }
 
 static int _cache_vol_attach(struct cmd_context *cmd,
-- 
2.35.3