pvmove: use toollib

Previously, pvmove used the function find_pv_in_vg() which did the
equivalent of process_each_pv() by doing:

  find_pv_by_name() -> get_pvs() ->

  get_pvs_internal() -> _get_pvs() -> get_vgids() ->

  /* equivalent to process_each_pv */
  dm_list_iterate_items(vgids)
    vg = vg_read_internal()
    dm_list_iterate_items(&vg->pvs)

With the found 'pv', it would do vg_read() on pv_vg_name(pv),
and then do the actual pvmove processing.

This commit simplifies by using process_each_pv() and putting
the actual pvmove processing into the "single" function.
This eliminates both find_pv_by_name() and the vg_read().
The processing code that followed vg_read remains the same.

The return code for the pvmove command is not based on the
process_each_pv return code, but is based on the success/fail
conditions in the existing code.

tools/pvmove.c

@@ -22,6 +22,21 @@
 
 #define PVMOVE_FIRST_TIME   0x00000001      /* Called for first time */
 
+struct pvmove_params {
+	char *pv_name_arg; /* original unmodified arg */
+	char *lv_name_arg; /* original unmodified arg */
+	alloc_policy_t alloc;
+	int pv_count;
+	char **pv_names;
+
+	union lvid *lvid;
+	char *id_vg_name;
+	char *id_lv_name;
+	unsigned in_progress;
+	int setup_result;
+	int found_pv;
+};
+
 static int _pvmove_target_present(struct cmd_context *cmd, int clustered)
 {
 	const struct segment_type *segtype;
@@ -586,63 +601,39 @@ static int _copy_id_components(struct cmd_context *cmd,
 	return 1;
 }
 
-static int _set_up_pvmove(struct cmd_context *cmd, const char *pv_name,
+static int _pvmove_setup_single(struct cmd_context *cmd,
-			  int argc, char **argv, union lvid *lvid, char **vg_name_copy,
+				struct volume_group *vg,
-			  char **lv_mirr_name)
+				struct physical_volume *pv,
+				struct processing_handle *handle)
 {
+	struct pvmove_params *pp = (struct pvmove_params *) handle->custom_handle;
 	const char *lv_name = NULL;
-	const char *vg_name;
-	char *pv_name_arg;
-	struct volume_group *vg;
 	struct dm_list *source_pvl;
 	struct dm_list *allocatable_pvs;
-	alloc_policy_t alloc;
 	struct dm_list *lvs_changed;
-	struct physical_volume *pv;
 	struct logical_volume *lv_mirr;
-	uint32_t lockd_state = 0;
+	const char *pv_name = pv_dev_name(pv);
 	unsigned flags = PVMOVE_FIRST_TIME;
 	unsigned exclusive;
 	int r = ECMD_FAILED;
 
-	pv_name_arg = argv[0];
+	pp->found_pv = 1;
-	argc--;
+	pp->setup_result = ECMD_FAILED;
-	argv++;
 
-	/* Find PV (in VG) */
+	if (pp->lv_name_arg) {
-	if (!(pv = find_pv_by_name(cmd, pv_name, 0, 0))) {
+		if (!(lv_name = _extract_lvname(cmd, vg->name, pp->lv_name_arg))) {
-		stack;
+			log_error("Failed to find an LV name.");
-		return EINVALID_CMD_LINE;
+			pp->setup_result = EINVALID_CMD_LINE;
-	}
+			return ECMD_FAILED;
-
-	vg_name = pv_vg_name(pv);
-
-	if (arg_count(cmd, name_ARG)) {
-		if (!(lv_name = _extract_lvname(cmd, vg_name, arg_value(cmd, name_ARG)))) {
-			stack;
-			free_pv_fid(pv);
-			return EINVALID_CMD_LINE;
 		}
 
 		if (!validate_name(lv_name)) {
 			log_error("Logical volume name %s is invalid", lv_name);
-			free_pv_fid(pv);
+			pp->setup_result = EINVALID_CMD_LINE;
-			return EINVALID_CMD_LINE;
+			return ECMD_FAILED;
 		}
 	}
 
-	/* Read VG */
-	log_verbose("Finding volume group \"%s\"", vg_name);
-
-	if (!lockd_vg(cmd, vg_name, "ex", 0, &lockd_state))
-		return_ECMD_FAILED;
-
-	vg = vg_read(cmd, vg_name, NULL, READ_FOR_UPDATE, lockd_state);
-	if (vg_read_error(vg)) {
-		release_vg(vg);
-		goto out_ret;
-	}
-
 	/*
 	 * We cannot move blocks from under the sanlock leases, so disallow
 	 * pvmoving any PVs used by the lvmlock LV.
@@ -657,8 +648,8 @@ static int _set_up_pvmove(struct cmd_context *cmd, const char *pv_name,
 				if (seg_type(lvseg, s) == AREA_PV) {
 					sanlock_pv = seg_pv(lvseg, s);
 					if (sanlock_pv->dev == pv->dev) {
-						log_error("Cannot pvmove device %s used for sanlock leases.", pv_dev_name(pv));
+						log_error("Cannot pvmove device %s used for sanlock leases.", pv_name);
-						goto out;
+						return ECMD_FAILED;
 					}
 				}
 			}
@@ -669,7 +660,7 @@ static int _set_up_pvmove(struct cmd_context *cmd, const char *pv_name,
 
 	if ((lv_mirr = find_pvmove_lv(vg, pv_dev(pv), PVMOVE))) {
 		log_print_unless_silent("Detected pvmove in progress for %s", pv_name);
-		if (argc || lv_name)
+		if (pp->pv_count || lv_name)
 			log_error("Ignoring remaining command line arguments");
 
 		if (!(lvs_changed = lvs_using_lv(cmd, vg, lv_mirr))) {
@@ -687,23 +678,22 @@ static int _set_up_pvmove(struct cmd_context *cmd, const char *pv_name,
 	} else {
 		/* Determine PE ranges to be moved */
 		if (!(source_pvl = create_pv_list(cmd->mem, vg, 1,
-						  &pv_name_arg, 0)))
+						  &pp->pv_name_arg, 0)))
 			goto_out;
 
-		alloc = (alloc_policy_t) arg_uint_value(cmd, alloc_ARG, ALLOC_INHERIT);
+		if (pp->alloc == ALLOC_INHERIT)
-		if (alloc == ALLOC_INHERIT)
+			pp->alloc = vg->alloc;
-			alloc = vg->alloc;
 
 		/* Get PVs we can use for allocation */
-		if (!(allocatable_pvs = _get_allocatable_pvs(cmd, argc, argv,
+		if (!(allocatable_pvs = _get_allocatable_pvs(cmd, pp->pv_count, pp->pv_names,
-							     vg, pv, alloc)))
+							     vg, pv, pp->alloc)))
 			goto_out;
 
 		if (!archive(vg))
 			goto_out;
 
 		if (!(lv_mirr = _set_up_pvmove_lv(cmd, vg, source_pvl, lv_name,
-						  allocatable_pvs, alloc,
+						  allocatable_pvs, pp->alloc,
 						  &lvs_changed, &exclusive)))
 			goto_out;
 	}
