[PATCH 0/4] Clean-up polldaemon to be more generic to allow re-use

[PATCH 0/4] Clean-up polldaemon to be more generic to allow re-use

[Mike Snitzer]

The polldaemon will be re-used for LVM2's snapshot-merge support.

This patchset provides the cleanup patches that a couple snapshot-merge
specific follow-on patches depend on.

NOTE: the 2/4 patch is provided as a separate patch for review purposes;
but in practice when it is committed it should be combined with the
3/4 patch to avoid reduced functionality (progress_display) if one were
to bisect.

[PATCH 1/4] Rename the polldaemon's mirror-specific names

[Mike Snitzer]

Just drop "mirror" or "mirr" part of names in polldaemon. The polldaemon will
also be used for polling snapshot merging, so naming the arguments with
"mirror" is misleading.

This patch does nothing but change variable and function names.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
Reviewed-by: Mike Snitzer <snitzer@redhat.com>

---
 tools/polldaemon.c |   57 ++++++++++++++++++++++++++---------------------------
 tools/polldaemon.h |    6 ++---
 2 files changed, 31 insertions(+), 32 deletions(-)

Index: lvm2/tools/polldaemon.c
===================================================================
--- lvm2.orig/tools/polldaemon.c
+++ lvm2/tools/polldaemon.c
@@ -63,11 +63,11 @@ static int _become_daemon(struct cmd_con
 	return 1;
 }
 
