From b71954e7599a651470e51020c9aa9205a1551ad5 Mon Sep 17 00:00:00 2001
From: root <root@localhost.localdomain>
Date: Wed, 13 Mar 2019 17:23:43 +0800
Subject: [PATCH 02/19] Fix use of orphan lock in commands

vgreduce, vgremove and vgcfgrestore were acquiring
the orphan lock in the midst of command processing
instead of at the start of the command.  (The orphan
lock moved to being acquired at the start of the
command back when pvcreate/vgcreate/vgextend were
reworked based on pvcreate_each_device.)

vgsplit also needed a small update to avoid reacquiring
a VG lock that it already held (for the new VG name).
---
 lib/metadata/metadata-exported.h |  4 ++++
 lib/metadata/metadata.c          | 10 ++--------
 lib/metadata/vg.c                | 14 ++++++--------
 tools/vgcfgrestore.c             | 12 ++++++------
 tools/vgreduce.c                 |  7 +++++++
 tools/vgremove.c                 |  7 +++++++
 tools/vgsplit.c                  |  2 ++
 7 files changed, 34 insertions(+), 22 deletions(-)

diff --git a/lib/metadata/metadata-exported.h b/lib/metadata/metadata-exported.h
index f4fb112..e2fe76b 100644
--- a/lib/metadata/metadata-exported.h
+++ b/lib/metadata/metadata-exported.h
@@ -704,6 +704,10 @@ struct volume_group *vg_read(struct cmd_context *cmd, const char *vg_name,
 			     const char *vgid, uint32_t read_flags, uint32_t lockd_state);
 struct volume_group *vg_read_for_update(struct cmd_context *cmd, const char *vg_name,
 			 const char *vgid, uint32_t read_flags, uint32_t lockd_state);
+struct volume_group *vg_read_orphans(struct cmd_context *cmd,
+                                             uint32_t warn_flags,
+                                             const char *orphan_vgname,
+                                             int *consistent);
 
 /* 
  * Test validity of a VG handle.
diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
index 2292568..2c81623 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -600,14 +600,8 @@ int vg_remove(struct volume_group *vg)
 {
 	int ret;
 
-	if (!lock_vol(vg->cmd, VG_ORPHANS, LCK_VG_WRITE, NULL)) {
-		log_error("Can't get lock for orphan PVs");
-		return 0;
-	}
-
 	ret = vg_remove_direct(vg);
 
-	unlock_vg(vg->cmd, vg, VG_ORPHANS);
 	return ret;
 }
 
@@ -3377,7 +3371,7 @@ static int _vg_read_orphan_pv(struct lvmcache_info *info, void *baton)
 }
 
 /* Make orphan PVs look like a VG. */
-static struct volume_group *_vg_read_orphans(struct cmd_context *cmd,
+struct volume_group *vg_read_orphans(struct cmd_context *cmd,
 					     uint32_t warn_flags,
 					     const char *orphan_vgname,
 					     int *consistent)
@@ -3768,7 +3762,7 @@ static struct volume_group *_vg_read(struct cmd_context *cmd,
 				  "with pre-commit.");
 			return NULL;
 		}
-		return _vg_read_orphans(cmd, warn_flags, vgname, consistent);
+		return vg_read_orphans(cmd, warn_flags, vgname, consistent);
 	}
 
 	uuid[0] = '\0';
