master - pvmove: reinstantiate clustered pvmove

master - pvmove: reinstantiate clustered pvmove

[Zdenek Kabelac]

Gitweb:        https://sourceware.org/git/?p=lvm2.git;a=commitdiff;h=083c221cbebccd8ea893e87f17c69fba378d8645
Commit:        083c221cbebccd8ea893e87f17c69fba378d8645
Parent:        34fb5202bdb3172a44fb3d957e184df4fb0412d8
Author:        Zdenek Kabelac <zkabelac@redhat.com>
AuthorDate:    Wed Jan 31 10:53:09 2018 +0100
Committer:     Zdenek Kabelac <zkabelac@redhat.com>
CommitterDate: Thu Feb 1 21:55:20 2018 +0100

pvmove: reinstantiate clustered pvmove

In fact  pvmove does support  'clustered-core' target for clustered
pvmove of LVs activated on multiple nodes.

This patch restores support for activation of pvmove on all nodes
for LVs that are also activate on all nodes.
---
 WHATS_NEW               |    1 +
 lib/activate/activate.c |    1 -
 lib/locking/locking.c   |   12 +++--
 lib/locking/locking.h   |    2 +-
 tools/pvmove.c          |  109 +++++++++++++++++++++++++++++++++++-----------
 5 files changed, 93 insertions(+), 32 deletions(-)

diff --git a/WHATS_NEW b/WHATS_NEW
index bc73cd7..75a147b 100644
--- a/WHATS_NEW
+++ b/WHATS_NEW
@@ -1,5 +1,6 @@
 Version 2.02.178 - 
 =====================================
+  Restore pvmove support for wide-clustered active volumes (2.02.177).
   Avoid non-exclusive activation of exclusive segment types.
   Fix trimming sibling PVs when doing a pvmove of raid subLVs.
   Preserve exclusive activation during thin snaphost merge.