@@ -716,7 +706,7 @@ static int _set_up_pvmove(struct cmd_context *cmd, const char *pv_name,
 	/* init_pvmove(1); */
 	/* vg->status |= PVMOVE; */
 
-	if (!_copy_id_components(cmd, lv_mirr, vg_name_copy, lv_mirr_name, lvid))
+	if (!_copy_id_components(cmd, lv_mirr, &pp->id_vg_name, &pp->id_lv_name, pp->lvid))
 		goto out;
 
 	if (flags & PVMOVE_FIRST_TIME)
@@ -724,70 +714,33 @@ static int _set_up_pvmove(struct cmd_context *cmd, const char *pv_name,
 			goto_out;
 
 	/* LVs are all in status LOCKED */
+	pp->setup_result = ECMD_PROCESSED;
 	r = ECMD_PROCESSED;
 out:
-	free_pv_fid(pv);
-	unlock_and_release_vg(cmd, vg, vg_name);
-out_ret:
-	/*
-	 * Release explicitly because the command may continue running
-	 * for some time monitoring the progress, and we don not want
-	 * or need the lockd lock held over that.
-	 */
-	if (!lockd_vg(cmd, vg_name, "un", 0, &lockd_state))
-		stack;
-
 	return r;
 }
 
-static int _read_poll_id_from_pvname(struct cmd_context *cmd, const char *pv_name,
+static int _pvmove_read_single(struct cmd_context *cmd,
-				     union lvid *lvid, char **vg_name_copy,
+				struct volume_group *vg,
-				     char **lv_name_copy, unsigned *in_progress)
+				struct physical_volume *pv,
+				struct processing_handle *handle)
 {
-	int ret = 0;
+	struct pvmove_params *pp = (struct pvmove_params *) handle->custom_handle;
-	const char *vg_name;
 	struct logical_volume *lv;
-	struct physical_volume *pv;
+	int ret = ECMD_FAILED;
-	struct volume_group *vg;
-	uint32_t lockd_state = 0;
 
-	if (!pv_name) {
+	pp->found_pv = 1;
-		log_error(INTERNAL_ERROR "Invalid PV name parameter.");
-		return 0;
-	}
-
-	if (!(pv = find_pv_by_name(cmd, pv_name, 0, 0)))
-		return_0;
-
-	vg_name = pv_vg_name(pv);
-
-	if (!lockd_vg(cmd, vg_name, "sh", 0, &lockd_state))
-		return_0;
-
-	/* need read-only access */
-	vg = vg_read(cmd, vg_name, NULL, 0, lockd_state);
-	if (vg_read_error(vg)) {
-		log_error("ABORTING: Can't read VG for %s.", pv_name);
-		release_vg(vg);
-		ret = 0;
-		goto out;
-	}
 
 	if (!(lv = find_pvmove_lv(vg, pv_dev(pv), PVMOVE))) {
 		log_print_unless_silent("%s: No pvmove in progress - already finished or aborted.",
-					pv_name);
+					pv_dev_name(pv));
-		ret = 1;
+		ret = ECMD_PROCESSED;
-		*in_progress = 0;
+		pp->in_progress = 0;
-	} else if (_copy_id_components(cmd, lv, vg_name_copy, lv_name_copy, lvid)) {
+	} else if (_copy_id_components(cmd, lv, &pp->id_vg_name, &pp->id_lv_name, pp->lvid)) {
-		ret = 1;
+		ret = ECMD_PROCESSED;
-		*in_progress = 1;
+		pp->in_progress = 1;
 	}
 
-	unlock_and_release_vg(cmd, vg, vg_name);
-out:
-	if (!lockd_vg(cmd, vg_name, "un", 0, &lockd_state))
-		stack;
-	free_pv_fid(pv);
 	return ret;
 }
 
