lvm2/6036-Remove-checking-for-locked-VGs.patch

From afb4ce9833dd60220f48a273c7b50c692c900164 Mon Sep 17 00:00:00 2001
From: root <root@localhost.localdomain>
Date: Sat, 23 Mar 2019 18:25:27 +0800
Subject: [PATCH] Remove checking for locked VGs

A few places were calling a function to check if a
VG lock was held.  The only place it was actually
needed is for pvcreate which wants to do its own
locking (and scanning) around process_each_pv.

The locking/scanning exceptions for pvcreate in
process_each_pv/vg_read can be enabled by just passing
a couple of flags instead of checking if the VG is
already locked.  This also means that these special
cases won't be enabled unknowingly in other places
where they shouldn't be used.
---
 lib/cache/lvmcache.c             | 13 -----
 lib/cache/lvmcache.h             |  1 -
 lib/metadata/metadata-exported.h |  2 +
 lib/metadata/metadata.c          | 18 ++----
 liblvm/lvm_vg.c                  |  2 -
 tools/toollib.c                  | 94 +++++++++-----------------------
 tools/vgreduce.c                 |  2 +-
 7 files changed, 35 insertions(+), 97 deletions(-)

diff --git a/lib/cache/lvmcache.c b/lib/cache/lvmcache.c
index 1a2e987..a48c5e7 100644
--- a/lib/cache/lvmcache.c
+++ b/lib/cache/lvmcache.c
@@ -536,9 +536,6 @@ void lvmcache_drop_metadata(const char *vgname, int drop_precommitted)
 	if (!_saved_vg_hash)
 		return;
 
