lvremove: fix failed remove of all LVs in shared VG

commit a125a3bb505cc "lv_remove: reduce commits for removed LVs"
changed "lvremove <vgname>" from removing one LV at a time,
to removing all LVs in one vg write/commit.  It also changed
the behavior if some of the LVs could not be removed, from
removing those LVs that could be removed, to removing nothing
if any LV could not be removed.  This caused a regression in
shared VGs using sanlock, in which the on-disk lease was
removed for any LV that could be removed, even if the command
decided to remove nothing.  This would leave LVs without a
valid ondisk lease, and "lock failed: error -221" would be
returned for any command attempting to lock the LV.

Fix this by not freeing the on-disk leases until after the
command has decided to go ahead and remove everything, and
has written the VG metadata.

Before the fix:

node1: lvchange -ay vg/lv1
node2: lvchange -ay vg/lv2

node1: lvs
  lv1  test -wi-a----- 4.00m
  lv2  test -wi------- 4.00m
node2: lvs
  lv1  test -wi------- 4.00m
  lv2  test -wi-a----- 4.00m

node1: lvremove -y vg/lv1 vg/lv2
  LV locked by other host: vg/lv2

(lvremove removed neither of the LVs, but it freed
the lock for lv1, which could have been removed
except for the proper locking failure on lv2.)

node1: lvs
  lv1  test -wi------- 4.00m
  lv2  test -wi------- 4.00m

node1: lvremove -y vg/lv1
LV vg/lv1 lock failed: error -221

(The lock for lv1 is gone, so nothing can be done with it.)

lib/locking/lvmlockd.c