-static int _check_mirror_status(struct cmd_context *cmd,
-				struct volume_group *vg,
-				struct logical_volume *lv_mirr,
-				const char *name, struct daemon_parms *parms,
-				int *finished)
+static int _check_lv_status(struct cmd_context *cmd,
+			    struct volume_group *vg,
+			    struct logical_volume *lv,
+			    const char *name, struct daemon_parms *parms,
+			    int *finished)
 {
 	struct dm_list *lvs_changed;
 	float segment_percent = 0.0, overall_percent = 0.0;
@@ -78,22 +78,22 @@ static int _check_mirror_status(struct c
 	*finished = 1;
 
 	if (parms->aborting) {
-		if (!(lvs_changed = lvs_using_lv(cmd, vg, lv_mirr))) {
+		if (!(lvs_changed = lvs_using_lv(cmd, vg, lv))) {
 			log_error("Failed to generate list of copied LVs: "
 				  "can't abort.");
 			return 0;
 		}
-		parms->poll_fns->finish_copy(cmd, vg, lv_mirr, lvs_changed);
+		parms->poll_fns->finish_copy(cmd, vg, lv, lvs_changed);
 		return 0;
 	}
 
-	if (!(status = lv_mirror_percent(cmd, lv_mirr, !parms->interval,
+	if (!(status = lv_mirror_percent(cmd, lv, !parms->interval,
 					 &segment_percent, &event_nr))) {
 		log_error("ABORTING: Mirror percentage check failed.");
 		return 0;
 	}
 
-	overall_status = copy_percent(lv_mirr, &overall_percent);
+	overall_status = copy_percent(lv, &overall_percent);
 	if (parms->progress_display)
 		log_print("%s: %s: %.1f%%", name, parms->progress_title,
 			  overall_percent);
@@ -107,22 +107,20 @@ static int _check_mirror_status(struct c
 		return 1;
 	}
 
-	if (!(lvs_changed = lvs_using_lv(cmd, vg, lv_mirr))) {
+	if (!(lvs_changed = lvs_using_lv(cmd, vg, lv))) {
 		log_error("ABORTING: Failed to generate list of copied LVs");
 		return 0;
 	}
 
 	/* Finished? Or progress to next segment? */
 	if (overall_status == TARGET_STATUS_FINISHED) {
-		if (!parms->poll_fns->finish_copy(cmd, vg, lv_mirr,
-						  lvs_changed))
+		if (!parms->poll_fns->finish_copy(cmd, vg, lv, lvs_changed))
 			return 0;
 	} else {
-		if (!parms->poll_fns->update_metadata(cmd, vg, lv_mirr,
+		if (!parms->poll_fns->update_metadata(cmd, vg, lv,
 						      lvs_changed, 0)) {
 			log_error("ABORTING: Segment progression failed.");
-			parms->poll_fns->finish_copy(cmd, vg, lv_mirr,
-						     lvs_changed);
+			parms->poll_fns->finish_copy(cmd, vg, lv, lvs_changed);
 			return 0;
 		}
 		*finished = 0;	/* Another segment */
@@ -131,11 +129,11 @@ static int _check_mirror_status(struct c
 	return 1;
 }
 
-static int _wait_for_single_mirror(struct cmd_context *cmd, const char *name, const char *uuid,
+static int _wait_for_single_lv(struct cmd_context *cmd, const char *name, const char *uuid,
 				   struct daemon_parms *parms)
 {
 	struct volume_group *vg;
-	struct logical_volume *lv_mirr;
+	struct logical_volume *lv;
 	int finished = 0;
 
 	/* Poll for mirror completion */
@@ -157,16 +155,15 @@ static int _wait_for_single_mirror(struc
 			return 0;
 		}
 
-		if (!(lv_mirr = parms->poll_fns->get_copy_lv(cmd, vg, name, uuid,
-							     parms->lv_type))) {
+		if (!(lv = parms->poll_fns->get_copy_lv(cmd, vg, name, uuid,
+							parms->lv_type))) {
 			log_error("ABORTING: Can't find mirror LV in %s for %s",
 				  vg->name, name);
 			unlock_and_release_vg(cmd, vg, vg->name);
 			return 0;
 		}
 
-		if (!_check_mirror_status(cmd, vg, lv_mirr, name, parms,
-					  &finished)) {
+		if (!_check_lv_status(cmd, vg, lv, name, parms, &finished)) {
 			unlock_and_release_vg(cmd, vg, vg->name);
 			return 0;
 		}
@@ -182,20 +179,20 @@ static int _poll_vg(struct cmd_context *
 {
 	struct daemon_parms *parms = (struct daemon_parms *) handle;
 	struct lv_list *lvl;
-	struct logical_volume *lv_mirr;
+	struct logical_volume *lv;
 	const char *name;
 	int finished;
 
 	dm_list_iterate_items(lvl, &vg->lvs) {
-		lv_mirr = lvl->lv;
-		if (!(lv_mirr->status & parms->lv_type))
+		lv = lvl->lv;
+		if (!(lv->status & parms->lv_type))
 			continue;
-		if (!(name = parms->poll_fns->get_copy_name_from_lv(lv_mirr)))
+		if (!(name = parms->poll_fns->get_copy_name_from_lv(lv)))
 			continue;
 		/* FIXME Need to do the activation from _set_up_pvmove here
 		 *       if it's not running and we're not aborting */
-		if (_check_mirror_status(cmd, vg, lv_mirr, name,
-					 parms, &finished) && !finished)
+		if (_check_lv_status(cmd, vg, lv, name,
+				     parms, &finished) && !finished)
 			parms->outstanding_count++;
 	}
 
@@ -251,12 +248,14 @@ int poll_daemon(struct cmd_context *cmd,
 	}
 
 	if (name) {
-		if (!_wait_for_single_mirror(cmd, name, uuid, &parms)) {
+		if (!_wait_for_single_lv(cmd, name, uuid, &parms)) {
 			stack;
 			return ECMD_FAILED;
 		}
-	} else
+	} else {
+		/* Only used for pvmove --abort */
 		_poll_for_all_vgs(cmd, &parms);
+	}
 
 	return ECMD_PROCESSED;
 }
Index: lvm2/tools/polldaemon.h
===================================================================
--- lvm2.orig/tools/polldaemon.h
+++ lvm2/tools/polldaemon.h
@@ -19,7 +19,7 @@
 #include "metadata-exported.h"
 
 struct poll_functions {
-	const char *(*get_copy_name_from_lv) (struct logical_volume *lv_mirr);
+	const char *(*get_copy_name_from_lv) (struct logical_volume *lv);
 	struct volume_group *(*get_copy_vg) (struct cmd_context *cmd,
 					     const char *name,
 					     const char *uuid);
@@ -30,11 +30,11 @@ struct poll_functions {
 					       uint32_t lv_type);
 	int (*update_metadata) (struct cmd_context *cmd,
 				struct volume_group *vg,
-				struct logical_volume *lv_mirr,
+				struct logical_volume *lv,
 				struct dm_list *lvs_changed, unsigned flags);
 	int (*finish_copy) (struct cmd_context *cmd,
 			    struct volume_group *vg,
-			    struct logical_volume *lv_mirr,
+			    struct logical_volume *lv,
 			    struct dm_list *lvs_changed);
 };
 

[PATCH 2/4] Eliminate _check_lv_status's 'name' argument

[Mike Snitzer]

_check_lv_status's 'name' argument sometimes contains a name of a VG and
sometimes name of an LV, so using it in generic code would be dangerous.
'name' was never used for anything other than progress_display.

The next patch will properly split out context specific
progress_display.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
Signed-off-by: Mike Snitzer <snitzer@redhat.com>

---
 tools/polldaemon.c |   18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

Index: lvm2/tools/polldaemon.c
===================================================================
--- lvm2.orig/tools/polldaemon.c
+++ lvm2/tools/polldaemon.c
@@ -66,7 +66,7 @@ static int _become_daemon(struct cmd_con
 static int _check_lv_status(struct cmd_context *cmd,
 			    struct volume_group *vg,
 			    struct logical_volume *lv,
-			    const char *name, struct daemon_parms *parms,
+			    struct daemon_parms *parms,
 			    int *finished)
 {
 	struct dm_list *lvs_changed;
@@ -94,12 +94,6 @@ static int _check_lv_status(struct cmd_c
 	}
 
 	overall_status = copy_percent(lv, &overall_percent);
-	if (parms->progress_display)
-		log_print("%s: %s: %.1f%%", name, parms->progress_title,
-			  overall_percent);
-	else
-		log_verbose("%s: %s: %.1f%%", name, parms->progress_title,
-			    overall_percent);
 
 	if (status < TARGET_STATUS_FINISHED) {
 		/* The only case the caller *should* try again later */
@@ -163,7 +157,7 @@ static int _wait_for_single_lv(struct cm
 			return 0;
 		}
 
-		if (!_check_lv_status(cmd, vg, lv, name, parms, &finished)) {
+		if (!_check_lv_status(cmd, vg, lv, parms, &finished)) {
 			unlock_and_release_vg(cmd, vg, vg->name);
 			return 0;
 		}
@@ -180,19 +174,19 @@ static int _poll_vg(struct cmd_context *
 	struct daemon_parms *parms = (struct daemon_parms *) handle;
 	struct lv_list *lvl;
 	struct logical_volume *lv;
-	const char *name;
 	int finished;
 
 	dm_list_iterate_items(lvl, &vg->lvs) {
 		lv = lvl->lv;
 		if (!(lv->status & parms->lv_type))
 			continue;
-		if (!(name = parms->poll_fns->get_copy_name_from_lv(lv)))
+		/* NOTE don't need name but this check has significance
+		 *      because it skips LVs that aren't mirrored */
+		if (parms->poll_fns->get_copy_name_from_lv(lv) == NULL)
 			continue;
 		/* FIXME Need to do the activation from _set_up_pvmove here
 		 *       if it's not running and we're not aborting */
-		if (_check_lv_status(cmd, vg, lv, name,
-				     parms, &finished) && !finished)
+		if (_check_lv_status(cmd, vg, lv, parms, &finished) && !finished)
 			parms->outstanding_count++;
 	}
 

[PATCH 3/4] Define new periodic polling method: poll_progress

[Mike Snitzer]

Define new polling method poll_progress that is called periodically. The method
can return:
0 --- error
PROGRESS_TRY_AGAIN --- should be called again after specified interval
PROGRESS_UPDATE_METADATA --- should call update_metadata method and continue
			     polling
PROGRESS_FINISHED --- should call finish_copy and exit polling

The hard-coded mirror-specific code in polldaemon was moved to function
poll_mirror_progress. The rest of the polldaemon is mirror-free so it can be
used for merging snapshots as well.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
Reviewed-by: Mike Snitzer <snitzer@redhat.com>

---
 tools/lvconvert.c  |    1 +
 tools/polldaemon.c |   49 +++++++++++++++++++++++++++++++++++++------------
 tools/polldaemon.h |   31 +++++++++++++++++++++----------
 tools/pvmove.c     |    1 +
 4 files changed, 60 insertions(+), 22 deletions(-)

Index: lvm2/tools/lvconvert.c
===================================================================
--- lvm2.orig/tools/lvconvert.c
+++ lvm2/tools/lvconvert.c
@@ -316,6 +316,7 @@ static struct poll_functions _lvconvert_
 	.get_copy_vg = _get_lvconvert_vg,
 	.get_copy_lv = _get_lvconvert_lv,
 	.update_metadata = _update_lvconvert_mirror,
+	.poll_progress = poll_mirror_progress,
 	.finish_copy = _finish_lvconvert_mirror,
 };
 
Index: lvm2/tools/polldaemon.c
===================================================================
--- lvm2.orig/tools/polldaemon.c
+++ lvm2/tools/polldaemon.c
@@ -63,6 +63,38 @@ static int _become_daemon(struct cmd_con
 	return 1;
 }
 
+int poll_mirror_progress(struct cmd_context *cmd, struct volume_group *vg,
+			 struct logical_volume *lv, struct daemon_parms *parms)
+{
+	float segment_percent = 0.0, overall_percent = 0.0;
+	int status, overall_status;
+	uint32_t event_nr = 0;
+
+	if (!(status = lv_mirror_percent(cmd, lv, !parms->interval,
+					 &segment_percent, &event_nr))) {
+		log_error("ABORTING: Mirror percentage check failed.");
+		return 0;
+	}
+
+	overall_status = copy_percent(lv, &overall_percent);
+	if (parms->progress_display)
+		log_print("%s: %s: %.1f%%", lv->name, parms->progress_title,
+			  overall_percent);
+	else
+		log_verbose("%s: %s: %.1f%%", lv->name, parms->progress_title,
+			    overall_percent);
+
+	if (status < TARGET_STATUS_FINISHED) {
+		/* The only case the caller *should* try again later */
+		return PROGRESS_TRY_AGAIN;
+	}
+
+	if (overall_status == TARGET_STATUS_FINISHED)
+		return PROGRESS_FINISHED;
+	else
+		return PROGRESS_UPDATE_METADATA;
+}
+
 static int _check_lv_status(struct cmd_context *cmd,
 			    struct volume_group *vg,
 			    struct logical_volume *lv,
@@ -70,9 +102,7 @@ static int _check_lv_status(struct cmd_c
 			    int *finished)
 {
 	struct dm_list *lvs_changed;
-	float segment_percent = 0.0, overall_percent = 0.0;
-	int status, overall_status;
-	uint32_t event_nr = 0;
+	int r;
 
 	/* By default, caller should not retry */
 	*finished = 1;
@@ -87,16 +117,11 @@ static int _check_lv_status(struct cmd_c
 		return 0;
 	}
 
-	if (!(status = lv_mirror_percent(cmd, lv, !parms->interval,
-					 &segment_percent, &event_nr))) {
-		log_error("ABORTING: Mirror percentage check failed.");
+	r = parms->poll_fns->poll_progress(cmd, vg, lv, parms);
+	if (!r)
 		return 0;
-	}
 
-	overall_status = copy_percent(lv, &overall_percent);
-
-	if (status < TARGET_STATUS_FINISHED) {
-		/* The only case the caller *should* try again later */
+	if (r == PROGRESS_TRY_AGAIN) {
 		*finished = 0;
 		return 1;
 	}
@@ -107,7 +132,7 @@ static int _check_lv_status(struct cmd_c
 	}
 
 	/* Finished? Or progress to next segment? */
-	if (overall_status == TARGET_STATUS_FINISHED) {
+	if (r == PROGRESS_FINISHED) {
 		if (!parms->poll_fns->finish_copy(cmd, vg, lv, lvs_changed))
 			return 0;
 	} else {
Index: lvm2/tools/polldaemon.h
===================================================================
--- lvm2.orig/tools/polldaemon.h
+++ lvm2/tools/polldaemon.h
@@ -18,6 +18,19 @@
 
 #include "metadata-exported.h"
 
+struct poll_functions;
+
+struct daemon_parms {
+	unsigned interval;
+	unsigned aborting;
+	unsigned background;
+	unsigned outstanding_count;
+	unsigned progress_display;
+	const char *progress_title;
+	uint32_t lv_type;
+	struct poll_functions *poll_fns;
+};
+
 struct poll_functions {
 	const char *(*get_copy_name_from_lv) (struct logical_volume *lv);
 	struct volume_group *(*get_copy_vg) (struct cmd_context *cmd,
@@ -28,6 +41,8 @@ struct poll_functions {
 					       const char *name,
 					       const char *uuid,
 					       uint32_t lv_type);
+	int (*poll_progress) (struct cmd_context *cmd, struct volume_group *vg,
+			 struct logical_volume *lv, struct daemon_parms *parms);
 	int (*update_metadata) (struct cmd_context *cmd,
 				struct volume_group *vg,
 				struct logical_volume *lv,
@@ -38,16 +53,12 @@ struct poll_functions {
 			    struct dm_list *lvs_changed);
 };
 
-struct daemon_parms {
-	unsigned interval;
-	unsigned aborting;
-	unsigned background;
-	unsigned outstanding_count;
-	unsigned progress_display;
-	const char *progress_title;
-	uint32_t lv_type;
-	struct poll_functions *poll_fns;
-};
+int poll_mirror_progress(struct cmd_context *cmd, struct volume_group *vg,
+			 struct logical_volume *lv, struct daemon_parms *parms);
+
+#define PROGRESS_TRY_AGAIN		1
+#define PROGRESS_UPDATE_METADATA	2
+#define PROGRESS_FINISHED		3
 
 int poll_daemon(struct cmd_context *cmd, const char *name, const char *uuid,
 		unsigned background,
Index: lvm2/tools/pvmove.c
===================================================================
--- lvm2.orig/tools/pvmove.c
+++ lvm2/tools/pvmove.c
@@ -562,6 +562,7 @@ static struct poll_functions _pvmove_fns
 	.get_copy_name_from_lv = get_pvmove_pvname_from_lv_mirr,
 	.get_copy_vg = _get_move_vg,
 	.get_copy_lv = find_pvmove_lv_from_pvname,
+	.poll_progress = poll_mirror_progress,
 	.update_metadata = _update_metadata,
 	.finish_copy = _finish_pvmove,
 };

[PATCH 4/4] Move activation of background polling processes to a common function.

[Mike Snitzer]

Move activation of background polling processes to a common function.

Avoids code duplication and fixes an associated where vgchange would
only restart pvmove and _not_ lvconvert (lvchange would restart
both).

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
Reviewed-by: Mike Snitzer <snitzer@redhat.com>

---
 tools/lvchange.c |   33 ++++++++++++++++++++-------------
 tools/tools.h    |    1 +
 tools/vgchange.c |   10 ++--------
 3 files changed, 23 insertions(+), 21 deletions(-)

Index: lvm2/tools/lvchange.c
===================================================================
--- lvm2.orig/tools/lvchange.c
+++ lvm2/tools/lvchange.c
@@ -101,11 +101,29 @@ static int lvchange_monitoring(struct cm
 	return 1;
 }
 
+void lvchange_activate_background_polling(struct cmd_context *cmd,
+					  struct logical_volume *lv)
+{
+	const char *pvname;
+
+	if ((lv->status & LOCKED) &&
+	    (pvname = get_pvmove_pvname_from_lv(lv))) {
+		log_verbose("Spawning background pvmove process for %s",
+			    pvname);
+		pvmove_poll(cmd, pvname, 1);
+	}
+
+	if (lv->status & CONVERTING) {
+		log_verbose("Spawning background lvconvert process for %s",
+			    lv->name);
+		lvconvert_poll(cmd, lv, 1);
+	}
+}
+
 static int lvchange_availability(struct cmd_context *cmd,
 				 struct logical_volume *lv)
 {
 	int activate;
-	const char *pvname;
 
 	activate = arg_uint_value(cmd, available_ARG, 0);
 
@@ -136,18 +154,7 @@ static int lvchange_availability(struct 
 				return_0;
 		}
 
-		if ((lv->status & LOCKED) &&
-		    (pvname = get_pvmove_pvname_from_lv(lv))) {
-			log_verbose("Spawning background pvmove process for %s",
-				    pvname);
-			pvmove_poll(cmd, pvname, 1);
-		}
-
-		if (lv->status & CONVERTING) {
-			log_verbose("Spawning background lvconvert process for %s",
-				    lv->name);
-			lvconvert_poll(cmd, lv, 1);
-		}
+		lvchange_activate_background_polling(cmd, lv);
 	}
 
 	return 1;
Index: lvm2/tools/tools.h
===================================================================
--- lvm2.orig/tools/tools.h
+++ lvm2/tools/tools.h
@@ -169,5 +169,6 @@ const char *command_name(struct cmd_cont
 
 int pvmove_poll(struct cmd_context *cmd, const char *pv, unsigned background);
 int lvconvert_poll(struct cmd_context *cmd, struct logical_volume *lv, unsigned background);
+void lvchange_activate_background_polling(struct cmd_context *cmd, struct logical_volume *lv);
 
 #endif
Index: lvm2/tools/vgchange.c
===================================================================
--- lvm2.orig/tools/vgchange.c
+++ lvm2/tools/vgchange.c
@@ -56,7 +56,6 @@ static int _activate_lvs_in_vg(struct cm
 {
 	struct lv_list *lvl;
 	struct logical_volume *lv;
-	const char *pvname;
 	int count = 0;
 
 	dm_list_iterate_items(lvl, &vg->lvs) {
@@ -91,13 +90,8 @@ static int _activate_lvs_in_vg(struct cm
 		} else if (!activate_lv(cmd, lv))
 			continue;
 
-		if ((lv->status & PVMOVE) &&
-		    (pvname = get_pvmove_pvname_from_lv_mirr(lv))) {
-			log_verbose("Spawning background process for %s %s",
-				    lv->name, pvname);
-			pvmove_poll(cmd, pvname, 1);
-			continue;
-		}
+		if (activate != CHANGE_AN && activate != CHANGE_ALN)
+			lvchange_activate_background_polling(cmd, lv);
 
 		count++;
 	}

[PATCH 4/4] Move activation of background polling processes to a common function.

[Alasdair G Kergon]

On Tue, Sep 29, 2009 at 11:22:57AM -0400, Mike Snitzer wrote:
> Move activation of background polling processes to a common function.
> Avoids code duplication and fixes an associated where vgchange would
> only restart pvmove and _not_ lvconvert (lvchange would restart
> both).
 
Applied, but I've retained the more-efficient version for pvmove for
vgchange: when you know you are looping through all the LVs in the VG
you can skip the normal (LOCKED) LVs which are being moved and trigger
from the PVMOVE ones that perform the pvmove.  As lvchange might only
be processing a single LV, you can't do that.

Alasdair

Re: [PATCH 2/4] Eliminate _check_lv_status's 'name' argument

[Mike Snitzer]

On Tue, Sep 29 2009 at 11:22am -0400,
Mike Snitzer <snitzer@redhat.com> wrote:

> _check_lv_status's 'name' argument sometimes contains a name of a VG and
> sometimes name of an LV, so using it in generic code would be dangerous.
> 'name' was never used for anything other than progress_display.
> 
> The next patch will properly split out context specific
> progress_display.

Updated patch is available here:
http://people.redhat.com/msnitzer/patches/snapshot-merge/lvm2/LVM2-2.02.54/lvm-poll-drop-argument-name-in-_check_lv_status.patch

Re: [PATCH 3/4] Define new periodic polling method: poll_progress

[Mike Snitzer]

On Tue, Sep 29 2009 at 11:22am -0400,
Mike Snitzer <snitzer@redhat.com> wrote:

> Define new polling method poll_progress that is called periodically. The method
> can return:
> 0 --- error
> PROGRESS_TRY_AGAIN --- should be called again after specified interval
> PROGRESS_UPDATE_METADATA --- should call update_metadata method and continue
> 			     polling
> PROGRESS_FINISHED --- should call finish_copy and exit polling
> 
> The hard-coded mirror-specific code in polldaemon was moved to function
> poll_mirror_progress. The rest of the polldaemon is mirror-free so it can be
> used for merging snapshots as well.

Updated patch is available here:
http://people.redhat.com/msnitzer/patches/snapshot-merge/lvm2/LVM2-2.02.54/lvm-poll-progress-method.patch