-	if (lvmcache_vgname_is_locked(VG_GLOBAL))
-		return;
-
 	/* For VG_ORPHANS, we need to invalidate all labels on orphan PVs. */
 	if (!strcmp(vgname, VG_ORPHANS)) {
 		_drop_metadata(FMT_TEXT_ORPHAN_VG_NAME, 0);
@@ -561,14 +558,6 @@ void lvmcache_lock_vgname(const char *vgname, int read_only __attribute__((unuse
 	}
 }
 
-int lvmcache_vgname_is_locked(const char *vgname)
-{
-	if (!_lock_hash)
-		return 0;
-
-	return dm_hash_lookup(_lock_hash, is_orphan_vg(vgname) ? VG_ORPHANS : vgname) ? 1 : 0;
-}
-
 void lvmcache_unlock_vgname(const char *vgname)
 {
 	if (!dm_hash_lookup(_lock_hash, vgname))
@@ -1997,8 +1986,6 @@ static int _lvmcache_update_vgname(struct lvmcache_info *info,
 	else if (!_lvmcache_update_vgid(NULL, vginfo, vgid)) /* Orphans */
 		return_0;
 
-	_update_cache_vginfo_lock_state(vginfo, lvmcache_vgname_is_locked(vgname));
-
 	/* FIXME Check consistency of list! */
 	vginfo->fmt = fmt;
 
diff --git a/lib/cache/lvmcache.h b/lib/cache/lvmcache.h
index e9e6932..0567a81 100644
--- a/lib/cache/lvmcache.h
+++ b/lib/cache/lvmcache.h
@@ -107,7 +107,6 @@ char *lvmcache_vgname_from_pvid(struct cmd_context *cmd, const char *pvid);
 const char *lvmcache_vgname_from_info(struct lvmcache_info *info);
 const struct format_type *lvmcache_fmt_from_info(struct lvmcache_info *info);
 int lvmcache_vgs_locked(void);
-int lvmcache_vgname_is_locked(const char *vgname);
 
 void lvmcache_seed_infos_from_lvmetad(struct cmd_context *cmd);
 
diff --git a/lib/metadata/metadata-exported.h b/lib/metadata/metadata-exported.h
index e2fe76b..a4ca17f 100644
--- a/lib/metadata/metadata-exported.h
+++ b/lib/metadata/metadata-exported.h
@@ -178,6 +178,8 @@
 #define READ_OK_NOTFOUND	0x00040000U
 #define READ_WARN_INCONSISTENT	0x00080000U
 #define READ_FOR_UPDATE		0x00100000U /* A meta-flag, useful with toollib for_each_* functions. */
+#define PROCESS_SKIP_SCAN	 0x00200000U /* skip lvmcache_label_scan in process_each_pv */
+#define PROCESS_SKIP_ORPHAN_LOCK 0x00400000U /* skip lock_vol(VG_ORPHAN) in vg_read */
 
 /* vg's "read_status" field */
 #define FAILED_INCONSISTENT	0x00000001U
diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
index 2c81623..237e57b 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -3118,12 +3118,6 @@ int vg_commit(struct volume_group *vg)
 	int cache_updated = 0;
 	struct pv_list *pvl;
 
-	if (!lvmcache_vgname_is_locked(vg->name)) {
-		log_error(INTERNAL_ERROR "Attempt to write new VG metadata "
-			  "without locking %s", vg->name);
-		return cache_updated;
-	}
-
 	cache_updated = _vg_commit_mdas(vg);
 
 	set_vg_notify(vg->cmd);
@@ -5465,8 +5459,8 @@ static struct volume_group *_vg_lock_and_read(struct cmd_context *cmd, const cha
 	uint32_t failure = 0;
 	uint32_t warn_flags = 0;
 	int is_shared = 0;
-	int already_locked;
 	int write_lock_held = (lock_flags == LCK_VG_WRITE);
+	int skip_lock = is_orphan_vg(vg_name) && (read_flags & PROCESS_SKIP_ORPHAN_LOCK);
 
 	if ((read_flags & READ_ALLOW_INCONSISTENT) || (lock_flags != LCK_VG_WRITE))
 		consistent = 0;
@@ -5477,15 +5471,13 @@ static struct volume_group *_vg_lock_and_read(struct cmd_context *cmd, const cha
 		return NULL;
 	}
 
-	already_locked = lvmcache_vgname_is_locked(vg_name);
-
-	if (!already_locked &&
+	if (!skip_lock &&
 	    !lock_vol(cmd, vg_name, lock_flags, NULL)) {
 		log_error("Can't get lock for %s", vg_name);
 		return _vg_make_handle(cmd, vg, FAILED_LOCKING);
 	}
 
-	if (already_locked)
+	if (skip_lock)
 		log_very_verbose("Locking %s already done", vg_name);
 
 	if (is_orphan_vg(vg_name))
@@ -5553,13 +5545,13 @@ static struct volume_group *_vg_lock_and_read(struct cmd_context *cmd, const cha
 		goto_bad;
 
 	if (!(vg = _vg_make_handle(cmd, vg, failure)) || vg_read_error(vg))
-		if (!already_locked)
+		if (!skip_lock)
 			unlock_vg(cmd, vg, vg_name);
 
 	return vg;
 
 bad:
-	if (!already_locked)
+	if (!skip_lock)
 		unlock_vg(cmd, vg, vg_name);
 
 bad_no_unlock:
diff --git a/liblvm/lvm_vg.c b/liblvm/lvm_vg.c
index ee6fa4e..616c78f 100644
--- a/liblvm/lvm_vg.c
+++ b/liblvm/lvm_vg.c
@@ -186,8 +186,6 @@ int lvm_vg_close(vg_t vg)
 	struct saved_env e = store_user_env(vg->cmd);
 	if (vg_read_error(vg) == FAILED_LOCKING)
 		release_vg(vg);
-	else if (!lvmcache_vgname_is_locked(vg->name))
-		release_vg(vg);
 	else
 		unlock_and_release_vg(vg->cmd, vg, vg->name);
 	restore_user_env(&e);
diff --git a/tools/toollib.c b/tools/toollib.c
index ce877cc..3210e28 100644
--- a/tools/toollib.c
+++ b/tools/toollib.c
@@ -1969,7 +1969,6 @@ static int _process_vgnameid_list(struct cmd_context *cmd, uint32_t read_flags,
 	int skip;
 	int notfound;
 	int process_all = 0;
-	int already_locked;
 	int do_report_ret_code = 1;
 
 	log_set_report_object_type(LOG_REPORT_OBJECT_TYPE_VG);
@@ -2012,8 +2011,6 @@ static int _process_vgnameid_list(struct cmd_context *cmd, uint32_t read_flags,
 			continue;
 		}
 
-		already_locked = lvmcache_vgname_is_locked(vg_name);
-
 		vg = vg_read(cmd, vg_name, vg_uuid, read_flags, lockd_state);
 		if (_ignore_vg(vg, vg_name, arg_vgnames, read_flags, &skip, &notfound)) {
 			stack;
@@ -2041,7 +2038,7 @@ static int _process_vgnameid_list(struct cmd_context *cmd, uint32_t read_flags,
 				ret_max = ret;
 		}
 
-		if (!vg_read_error(vg) && !already_locked)
+		if (!vg_read_error(vg))
 			unlock_vg(cmd, vg, vg_name);
 endvg:
 		release_vg(vg);
@@ -3681,8 +3678,6 @@ static int _process_lv_vgnameid_list(struct cmd_context *cmd, uint32_t read_flag
 			continue;
 		}
 
-		already_locked = lvmcache_vgname_is_locked(vg_name);
-
 		vg = vg_read(cmd, vg_name, vg_uuid, read_flags, lockd_state);
 		if (_ignore_vg(vg, vg_name, arg_vgnames, read_flags, &skip, &notfound)) {
 			stack;
@@ -3701,8 +3696,7 @@ static int _process_lv_vgnameid_list(struct cmd_context *cmd, uint32_t read_flag
 		if (ret > ret_max)
 			ret_max = ret;
 
-		if (!already_locked)
-			unlock_vg(cmd, vg, vg_name);
+		unlock_vg(cmd, vg, vg_name);
 endvg:
 		release_vg(vg);
 		if (!lockd_vg(cmd, vg_name, "un", 0, &lockd_state))
@@ -4400,7 +4394,7 @@ static int _process_pvs_in_vgs(struct cmd_context *cmd, uint32_t read_flags,
 	int ret;
 	int skip;
 	int notfound;
-	int already_locked;
+	int skip_lock;
 	int do_report_ret_code = 1;
 
 	log_set_report_object_type(LOG_REPORT_OBJECT_TYPE_VG);
@@ -4434,7 +4428,7 @@ static int _process_pvs_in_vgs(struct cmd_context *cmd, uint32_t read_flags,
 
 		log_debug("Processing PVs in VG %s", vg_name);
 
-		already_locked = lvmcache_vgname_is_locked(vg_name);
+		skip_lock = is_orphan_vg(vg_name) && (read_flags & PROCESS_SKIP_ORPHAN_LOCK);
 
 		vg = vg_read(cmd, vg_name, vg_uuid, read_flags, lockd_state);
 		if (_ignore_vg(vg, vg_name, NULL, read_flags, &skip, &notfound)) {
@@ -4462,7 +4456,7 @@ static int _process_pvs_in_vgs(struct cmd_context *cmd, uint32_t read_flags,
 		if (ret > ret_max)
 			ret_max = ret;
 
-		if (!skip && !already_locked)
+		if (!skip && !skip_lock)
 			unlock_vg(cmd, vg, vg->name);
 endvg:
 		release_vg(vg);
@@ -4501,7 +4495,6 @@ int process_each_pv(struct cmd_context *cmd,
 	struct device_id_list *dil;
 	int process_all_pvs;
 	int process_all_devices;
-	int orphans_locked;
 	int ret_max = ECMD_PROCESSED;
 	int ret;
 
@@ -4549,8 +4542,6 @@ int process_each_pv(struct cmd_context *cmd,
 		return ECMD_FAILED;
 	}
 
-	orphans_locked = lvmcache_vgname_is_locked(VG_ORPHANS);
-
 	process_all_pvs = dm_list_empty(&arg_pvnames) && dm_list_empty(&arg_tags);
 
 	process_all_devices = process_all_pvs && (cmd->cname->flags & ENABLE_ALL_DEVS) && all_is_set;
@@ -4561,22 +4552,8 @@ int process_each_pv(struct cmd_context *cmd,
 		goto_out;
 	}
 
-	/*
-	 * This full scan would be done by _get_all_devices() if
-	 * it were not done here first.  It's called here first
-	 * so that get_vgnameids() will look at any new devices.
-	 * When orphans is already locked, these steps are done
-	 * before process_each_pv is called.
-	 */
-	if (!trust_cache() && !orphans_locked) {
-		lvmcache_destroy(cmd, 1, 0);
-
-		/*
-		 * Scan all devices to populate lvmcache with initial
-		 * list of PVs and VGs.
-		 */
+	if (!(read_flags & PROCESS_SKIP_SCAN))
 		lvmcache_label_scan(cmd);
-	}
 
 	if (!get_vgnameids(cmd, &all_vgnameids, only_this_vgname, 1)) {
 		ret_max = ret;
@@ -4647,11 +4624,9 @@ int process_each_pv(struct cmd_context *cmd,
 		ret_max = ret;
 
 	/*
-	 * If the orphans lock was held, there shouldn't be missed devices.  If
-	 * there were, we cannot clear the cache while holding the orphans lock
-	 * anyway.
+	 * If the orphans lock was held, there shouldn't be missed devices.
 	 */
-	if (orphans_locked)
+	if (read_flags & PROCESS_SKIP_ORPHAN_LOCK)
 		goto skip_missed;
 
 	/*
@@ -4678,12 +4653,7 @@ int process_each_pv(struct cmd_context *cmd,
 
 		log_verbose("Some PVs were not found in first search, retrying.");
 
-		lvmcache_destroy(cmd, 0, 0);
-		if (!lvmcache_init(cmd)) {
-			log_error("Failed to initalize lvm cache.");
-			ret_max = ECMD_FAILED;
-			goto out;
-		}
+		lvmcache_label_scan(cmd);
 		lvmcache_seed_infos_from_lvmetad(cmd);
 
 		ret = _process_pvs_in_vgs(cmd, read_flags, &all_vgnameids, &all_devices,
@@ -5514,25 +5484,16 @@ int pvcreate_each_device(struct cmd_context *cmd,
 		dm_list_add(&pp->arg_devices, &pd->list);
 	}
 
-	/*
-	 * Clear the cache before acquiring the orphan lock.  (Clearing the
-	 * cache with locks held is an error.)  We want the orphan lock
-	 * acquired before process_each_pv.  If the orphan lock is not held
-	 * when process_each_pv is called, then process_each_pv clears the
-	 * cache.
-	 */
-	lvmcache_destroy(cmd, 1, 0);
-
-	/*
-	 * If no prompts require a user response, this orphan lock is held
-	 * throughout, and pvcreate_each_device() returns with it held so that
-	 * vgcreate/vgextend use the PVs created here to add to a VG.
-	 */
 	if (!lock_vol(cmd, VG_ORPHANS, LCK_VG_WRITE, NULL)) {
 		log_error("Can't get lock for orphan PVs.");
 		return 0;
 	}
 
+	/*
+	 * Scan before calling process_each_pv so we can set up the PV args
+	 * first.  We can then skip the scan that would normally occur at the
+	 * beginning of process_each_pv.
+	 */
 	lvmcache_label_scan(cmd);
 
 	/*
@@ -5556,9 +5517,8 @@ int pvcreate_each_device(struct cmd_context *cmd,
 	 * If it's added to arg_process but needs a prompt or force option, then
 	 * a corresponding prompt entry is added to pp->prompts.
 	 */
-	process_each_pv(cmd, 0, NULL, NULL, 1, 0, handle,
-			pp->is_remove ? _pvremove_check_single : _pvcreate_check_single);
-
+	process_each_pv(cmd, 0, NULL, NULL, 1, PROCESS_SKIP_SCAN | PROCESS_SKIP_ORPHAN_LOCK,
+			handle, pp->is_remove ? _pvremove_check_single : _pvcreate_check_single);
 	/*
 	 * A fatal error was found while checking.
 	 */
@@ -5639,9 +5599,11 @@ int pvcreate_each_device(struct cmd_context *cmd,
 		goto do_command;
 
 	/*
-	 * Prompts require asking the user, so release the orphans lock, ask
-	 * the questions, reacquire the orphans lock, verify that the PVs were
-	 * not used during the questions, then do the create steps.
+	 * Prompts require asking the user and make take some time, during
+	 * which we don't want to block other commands.  So, release the lock
+	 * to prevent blocking other commands while we wait.  After a response
+	 * from the user, reacquire the lock, verify that the PVs were not used
+	 * during the wait, then do the create steps.
 	 */
 	unlock_vg(cmd, NULL, VG_ORPHANS);
 
@@ -5676,14 +5638,11 @@ int pvcreate_each_device(struct cmd_context *cmd,
 	}
 
 	/*
-	 * Clear the cache, reacquire the orphans write lock, then check again
-	 * that the devices can still be used.  If the second loop finds them
-	 * changed, or can't find them any more, then they aren't used.
-	 * Clear the cache here before locking orphans, since it won't be
-	 * done by process_each_pv with orphans already locked.
+	 * Reacquire the lock that was released above before waiting, then
+	 * check again that the devices can still be used.  If the second loop
+	 * finds them changed, or can't find them any more, then they aren't
+	 * used.
 	 */
-	lvmcache_destroy(cmd, 1, 0);
-
 	if (!lock_vol(cmd, VG_ORPHANS, LCK_VG_WRITE, NULL)) {
 		log_error("Can't get lock for orphan PVs.");
 		goto out;
@@ -5705,7 +5664,8 @@ int pvcreate_each_device(struct cmd_context *cmd,
 	 */
 	dm_list_splice(&pp->arg_confirm, &pp->arg_process);
 
-	process_each_pv(cmd, 0, NULL, NULL, 1, 0, handle, _pv_confirm_single);
+	process_each_pv(cmd, 0, NULL, NULL, 1, PROCESS_SKIP_SCAN | PROCESS_SKIP_ORPHAN_LOCK,
+			handle, _pv_confirm_single);
 
 	dm_list_iterate_items(pd, &pp->arg_confirm)
 		log_error("Device %s %s.", pd->name, dev_cache_filtered_reason(pd->name));
diff --git a/tools/vgreduce.c b/tools/vgreduce.c
index aa35453..aa68f0f 100644
--- a/tools/vgreduce.c
+++ b/tools/vgreduce.c
@@ -239,7 +239,7 @@ int vgreduce(struct cmd_context *cmd, int argc, char **argv)
 
 		/* 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);
+		ret = process_each_pv(cmd, argc, argv, vg_name, 0, READ_FOR_UPDATE | PROCESS_SKIP_ORPHAN_LOCK, handle, _vgreduce_single);
 		unlock_vg(cmd, NULL, VG_ORPHANS);
 		goto out;
 	}
-- 
2.19.1