Improve checks for non-lvmlockd casedev-dct-lvmlockd-AT

3 files changed, 87 insertions, 23 deletions

diff --git a/lib/locking/lvmlockd.c b/lib/locking/lvmlockd.c
index 00448cf9e..b77416d83 100644
--- a/lib/locking/lvmlockd.c
+++ b/lib/locking/lvmlockd.c
@@ -1101,7 +1101,28 @@ int lockd_gl_create(struct cmd_context *cmd, const char *def_mode, const char *v
 	int retries = 0;
 	int result;
 
-	/* A specific lock mode was given on the command line. */
+	/*
+	 * There are four variations of creating a local/lockd VG
+	 * with/without use_lvmlockd set.
+	 *
+	 * use_lvmlockd=1, lockd VG:
+	 * This function should acquire or create the global lock.
+	 *
+	 * use_lvmlockd=0, local VG:
+	 * This function is a no-op, just returns 1.
+	 *
+	 * use_lvmlockd=0, lockd VG
+	 * An error is returned in vgcreate_params_set_from_args (before this is called).
+	 *
+	 * use_lvmlockd=1, local VG
+	 * This function should acquire the global lock.
+	 */
+	if (!_use_lvmlockd && !is_lockd_type(vg_lock_type))
+		return 1;
+
+	/*
+	 * A specific lock mode was given on the command line.
+	 */
 	if (cmd->lock_gl_mode) {
 		mode = cmd->lock_gl_mode;
 		if (mode && def_mode && strcmp(mode, "enable") && (_mode_compare(mode, def_mode) < 0)) {
@@ -1292,6 +1313,36 @@ int lockd_gl_create(struct cmd_context *cmd, const char *def_mode, const char *v
  * a change is made to the state associated with the global lock, and
  * if the local version number is lower than the version number in the
  * lock, then the local lvmetad state must be updated.
+ *
+ * Effects of misconfiguring use_lvmlockd.
+ *
+ * - Setting use_lvmlockd=1 tells lvm commands to use the global lock.
+ * This should not be set unless a lock manager and lockd VGs will
+ * be used.  Setting use_lvmlockd=1 without setting up a lock manager
+ * or using lockd VGs will cause lvm commands to fail when they attempt
+ * to change any global state (requiring the ex global lock), and will
+ * cause warnings when the commands read global state (requiring the sh
+ * global lock).  In this condition, lvm is nominally useful, and local
+ * VGs can continue to be used mostly as usual.  But, the warnings/errors
+ * should lead a user to either set up a lock manager and lockd VGs,
+ * or set use_lvmlockd to 0.
+ *
+ * - Setting use_lvmlockd=0 tells lvm commands to not use the global lock.
+ * If use_lvmlockd=0 when lockd VGs exist which require lvmlockd, the
+ * lockd_gl() calls become no-ops, but the lockd_vg() calls for the lockd
+ * VGs will fail.  The warnings/errors from accessing the lockd VGs
+ * should lead the user to set use_lvmlockd to 1 and run the necessary
+ * lock manager.  In this condition, lvm reverts to the behavior of
+ * the following case, in which system ID largely protects shared
+ * devices, but has limitations.
+ *
+ * - Setting use_lvmlockd=0 with shared devices, no lockd VGs and
+ * no lock manager is a recognized mode of operation that is
+ * described in the lvmsystemid man page.  Using lvm on shared
+ * devices this way is made safe by using system IDs to assign
+ * ownership of VGs to single hosts.  The main limitation of this
+ * mode (among others outlined in the man page), is that orphan PVs
+ * are not protected from concurrent used among hosts.
  */
 
 int lockd_gl(struct cmd_context *cmd, const char *def_mode, uint32_t flags)
@@ -1302,8 +1353,21 @@ int lockd_gl(struct cmd_context *cmd, const char *def_mode, uint32_t flags)
 	int retries = 0;
 	int result;
 
-	/* A specific lock mode was given on the command line. */
-	if (!(flags & LDGL_MODE_NOARG) && cmd->lock_gl_mode) {
+	if (!_use_lvmlockd)
+		return 1;
+
+	if (def_mode && !strcmp(def_mode, "un")) {
+		if (cmd->lock_gl_mode && !strcmp(cmd->lock_gl_mode, "na"))
+			return 1;
+
+		mode = "un";
+		goto req;
+	}
+
+	/*
+	 * A specific lock mode was given on the command line.
+	 */
+	if (cmd->lock_gl_mode) {
 		mode = cmd->lock_gl_mode;
 		if (mode && def_mode && (_mode_compare(mode, def_mode) < 0)) {
 			if (!find_config_tree_bool(cmd, global_allow_override_lock_modes_CFG, NULL)) {
@@ -1482,10 +1546,13 @@ int lockd_vg(struct cmd_context *cmd, const char *vg_name, const char *def_mode,
 {
 	const char *mode = NULL;
 	uint32_t lockd_flags;
+	uint32_t prev_state = *lockd_state;
 	int retries = 0;
 	int result;
 	int ret;
 
+	*lockd_state = 0;
+
 	if (!is_real_vg(vg_name))
 		return 1;
 
@@ -1511,7 +1578,7 @@ int lockd_vg(struct cmd_context *cmd, const char *vg_name, const char *def_mode,
 		if (cmd->lock_vg_mode && !strcmp(cmd->lock_vg_mode, "na"))
 			return 1;
 
-		if (*lockd_state & LDST_FAIL) {
+		if (prev_state & LDST_FAIL) {
 			log_debug("VG %s unlock skipped: lockd_state is failed", vg_name);
 			return 1;
 		}
@@ -1520,13 +1587,10 @@ int lockd_vg(struct cmd_context *cmd, const char *vg_name, const char *def_mode,
 		goto req;
 	}
 
-	*lockd_state = 0;
-
 	/*
 	 * A specific lock mode was given on the command line.
-	 * LDVG_MODE_NOARG disables getting the mode from --lock-vg arg.
 	 */
-	if (!(flags & LDVG_MODE_NOARG) && cmd->lock_vg_mode) {
+	if (cmd->lock_vg_mode) {
 		mode = cmd->lock_vg_mode;
 		if (mode && def_mode && (_mode_compare(mode, def_mode) < 0)) {
 			if (!find_config_tree_bool(cmd, global_allow_override_lock_modes_CFG, NULL)) {
@@ -1559,17 +1623,16 @@ int lockd_vg(struct cmd_context *cmd, const char *vg_name, const char *def_mode,
 
 	if (!strcmp(mode, "ex"))
 		*lockd_state |= LDST_EX;
-	else if (!strcmp(mode, "sh"))
-		*lockd_state |= LDST_SH;
-
  req:
 	if (!_lockd_request(cmd, "lock_vg",
 			      vg_name, NULL, NULL, NULL, NULL, NULL, mode, NULL,
 			      &result, &lockd_flags)) {
 		/*
 		 * No result from lvmlockd, it is probably not running.
-		 * Decide if it is ok to continue without a lock after
-		 * reading the VG and looking at the lock_type.
+		 * Decide if it is ok to continue without a lock in
+		 * access_vg_lock_type() after the VG has been read and
+		 * the lock_type can be checked.  We don't care about
+		 * this error for local VGs, but we do care for lockd VGs.
 		 */
 		*lockd_state |= LDST_FAIL_REQUEST;
 		return 1;
@@ -1937,6 +2000,12 @@ int lockd_lv(struct cmd_context *cmd, struct logical_volume *lv,
 	if (!is_lockd_type(lv->vg->lock_type))
 		return 1;
 
+	if (!_use_lvmlockd) {
+		log_error("LV in VG %s with lock_type %s requires lvmlockd.",
+			  lv->vg->name, lv->vg->lock_type);
+		return 0;
+	}
+
 	if (lv_is_thin_type(lv))
 		return _lockd_lv_thin(cmd, lv, def_mode, flags);
 
diff --git a/lib/locking/lvmlockd.h b/lib/locking/lvmlockd.h
index 6db632df0..21b49e7fb 100644
--- a/lib/locking/lvmlockd.h
+++ b/lib/locking/lvmlockd.h
@@ -17,17 +17,12 @@
 #define LOCKD_SANLOCK_LV_NAME "lvmlock"
 
 /* lockd_gl flags */
-#define LDGL_MODE_NOARG           0x00000001
-#define LDGL_SKIP_CACHE_VALIDATE  0x00000002
-#define LDGL_UPDATE_NAMES         0x00000004
-
-/* lockd_vg flags */
-#define LDVG_MODE_NOARG           0x00000001
+#define LDGL_SKIP_CACHE_VALIDATE  0x00000001
+#define LDGL_UPDATE_NAMES         0x00000002
 
 /* lockd_lv flags */
-#define LDLV_MODE_NOARG           0x00000001
-#define LDLV_MODE_NO_SH           0x00000002
-#define LDLV_PERSISTENT           0x00000004
+#define LDLV_MODE_NO_SH           0x00000001
+#define LDLV_PERSISTENT           0x00000002
 
 /* lvmlockd result flags */
 #define LD_RF_NO_LOCKSPACES     0x00000001
diff --git a/lib/metadata/lv_manip.c b/lib/metadata/lv_manip.c
index ad8eccbd7..7de4dd9cb 100644
--- a/lib/metadata/lv_manip.c
+++ b/lib/metadata/lv_manip.c
@@ -5830,7 +5830,7 @@ int lv_remove_single(struct cmd_context *cmd, struct logical_volume *lv,
 
 	backup(vg);
 
-	lockd_lv(cmd, lock_lv, "un", LDLV_PERSISTENT | LDLV_MODE_NOARG);
+	lockd_lv(cmd, lock_lv, "un", LDLV_PERSISTENT);
 	lockd_free_lv(cmd, vg, lv->name, &lv->lvid.id[1], lv->lock_args);
 
 	if (!suppress_remove_message && visible)