@@ -859,11 +812,12 @@ int pvmove_poll(struct cmd_context *cmd, const char *pv_name,
 
 int pvmove(struct cmd_context *cmd, int argc, char **argv)
 {
-	char *colon;
+	struct pvmove_params pp = { 0 };
-	int ret;
+	struct processing_handle *handle = NULL;
-	unsigned in_progress = 1;
 	union lvid *lvid = NULL;
-	char *pv_name = NULL, *vg_name = NULL, *lv_name = NULL;
+	char *pv_name = NULL;
+	char *colon;
+	unsigned is_abort = arg_is_set(cmd, abort_ARG);
 
 	/* dm raid1 target must be present in every case */
 	if (!_pvmove_target_present(cmd, 0)) {
@@ -895,6 +849,12 @@ int pvmove(struct cmd_context *cmd, int argc, char **argv)
 			log_error("Failed to allocate lvid.");
 			return ECMD_FAILED;
 		}
+		pp.lvid = lvid;
+
+		if (!(pp.pv_name_arg = dm_pool_strdup(cmd->mem, argv[0]))) {
+			log_error("Failed to clone PV name.");
+			return ECMD_FAILED;
+		}
 
 		if (!(pv_name = dm_pool_strdup(cmd->mem, argv[0]))) {
 			log_error("Failed to clone PV name.");
@@ -907,31 +867,56 @@ int pvmove(struct cmd_context *cmd, int argc, char **argv)
 		if (colon)
 			*colon = '\0';
 
-		/*
+		argc--;
-		 * To do a reverse mapping from PV name to VG name, we need the
+		argv++;
-		 * correct global mapping of PVs to VGs.
+
-		 */
+		pp.pv_count = argc;
-		if (!lockd_gl(cmd, "sh", 0)) {
+		pp.pv_names = argv;
-			stack;
+
+		if (arg_count(cmd, name_ARG)) {
+			if (!(pp.lv_name_arg = dm_pool_strdup(cmd->mem, arg_value(cmd, name_ARG))))  {
+				log_error("Failed to clone LV name.");
+				return ECMD_FAILED;
+			}
+		}
+
+		pp.alloc = (alloc_policy_t) arg_uint_value(cmd, alloc_ARG, ALLOC_INHERIT);
+
+		pp.in_progress = 1;
+
+		/* Normal pvmove setup requires ex lock from lvmlockd. */
+		if (is_abort)
+			cmd->lockd_vg_default_sh = 1;
+
+		if (!(handle = init_processing_handle(cmd))) {
+			log_error("Failed to initialize processing handle.");
 			return ECMD_FAILED;
 		}
 
-		/*
+		handle->custom_handle = &pp;
-		 * FIXME: use process_each_pv() where the "single" function
+
-		 * depends on the abort arg.  The single functions would not
+		process_each_pv(cmd, 1, &pv_name, NULL,
-		 * need to use find_pv_by_name() (which includes a hidden
+				is_abort ? 0 : READ_FOR_UPDATE,
-		 * equivalent of process_each_pv), or vg_read().
+				handle,
-		 */
+				is_abort ? &_pvmove_read_single : &_pvmove_setup_single);
-		if (!arg_count(cmd, abort_ARG)) {
+
-			if ((ret = _set_up_pvmove(cmd, pv_name, argc, argv, lvid, &vg_name, &lv_name)) != ECMD_PROCESSED) {
+		destroy_processing_handle(cmd, handle);
+
+		if (!is_abort) {
+			if (!pp.found_pv) {
 				stack;
-				return ret;
+				return EINVALID_CMD_LINE;
+			}
+
+			if (pp.setup_result != ECMD_PROCESSED) {
+				stack;
+				return pp.setup_result;
 			}
 		} else {
-			if (!_read_poll_id_from_pvname(cmd, pv_name, lvid, &vg_name, &lv_name, &in_progress))
+			if (!pp.found_pv)
 				return_ECMD_FAILED;
 
-			if (!in_progress)
+			if (!pp.in_progress)
 				return ECMD_PROCESSED;
 		}
 
@@ -943,6 +928,7 @@ int pvmove(struct cmd_context *cmd, int argc, char **argv)
 		lockd_gl(cmd, "un", 0);
 	}
 
-	return pvmove_poll(cmd, pv_name, lvid ? lvid->s : NULL, vg_name, lv_name,
+	return pvmove_poll(cmd, pv_name, lvid ? lvid->s : NULL,
+			   pp.id_vg_name, pp.id_lv_name,
 			   arg_is_set(cmd, background_ARG));
 }

tools/toollib.h

@@ -102,7 +102,7 @@ int process_each_vg(struct cmd_context *cmd, int argc, char **argv,
 		    process_single_vg_fn_t process_single_vg);
 
 int process_each_pv(struct cmd_context *cmd, int argc, char **argv,
-		    const char *vg_name, uint32_t lock_type,
+		    const char *vg_name, uint32_t read_flags,
 		    struct processing_handle *handle,
 		    process_single_pv_fn_t process_single_pv);