diff --git a/lib/metadata/vg.c b/lib/metadata/vg.c
index b8b1501..3bde1fd 100644
--- a/lib/metadata/vg.c
+++ b/lib/metadata/vg.c
@@ -722,6 +722,7 @@ int vgreduce_single(struct cmd_context *cmd, struct volume_group *vg,
 {
 	struct pv_list *pvl;
 	struct volume_group *orphan_vg = NULL;
+	int consistent;
 	int r = 0;
 	const char *name = pv_dev_name(pv);
 
@@ -730,6 +731,8 @@ int vgreduce_single(struct cmd_context *cmd, struct volume_group *vg,
 		return r;
 	}
 
+	log_debug("vgreduce_single VG %s PV %s", vg->name, pv_dev_name(pv));
+
 	if (pv_pe_alloc_count(pv)) {
 		log_error("Physical volume \"%s\" still in use", name);
 		return r;
@@ -741,11 +744,6 @@ int vgreduce_single(struct cmd_context *cmd, struct volume_group *vg,
 		return r;
 	}
 
-	if (!lock_vol(cmd, VG_ORPHANS, LCK_VG_WRITE, NULL)) {
-		log_error("Can't get lock for orphan PVs");
-		return r;
-	}
-
 	pvl = find_pv_in_vg(vg, name);
 
 	if (!archive(vg))
@@ -767,8 +765,8 @@ int vgreduce_single(struct cmd_context *cmd, struct volume_group *vg,
 	vg->free_count -= pv_pe_count(pv) - pv_pe_alloc_count(pv);
 	vg->extent_count -= pv_pe_count(pv);
 
-	orphan_vg = vg_read_for_update(cmd, vg->fid->fmt->orphan_vg_name,
-				       NULL, 0, 0);
+	/* FIXME: we don't need to vg_read the orphan vg here */
+	orphan_vg = vg_read_orphans(cmd, 0, vg->fid->fmt->orphan_vg_name, &consistent);
 
 	if (vg_read_error(orphan_vg))
 		goto bad;
@@ -806,6 +804,6 @@ bad:
 	/* If we are committing here or we had an error then we will free fid */
 	if (pvl && (commit || r != 1))
 		free_pv_fid(pvl->pv);
-	unlock_and_release_vg(cmd, orphan_vg, VG_ORPHANS);
+	release_vg(orphan_vg);
 	return r;
 }
diff --git a/tools/vgcfgrestore.c b/tools/vgcfgrestore.c
index 48a2fa4..37df8cf 100644
--- a/tools/vgcfgrestore.c
+++ b/tools/vgcfgrestore.c
@@ -136,14 +136,14 @@ int vgcfgrestore(struct cmd_context *cmd, int argc, char **argv)
 		lvmetad_rescan = 1;
 	}
 
-	if (!lock_vol(cmd, vg_name, LCK_VG_WRITE, NULL)) {
-		log_error("Unable to lock volume group %s", vg_name);
+	if (!lock_vol(cmd, VG_ORPHANS, LCK_VG_WRITE, NULL)) {
+		log_error("Unable to lock orphans.");
 		return ECMD_FAILED;
 	}
 
-	if (!lock_vol(cmd, VG_ORPHANS, LCK_VG_WRITE, NULL)) {
-		log_error("Unable to lock orphans");
-		unlock_vg(cmd, NULL, vg_name);
+	if (!lock_vol(cmd, vg_name, LCK_VG_WRITE, NULL)) {
+		log_error("Unable to lock volume group %s.", vg_name);
+		unlock_vg(cmd, NULL, VG_ORPHANS);
 		return ECMD_FAILED;
 	}
 
@@ -156,8 +156,8 @@ int vgcfgrestore(struct cmd_context *cmd, int argc, char **argv)
 				       arg_str_value(cmd, file_ARG, ""),
 				       arg_count(cmd, force_long_ARG)) :
 	      backup_restore(cmd, vg_name, arg_count(cmd, force_long_ARG)))) {
-		unlock_vg(cmd, NULL, VG_ORPHANS);
 		unlock_vg(cmd, NULL, vg_name);
+		unlock_vg(cmd, NULL, VG_ORPHANS);
 		log_error("Restore failed.");
 		ret = ECMD_FAILED;
 		goto rescan;
diff --git a/tools/vgreduce.c b/tools/vgreduce.c
index e8479a8..aa35453 100644
--- a/tools/vgreduce.c
+++ b/tools/vgreduce.c
@@ -231,9 +231,16 @@ int vgreduce(struct cmd_context *cmd, int argc, char **argv)
 	handle->custom_handle = &vp;
 
 	if (!repairing) {
+		if (!lock_vol(cmd, VG_ORPHANS, LCK_VG_WRITE, NULL)) {
+			log_error("Can't get lock for orphan PVs");
+			ret = ECMD_FAILED;
+			goto out;
+		}
+
 		/* FIXME: Pass private struct through to all these functions */
 		/* and update in batch afterwards? */
 		ret = process_each_pv(cmd, argc, argv, vg_name, 0, READ_FOR_UPDATE, handle, _vgreduce_single);
+		unlock_vg(cmd, NULL, VG_ORPHANS);
 		goto out;
 	}
 
diff --git a/tools/vgremove.c b/tools/vgremove.c
index 8bf3841..5010e7d 100644
--- a/tools/vgremove.c
+++ b/tools/vgremove.c
@@ -108,10 +108,17 @@ int vgremove(struct cmd_context *cmd, int argc, char **argv)
 	 */
 	cmd->lockd_gl_disable = 1;
 
+	if (!lock_vol(cmd, VG_ORPHANS, LCK_VG_WRITE, NULL)) {
+		log_error("Can't get lock for orphan PVs");
+		return ECMD_FAILED;
+	}
+
 	cmd->handles_missing_pvs = 1;
 	ret = process_each_vg(cmd, argc, argv, NULL, NULL,
 			      READ_FOR_UPDATE, 0,
 			      NULL, &_vgremove_single);
 
+	unlock_vg(cmd, NULL, VG_ORPHANS);
+
 	return ret;
 }
diff --git a/tools/vgsplit.c b/tools/vgsplit.c
index 2d39111..fb1fb6b 100644
--- a/tools/vgsplit.c
+++ b/tools/vgsplit.c
@@ -749,8 +749,10 @@ int vgsplit(struct cmd_context *cmd, int argc, char **argv)
 
 	/*
 	 * Finally, remove the EXPORTED flag from the new VG and write it out.
+	 * We need to unlock vg_to because vg_read_for_update wants to lock it.
 	 */
 	if (!test_mode()) {
+		unlock_vg(cmd, NULL, vg_name_to);
 		release_vg(vg_to);
 		vg_to = vg_read_for_update(cmd, vg_name_to, NULL,
 					   READ_ALLOW_EXPORTED, 0);
-- 
2.19.1