@@ -3136,9 +3136,8 @@ out:
 }
 
 static int _free_lv(struct cmd_context *cmd, struct volume_group *vg,
-		    const char *lv_name, struct id *lv_id, const char *lock_args)
+		    const char *lv_name, char *lv_uuid, const char *lock_args)
 {
-	char lv_uuid[64] __attribute__((aligned(8)));
 	daemon_reply reply;
 	int result;
 	int ret;
@@ -3148,9 +3147,6 @@ static int _free_lv(struct cmd_context *cmd, struct volume_group *vg,
 	if (!_lvmlockd_connected)
 		return 0;
 
-	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",
@@ -3367,9 +3363,42 @@ int lockd_init_lv(struct cmd_context *cmd, struct volume_group *vg, struct logic
 
 /* lvremove */
 
-int lockd_free_lv(struct cmd_context *cmd, struct volume_group *vg,
+struct free_lv_info {
+	struct dm_list list;
+	char *uuid;
+	char *name;
+	char *args;
+};
+
+void lockd_free_removed_lvs(struct cmd_context *cmd, struct volume_group *vg, int remove_success)
+{
+	struct free_lv_info *fli;
+
+	/*
+	 * If lvremove has decided to remove none of the LVs, this will be 0
+	 * and we don't free any of the locks.
+	 */
+	if (remove_success) {
+		dm_list_iterate_items(fli, &vg->lockd_free_lvs)
+			_free_lv(cmd, vg, fli->name, fli->uuid, fli->args);
+	}
+	vg->needs_lockd_free_lvs = 0;
+	dm_list_init(&vg->lockd_free_lvs);
+}
+
+/*
+ * The LV lock will be freed later by lockd_free_removed_lvs() if the lvremove
+ * command decides to go ahead and remove the LV.  If lvremove finds that it
+ * cannot remove one the LVs that has been requested for removal, then it will
+ * remove none of the LVs, and lockd_free_removed_lvs() will be called with
+ * remove_success == 0, and it will not free any of the LV locks.
+ */
+int lockd_free_lv_after_update(struct cmd_context *cmd, struct volume_group *vg,
 		  const char *lv_name, struct id *lv_id, const char *lock_args)
 {
+	struct free_lv_info *fli;
+	char lv_uuid[64] __attribute__((aligned(8)));
+
 	switch (get_lock_type_from_string(vg->lock_type)) {
 	case LOCK_TYPE_NONE:
 	case LOCK_TYPE_CLVM:
@@ -3379,7 +3408,47 @@ int lockd_free_lv(struct cmd_context *cmd, struct volume_group *vg,
 	case LOCK_TYPE_IDM:
 		if (!lock_args)
 			return 1;
-		return _free_lv(cmd, vg, lv_name, lv_id, lock_args);
+		break;
+	default:
+		log_error("lockd_free_lv_after_update: unknown lock_type.");
+		return 0;
+	}
+
+	if (!id_write_format(lv_id, lv_uuid, sizeof(lv_uuid)))
+		return_0;
+
+	/* save lv info to send the free_lv messages later */
+	if (!(fli = dm_pool_zalloc(vg->vgmem, sizeof(*fli))))
+		return_0;
+	if (!(fli->uuid = dm_pool_strdup(vg->vgmem, lv_uuid)))
+		return_0;
+	if (!(fli->name = dm_pool_strdup(vg->vgmem, lv_name)))
+		return_0;
+	if (!(fli->args = dm_pool_strdup(vg->vgmem, lock_args)))
+		return_0;
+	dm_list_add(&vg->lockd_free_lvs, &fli->list);
+	vg->needs_lockd_free_lvs = 1;
+	return 1;
+}
+
+int lockd_free_lv(struct cmd_context *cmd, struct volume_group *vg,
+		  const char *lv_name, struct id *lv_id, const char *lock_args)
+{
+	char lv_uuid[64] __attribute__((aligned(8)));
+
+	if (!id_write_format(lv_id, lv_uuid, sizeof(lv_uuid)))
+		return_0;
+
+	switch (get_lock_type_from_string(vg->lock_type)) {
+	case LOCK_TYPE_NONE:
+	case LOCK_TYPE_CLVM:
+		return 1;
+	case LOCK_TYPE_DLM:
+	case LOCK_TYPE_SANLOCK:
+	case LOCK_TYPE_IDM:
+		if (!lock_args)
+			return 1;
+		return _free_lv(cmd, vg, lv_name, lv_uuid, lock_args);
 	default:
 		log_error("lockd_free_lv: unknown lock_type.");
 		return 0;

lib/locking/lvmlockd.h

@@ -113,6 +113,9 @@ int lockd_init_lv_args(struct cmd_context *cmd, struct volume_group *vg,
 		       struct logical_volume *lv, const char *lock_type, const char **lock_args);
 int lockd_free_lv(struct cmd_context *cmd, struct volume_group *vg,
 		  const char *lv_name, struct id *lv_id, const char *lock_args);
+int lockd_free_lv_after_update(struct cmd_context *cmd, struct volume_group *vg,
+		  const char *lv_name, struct id *lv_id, const char *lock_args);
+void lockd_free_removed_lvs(struct cmd_context *cmd, struct volume_group *vg, int remove_success);
 
 const char *lockd_running_lock_type(struct cmd_context *cmd, int *found_multiple);
 
@@ -262,6 +265,17 @@ static inline int lockd_free_lv(struct cmd_context *cmd, struct volume_group *vg
 	return 1;
 }
 
+static inline int lockd_free_lv_after_update(struct cmd_context *cmd, struct volume_group *vg,
+		  const char *lv_name, struct id *lv_id, const char *lock_args)
+{
+	return 1;
+}
+
+static inline void lockd_free_removed_lvs(struct cmd_context *cmd, struct volume_group *vg, int remove_success)
+{
+	return 1;
+}
+
 static inline const char *lockd_running_lock_type(struct cmd_context *cmd, int *found_multiple)
 {
 	log_error("Using a shared lock type requires lvmlockd.");

lib/metadata/lv_manip.c

@@ -1213,7 +1213,7 @@ static int _release_and_discard_lv_segment_area(struct lv_segment *seg, uint32_t
 		if (vg_is_shared(vg)) {
 			if (!lockd_lv_name(vg->cmd, vg, lv->name, &lv->lvid.id[1], lv->lock_args, "un", LDLV_PERSISTENT))
 				log_error("Failed to unlock vdo pool in lvmlockd.");
-			lockd_free_lv(vg->cmd, vg, lv->name, &lv->lvid.id[1], lv->lock_args);
+			lockd_free_lv_after_update(vg->cmd, vg, lv->name, &lv->lvid.id[1], lv->lock_args);
 		}
 		return 1;
 	}
@@ -7792,7 +7792,7 @@ int lv_remove_single(struct cmd_context *cmd, struct logical_volume *lv,
 		if (!lockd_lv(cmd, lv, "un", LDLV_PERSISTENT))
 			log_warn("WARNING: Failed to unlock %s.", display_lvname(lv));
 	}
-	lockd_free_lv(cmd, vg, lv->name, &lv->lvid.id[1], lv->lock_args);
+	lockd_free_lv_after_update(cmd, vg, lv->name, &lv->lvid.id[1], lv->lock_args);
 
 	if (!suppress_remove_message && (visible || historical)) {
 		(void) dm_snprintf(msg, sizeof(msg),

lib/metadata/vg.c

@@ -55,6 +55,7 @@ struct volume_group *alloc_vg(const char *pool_name, struct cmd_context *cmd,
 	dm_list_init(&vg->removed_historical_lvs);
 	dm_list_init(&vg->removed_pvs);
 	dm_list_init(&vg->msg_list);
+	dm_list_init(&vg->lockd_free_lvs);
 
 	log_debug_mem("Allocated VG %s at %p.", vg->name ? : "<no name>", (void *)vg);
 

lib/metadata/vg.h

@@ -45,6 +45,7 @@ struct volume_group {
 	unsigned lockd_not_started : 1;
 	unsigned needs_backup : 1;
 	unsigned needs_write_and_commit : 1;
+	unsigned needs_lockd_free_lvs : 1;
 	uint32_t write_count; /* count the number of vg_write calls */
 	uint32_t buffer_size_hint; /* hint with buffer size of parsed VG */
 
@@ -132,6 +133,7 @@ struct volume_group {
 	struct logical_volume *pool_metadata_spare_lv; /* one per VG */
 	struct logical_volume *sanlock_lv; /* one per VG */
 	struct dm_list msg_list;
+	struct dm_list lockd_free_lvs;
 };
 
 struct volume_group *alloc_vg(const char *pool_name, struct cmd_context *cmd,

tools/toollib.c

@@ -3673,6 +3673,9 @@ int process_each_lv_in_vg(struct cmd_context *cmd, struct volume_group *vg,
 	    (!vg_write(vg) || !vg_commit(vg)))
 		ret_max = ECMD_FAILED;
 
+	if (vg->needs_lockd_free_lvs)
+		lockd_free_removed_lvs(cmd, vg, (ret_max == ECMD_PROCESSED));
+
 	if (lvargs_supplied) {
 		/*
 		 * FIXME: lvm supports removal of LV with all its dependencies