lvmlockd: vg_validate and vgcreate/vgchange changes

Check lock_type and lock_args in vg_validate.

Make some minor changes to when lock_type and lock_args
are written during vgcreate and vgchange to eliminate
intermediate states of the VG.

6 files changed, 167 insertions, 41 deletions

diff --git a/lib/locking/lvmlockd.c b/lib/locking/lvmlockd.c
index 6803d1529..1c7add7d1 100644
--- a/lib/locking/lvmlockd.c
+++ b/lib/locking/lvmlockd.c
@@ -352,8 +352,6 @@ static int _create_sanlock_lv(struct cmd_context *cmd, struct volume_group *vg,
 		return 0;
 	}
 
-	lv_set_hidden(lv);
-	lv->status |= LOCKD_SANLOCK_LV;
 	vg->sanlock_lv = lv;
 
 	return 1;
@@ -511,16 +509,15 @@ static int _init_vg_dlm(struct cmd_context *cmd, struct volume_group *vg)
 
 	switch (result) {
 	case 0:
-		log_print_unless_silent("VG %s initialized %s lockspace", vg->name, vg->lock_type);
 		break;
 	case -ELOCKD:
 		log_error("VG %s init failed: lvmlockd not available", vg->name);
 		break;
 	case -EARGS:
-		log_error("VG %s init failed: invalid parameters for %s", vg->name, vg->lock_type);
+		log_error("VG %s init failed: invalid parameters for dlm", vg->name);
 		break;
 	case -EMANAGER:
-		log_error("VG %s init failed: lock manager %s is not running", vg->name, vg->lock_type);
+		log_error("VG %s init failed: lock manager dlm is not running", vg->name);
 		break;
 	default:
 		log_error("VG %s init failed: %d", vg->name, result);
@@ -541,6 +538,7 @@ static int _init_vg_dlm(struct cmd_context *cmd, struct volume_group *vg)
 		goto out;
 	}
 
