Avoid nested vg_reads when processing PVs in VGs and fix associated locking.

7 files changed, 72 insertions, 51 deletions

diff --git a/WHATS_NEW b/WHATS_NEW
index fdb4aaa5..47a74eb7 100644
--- a/WHATS_NEW
+++ b/WHATS_NEW
@@ -1,5 +1,6 @@
 Version 2.02.29 -
 ==================================
+  Avoid nested vg_reads when processing PVs in VGs and fix associated locking.
   Accept sizes with --readahead argument.
   Store size arguments as sectors internally.
   Attempt to remove incomplete LVs with lvcreate zeroing/activation problems.
diff --git a/tools/pvdisplay.c b/tools/pvdisplay.c
index 49570594..c82ba598 100644
--- a/tools/pvdisplay.c
+++ b/tools/pvdisplay.c
@@ -16,7 +16,7 @@
 #include "tools.h"
 
 static int _pvdisplay_single(struct cmd_context *cmd,
-			     struct volume_group *vg __attribute((unused)),
+			     struct volume_group *vg,
 			     struct physical_volume *pv, void *handle)
 {
 	struct pv_list *pvl;
@@ -27,27 +27,27 @@ static int _pvdisplay_single(struct cmd_context *cmd,
 	const char *pv_name = pv_dev_name(pv);
 	const char *vg_name = NULL;
 
-	 if (!is_orphan(pv)) {
+	 if (!is_orphan(pv) && !vg) {
 		vg_name = pv_vg_name(pv);
-	         if (!lock_vol(cmd, vg_name, LCK_VG_READ)) {
-	                 log_error("Can't lock %s: skipping", vg_name);
-	                 return ECMD_FAILED;
-	         }
-
-	         if (!(vg = vg_read(cmd, vg_name, (char *)&pv->vgid, &consistent))) {
-	                 log_error("Can't read %s: skipping", vg_name);
-	                 goto out;
-	         }
-
-		 if (!vg_check_status(vg, CLUSTERED)) {
-	                 ret = ECMD_FAILED;
-	                 goto out;
-	         }
+         	if (!lock_vol(cmd, vg_name, LCK_VG_READ)) {
+                 	log_error("Can't lock %s: skipping", vg_name);
+                 	return ECMD_FAILED;
+         	}
+
+         	if (!(vg = vg_read(cmd, vg_name, (char *)&pv->vgid, &consistent))) {
+                 	log_error("Can't read %s: skipping", vg_name);
+                 	goto out;
+         	}
+
+		if (!vg_check_status(vg, CLUSTERED)) {
+			ret = ECMD_FAILED;
+			goto out;
+		}
 
-		 /*
-		  * Replace possibly incomplete PV structure with new one
-		  * allocated in vg_read() path.
-		  */
+	 	/*
+		 * Replace possibly incomplete PV structure with new one
+		 * allocated in vg_read() path.
+		 */
 		 if (!(pvl = find_pv_in_vg(vg, pv_name))) {
 			 log_error("Unable to find \"%s\" in volume group \"%s\"",
 				   pv_name, vg->name);
@@ -119,5 +119,6 @@ int pvdisplay(struct cmd_context *cmd, int argc, char **argv)
 		return EINVALID_CMD_LINE;
 	}
 
-	return process_each_pv(cmd, argc, argv, NULL, NULL, _pvdisplay_single);
+	return process_each_pv(cmd, argc, argv, NULL, LCK_VG_READ, NULL,
+			       _pvdisplay_single);
 }
diff --git a/tools/pvresize.c b/tools/pvresize.c
index 18494a2c..f3b9d392 100644
--- a/tools/pvresize.c
+++ b/tools/pvresize.c
@@ -61,7 +61,8 @@ int pvresize(struct cmd_context *cmd, int argc, char **argv)
 	params.done = 0;
 	params.total = 0;
 
-	ret = process_each_pv(cmd, argc, argv, NULL, &params, _pvresize_single);
+	ret = process_each_pv(cmd, argc, argv, NULL, LCK_VG_WRITE, &params,
+			      _pvresize_single);
 
 	log_print("%d physical volume(s) resized / %d physical volume(s) "
 		  "not resized", params.done, params.total - params.done);
diff --git a/tools/reporter.c b/tools/reporter.c
index d603e91b..0c1b0f03 100644
--- a/tools/reporter.c
+++ b/tools/reporter.c
@@ -54,33 +54,15 @@ static int _segs_single(struct cmd_context *cmd __attribute((unused)),
 	return ECMD_PROCESSED;
 }
 
-static int _pvsegs_sub_single(struct cmd_context *cmd, struct volume_group *vg,
+static int _pvsegs_sub_single(struct cmd_context *cmd __attribute((unused)),
+			      struct volume_group *vg,
 			      struct pv_segment *pvseg, void *handle)
 {
-	int consistent = 0;
-	struct physical_volume *pv = pvseg->pv;
 	int ret = ECMD_PROCESSED;
 
-	if (!lock_vol(cmd, pv_vg_name(pv), LCK_VG_READ)) {
-		log_error("Can't lock %s: skipping", pv_vg_name(pv));
-		return ECMD_FAILED;
-	}
-
-	if (!(vg = vg_read(cmd, pv_vg_name(pv), NULL, &consistent))) {
-		log_error("Can't read %s: skipping", pv_vg_name(pv));
-		goto out;
-	}
-
-	if (!vg_check_status(vg, CLUSTERED)) {
+	if (!report_object(handle, vg, NULL, pvseg->pv, NULL, pvseg))
 		ret = ECMD_FAILED;
-		goto out;
-	}
 
-	if (!report_object(handle, vg, NULL, pv, NULL, pvseg))
-		ret = ECMD_FAILED;
-
-out:
-	unlock_vg(cmd, pv_vg_name(pv));
 	return ret;
 }
 
@@ -108,7 +90,7 @@ static int _pvs_single(struct cmd_context *cmd, struct volume_group *vg,
 	int ret = ECMD_PROCESSED;
 	const char *vg_name = NULL;
 
-	if (!is_orphan(pv)) {
+	if (!is_orphan(pv) && !vg) {
 		vg_name = pv_vg_name(pv);
 
 		if (!lock_vol(cmd, vg_name, LCK_VG_READ)) {
@@ -334,7 +316,7 @@ static int _report(struct cmd_context *cmd, int argc, char **argv,
 		break;
 	case PVS:
 		if (args_are_pvs)
-			r = process_each_pv(cmd, argc, argv, NULL,
+			r = process_each_pv(cmd, argc, argv, NULL, LCK_VG_READ,
 					    report_handle, &_pvs_single);
 		else
 			r = process_each_vg(cmd, argc, argv, LCK_VG_READ, 0,
@@ -346,7 +328,7 @@ static int _report(struct cmd_context *cmd, int argc, char **argv,
 		break;
 	case PVSEGS:
 		if (args_are_pvs)
-			r = process_each_pv(cmd, argc, argv, NULL,
+			r = process_each_pv(cmd, argc, argv, NULL, LCK_VG_READ,
 					    report_handle, &_pvsegs_single);
 		else
 			r = process_each_vg(cmd, argc, argv, LCK_VG_READ, 0,
diff --git a/tools/toollib.c b/tools/toollib.c
index 1f0f25fa..7d071297 100644
--- a/tools/toollib.c
+++ b/tools/toollib.c
@@ -424,8 +424,28 @@ int process_each_segment_in_pv(struct cmd_context *cmd,
 						      void *handle))
 {
 	struct pv_segment *pvseg;
+	const char *vg_name = NULL;
 	int ret_max = 0;
 	int ret;
+	int consistent = 0;
+
+	if (!vg) {
+		vg_name = pv_vg_name(pv);
+		if (!lock_vol(cmd, vg_name, LCK_VG_READ)) {
+			log_error("Can't lock %s: skipping", vg_name);
+			return ECMD_FAILED;
+		}
+
+		if (!(vg = vg_read(cmd, vg_name, NULL, &consistent))) {
+			log_error("Can't read %s: skipping", vg_name);
+			goto out;
+		}
+
+		if (!vg_check_status(vg, CLUSTERED)) {
+			ret = ECMD_FAILED;
+			goto out;
+		}
+	}
 
 	list_iterate_items(pvseg, &pv->segments) {
 		ret = process_single(cmd, vg, pvseg, handle);
@@ -435,6 +455,10 @@ int process_each_segment_in_pv(struct cmd_context *cmd,
 			return ret_max;
 	}
 
+out:
+	if (vg_name)
+		unlock_vg(cmd, vg_name);
+
 	return ret_max;
 }
 
@@ -665,7 +689,7 @@ static int _process_all_devs(struct cmd_context *cmd, void *handle,
 }
 
 int process_each_pv(struct cmd_context *cmd, int argc, char **argv,
-		    struct volume_group *vg, void *handle,
+		    struct volume_group *vg, uint32_t lock_type, void *handle,
 		    int (*process_single) (struct cmd_context * cmd,
 					   struct volume_group * vg,
 					   struct physical_volume * pv,
@@ -735,20 +759,32 @@ int process_each_pv(struct cmd_context *cmd, int argc, char **argv,
 		if (!list_empty(&tags) && (vgnames = get_vgs(cmd, 0)) &&
 		    !list_empty(vgnames)) {
 			list_iterate_items(sll, vgnames) {
+				if (!lock_vol(cmd, sll->str, lock_type)) {
+					log_error("Can't lock %s: skipping", sll->str);
+					continue;
+				}
 				if (!(vg = vg_read(cmd, sll->str, NULL, &consistent))) {
 					log_error("Volume group \"%s\" not found", sll->str);
+					unlock_vg(cmd, sll->str);
 					ret_max = ECMD_FAILED;
 					continue;
 				}
-				if (!consistent)
+				if (!consistent) {
+					unlock_vg(cmd, sll->str);
 					continue;
+				}
 
-				if (!vg_check_status(vg, CLUSTERED))
+				if (!vg_check_status(vg, CLUSTERED)) {
+					unlock_vg(cmd, sll->str);
 					continue;
+				}
 
 				ret = process_each_pv_in_vg(cmd, vg, &tags,
 							    handle,
 							    process_single);
+
+				unlock_vg(cmd, sll->str);
+
 				if (ret > ret_max)
 					ret_max = ret;
 				if (sigint_caught())
diff --git a/tools/toollib.h b/tools/toollib.h
index 8ee539b0..b89f0c3f 100644
--- a/tools/toollib.h
+++ b/tools/toollib.h
@@ -34,7 +34,7 @@ int process_each_vg(struct cmd_context *cmd, int argc, char **argv,
 					   int consistent, void *handle));
 
 int process_each_pv(struct cmd_context *cmd, int argc, char **argv,
-		    struct volume_group *vg, void *handle,
+		    struct volume_group *vg, uint32_t lock_type, void *handle,
 		    int (*process_single) (struct cmd_context * cmd,
 					   struct volume_group * vg,
 					   struct physical_volume * pv,
diff --git a/tools/vgreduce.c b/tools/vgreduce.c
index 134a5e28..6352119f 100644
--- a/tools/vgreduce.c
+++ b/tools/vgreduce.c
@@ -539,7 +539,7 @@ int vgreduce(struct cmd_context *cmd, int argc, char **argv)
 
 		/* FIXME: Pass private struct through to all these functions */
 		/* and update in batch here? */
-		ret = process_each_pv(cmd, argc, argv, vg, NULL,
+		ret = process_each_pv(cmd, argc, argv, vg, LCK_VG_WRITE, NULL,
 				      _vgreduce_single);
 
 	}