diff --git a/lib/activate/activate.c b/lib/activate/activate.c
index 18cc7cf..7a37130 100644
--- a/lib/activate/activate.c
+++ b/lib/activate/activate.c
@@ -2576,7 +2576,6 @@ static int _lv_activate(struct cmd_context *cmd, const char *lvid_s,
 
 	if (!laopts->exclusive &&
 	    (lv_is_origin(lv) ||
-	     lv_is_pvmove(lv) ||
 	     seg_only_exclusive(first_seg(lv))))  {
 		log_error(INTERNAL_ERROR "Trying non-exlusive activation of %s with "
 			  "a volume type %s requiring exclusive activation.",
diff --git a/lib/locking/locking.c b/lib/locking/locking.c
index 8daa61e..1a3ce9d 100644
--- a/lib/locking/locking.c
+++ b/lib/locking/locking.c
@@ -399,15 +399,19 @@ int activate_lv_excl(struct cmd_context *cmd, const struct logical_volume *lv)
 }
 
 /* Lock a list of LVs */
-int activate_lvs(struct cmd_context *cmd, struct dm_list *lvs)
+int activate_lvs(struct cmd_context *cmd, struct dm_list *lvs, unsigned exclusive)
 {
 	struct dm_list *lvh;
 	struct lv_list *lvl;
 
 	dm_list_iterate_items(lvl, lvs) {
-		if (!activate_lv_excl_local(cmd, lvl->lv)) {
-			log_error("Failed to locally exclusively activate %s.",
-				  display_lvname(lvl->lv));
+		if (!exclusive && !lv_is_active_exclusive(lvl->lv)) {
+			if (!activate_lv(cmd, lvl->lv)) {
+				log_error("Failed to activate %s", display_lvname(lvl->lv));
+				return 0;
+			}
+		} else if (!activate_lv_excl(cmd, lvl->lv)) {
+			log_error("Failed to activate %s", display_lvname(lvl->lv));
 			dm_list_uniterate(lvh, lvs, &lvl->list) {
 				lvl = dm_list_item(lvh, struct lv_list);
 				if (!deactivate_lv(cmd, lvl->lv))
diff --git a/lib/locking/locking.h b/lib/locking/locking.h
index 47841ed..f2fbb00 100644
--- a/lib/locking/locking.h
+++ b/lib/locking/locking.h
@@ -262,6 +262,6 @@ int sync_dev_names(struct cmd_context* cmd);
 
 /* Process list of LVs */
 struct volume_group;
-int activate_lvs(struct cmd_context *cmd, struct dm_list *lvs);
+int activate_lvs(struct cmd_context *cmd, struct dm_list *lvs, unsigned exclusive);
 
 #endif
diff --git a/tools/pvmove.c b/tools/pvmove.c
index b3d1d89..cbd5cb8 100644
--- a/tools/pvmove.c
+++ b/tools/pvmove.c
@@ -64,6 +64,16 @@ static int _pvmove_target_present(struct cmd_context *cmd, int clustered)
 	return found;
 }
 
+static unsigned _pvmove_is_exclusive(struct cmd_context *cmd,
+				     struct volume_group *vg)
+{
+	if (vg_is_clustered(vg))
+		if (!_pvmove_target_present(cmd, 1))
+			return 1;
+
+	return 0;
+}
+
 /* Allow /dev/vgname/lvname, vgname/lvname or lvname */
 static const char *_extract_lvname(struct cmd_context *cmd, const char *vgname,
 				   const char *arg)
@@ -320,7 +330,8 @@ static struct logical_volume *_set_up_pvmove_lv(struct cmd_context *cmd,
 						const char *lv_name,
 						struct dm_list *allocatable_pvs,
 						alloc_policy_t alloc,
-						struct dm_list **lvs_changed)
+						struct dm_list **lvs_changed,
+						unsigned *exclusive)
 {
 	struct logical_volume *lv_mirr, *lv;
 	struct lv_segment *seg;
@@ -329,6 +340,8 @@ static struct logical_volume *_set_up_pvmove_lv(struct cmd_context *cmd,
 	uint32_t log_count = 0;
 	int lv_found = 0;
 	int lv_skipped = 0;
+	int lv_active_count = 0;
+	int lv_exclusive_count = 0;
 
 	/* FIXME Cope with non-contiguous => splitting existing segments */
 	if (!(lv_mirr = lv_create_empty("pvmove%d", NULL,
@@ -422,33 +435,54 @@ static struct logical_volume *_set_up_pvmove_lv(struct cmd_context *cmd,
 		if (!lv_is_on_pvs(lv, source_pvl))
 			continue;
 
-		seg = first_seg(lv);
-		if (seg_is_raid(seg) || seg_is_mirrored(seg) ||
-		    lv_is_thin_volume(lv) || lv_is_thin_pool(lv)) {
-			/*
-			 * Pass over top-level LVs - they were handled.
-			 * Allow sub-LVs to proceed.
-			 */
-			continue;
-		}
-
 		if (lv_is_locked(lv)) {
 			lv_skipped = 1;
 			log_print_unless_silent("Skipping locked LV %s.", display_lvname(lv));
 			continue;
 		}
 
-		if (vg_is_clustered(vg) &&
-		    lv_is_visible(lv) &&
-		    lv_is_active(lv) &&
-		    !lv_is_active_exclusive_locally(lv)) {
-			lv_skipped = 1;
-			log_print_unless_silent("Skipping LV %s which is active, "
-						"but not locally exclusively.",
-						display_lvname(lv));
-			continue;
+		if (vg_is_clustered(vg) && lv_is_visible(lv)) {
+			if (lv_is_active_exclusive_locally(lv)) {
+				if (lv_active_count) {
+					log_error("Cannot move in clustered VG %s "
+						  "if some LVs are activated "
+						  "exclusively while others don't.",
+						  vg->name);
+					return NULL;
+				}
+
+				lv_exclusive_count++;
+			} else if (lv_is_active(lv)) {
+				if (seg_only_exclusive(first_seg(lv))) {
+					lv_skipped = 1;
+					log_print_unless_silent("Skipping LV %s which is active, "
+								"but not locally exclusively.",
+							display_lvname(lv));
+					continue;
+				}
+
+				if (*exclusive) {
+					log_error("Cannot move in clustered VG %s, "
+						  "clustered mirror (cmirror) not detected "
+						  "and LVs are activated non-exclusively.",
+						  vg->name);
+					return NULL;
+				}
+
+				lv_active_count++;
+			}
 		}
 
+		seg = first_seg(lv);
+		if (seg_is_raid(seg) || seg_is_mirrored(seg) ||
+		    seg_is_cache(seg) || seg_is_cache_pool(seg) ||
+		    seg_is_thin(seg) || seg_is_thin_pool(seg))
+			/*
+			 * Pass over top-level LVs - they were handled.
+			 * Allow sub-LVs to proceed.
+			 */
+			continue;
+
 		if (!_insert_pvmove_mirrors(cmd, lv_mirr, source_pvl, lv,
 					    *lvs_changed))
 			return_NULL;
@@ -483,15 +517,35 @@ static struct logical_volume *_set_up_pvmove_lv(struct cmd_context *cmd,
 		return NULL;
 	}
 
+	if (lv_exclusive_count)
+		*exclusive = 1;
+
 	return lv_mirr;
 }
 
+static int _activate_lv(struct cmd_context *cmd, struct logical_volume *lv_mirr,
+			unsigned exclusive)
+{
+	int r = 0;
+
+	if (exclusive || lv_is_active_exclusive(lv_mirr))
+		r = activate_lv_excl(cmd, lv_mirr);
+	else
+		r = activate_lv(cmd, lv_mirr);
+
+	if (!r)
+		stack;
+
+	return r;
+}
+
 /*
  * Called to set up initial pvmove LV only.
  * (Not called after first or any other section completes.)
  */
 static int _update_metadata(struct logical_volume *lv_mirr,
-			    struct dm_list *lvs_changed)
+			    struct dm_list *lvs_changed,
+			    unsigned exclusive)
 {
 	struct lv_list *lvl;
 	struct logical_volume *lv = lv_mirr;
@@ -505,7 +559,7 @@ static int _update_metadata(struct logical_volume *lv_mirr,
                 return_0;
 
 	/* Ensure mirror LV is active */
-	if (!activate_lv_excl_local(lv_mirr->vg->cmd, lv_mirr)) {
+	if (!_activate_lv(lv_mirr->vg->cmd, lv_mirr, exclusive)) {
 		if (test_mode())
 			return 1;
 
@@ -548,6 +602,7 @@ static int _pvmove_setup_single(struct cmd_context *cmd,
 	struct logical_volume *lv = NULL;
 	const char *pv_name = pv_dev_name(pv);
 	unsigned flags = PVMOVE_FIRST_TIME;
+	unsigned exclusive;
 	int r = ECMD_FAILED;
 
 	pp->found_pv = 1;
@@ -594,6 +649,8 @@ static int _pvmove_setup_single(struct cmd_context *cmd,
 		}
 	}
 
+	exclusive = _pvmove_is_exclusive(cmd, vg);
+
 	if ((lv_mirr = find_pvmove_lv(vg, pv_dev(pv), PVMOVE))) {
 		log_print_unless_silent("Detected pvmove in progress for %s.", pv_name);
 		if (pp->pv_count || lv_name)
@@ -605,7 +662,7 @@ static int _pvmove_setup_single(struct cmd_context *cmd,
 		}
 
 		/* Ensure mirror LV is active */
-		if (!activate_lv_excl_local(cmd, lv_mirr)) {
+		if (!_activate_lv(cmd, lv_mirr, exclusive)) {
 			log_error("ABORTING: Temporary mirror activation failed.");
 			goto out;
 		}
@@ -630,12 +687,12 @@ static int _pvmove_setup_single(struct cmd_context *cmd,
 
 		if (!(lv_mirr = _set_up_pvmove_lv(cmd, vg, source_pvl, lv_name,
 						  allocatable_pvs, pp->alloc,
-						  &lvs_changed)))
+						  &lvs_changed, &exclusive)))
 			goto_out;
 	}
 
 	/* Lock lvs_changed and activate (with old metadata) */
-	if (!activate_lvs(cmd, lvs_changed))
+	if (!activate_lvs(cmd, lvs_changed, exclusive))
 		goto_out;
 
 	/* FIXME Presence of a mirror once set PVMOVE - now remove associated logic */
@@ -646,7 +703,7 @@ static int _pvmove_setup_single(struct cmd_context *cmd,
 		goto out;
 
 	if (flags & PVMOVE_FIRST_TIME)
-		if (!_update_metadata(lv_mirr, lvs_changed))
+		if (!_update_metadata(lv_mirr, lvs_changed, exclusive))
 			goto_out;
 
 	/* LVs are all in status LOCKED */

master - pvmove: reinstantiate clustered pvmove

[Eric Ren]

Hello Zdenek,

I've tried this patch with clvmd and cmirrord running, and all LVs in 
clustered VG being activated
on both nodes. But, pvmove still cannot work as expect - move data on 
underlying PV of the the
non-exclusive activated LV.

==========
tw1:~ # pgrep -a mirrord
11931 cmirrord
tw1:~ # pgrep -a clvmd
11748 /usr/sbin/clvmd -T90 -d0

tw1:~ # vgs -o+vg_clustered vgtest2
 ??? VG????? #PV #LV #SN Attr?? VSize VFree Clustered
 ??? vgtest2?? 2?? 2?? 0 wz--nc 9.30g 6.30g? clustered
tw1:~ # lvs -o+lv_active_exclusively,lv_active_locally vgtest2
 ??? LV?? VG????? Attr?????? LSize Pool Origin Data%? Meta%? Move Log 
Cpy%Sync Convert ActExcl??? ActLocal
 ??? lv1? vgtest2 -wi-a----- 2.00g active locally
 ??? lv2? vgtest2 -wi-a----- 1.00g active locally
tw1:~ # pvs -S vg_name=vgtest2
 ??? PV???????? VG????? Fmt? Attr PSize PFree
 ??? /dev/vdb1? vgtest2 lvm2 a--? 4.65g 4.65g
 ??? /dev/vdb2? vgtest2 lvm2 a--? 4.65g 1.65g

tw1:~ # pvmove /dev/vdb2
 ??? Cannot move in clustered VG vgtest2, clustered mirror (cmirror) not 
detected and LVs are activated non-exclusively.
============


GDB it a little bit. The problem seems because:

_pvmove_target_present(cmd, 1)


will always return 0 - "not found".

During one pvmove command, the _pvmove_target_present() is invoked 
twice. At first call,
"segtype->ops->target_present()", i.e _mirrored_target_present() will 
set "_mirrored_checked = 0".

At the second call, _mirrored_target_present() will not go through the 
following code to get the
"_mirror_attributes":

"""
400 static int _mirrored_target_present(struct cmd_context *cmd,
401???????????????????????????????????? const struct lv_segment *seg,
402???????????????????????????????????? unsigned *attributes)
...
442 #ifdef CMIRRORD_PIDFILE
443???????????????? /*
444????????????????? * The cluster mirror log daemon must be running,
445????????????????? * otherwise, the kernel module will fail to make
446????????????????? * contact.
447????????????????? */
448???????????????? if (cmirrord_is_running()) {
449???????????????????????? struct utsname uts;
450???????????????????????? unsigned kmaj, kmin, krel;
451???????????????????????? /*
452????????????????????????? * The dm-log-userspace module was added to the
453????????????????????????? * 2.6.31 kernel.
454????????????????????????? */
455???????????????????????? if (!uname(&uts) &&
456???????????????????????????? (sscanf(uts.release, "%u.%u.%u", &kmaj, 
&kmin, &krel) == 3) &&
457???????????????????????????? KERNEL_VERSION(kmaj, kmin, krel) < 
KERNEL_VERSION(2, 6, 31)) {
458???????????????????????????????? if (module_present(cmd, 
MODULE_NAME_LOG_CLUSTERED))
459???????????????????????????????? _mirror_attributes |= 
MIRROR_LOG_CLUSTERED;
460???????????????????????? } else if (module_present(cmd, 
MODULE_NAME_LOG_USERSPACE))
461???????????????????????????????? _mirror_attributes |= 
MIRROR_LOG_CLUSTERED;
462
463???????????????????????? if (!(_mirror_attributes & 
MIRROR_LOG_CLUSTERED))
464???????????????????????????????? log_verbose("Cluster mirror log 
module is not available.");
465???????????????? } else
466???????????????????????? log_verbose("Cluster mirror log daemon is 
not running.");
467 #else
468???????????????? log_verbose("Cluster mirror log daemon not included 
in build.");
469 #endif
...
"""

even if it is asked to back the "target attributes" by 
_pvmove_target_present(cmd, 1).
As result, _pvmove_target_present(cmd, 1) will always return "0", 
because the "attributes"
is empty.

"""
 ?40 static int _pvmove_target_present(struct cmd_context *cmd, int 
clustered)
 ?41 {
 ?42???????? const struct segment_type *segtype;
 ?43???????? unsigned attr = 0;
 ?44???????? int found = 1;
 ?45???????? static int _clustered_found = -1;
 ?46
 ?47???????? if (clustered && _clustered_found >= 0)
 ?48???????????????? return _clustered_found;
 ?49
 ?50???????? if (!(segtype = get_segtype_from_string(cmd, 
SEG_TYPE_NAME_MIRROR)))
 ?51???????????????? return_0;
 ?52
 ?53???????? if (activation() && segtype->ops->target_present &&
 ?54???????????? !segtype->ops->target_present(cmd, NULL, clustered ? 
&attr : NULL))
 ?55???????????????? found = 0;
 ?56
 ?57???????? if (activation() && clustered) {
 ?58???????????????? if (found && (attr & MIRROR_LOG_CLUSTERED))
 ?59???????????????????????? _clustered_found = found = 1;
 ?60???????????????? else
 ?61???????????????????????? _clustered_found = found = 0;
 ?62???????? }
 ?63
 ?64???????? return found;
 ?65 }
"""

Could you take a look at this?

Thanks!
Eric



On 02/02/2018 04:58 AM, Zdenek Kabelac wrote:
> Gitweb:        https://sourceware.org/git/?p=lvm2.git;a=commitdiff;h=083c221cbebccd8ea893e87f17c69fba378d8645
> Commit:        083c221cbebccd8ea893e87f17c69fba378d8645
> Parent:        34fb5202bdb3172a44fb3d957e184df4fb0412d8
> Author:        Zdenek Kabelac <zkabelac@redhat.com>
> AuthorDate:    Wed Jan 31 10:53:09 2018 +0100
> Committer:     Zdenek Kabelac <zkabelac@redhat.com>
> CommitterDate: Thu Feb 1 21:55:20 2018 +0100
>
> pvmove: reinstantiate clustered pvmove
>
> In fact  pvmove does support  'clustered-core' target for clustered
> pvmove of LVs activated on multiple nodes.
>
> This patch restores support for activation of pvmove on all nodes
> for LVs that are also activate on all nodes.
> ---
>   WHATS_NEW               |    1 +
>   lib/activate/activate.c |    1 -
>   lib/locking/locking.c   |   12 +++--
>   lib/locking/locking.h   |    2 +-
>   tools/pvmove.c          |  109 +++++++++++++++++++++++++++++++++++-----------
>   5 files changed, 93 insertions(+), 32 deletions(-)
>
> diff --git a/WHATS_NEW b/WHATS_NEW
> index bc73cd7..75a147b 100644
> --- a/WHATS_NEW
> +++ b/WHATS_NEW
> @@ -1,5 +1,6 @@
>   Version 2.02.178 -
>   =====================================
> +  Restore pvmove support for wide-clustered active volumes (2.02.177).
>     Avoid non-exclusive activation of exclusive segment types.
>     Fix trimming sibling PVs when doing a pvmove of raid subLVs.
>     Preserve exclusive activation during thin snaphost merge.
> diff --git a/lib/activate/activate.c b/lib/activate/activate.c
> index 18cc7cf..7a37130 100644
> --- a/lib/activate/activate.c
> +++ b/lib/activate/activate.c
> @@ -2576,7 +2576,6 @@ static int _lv_activate(struct cmd_context *cmd, const char *lvid_s,
>   
>   	if (!laopts->exclusive &&
>   	    (lv_is_origin(lv) ||
> -	     lv_is_pvmove(lv) ||
>   	     seg_only_exclusive(first_seg(lv))))  {
>   		log_error(INTERNAL_ERROR "Trying non-exlusive activation of %s with "
>   			  "a volume type %s requiring exclusive activation.",
> diff --git a/lib/locking/locking.c b/lib/locking/locking.c
> index 8daa61e..1a3ce9d 100644
> --- a/lib/locking/locking.c
> +++ b/lib/locking/locking.c
> @@ -399,15 +399,19 @@ int activate_lv_excl(struct cmd_context *cmd, const struct logical_volume *lv)
>   }
>   
>   /* Lock a list of LVs */
> -int activate_lvs(struct cmd_context *cmd, struct dm_list *lvs)
> +int activate_lvs(struct cmd_context *cmd, struct dm_list *lvs, unsigned exclusive)
>   {
>   	struct dm_list *lvh;
>   	struct lv_list *lvl;
>   
>   	dm_list_iterate_items(lvl, lvs) {
> -		if (!activate_lv_excl_local(cmd, lvl->lv)) {
> -			log_error("Failed to locally exclusively activate %s.",
> -				  display_lvname(lvl->lv));
> +		if (!exclusive && !lv_is_active_exclusive(lvl->lv)) {
> +			if (!activate_lv(cmd, lvl->lv)) {
> +				log_error("Failed to activate %s", display_lvname(lvl->lv));
> +				return 0;
> +			}
> +		} else if (!activate_lv_excl(cmd, lvl->lv)) {
> +			log_error("Failed to activate %s", display_lvname(lvl->lv));
>   			dm_list_uniterate(lvh, lvs, &lvl->list) {
>   				lvl = dm_list_item(lvh, struct lv_list);
>   				if (!deactivate_lv(cmd, lvl->lv))
> diff --git a/lib/locking/locking.h b/lib/locking/locking.h
> index 47841ed..f2fbb00 100644
> --- a/lib/locking/locking.h
> +++ b/lib/locking/locking.h
> @@ -262,6 +262,6 @@ int sync_dev_names(struct cmd_context* cmd);
>   
>   /* Process list of LVs */
>   struct volume_group;
> -int activate_lvs(struct cmd_context *cmd, struct dm_list *lvs);
> +int activate_lvs(struct cmd_context *cmd, struct dm_list *lvs, unsigned exclusive);
>   
>   #endif
> diff --git a/tools/pvmove.c b/tools/pvmove.c
> index b3d1d89..cbd5cb8 100644
> --- a/tools/pvmove.c
> +++ b/tools/pvmove.c
> @@ -64,6 +64,16 @@ static int _pvmove_target_present(struct cmd_context *cmd, int clustered)
>   	return found;
>   }
>   
> +static unsigned _pvmove_is_exclusive(struct cmd_context *cmd,
> +				     struct volume_group *vg)
> +{
> +	if (vg_is_clustered(vg))
> +		if (!_pvmove_target_present(cmd, 1))
> +			return 1;
> +
> +	return 0;
> +}
> +
>   /* Allow /dev/vgname/lvname, vgname/lvname or lvname */
>   static const char *_extract_lvname(struct cmd_context *cmd, const char *vgname,
>   				   const char *arg)
> @@ -320,7 +330,8 @@ static struct logical_volume *_set_up_pvmove_lv(struct cmd_context *cmd,
>   						const char *lv_name,
>   						struct dm_list *allocatable_pvs,
>   						alloc_policy_t alloc,
> -						struct dm_list **lvs_changed)
> +						struct dm_list **lvs_changed,
> +						unsigned *exclusive)
>   {
>   	struct logical_volume *lv_mirr, *lv;
>   	struct lv_segment *seg;
> @@ -329,6 +340,8 @@ static struct logical_volume *_set_up_pvmove_lv(struct cmd_context *cmd,
>   	uint32_t log_count = 0;
>   	int lv_found = 0;
>   	int lv_skipped = 0;
> +	int lv_active_count = 0;
> +	int lv_exclusive_count = 0;
>   
>   	/* FIXME Cope with non-contiguous => splitting existing segments */
>   	if (!(lv_mirr = lv_create_empty("pvmove%d", NULL,
> @@ -422,33 +435,54 @@ static struct logical_volume *_set_up_pvmove_lv(struct cmd_context *cmd,
>   		if (!lv_is_on_pvs(lv, source_pvl))
>   			continue;
>   
> -		seg = first_seg(lv);
> -		if (seg_is_raid(seg) || seg_is_mirrored(seg) ||
> -		    lv_is_thin_volume(lv) || lv_is_thin_pool(lv)) {
> -			/*
> -			 * Pass over top-level LVs - they were handled.
> -			 * Allow sub-LVs to proceed.
> -			 */
> -			continue;
> -		}
> -
>   		if (lv_is_locked(lv)) {
>   			lv_skipped = 1;
>   			log_print_unless_silent("Skipping locked LV %s.", display_lvname(lv));
>   			continue;
>   		}
>   
> -		if (vg_is_clustered(vg) &&
> -		    lv_is_visible(lv) &&
> -		    lv_is_active(lv) &&
> -		    !lv_is_active_exclusive_locally(lv)) {
> -			lv_skipped = 1;
> -			log_print_unless_silent("Skipping LV %s which is active, "
> -						"but not locally exclusively.",
> -						display_lvname(lv));
> -			continue;
> +		if (vg_is_clustered(vg) && lv_is_visible(lv)) {
> +			if (lv_is_active_exclusive_locally(lv)) {
> +				if (lv_active_count) {
> +					log_error("Cannot move in clustered VG %s "
> +						  "if some LVs are activated "
> +						  "exclusively while others don't.",
> +						  vg->name);
> +					return NULL;
> +				}
> +
> +				lv_exclusive_count++;
> +			} else if (lv_is_active(lv)) {
> +				if (seg_only_exclusive(first_seg(lv))) {
> +					lv_skipped = 1;
> +					log_print_unless_silent("Skipping LV %s which is active, "
> +								"but not locally exclusively.",
> +							display_lvname(lv));
> +					continue;
> +				}
> +
> +				if (*exclusive) {
> +					log_error("Cannot move in clustered VG %s, "
> +						  "clustered mirror (cmirror) not detected "
> +						  "and LVs are activated non-exclusively.",
> +						  vg->name);
> +					return NULL;
> +				}
> +
> +				lv_active_count++;
> +			}
>   		}
>   
> +		seg = first_seg(lv);
> +		if (seg_is_raid(seg) || seg_is_mirrored(seg) ||
> +		    seg_is_cache(seg) || seg_is_cache_pool(seg) ||
> +		    seg_is_thin(seg) || seg_is_thin_pool(seg))
> +			/*
> +			 * Pass over top-level LVs - they were handled.
> +			 * Allow sub-LVs to proceed.
> +			 */
> +			continue;
> +
>   		if (!_insert_pvmove_mirrors(cmd, lv_mirr, source_pvl, lv,
>   					    *lvs_changed))
>   			return_NULL;
> @@ -483,15 +517,35 @@ static struct logical_volume *_set_up_pvmove_lv(struct cmd_context *cmd,
>   		return NULL;
>   	}
>   
> +	if (lv_exclusive_count)
> +		*exclusive = 1;
> +
>   	return lv_mirr;
>   }
>   
> +static int _activate_lv(struct cmd_context *cmd, struct logical_volume *lv_mirr,
> +			unsigned exclusive)
> +{
> +	int r = 0;
> +
> +	if (exclusive || lv_is_active_exclusive(lv_mirr))
> +		r = activate_lv_excl(cmd, lv_mirr);
> +	else
> +		r = activate_lv(cmd, lv_mirr);
> +
> +	if (!r)
> +		stack;
> +
> +	return r;
> +}
> +
>   /*
>    * Called to set up initial pvmove LV only.
>    * (Not called after first or any other section completes.)
>    */
>   static int _update_metadata(struct logical_volume *lv_mirr,
> -			    struct dm_list *lvs_changed)
> +			    struct dm_list *lvs_changed,
> +			    unsigned exclusive)
>   {
>   	struct lv_list *lvl;
>   	struct logical_volume *lv = lv_mirr;
> @@ -505,7 +559,7 @@ static int _update_metadata(struct logical_volume *lv_mirr,
>                   return_0;
>   
>   	/* Ensure mirror LV is active */
> -	if (!activate_lv_excl_local(lv_mirr->vg->cmd, lv_mirr)) {
> +	if (!_activate_lv(lv_mirr->vg->cmd, lv_mirr, exclusive)) {
>   		if (test_mode())
>   			return 1;
>   
> @@ -548,6 +602,7 @@ static int _pvmove_setup_single(struct cmd_context *cmd,
>   	struct logical_volume *lv = NULL;
>   	const char *pv_name = pv_dev_name(pv);
>   	unsigned flags = PVMOVE_FIRST_TIME;
> +	unsigned exclusive;
>   	int r = ECMD_FAILED;
>   
>   	pp->found_pv = 1;
> @@ -594,6 +649,8 @@ static int _pvmove_setup_single(struct cmd_context *cmd,
>   		}
>   	}
>   
> +	exclusive = _pvmove_is_exclusive(cmd, vg);
> +
>   	if ((lv_mirr = find_pvmove_lv(vg, pv_dev(pv), PVMOVE))) {
>   		log_print_unless_silent("Detected pvmove in progress for %s.", pv_name);
>   		if (pp->pv_count || lv_name)
> @@ -605,7 +662,7 @@ static int _pvmove_setup_single(struct cmd_context *cmd,
>   		}
>   
>   		/* Ensure mirror LV is active */
> -		if (!activate_lv_excl_local(cmd, lv_mirr)) {
> +		if (!_activate_lv(cmd, lv_mirr, exclusive)) {
>   			log_error("ABORTING: Temporary mirror activation failed.");
>   			goto out;
>   		}
> @@ -630,12 +687,12 @@ static int _pvmove_setup_single(struct cmd_context *cmd,
>   
>   		if (!(lv_mirr = _set_up_pvmove_lv(cmd, vg, source_pvl, lv_name,
>   						  allocatable_pvs, pp->alloc,
> -						  &lvs_changed)))
> +						  &lvs_changed, &exclusive)))
>   			goto_out;
>   	}
>   
>   	/* Lock lvs_changed and activate (with old metadata) */
> -	if (!activate_lvs(cmd, lvs_changed))
> +	if (!activate_lvs(cmd, lvs_changed, exclusive))
>   		goto_out;
>   
>   	/* FIXME Presence of a mirror once set PVMOVE - now remove associated logic */
> @@ -646,7 +703,7 @@ static int _pvmove_setup_single(struct cmd_context *cmd,
>   		goto out;
>   
>   	if (flags & PVMOVE_FIRST_TIME)
> -		if (!_update_metadata(lv_mirr, lvs_changed))
> +		if (!_update_metadata(lv_mirr, lvs_changed, exclusive))
>   			goto_out;
>   
>   	/* LVs are all in status LOCKED */
>
> --
> lvm-devel mailing list
> lvm-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/lvm-devel
>

master - pvmove: reinstantiate clustered pvmove

[Eric Ren]

> GDB it a little bit. The problem seems because:
>
> _pvmove_target_present(cmd, 1)
>
>
> will always return 0 - "not found".
>
> During one pvmove command, the _pvmove_target_present() is invoked 
> twice. At first call,
> "segtype->ops->target_present()", i.e _mirrored_target_present() will 
> set "_mirrored_checked = 0".

Ooops, s/"_mirrored_checked = 0"/"_mirrored_checked = 1"

Sorry.

Eric

master - pvmove: reinstantiate clustered pvmove

[Zdenek Kabelac]

Dne 7.2.2018 v 08:17 Eric Ren napsal(a):
> Hello Zdenek,
> 
> I've tried this patch with clvmd and cmirrord running, and all LVs in 
> clustered VG being activated
> on both nodes. But, pvmove still cannot work as expect - move data on 
> underlying PV of the the
> non-exclusive activated LV.
> 
> ==========
> tw1:~ # pgrep -a mirrord
> 11931 cmirrord
> tw1:~ # pgrep -a clvmd
> 11748 /usr/sbin/clvmd -T90 -d0
> 
> tw1:~ # vgs -o+vg_clustered vgtest2
>  ??? VG????? #PV #LV #SN Attr?? VSize VFree Clustered
>  ??? vgtest2?? 2?? 2?? 0 wz--nc 9.30g 6.30g? clustered
> tw1:~ # lvs -o+lv_active_exclusively,lv_active_locally vgtest2
>  ??? LV?? VG????? Attr?????? LSize Pool Origin Data%? Meta%? Move Log Cpy%Sync 
> Convert ActExcl??? ActLocal
>  ??? lv1? vgtest2 -wi-a----- 2.00g active locally
>  ??? lv2? vgtest2 -wi-a----- 1.00g active locally
> tw1:~ # pvs -S vg_name=vgtest2
>  ??? PV???????? VG????? Fmt? Attr PSize PFree
>  ??? /dev/vdb1? vgtest2 lvm2 a--? 4.65g 4.65g
>  ??? /dev/vdb2? vgtest2 lvm2 a--? 4.65g 1.65g
> 
> tw1:~ # pvmove /dev/vdb2
>  ??? Cannot move in clustered VG vgtest2, clustered mirror (cmirror) not 
> detected and LVs are activated non-exclusively.
> ============
> 
> 
> GDB it a little bit. The problem seems because:
> 
> _pvmove_target_present(cmd, 1)
> 
> 
> will always return 0 - "not found".
> 
> During one pvmove command, the _pvmove_target_present() is invoked twice. At 
> first call,
> "segtype->ops->target_present()", i.e _mirrored_target_present() will set 
> "_mirrored_checked = 0".
> 
> At the second call, _mirrored_target_present() will not go through the 
> following code to get the
> "_mirror_attributes":
> 


Hi

I think I've intentionally kept away locally active LVs,
but in your case where LV is locally active just on a single node,
it still possible we can use such LV for pvmove - although during
pvmove 'restart' it  will be only  'exclusively' activated.

I'll try to think how to 'integrate' back support for  locally
active LVs on a single node back as well.


Regards

Zdenk

master - pvmove: reinstantiate clustered pvmove

[Eric Ren]

Hi Zdenek,

Thanks for your response :)

On 02/08/2018 12:49 AM, Zdenek Kabelac wrote:
> Dne 7.2.2018 v 08:17 Eric Ren napsal(a):
>> Hello Zdenek,
>>
>> I've tried this patch with clvmd and cmirrord running, and all LVs in 
>> clustered VG being activated
>> on both nodes. But, pvmove still cannot work as expect - move data on 
>> underlying PV of the the
>> non-exclusive activated LV.
>>
>> ==========
>> tw1:~ # pgrep -a mirrord
>> 11931 cmirrord
>> tw1:~ # pgrep -a clvmd
>> 11748 /usr/sbin/clvmd -T90 -d0
>>
>> tw1:~ # vgs -o+vg_clustered vgtest2
>> ???? VG????? #PV #LV #SN Attr?? VSize VFree Clustered
>> ???? vgtest2?? 2?? 2?? 0 wz--nc 9.30g 6.30g? clustered
>> tw1:~ # lvs -o+lv_active_exclusively,lv_active_locally vgtest2
>> ???? LV?? VG????? Attr?????? LSize Pool Origin Data%? Meta% Move Log 
>> Cpy%Sync Convert ActExcl??? ActLocal
>> ???? lv1? vgtest2 -wi-a----- 2.00g active locally
>> ???? lv2? vgtest2 -wi-a----- 1.00g active locally
>> tw1:~ # pvs -S vg_name=vgtest2
>> ???? PV???????? VG????? Fmt? Attr PSize PFree
>> ???? /dev/vdb1? vgtest2 lvm2 a--? 4.65g 4.65g
>> ???? /dev/vdb2? vgtest2 lvm2 a--? 4.65g 1.65g
>>
>> tw1:~ # pvmove /dev/vdb2
>> ???? Cannot move in clustered VG vgtest2, clustered mirror (cmirror) 
>> not detected and LVs are activated non-exclusively.
>> ============
>>
>>
>> GDB it a little bit. The problem seems because:
>>
>> _pvmove_target_present(cmd, 1)
>>
>>
>> will always return 0 - "not found".
>>
>> During one pvmove command, the _pvmove_target_present() is invoked 
>> twice. At first call,
>> "segtype->ops->target_present()", i.e _mirrored_target_present() will 
>> set "_mirrored_checked = 0".
>>
>> At the second call, _mirrored_target_present() will not go through 
>> the following code to get the
>> "_mirror_attributes":
>>
>
>
> Hi
>
> I think I've intentionally kept away locally active LVs,

You mean so far pvmove can only work like this:

- pvmove on the node that the LV is _not_active, but the LV can
 ?be active on another node, so that users will not suffer downtime
 ?issue

Do I understand it right?

But it still cannot work in such case that doing pvmove the inactive-LV 
node.

====
tw1:~ # lvs
 ??? LV?? VG????? Attr?????? LSize Pool Origin Data%? Meta%? Move Log 
Cpy%Sync Convert
 ??? lv1? vgtest2 -wi------- 2.00g
 ??? lv2? vgtest2 -wi------- 1.00g
tw2:~ # lvs
 ??? LV?? VG????? Attr?????? LSize Pool Origin Data%? Meta%? Move Log 
Cpy%Sync Convert
 ??? lv1? vgtest2 -wi-a----- 2.00g
 ??? lv2? vgtest2 -wi-a----- 1.00g

tw1:~ # pvmove /dev/vdb1
 ??? Cannot move in clustered VG vgtest2, clustered mirror (cmirror) not 
detected and LVs are activated non-exclusively.
====

It even cannot work on the node that the LV is exclusively activated:

====
tw1:~ # vgchange -aly vgtest2
 ??? 2 logical volume(s) in volume group "vgtest2" now active
tw2:~ # lvs
 ??? LV?? VG????? Attr?????? LSize Pool Origin Data%? Meta%? Move Log 
Cpy%Sync Convert
 ??? lv1? vgtest2 -wi------- 2.00g
 ??? lv2? vgtest2 -wi------- 1.00g

tw1:~ # lvs
 ??? LV?? VG????? Attr?????? LSize Pool Origin Data%? Meta%? Move Log 
Cpy%Sync Convert
 ??? lv1? vgtest2 -wi-a----- 2.00g
 ??? lv2? vgtest2 -wi-a----- 1.00g
tw1:~ # pvmove /dev/vdb1
 ??? Cannot move in clustered VG vgtest2, clustered mirror (cmirror) not 
detected and LVs are activated non-exclusively.
====


> but in your case where LV is locally active just on a single node,

Actually, the LVs in vgtest2 are active on both of two nodes:

====
tw1:~ # lvs
 ??? LV?? VG????? Attr?????? LSize Pool Origin Data%? Meta%? Move Log 
Cpy%Sync Convert
 ??? lv1? vgtest2 -wi-a----- 2.00g
 ??? lv2? vgtest2 -wi-a----- 1.00g
tw1:~ # lvs 
-o+lv_active_exclusively,lv_active_locally,lv_active_remotely vgtest2
 ??? LV?? VG????? Attr?????? LSize Pool Origin Data%? Meta%? Move Log 
Cpy%Sync Convert ActExcl??? ActLocal?????? ActRemote
 ??? lv1? vgtest2 -wi-a----- 2.00g active locally??? unknown
 ??? lv2? vgtest2 -wi-a----- 1.00g active locally??? unknown

tw2:~ # lvs vgtest2
 ??? LV?? VG????? Attr?????? LSize Pool Origin Data%? Meta%? Move Log 
Cpy%Sync Convert
 ??? lv1? vgtest2 -wi-a----- 2.00g
 ??? lv2? vgtest2 -wi-a----- 1.00g
tw2:~ # lvs 
-o+lv_active_exclusively,lv_active_locally,lv_active_remotely vgtest2
 ??? LV?? VG????? Attr?????? LSize Pool Origin Data%? Meta%? Move Log 
Cpy%Sync Convert ActExcl??? ActLocal?????? ActRemote
 ??? lv1? vgtest2 -wi-a----- 2.00g active locally??? unknown
 ??? lv2? vgtest2 -wi-a----- 1.00g
====

But, I don't know why the "ActRemote" is "unknown" :)

> it still possible we can use such LV for pvmove - although during
> pvmove 'restart' it? will be only? 'exclusively' activated.

Yes, I also noticed this interesting behavior - I'm doubt that it might 
bring
trouble in HA cluster if cluster FS is sitting on that LV.

>
> I'll try to think how to 'integrate' back support for? locally
> active LVs on a single node back as well.

Sorry, I'm little puzzled that what is our expectation scenarios that 
pvmove can be used :-/

You mean pvmove can only support for concurrent LV activation on 
multiple nodes so far?
But, it seems not working on my setup as described above :)

Thanks,
Eric

master - pvmove: reinstantiate clustered pvmove

[Zdenek Kabelac]

Dne 8.2.2018 v 02:45 Eric Ren napsal(a):
> Hi Zdenek,
> 
> Thanks for your response :)
> 
> On 02/08/2018 12:49 AM, Zdenek Kabelac wrote:
>> Dne 7.2.2018 v 08:17 Eric Ren napsal(a):
>>> Hello Zdenek,
>>>
>>> I've tried this patch with clvmd and cmirrord running, and all LVs in 
>>> clustered VG being activated
>>> on both nodes. But, pvmove still cannot work as expect - move data on 
>>> underlying PV of the the
>>> non-exclusive activated LV.
>>>
>>>
>>> will always return 0 - "not found".
>>>
>>> During one pvmove command, the _pvmove_target_present() is invoked twice. 
>>> At first call,
>>> "segtype->ops->target_present()", i.e _mirrored_target_present() will set 
>>> "_mirrored_checked = 0".
>>>
>>> At the second call, _mirrored_target_present() will not go through the 
>>> following code to get the
>>> "_mirror_attributes":
>>>
>>
>>
>> Hi
>>
>> I think I've intentionally kept away locally active LVs,
> 
> You mean so far pvmove can only work like this:
> 
> - pvmove on the node that the LV is _not_active, but the LV can
>  ?be active on another node, so that users will not suffer downtime
>  ?issue


The problem with just locally active LVs is - they can be active on any node.

Current logic of PV knows only 2 states:

1. Either set of LVs is active exclusively - then pvmove can run in exclusive 
(no-clustered) mode locally on 1 node.

2. LVs are active on all nodes - then you need cluster-wide pvmove.


---


But then there is 'fuzzy' world - where LVs are active non-exclusively - but 
we don't exactly know on how many nodes (we can count them - but it's not 
currently done this way)

So in this fuzzy world -  lvm2 previously actually tried to active those LVs 
everywhere - but this was not correct - since if user didn't activated LV
on node X - lvm2 should not active LV there just because of pvmove.

> 
> Do I understand it right?
> 
> But it still cannot work in such case that doing pvmove the inactive-LV node.


Yep - current lvm2 logic does not allow  pvmoving extents for inactive  LV 
segments.

My long-term plan is to actually add support for this - as this would 
effectively resolve several problem - but it's very complicated rework
internally.


> 
> It even cannot work on the node that the LV is exclusively activated:
> 
> ====
> tw1:~ # vgchange -aly vgtest2
>  ??? 2 logical volume(s) in volume group "vgtest2" now active
> tw2:~ # lvs
>  ??? LV?? VG????? Attr?????? LSize Pool Origin Data%? Meta%? Move Log Cpy%Sync 
> Convert
>  ??? lv1? vgtest2 -wi------- 2.00g
>  ??? lv2? vgtest2 -wi------- 1.00g
> 
> tw1:~ # lvs
>  ??? LV?? VG????? Attr?????? LSize Pool Origin Data%? Meta%? Move Log Cpy%Sync 
> Convert
>  ??? lv1? vgtest2 -wi-a----- 2.00g
>  ??? lv2? vgtest2 -wi-a----- 1.00g
> tw1:~ # pvmove /dev/vdb1
>  ??? Cannot move in clustered VG vgtest2, clustered mirror (cmirror) not 
> detected and LVs are activated non-exclusively.


You really need to use  'exclusively' activated LVs.

-aly is just a local activation (so any node can grab one)

-aey is your wanted exclusive activation you should actually use here.

Also note - many target type can by activated only exclusively anyway - so 
they do take exclusive lock for 'local' one anyway.

So it's just  linear/strip + mirror where you can play with real
cluster-wide multinode activation.

>> it still possible we can use such LV for pvmove - although during
>> pvmove 'restart' it? will be only? 'exclusively' activated.
> 
> Yes, I also noticed this interesting behavior - I'm doubt that it might bring
> trouble in HA cluster if cluster FS is sitting on that LV.

Main question here I'd have is -

Why do you actually need to use  -aly for this case instead of  -aey ??


Solving '-aly' properly is not very simple - I'll need to think about
tree activation logic here for a while.

So is there a case in your  HA setup where you need to activate
at the same time  LV on both nodes with -aly ?
(since clearly  -aey will not let you do this).


Zdenek

master - pvmove: reinstantiate clustered pvmove

[Eric Ren]

Hi Zdeneck,

Thanks for your kind response!
>> You mean so far pvmove can only work like this:
>>
>> - pvmove on the node that the LV is _not_active, but the LV can
>> ??be active on another node, so that users will not suffer downtime
>> ??issue
>
>
> The problem with just locally active LVs is - they can be active on 
> any node.
>
> Current logic of PV knows only 2 states:
>
> 1. Either set of LVs is active exclusively - then pvmove can run in 
> exclusive (no-clustered) mode locally on 1 node.

Got it. My fault! I meant to use 'vgchange -aey', not 'vgchange -aly'. 
Yeah, it works:

tw1:~ # vgchange -aey vgtest2
 ??? 2 logical volume(s) in volume group "vgtest2" now active
tw1:~ # pvmove /dev/vdb1
 ??? Increasing mirror region size from 0??? to 2.00 KiB
 ??? /dev/vdb1: Moved: 4.17%
 ??? /dev/vdb1: Moved: 38.02%
 ??? /dev/vdb1: Moved: 66.67%
 ??? /dev/vdb1: Moved: 79.56%
 ??? /dev/vdb1: Moved: 100.00%


>
> 2. LVs are active on all nodes - then you need cluster-wide pvmove.

This is the one that seems not work properly for me. As said in my first 
reply, it might be a bug:

"""
GDB it a little bit. The problem seems because:

_pvmove_target_present(cmd, 1)


will always return 0 - "not found".

During one pvmove command, the _pvmove_target_present() is invoked 
twice. At first call,
"segtype->ops->target_present()", i.e _mirrored_target_present() will 
set "_mirrored_checked = 1".

At the second call, _mirrored_target_present() will _not_ go through the 
following code to get the
"_mirror_attributes":
...
even if it is asked to back the "target attributes" by 
_pvmove_target_present(cmd, 1).
As result, _pvmove_target_present(cmd, 1) will always return "0", 
because the "attributes"
is empty.
"""

Could you please have a check there?

Either I used "vgchange -ay vgtest" or "vgchange -asy vgtest" to active 
the LVs on all nodes. It just doesn't work:

"""
tw1:~ # vgchange -asy vgtest2
 ??? 2 logical volume(s) in volume group "vgtest2" now active
tw1:~ # lvs
 ??? LV?? VG????? Attr?????? LSize Pool Origin Data%? Meta%? Move Log 
Cpy%Sync Convert
 ??? lv1? vgtest2 -wi-a----- 2.00g
 ??? lv2? vgtest2 -wi-a----- 1.00g
tw1:~ # pvmove /dev/vdb1
 ??? No data to move for vgtest2.
tw1:~ # pvmove /dev/vdb2
 ??? Cannot move in clustered VG vgtest2, clustered mirror (cmirror) not 
detected and LVs are activated non-exclusively.
tw2:~ # lvs
 ??? LV?? VG????? Attr?????? LSize Pool Origin Data%? Meta%? Move Log 
Cpy%Sync Convert
 ??? lv1? vgtest2 -wi-a----- 2.00g
 ??? lv2? vgtest2 -wi-a----- 1.00g
"""

or

"""
tw1:~ # vgchange -an vgtest2
 ??? 0 logical volume(s) in volume group "vgtest2" now active
tw1:~ # vgchange -ay vgtest2
 ??? 2 logical volume(s) in volume group "vgtest2" now active
tw1:~ # lvs
 ??? LV?? VG????? Attr?????? LSize Pool Origin Data%? Meta%? Move Log 
Cpy%Sync Convert
 ??? lv1? vgtest2 -wi-a----- 2.00g
 ??? lv2? vgtest2 -wi-a----- 1.00g
tw1:~ # pvmove /dev/vdb2
 ??? Cannot move in clustered VG vgtest2, clustered mirror (cmirror) not 
detected and LVs are activated non-exclusively.


tw2:~ # lvs
 ??? LV?? VG????? Attr?????? LSize Pool Origin Data%? Meta%? Move Log 
Cpy%Sync Convert
 ??? lv1? vgtest2 -wi-a----- 2.00g
 ??? lv2? vgtest2 -wi-a----- 1.00g
"""

>
>
> ---
>
>
> But then there is 'fuzzy' world - where LVs are active non-exclusively 
> - but we don't exactly know on how many nodes (we can count them - but 
> it's not currently done this way)
>
> So in this fuzzy world -? lvm2 previously actually tried to active 
> those LVs everywhere - but this was not correct - since if user didn't 
> activated LV
> on node X - lvm2 should not active LV there just because of pvmove.

Got it.

>
>>
>> Do I understand it right?
>>
>> But it still cannot work in such case that doing pvmove the 
>> inactive-LV node.
>
>
> Yep - current lvm2 logic does not allow? pvmoving extents for 
> inactive? LV segments.
>
> My long-term plan is to actually add support for this - as this would 
> effectively resolve several problem - but it's very complicated rework
> internally.

Got it, thanks!

>
>
>>
>> It even cannot work on the node that the LV is exclusively activated:
>> ...
>
> You really need to use? 'exclusively' activated LVs.
>
> -aly is just a local activation (so any node can grab one)
>
> -aey is your wanted exclusive activation you should actually use here.

Yes, I meant -aey, sorry!

>
> Also note - many target type can by activated only exclusively anyway 
> - so they do take exclusive lock for 'local' one anyway.
>
> So it's just? linear/strip + mirror where you can play with real
> cluster-wide multinode activation.

I know as you've told me on IRC times ago, thanks.

>
>>> it still possible we can use such LV for pvmove - although during
>>> pvmove 'restart' it? will be only? 'exclusively' activated.
>>
>> Yes, I also noticed this interesting behavior - I'm doubt that it 
>> might bring
>> trouble in HA cluster if cluster FS is sitting on that LV.
>
> Main question here I'd have is -
>
> Why do you actually need to use? -aly for this case instead of -aey ??

Actually, I am expecting pvmove can be used on LVs that are active on 
all nodes, as
you also mentioned above. "-aly" is my mistake, sorry :)

>
>
> Solving '-aly' properly is not very simple - I'll need to think about
> tree activation logic here for a while.
>
> So is there a case in your? HA setup where you need to activate
> at the same time? LV on both nodes with -aly ?
> (since clearly? -aey will not let you do this).

No, It's not what I am pursuing here. What I expect is:

there should be no downtime during pvmove when I use cluster FS on the LV.

Thanks a lot!
Eric

master - pvmove: reinstantiate clustered pvmove

[Zdenek Kabelac]

Dne 9.2.2018 v 03:21 Eric Ren napsal(a):
> Hi Zdeneck,
> 
> Thanks for your kind response!
>>> You mean so far pvmove can only work like this:
>>>
>>> - pvmove on the node that the LV is _not_active, but the LV can
>>> ??be active on another node, so that users will not suffer downtime
>>> ??issue
>>
>>
>> The problem with just locally active LVs is - they can be active on any node.
>>
>> Current logic of PV knows only 2 states:
>>
>> 1. Either set of LVs is active exclusively - then pvmove can run in 
>> exclusive (no-clustered) mode locally on 1 node.
> 
> Got it. My fault! I meant to use 'vgchange -aey', not 'vgchange -aly'. Yeah, 
> it works:
> 
> tw1:~ # vgchange -aey vgtest2
>  ??? 2 logical volume(s) in volume group "vgtest2" now active
> tw1:~ # pvmove /dev/vdb1
>  ??? Increasing mirror region size from 0??? to 2.00 KiB

^^^^ this is actually somewhat suspicious.... I'll need to check how that 
happens  - it should be at least 1/2 MiB by default I think.


>  ??? /dev/vdb1: Moved: 4.17%
>  ??? /dev/vdb1: Moved: 38.02%
>  ??? /dev/vdb1: Moved: 66.67%
>  ??? /dev/vdb1: Moved: 79.56%
>  ??? /dev/vdb1: Moved: 100.00%
> 
> 
>>
>> 2. LVs are active on all nodes - then you need cluster-wide pvmove.
> 
> This is the one that seems not work properly for me. As said in my first 
> reply, it might be a bug:
> 
> """
> GDB it a little bit. The problem seems because:
> 
> _pvmove_target_present(cmd, 1)
> 
> 
> will always return 0 - "not found".


Do you have 'cmirrord' properly configured & running ?

Can you capture log from this tool (i.e. running in foreground and grabbing 
its output)


>> But then there is 'fuzzy' world - where LVs are active non-exclusively - but 
>> we don't exactly know on how many nodes (we can count them - but it's not 
>> currently done this way)
>>
>> So in this fuzzy world -? lvm2 previously actually tried to active those LVs 
>> everywhere - but this was not correct - since if user didn't activated LV
>> on node X - lvm2 should not active LV there just because of pvmove.
> 
> Got it.

We did some more 'brainstorming' - with a result, that it actually doesn't
matter if LV is activate on more nodes - as long as there is cmirrord running,
and lvm2 is able to activate LV on multiple nodes - it should be always
possible to create proper tree mapping.

But let's just see how it will work in reality - it will require some more 
thinking....

>> Why do you actually need to use? -aly for this case instead of -aey ??
> 
> Actually, I am expecting pvmove can be used on LVs that are active on all 
> nodes, as
> you also mentioned above. "-aly" is my mistake, sorry :)

This case should 'mostly' work - except there are still bugs in proper 
recognizing which LVs can be processed in which mode.

Use may need to run 2 pvmoves - one for  cluster-wide active LVs,
and 2nd. for exclusively activated ones - we can't handle ATM both at once.

Zdenek

master - pvmove: reinstantiate clustered pvmove

[Eric Ren]

Hi Zdeneck,

I tested with LVM 2.02.177, and with the following patches backported:

 ? + 0001-pvmove-fix-_remove_sibling_pvs_from_trim_list.patch
 ? + 0002-pvmove-better-check-for-exclusive-LV.patch
 ? + 0003-pvmove-drop-misleading-pvmove-restriction-for-cluste.patch
 ? + 0004-cleanup-enhance-messages.patch
 ? + 0005-cleanup-drop-unused-code.patch
 ? + 0006-activation-guard-exclusive-activation.patch
 ? + 0007-activation-move-check-later.patch
 ? + 0008-pvmove-reinstantiate-clustered-pvmove.patch

So the problem might be only on my side, if your testing is fine with 
the latest
upstream. I am just waiting for the 2.02.178 to come out and try it again.

I will be on vacation for the coming Chinese New Year. I will try more 
carefully
as you suggested after coming back :)

On 02/09/2018 08:05 PM, Zdenek Kabelac wrote:
> Dne 9.2.2018 v 03:21 Eric Ren napsal(a):
>> Hi Zdeneck,
>>
>> Thanks for your kind response!
>>>> You mean so far pvmove can only work like this:
>>>>
>>>> - pvmove on the node that the LV is _not_active, but the LV can
>>>> ??be active on another node, so that users will not suffer downtime
>>>> ??issue
>>>
>>>
>>> The problem with just locally active LVs is - they can be active on 
>>> any node.
>>>
>>> Current logic of PV knows only 2 states:
>>>
>>> 1. Either set of LVs is active exclusively - then pvmove can run in 
>>> exclusive (no-clustered) mode locally on 1 node.
>>
>> Got it. My fault! I meant to use 'vgchange -aey', not 'vgchange 
>> -aly'. Yeah, it works:
>>
>> tw1:~ # vgchange -aey vgtest2
>> ???? 2 logical volume(s) in volume group "vgtest2" now active
>> tw1:~ # pvmove /dev/vdb1
>> ???? Increasing mirror region size from 0??? to 2.00 KiB
>
> ^^^^ this is actually somewhat suspicious.... I'll need to check how 
> that happens? - it should be at least 1/2 MiB by default I think.

In my lvm.conf:

""""
 ??? # For RAID or 'mirror' segment types, 'raid_region_size' is the
 ??? # size (in KiB) of each:
 ??? # - synchronization operation when initializing
 ??? # - each copy operation when performing a 'pvmove' (using 'mirror' 
segtype)
 ??? # This setting has replaced 'mirror_region_size' since version 2.02.99
 ??? raid_region_size = 512
""""

>
> Do you have 'cmirrord' properly configured & running ?

'cmirrord' and 'clvmd' are running, but I don't know how to configure 
'cmirrord':

"""
tw1:~ # pgrep -a cmirrord
11931 cmirrord
tw1:~ # pgrep -a clvmd
11748 /usr/sbin/clvmd -T90 -d0

"""

>
> Can you capture log from this tool (i.e. running in foreground and 
> grabbing its output)

Hmm, I had a quick testing:

Terminal 1, without anything ouput during pvmove:
"""
tw1:~ # cmirrord -f
Starting cmirrord:
"""

Terminal 2:
"""
tw1:~ # pvmove /dev/vdb2 -v
 ??? ? Cluster mirror log daemon not included in build.
 ??? ? Wiping internal VG cache
 ??? ? Wiping cache of LVM-capable devices
 ??? ? Skipping foreign volume group vgtest3
 ??? ? Archiving volume group "vgtest2" metadata (seqno 25).
 ??? ? Creating logical volume pvmove0
 ??? Cannot move in clustered VG vgtest2, clustered mirror (cmirror) not 
detected and LVs are activated non-exclusively.
"""

Hmm, what does "Cluster mirror log daemon not included in build." mean?

There should be two daemons: cmirrord and the cluster mirror log? I think
cmirrord is Cluster mirror log daemon?

Ah, I think I know what is going wrong... our package team splat 
lvm2.spec into lvm2.spec,
lvm2-clvm.spec and device-mapper.spec.

In lvm2.spec, they don't configure "--enable-cmirrord". Even though they 
have it in lvm2-clvm.spec,
they finally only ships the cluster-related binaries, and drop the main 
'lvm' binary built with "--enable-cmirrord".
I hate that hacking stuff, but...

Sorry for the noise, I will fix up it and try again. Thanks for your help!


>
>
>>> But then there is 'fuzzy' world - where LVs are active 
>>> non-exclusively - but we don't exactly know on how many nodes (we 
>>> can count them - but it's not currently done this way)
>>>
>>> So in this fuzzy world -? lvm2 previously actually tried to active 
>>> those LVs everywhere - but this was not correct - since if user 
>>> didn't activated LV
>>> on node X - lvm2 should not active LV there just because of pvmove.
>>
>> Got it.
>
> We did some more 'brainstorming' - with a result, that it actually 
> doesn't
> matter if LV is activate on more nodes - as long as there is cmirrord 
> running,
> and lvm2 is able to activate LV on multiple nodes - it should be always
> possible to create proper tree mapping.
>
> But let's just see how it will work in reality - it will require some 
> more thinking....
>
>>> Why do you actually need to use? -aly for this case instead of -aey ??
>>
>> Actually, I am expecting pvmove can be used on LVs that are active on 
>> all nodes, as
>> you also mentioned above. "-aly" is my mistake, sorry :)
>
> This case should 'mostly' work - except there are still bugs in proper 
> recognizing which LVs can be processed in which mode.
>
> Use may need to run 2 pvmoves - one for? cluster-wide active LVs,
> and 2nd. for exclusively activated ones - we can't handle ATM both at 
> once.

Thanks!

Regards,
Eric

master - pvmove: reinstantiate clustered pvmove

[Zdenek Kabelac]

> Got it.
>>
>> We did some more 'brainstorming' - with a result, that it actually doesn't
>> matter if LV is activate on more nodes - as long as there is cmirrord running,
>> and lvm2 is able to activate LV on multiple nodes - it should be always
>> possible to create proper tree mapping.
>>
>> But let's just see how it will work in reality - it will require some more 
>> thinking....
>>


Hi

I've pushed 2 new commits for pvmove and its clustered use-case.

So please give them some testing if you can spot something strange or if you 
think there could be some better logic behind it.

I've tried to accept wider range of active LVs - but it's for
further testing still whether it's got any better.

https://www.redhat.com/archives/lvm-devel/2018-February/msg00038.html
https://www.redhat.com/archives/lvm-devel/2018-February/msg00039.html

Regards

Zdenek

master - pvmove: reinstantiate clustered pvmove

[Eric Ren]

On 02/15/2018 09:20 PM, Zdenek Kabelac wrote:
>> Got it.
>>>
>>> We did some more 'brainstorming' - with a result, that it actually 
>>> doesn't
>>> matter if LV is activate on more nodes - as long as there is 
>>> cmirrord running,
>>> and lvm2 is able to activate LV on multiple nodes - it should be always
>>> possible to create proper tree mapping.
>>>
>>> But let's just see how it will work in reality - it will require 
>>> some more thinking....
>>>
>
>
> Hi
>
> I've pushed 2 new commits for pvmove and its clustered use-case.
>
> So please give them some testing if you can spot something strange or 
> if you think there could be some better logic behind it.

Hi Zdenek,

Thanks very much. I am just back from holiday today, will find time to 
have a try.

Regards,
Eric

>
> I've tried to accept wider range of active LVs - but it's for
> further testing still whether it's got any better.
>
> https://www.redhat.com/archives/lvm-devel/2018-February/msg00038.html
> https://www.redhat.com/archives/lvm-devel/2018-February/msg00039.html
>
> Regards
>
> Zdenek
>
> -- 
> lvm-devel mailing list
> lvm-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/lvm-devel
>