+	vg->lock_type = "dlm";
 	vg->lock_args = vg_lock_args;
 
 	if (!vg_write(vg) || !vg_commit(vg)) {
@@ -579,6 +577,12 @@ static int _init_vg_sanlock(struct cmd_context *cmd, struct volume_group *vg)
 	if (!(extend_mb = find_config_tree_int(cmd, global_sanlock_lv_extend_CFG, NULL)))
 		extend_mb = DEFAULT_SANLOCK_LV_EXTEND_MB;
 
+	/*
+	 * Creating the sanlock LV writes the VG containing the new lvmlock
+	 * LV, then activates the lvmlock LV.  The lvmlock LV must be active
+	 * before we ask lvmlockd to initialize the VG because sanlock needs
+	 * to initialize leases on the lvmlock LV.
+	 */
 	if (!_create_sanlock_lv(cmd, vg, LOCKD_SANLOCK_LV_NAME, extend_mb)) {
 		log_error("Failed to create internal lv.");
 		return 0;
@@ -607,16 +611,15 @@ static int _init_vg_sanlock(struct cmd_context *cmd, struct volume_group *vg)
 
 	switch (result) {
 	case 0:
-		log_print_unless_silent("VG %s initialized %s lockspace", vg->name, vg->lock_type);
 		break;
 	case -ELOCKD:
 		log_error("VG %s init failed: lvmlockd not available", vg->name);
 		break;
 	case -EARGS:
-		log_error("VG %s init failed: invalid parameters for %s", vg->name, vg->lock_type);
+		log_error("VG %s init failed: invalid parameters for sanlock", vg->name);
 		break;
 	case -EMANAGER:
-		log_error("VG %s init failed: lock manager %s is not running", vg->name, vg->lock_type);
+		log_error("VG %s init failed: lock manager sanlock is not running", vg->name);
 		break;
 	case -EMSGSIZE:
 		log_error("VG %s init failed: no disk space for leases", vg->name);
@@ -640,6 +643,10 @@ static int _init_vg_sanlock(struct cmd_context *cmd, struct volume_group *vg)
 		goto out;
 	}
 
+	lv_set_hidden(vg->sanlock_lv);
+	vg->sanlock_lv->status |= LOCKD_SANLOCK_LV;
+
+	vg->lock_type = "sanlock";
 	vg->lock_args = vg_lock_args;
 
 	if (!vg_write(vg) || !vg_commit(vg)) {
@@ -759,9 +766,10 @@ static int _free_vg_sanlock(struct cmd_context *cmd, struct volume_group *vg)
 
 /* vgcreate */
 
-int lockd_init_vg(struct cmd_context *cmd, struct volume_group *vg)
-{       
-	switch (lock_type_to_num(vg->lock_type)) {
+int lockd_init_vg(struct cmd_context *cmd, struct volume_group *vg,
+		  const char *lock_type)
+{
+	switch (lock_type_to_num(lock_type)) {
 	case LOCK_TYPE_NONE:
 	case LOCK_TYPE_CLVM:
 		return 1;
diff --git a/lib/locking/lvmlockd.h b/lib/locking/lvmlockd.h
index 7a50ebc37..33d3e04e5 100644
--- a/lib/locking/lvmlockd.h
+++ b/lib/locking/lvmlockd.h
@@ -106,7 +106,7 @@ void lvmlockd_disconnect(void);
 
 /* vgcreate/vgremove use init/free */
 
-int lockd_init_vg(struct cmd_context *cmd, struct volume_group *vg);
+int lockd_init_vg(struct cmd_context *cmd, struct volume_group *vg, const char *lock_type);
 int lockd_free_vg_before(struct cmd_context *cmd, struct volume_group *vg);
 void lockd_free_vg_final(struct cmd_context *cmd, struct volume_group *vg);
 
@@ -177,7 +177,7 @@ static inline int lvmlockd_use(void)
 	return 0;
 }
 
-static inline int lockd_init_vg(struct cmd_context *cmd, struct volume_group *vg)
+static inline int lockd_init_vg(struct cmd_context *cmd, struct volume_group *vg, const char *lock_type)
 {
 	return 1;
 }
diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
index cf7b71911..8f65eb51b 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -2439,6 +2439,7 @@ struct validate_hash {
 	struct dm_hash_table *lvname;
 	struct dm_hash_table *lvid;
 	struct dm_hash_table *pvid;
+	struct dm_hash_table *lv_lock_args;
 };
 
 /*
@@ -2797,6 +2798,87 @@ int vg_validate(struct volume_group *vg)
 
 	if (vg_max_lv_reached(vg))
 		stack;
+
+	if (!(vhash.lv_lock_args = dm_hash_create(lv_count))) {
+		log_error("Failed to allocate lv_lock_args hash");
+		r = 0;
+		goto out;
+	}
+
+	if (is_lockd_type(vg->lock_type)) {
+		if (!vg->lock_args) {
+			log_error(INTERNAL_ERROR "VG %s with lock_type %s without lock_args",
+				  vg->name, vg->lock_type);
+			r = 0;
+		}
+
+		if (vg_is_clustered(vg)) {
+			log_error(INTERNAL_ERROR "VG %s with lock_type %s is clustered",
+				  vg->name, vg->lock_type);
+			r = 0;
+		}
+
+		if (vg->system_id && vg->system_id[0]) {
+			log_error(INTERNAL_ERROR "VG %s with lock_type %s has system_id %s",
+				  vg->name, vg->lock_type, vg->system_id);
+			r = 0;
+		}
+
+		if (strcmp(vg->lock_type, "sanlock") && strcmp(vg->lock_type, "dlm")) {
+			log_error(INTERNAL_ERROR "VG %s has unknown lock_type %s",
+				  vg->name, vg->lock_type);
+			r = 0;
+		}
+	} else {
+		if (vg->lock_args) {
+			log_error(INTERNAL_ERROR "VG %s has lock_args %s without lock_type",
+				  vg->name, vg->lock_args);
+			r = 0;
+		}
+	}
+
+	dm_list_iterate_items(lvl, &vg->lvs) {
+		if (is_lockd_type(vg->lock_type)) {
+			if (lockd_lv_uses_lock(lvl->lv)) {
+				if (vg->skip_validate_lock_args) {
+					continue;
+				} else if (!lvl->lv->lock_args) {
+					log_error(INTERNAL_ERROR "LV %s/%s missing lock_args",
+						  vg->name, lvl->lv->name);
+					r = 0;
+				} else if (!strcmp(vg->lock_type, "sanlock")) {
+					if (dm_hash_lookup(vhash.lv_lock_args, lvl->lv->lock_args)) {
+						log_error(INTERNAL_ERROR "LV %s/%s has duplicate lock_args %s.",
+							  vg->name, lvl->lv->name, lvl->lv->lock_args);
+						r = 0;
+					}
+
+					if (!dm_hash_insert(vhash.lv_lock_args, lvl->lv->lock_args, lvl)) {
+						log_error("Failed to hash lvname.");
+						r = 0;
+					}
+
+				} else if (!strcmp(vg->lock_type, "dlm") && strcmp(lvl->lv->lock_args, "dlm")) {
+					log_error(INTERNAL_ERROR "LV %s/%s bad dlm lock_args %s",
+						  vg->name, lvl->lv->name, lvl->lv->lock_args);
+					r = 0;
+				}
+			} else {
+				if (lvl->lv->lock_args) {
+					log_error(INTERNAL_ERROR "LV %s/%s shouldn't have lock_args",
+						  vg->name, lvl->lv->name);
+					r = 0;
+				}
+			}
+		} else {
+			if (lvl->lv->lock_args) {
+				log_error(INTERNAL_ERROR "LV %s/%s with no lock_type has lock_args %s",
+					  vg->name, lvl->lv->name, lvl->lv->lock_args);
+				r = 0;
+			}
+		}
+	}
+
 out:
 	if (vhash.lvid)
 		dm_hash_destroy(vhash.lvid);
@@ -2804,6 +2886,8 @@ out:
 		dm_hash_destroy(vhash.lvname);
 	if (vhash.pvid)
 		dm_hash_destroy(vhash.pvid);
+	if (vhash.lv_lock_args)
+		dm_hash_destroy(vhash.lv_lock_args);
 
 	return r;
 }
diff --git a/lib/metadata/vg.h b/lib/metadata/vg.h
index b676c3cfd..a21af8b06 100644
--- a/lib/metadata/vg.h
+++ b/lib/metadata/vg.h
@@ -49,6 +49,7 @@ struct volume_group {
 	struct dm_list *cmd_vgs;/* List of wanted/locked and opened VGs */
 	uint32_t cmd_missing_vgs;/* Flag marks missing VG */
 	uint32_t seqno;		/* Metadata sequence number */
+	unsigned skip_validate_lock_args : 1;
 
 	/*
 	 * The parsed on-disk copy of this VG; is NULL if this is the on-disk
diff --git a/tools/vgchange.c b/tools/vgchange.c
index ec713aa67..d739ed2a0 100644
--- a/tools/vgchange.c
+++ b/tools/vgchange.c
@@ -519,7 +519,6 @@ static int _vgchange_locktype(struct cmd_context *cmd,
 			      struct volume_group *vg)
 {
 	const char *lock_type = arg_str_value(cmd, locktype_ARG, NULL);
-	const char *lock_args = NULL;
 	struct lv_list *lvl;
 	struct logical_volume *lv;
 
@@ -555,6 +554,13 @@ static int _vgchange_locktype(struct cmd_context *cmd,
 		return 1;
 	}
 
+	if (!vg->lock_type) {
+		if (vg_is_clustered(vg))
+			vg->lock_type = "clvm";
+		else
+			vg->lock_type = "none";
+	}
+
 	if (!strcmp(vg->lock_type, lock_type)) {
 		log_warn("New lock_type %s matches the current lock_type %s.",
 			 lock_type, vg->lock_type);
@@ -654,41 +660,64 @@ static int _vgchange_locktype(struct cmd_context *cmd,
 		return 1;
 	}
 
-	/*
-	 * ... to lockd type.  These are the same steps vgcreate uses.
-	 */
+	/* ... to lockd type */
 	if (is_lockd_type(lock_type)) {
-		if (!vg_set_lock_type(vg, lock_type))
-			return 0;
-
-		if (!lockd_init_vg(cmd, vg)) {
-			log_error("Failed to initialize lock args for lock type %s", lock_type);
-			return 0;
+		/*
+		 * For lock_type dlm, lockd_init_vg() will do a single
+		 * vg_write() that sets lock_type, sets lock_args, clears
+		 * system_id, and sets all LV lock_args to dlm.
+		 */
+		if (!strcmp(lock_type, "dlm")) {
+			dm_list_iterate_items(lvl, &vg->lvs) {
+				lv = lvl->lv;
+				if (lockd_lv_uses_lock(lv))
+					lv->lock_args = "dlm";
+			}
 		}
 
-		vg->system_id = NULL;
-
-		/* These are the same steps lvcreate uses within a lockd type VG. */
+		/*
+		 * See below.  We cannot set valid LV lock_args until stage 1
+		 * of the change is done, so we need to skip the validation of
+		 * the lock_args during stage 1.
+		 */
+		if (!strcmp(lock_type, "sanlock"))
+			vg->skip_validate_lock_args = 1;
 
-		dm_list_iterate_items(lvl, &vg->lvs) {
-			lv = lvl->lv;
+		vg->system_id = NULL;
 
-			/* Some LV types have no lock. */
-			if (!lockd_lv_uses_lock(lv))
-				continue;
+		if (!lockd_init_vg(cmd, vg, lock_type)) {
+			log_error("Failed to initialize lock args for lock type %s", lock_type);
+			return 0;
+		}
 
-			if (!lockd_init_lv_args(cmd, vg, lv, lock_type, &lock_args)) {
-				log_error("Failed to init %s lock args LV %s/%s",
-					  lock_type, vg->name, lv->name);
-				return 0;
+		/*
+		 * For lock_type sanlock, there must be multiple steps
+		 * because the VG needs an active lvmlock LV before
+		 * LV lock areas can be allocated, which must be done
+		 * before LV lock_args are written.  So, the LV lock_args
+		 * remain unset during the first stage of the conversion.
+		 *
+		 * Stage 1:
+		 * lockd_init_vg() creates and activates the lvmlock LV,
+		 * then sets lock_type, sets lock_args, and clears system_id.
+		 *
+		 * Stage 2:
+		 * We get here, and can now set LV lock_args.  This uses
+		 * the standard code path for allocating LV locks in
+		 * vg_write() by setting LV lock_args to "pending",
+		 * which tells vg_write() to call lockd_init_lv()
+		 * and sets the lv->lock_args value before writing the VG.
+		 */
+		if (!strcmp(lock_type, "sanlock")) {
+			dm_list_iterate_items(lvl, &vg->lvs) {
+				lv = lvl->lv;
+				if (lockd_lv_uses_lock(lv))
+					lv->lock_args = "pending";
 			}
 
-			lv->lock_args = lock_args;
+			vg->skip_validate_lock_args = 0;
 		}
 
-		if (!lockd_start_vg(cmd, vg))
-			log_error("Failed to start locking for VG %s", vg->name);
-
 		return 1;
 	}
 
diff --git a/tools/vgcreate.c b/tools/vgcreate.c
index 67147bfcc..20ba4aa31 100644
--- a/tools/vgcreate.c
+++ b/tools/vgcreate.c
@@ -78,7 +78,6 @@ int vgcreate(struct cmd_context *cmd, int argc, char **argv)
 	    !vg_set_max_pv(vg, vp_new.max_pv) ||
 	    !vg_set_alloc_policy(vg, vp_new.alloc) ||
 	    !vg_set_clustered(vg, vp_new.clustered) ||
-	    !vg_set_lock_type(vg, vp_new.lock_type) ||
 	    !vg_set_system_id(vg, vp_new.system_id) ||
 	    !vg_set_mda_copies(vg, vp_new.vgmetadatacopies))
 		goto bad_orphan;
@@ -127,7 +126,12 @@ int vgcreate(struct cmd_context *cmd, int argc, char **argv)
 	if (!vg_write(vg) || !vg_commit(vg))
 		goto_bad;
 
-	if (!lockd_init_vg(cmd, vg)) {
+	/*
+	 * The VG is initially written without lock_type set, i.e. it starts as
+	 * a local VG.  lockd_init_vg() then writes the VG a second time with
+	 * both lock_type and lock_args set.
+	 */
+	if (!lockd_init_vg(cmd, vg, vp_new.lock_type)) {
 		log_error("Failed to initialize lock args for lock type %s",
 			  vp_new.lock_type);
 		vg_remove_pvs(vg);