missing stack

This and prior 2 commits resolve Red Hat bz 252041:
Ability to vgsplit an active Volume Group where the split involves only inactive LVs

VERSION

@@ -1 +1 @@
-2.02.34-cvs (2008-04-10)
+2.02.35-cvs (2008-04-15)

WHATS_NEW

@@ -1,5 +1,16 @@
+Version 2.02.35 - 15th April 2008
+=================================
+  Drop cached VG metadata before and after committing changes to it.
+  Rename P_global to P_#global.
+  Don't attempt remote metadata backups of non-clustered VGs. (2.02.29)
+  Don't store fid in VG metadata cache to avoid clvmd segfault. (2.02.34)
+  Update vgsplit tests to verify loosening of active LV restriction.
+  Update vgsplit to only restrict split with active LVs involved in split.
+  Add lv_is_active() to determine whether an lv is active.
+
 Version 2.02.34 - 10th April 2008
 =================================
+  Improve preferred_names lvm.conf example.
   Fix vgdisplay 'Cur LV' field to match lvdisplay output.
   Fix lv_count report field to exclude hidden LVs.
   Add vg_is_clustered() helper function.

daemons/clvmd/clvmd-command.c

@@ -118,9 +118,11 @@ int do_command(struct local_client *client, struct clvm_header *msg, int msglen,
 		lockname = &args[2];
 		/* Check to see if the VG is in use by LVM1 */
 		status = do_check_lvm1(lockname);
-		/* P_global causes a cache refresh */
-		if (strcmp(lockname, "P_global") == 0)
+		/* P_#global causes a cache refresh */
+		if (strcmp(lockname, "P_#global") == 0)
 		      do_refresh_cache();
+		else if (strncmp(lockname, "P_", 2) == 0)
+			lvmcache_drop_metadata(lockname + 2);
 
 		break;
 

doc/example.conf

@@ -25,9 +25,8 @@ devices {
     # list of regular expressions in turn and the first match is used.
     preferred_names = [ ]
 
-    # If device-mapper multipath is used, more descriptive names might
-    # be preferred over the dm-N names:
-    # preferred_names = [ "^/dev/mapper/mpath" ]
+    # Try to avoid using undescriptive /dev/dm-N names, if present.
+    # preferred_names = [ "^/dev/mpath/", "^/dev/mapper/mpath", "^/dev/[hs]d" ]
 
     # A filter that tells LVM2 to only use a restricted set of devices.
     # The filter consists of an array of regular expressions.  These

lib/activate/activate.c

@@ -673,6 +673,38 @@ int lvs_in_vg_opened(const struct volume_group *vg)
 	return count;
 }
 
+/*
+ * Determine whether an LV is active locally or in a cluster.
+ * Assumes vg lock held.
+ * Returns:
+ * 0 - not active locally or on any node in cluster
+ * 1 - active either locally or some node in the cluster
+ */
+int lv_is_active(struct logical_volume *lv)
+{
+	if (_lv_active(lv->vg->cmd, lv, 0))
+		return 1;
+
+	if (!vg_is_clustered(lv->vg))
+		return 0;
+
+	/*
+	 * FIXME: Cluster does not report per-node LV activation status.
+	 * Currently the best we can do is try exclusive local activation.
+	 * If that succeeds, we know the LV is not active elsewhere in the
+	 * cluster.
+	 */
+	if (activate_lv_excl(lv->vg->cmd, lv)) {
+		deactivate_lv(lv->vg->cmd, lv);
+		return 0;
+	}
+
+	/*
+	 * Exclusive local activation failed so assume it is active elsewhere.
+	 */
+	return 1;
+}
+
 /*
  * Returns 0 if an attempt to (un)monitor the device failed.
  * Returns 1 otherwise.

lib/activate/activate.h

@@ -91,6 +91,7 @@ int lvs_in_vg_activated(struct volume_group *vg);
 int lvs_in_vg_activated_by_uuid_only(struct volume_group *vg);
 int lvs_in_vg_opened(const struct volume_group *vg);
 
+int lv_is_active(struct logical_volume *lv);
 
 int monitor_dev_for_events(struct cmd_context *cmd,
 			    struct logical_volume *lv, int do_reg);

lib/cache/lvmcache.c

@@ -63,7 +63,6 @@ static void _free_cached_vgmetadata(struct lvmcache_vginfo *vginfo)
 	dm_free(vginfo->vgmetadata);
 
 	vginfo->vgmetadata = NULL;
-	vginfo->fid = NULL;
 
 	log_debug("Metadata cache: VG %s wiped.", vginfo->vgname);
 }
@@ -81,7 +80,6 @@ static void _store_metadata(struct lvmcache_vginfo *vginfo,
 		return;
 	}
 
-	vginfo->fid = vg->fid;
 	vginfo->precommitted = precommitted;
 
 	log_debug("Metadata cache: VG %s stored (%d bytes%s).", vginfo->vgname,
@@ -132,6 +130,16 @@ static void _update_cache_lock_state(const char *vgname, int locked)
 	_update_cache_vginfo_lock_state(vginfo, locked);
 }
 
+void lvmcache_drop_metadata(const char *vgname)
+{
+	struct lvmcache_vginfo *vginfo;
+
+	if (!(vginfo = vginfo_from_vgname(vgname, NULL)))
+		return;
+
+	_free_cached_vgmetadata(vginfo);
+}
+
 void lvmcache_lock_vgname(const char *vgname, int read_only __attribute((unused)))
 {
 	if (!_lock_hash && !lvmcache_init()) {
@@ -400,6 +408,7 @@ struct volume_group *lvmcache_get_vg(const char *vgid, unsigned precommitted)
 {
 	struct lvmcache_vginfo *vginfo;
 	struct volume_group *vg;
+	struct format_instance *fid;
 
 	if (!vgid || !(vginfo = vginfo_from_vgid(vgid)) || !vginfo->vgmetadata)
 		return NULL;
@@ -411,7 +420,12 @@ struct volume_group *lvmcache_get_vg(const char *vgid, unsigned precommitted)
 	    (!precommitted && vginfo->precommitted))
 		return NULL;
 
-	if (!(vg = import_vg_from_buffer(vginfo->vgmetadata, vginfo->fid)) ||
+	if (!(fid =  vginfo->fmt->ops->create_instance(vginfo->fmt,
+						       vginfo->vgname,
+						       vgid, NULL)))
+		return_NULL;
+
+	if (!(vg = import_vg_from_buffer(vginfo->vgmetadata, fid)) ||
 	    !vg_validate(vg)) {
 		_free_cached_vgmetadata(vginfo);
 		return_NULL;

lib/cache/lvmcache.h

@@ -45,7 +45,6 @@ struct lvmcache_vginfo {
 	struct lvmcache_vginfo *next; /* Another VG with same name? */
 	char *creation_host;
 	char *vgmetadata;	/* Copy of VG metadata as format_text string */
-	struct format_instance *fid;	/* fid associated with vgmetadata */
 	unsigned precommitted;	/* Is vgmetadata live or precommitted? */
 };
 
@@ -112,5 +111,6 @@ struct list *lvmcache_get_pvids(struct cmd_context *cmd, const char *vgname,
 
 /* Returns cached volume group metadata. */
 struct volume_group *lvmcache_get_vg(const char *vgid, unsigned precommitted);
+void lvmcache_drop_metadata(const char *vgname);
 
 #endif

lib/datastruct/list.c

@@ -65,6 +65,15 @@ void list_del(struct list *elem)
 	elem->p->n = elem->n;
 }
 
+/*
+ * Remove an element from existing list and insert before 'head'.
+ */
+void list_move(struct list *head, struct list *elem)
+{
+        list_del(elem);
+        list_add(head, elem);
+}
+
 /*
  * Is the list empty?
  */

lib/datastruct/list.h

@@ -55,14 +55,9 @@ void list_add_h(struct list *head, struct list *elem);
 void list_del(struct list *elem);
 
 /*
- * Move an element from an existing list to list 'head'.
- * Insert the element before 'head'.
+ * Remove an element from existing list and insert before 'head'.
  */
-static inline void list_move(struct list *item, struct list *head)
-{
-        list_del(item);
-        list_add(head, item);
-}
+void list_move(struct list *head, struct list *elem);
 
 /*
  * Is the list empty?

lib/display/display.c

@@ -670,7 +670,7 @@ void vgdisplay_colons(const struct volume_group *vg)
 		active_pvs = vg->pv_count;
 
 	list_iterate_items(lvl, &vg->lvs)
-		if (lv_is_visible(lvl->lv) || (lvl->lv->status & SNAPSHOT))
+		if (lv_is_visible(lvl->lv) && !(lvl->lv->status & SNAPSHOT))
 			lv_count++;
 
 	switch (vg->status & (LVM_READ | LVM_WRITE)) {

lib/locking/cluster_locking.c

@@ -389,8 +389,8 @@ int lock_resource(struct cmd_context *cmd, const char *resource, uint32_t flags)
 		/* If the VG name is empty then lock the unused PVs */
 		if (!*resource)	/* FIXME Deprecated */
 			dm_snprintf(lockname, sizeof(lockname), "P_orphans");
-		else if (*resource == '#')
-			dm_snprintf(lockname, sizeof(lockname), "P_%s", resource + 1);
+		else if (*resource == '#' || (flags & LCK_CACHE))
+			dm_snprintf(lockname, sizeof(lockname), "P_%s", resource);
 		else
 			dm_snprintf(lockname, sizeof(lockname), "V_%s",
 				     resource);
@@ -438,8 +438,10 @@ int lock_resource(struct cmd_context *cmd, const char *resource, uint32_t flags)
 		return 0;
 	}
 
-	/* If we are unlocking a VG, then trigger remote metadata backups */
-	if (cluster_cmd == CLVMD_CMD_LOCK_VG && ((flags & LCK_TYPE_MASK) == LCK_UNLOCK)) {
+	/* If we are unlocking a clustered VG, then trigger remote metadata backups */
+	if (cluster_cmd == CLVMD_CMD_LOCK_VG &&
+	    ((flags & LCK_TYPE_MASK) == LCK_UNLOCK) &&
+	    (flags & LCK_CLUSTER_VG)) {
 		log_very_verbose("Requesing backup of VG metadata for %s", resource);
 		_lock_for_cluster(CLVMD_CMD_VG_BACKUP, LCK_CLUSTER_VG, resource);
 	}

lib/locking/file_locking.c

@@ -21,6 +21,7 @@
 #include "defaults.h"
 #include "lvm-file.h"
 #include "lvm-string.h"
+#include "lvmcache.h"
 
 #include <limits.h>
 #include <unistd.h>
@@ -209,6 +210,10 @@ static int _file_lock_resource(struct cmd_context *cmd, const char *resource,
 
 	switch (flags & LCK_SCOPE_MASK) {
 	case LCK_VG:
+		if (flags & LCK_CACHE) {
+			lvmcache_drop_metadata(resource);
+			break;
+		}
 		if (!*resource)	/* FIXME Deprecated */
 			dm_snprintf(lockfile, sizeof(lockfile),
 				     "%s/P_orphans", _lock_dir);

lib/locking/locking.c

@@ -324,7 +324,8 @@ static int _lock_vol(struct cmd_context *cmd, const char *resource, uint32_t fla
 	assert(resource);
 
 	if ((ret = _locking.lock_resource(cmd, resource, flags))) {
-		if ((flags & LCK_SCOPE_MASK) == LCK_VG) {
+		if ((flags & LCK_SCOPE_MASK) == LCK_VG &&
+		    !(flags & LCK_CACHE)) {
 			if ((flags & LCK_TYPE_MASK) == LCK_UNLOCK)
 				lvmcache_unlock_vgname(resource);
 			else

lib/locking/locking.h

@@ -69,6 +69,7 @@ int check_lvm1_vg_inactive(struct cmd_context *cmd, const char *vgname);
 #define LCK_HOLD	0x00000020U	/* Hold lock when lock_vol returns? */
 #define LCK_LOCAL	0x00000040U	/* Don't propagate to other nodes */
 #define LCK_CLUSTER_VG	0x00000080U	/* VG is clustered */
+#define LCK_CACHE	0x00000100U	/* Operation on cache using P_ lock */
 
 /*
  * Additional lock bits for cluster communication
@@ -91,6 +92,7 @@ int check_lvm1_vg_inactive(struct cmd_context *cmd, const char *vgname);
 #define LCK_VG_READ		(LCK_VG | LCK_READ | LCK_HOLD)
 #define LCK_VG_WRITE		(LCK_VG | LCK_WRITE | LCK_HOLD)
 #define LCK_VG_UNLOCK		(LCK_VG | LCK_UNLOCK)
+#define LCK_VG_DROP_CACHE	(LCK_VG | LCK_WRITE | LCK_CACHE)
 
 #define LCK_LV_EXCLUSIVE	(LCK_LV | LCK_EXCL | LCK_NONBLOCK)
 #define LCK_LV_SUSPEND		(LCK_LV | LCK_WRITE | LCK_NONBLOCK)
@@ -116,6 +118,8 @@ int check_lvm1_vg_inactive(struct cmd_context *cmd, const char *vgname);
 	lock_lv_vol(cmd, lv, LCK_LV_ACTIVATE | LCK_HOLD | LCK_LOCAL)
 #define deactivate_lv_local(cmd, lv)	\
 	lock_lv_vol(cmd, lv, LCK_LV_DEACTIVATE | LCK_LOCAL)
+#define drop_cached_metadata(vg)	\
+	lock_vol((vg)->cmd, (vg)->name, LCK_VG_DROP_CACHE)
 
 /* Process list of LVs */
 int suspend_lvs(struct cmd_context *cmd, struct list *lvs);

lib/metadata/metadata.c

@@ -716,7 +716,7 @@ int vg_split_mdas(struct cmd_context *cmd __attribute((unused)),
 			if (is_orphan_vg(vg_to->name))
 				list_del(&mda->list);
 			else
-				list_move(&mda->list, mdas_to);
+				list_move(mdas_to, &mda->list);
 		}
 	}
 
@@ -885,6 +885,7 @@ struct pv_list *find_pv_in_pv_list(const struct list *pl,
 	list_iterate_items(pvl, pl)
 		if (pvl->pv == pv)
 			return pvl;
+
 	return NULL;
 }
 
@@ -957,6 +958,7 @@ struct lv_list *find_lv_in_lv_list(const struct list *ll,
 	list_iterate_items(lvl, ll)
 		if (lvl->lv == lv)
 			return lvl;
+
 	return NULL;
 }
 
@@ -1331,6 +1333,11 @@ int vg_commit(struct volume_group *vg)
 		return cache_updated;
 	}
 
+	if (!drop_cached_metadata(vg)) {
+		log_error("Unable to drop cached metadata for VG %s.", vg->name);
+		return 0;
+	}
+
 	/* Commit to each copy of the metadata area */
 	list_iterate_items(mda, &vg->fid->metadata_areas) {
 		failed = 0;
@@ -1346,6 +1353,10 @@ int vg_commit(struct volume_group *vg)
 		}
 	}
 
+	if (!drop_cached_metadata(vg))
+		log_error("Attempt to drop cached metadata failed "
+			  "after commit for VG %s.", vg->name);
+
 	/* If at least one mda commit succeeded, it was committed */
 	return cache_updated;
 }

lib/mirror/mirrored.c

@@ -373,7 +373,7 @@ static int _mirrored_target_present(const struct lv_segment *seg __attribute((un
 
 	/*
 	 * Check only for modules if atttributes requested and no previous check.
-	 * FIXME: need better check
+	 * FIXME: Fails incorrectly if cmirror was built into kernel.
 	 */
 	if (attributes) {
 		if (!_mirror_attributes && module_present("cmirror"))

libdm/datastruct/list.c

@@ -65,6 +65,15 @@ void list_del(struct list *elem)
 	elem->p->n = elem->n;
 }
 
+/*
+ * Remove an element from existing list and insert before 'head'.
+ */
+void list_move(struct list *head, struct list *elem)
+{
+        list_del(elem);
+        list_add(head, elem);
+}
+
 /*
  * Is the list empty?
  */

libdm/datastruct/list.h

@@ -55,14 +55,9 @@ void list_add_h(struct list *head, struct list *elem);
 void list_del(struct list *elem);
 
 /*
- * Move an element from an existing list to list 'head'.
- * Insert the element before 'head'.
+ * Remove an element from existing list and insert before 'head'.
  */
-static inline void list_move(struct list *item, struct list *head)
-{
-        list_del(item);
-        list_add(head, item);
-}
+void list_move(struct list *head, struct list *elem);
 
 /*
  * Is the list empty?

man/lvm.conf.5

@@ -84,7 +84,7 @@ Defaults to "/dev".
 \fBpreferred_names\fP \(em List of patterns compared in turn against
 all the pathnames referencing the same device in in the scanned directories.
 The pathname that matches the earliest pattern in the list is the
-one used in any output.  As an example, if device mapper multipathing
+one used in any output.  As an example, if device-mapper multipathing
 is used, the following will select multipath device names:
 .br
 \fBdevices { preferred_names = [ "^/dev/mapper/mpath" ] }\fP

test/lvm-utils.sh

@@ -86,6 +86,18 @@ fi
   return $(test $(lvs --noheadings -o $2 $1) == $3)
 }
 
+vg_validate_pvlv_counts_()
+{
+	local local_vg=$1
+	local num_pvs=$2
+	local num_lvs=$3
+	local num_snaps=$4
+
+	check_vg_field_ $local_vg pv_count $num_pvs &&
+	check_vg_field_ $local_vg lv_count $num_lvs &&
+	check_vg_field_ $local_vg snap_count $num_snaps
+}
+
 dmsetup_has_dm_devdir_support_()
 {
   # Detect support for the envvar.  If it's supported, the

test/t-lvresize-usage.sh

@@ -1,5 +1,5 @@
 #!/bin/sh
-# Copyright (C) 2007 Red Hat, Inc. All rights reserved.
+# Copyright (C) 2007-2008 Red Hat, Inc. All rights reserved.
 #
 # This copyrighted material is made available to anyone wishing to use,
 # modify, copy, or redistribute it subject to the terms and conditions

test/t-vgmerge-operation.sh

@@ -1,5 +1,5 @@
 #!/bin/sh
-# Copyright (C) 2007 Red Hat, Inc. All rights reserved.
+# Copyright (C) 2007-2008 Red Hat, Inc. All rights reserved.
 #
 # This copyrighted material is made available to anyone wishing to use,
 # modify, copy, or redistribute it subject to the terms and conditions
@@ -23,18 +23,6 @@ cleanup_()
   rm -f "$f1" "$f2" "$f3" "$f4"
 }
 
-vg_validate_pvlv_counts_()
-{
-	local local_vg=$1
-	local num_pvs=$2
-	local num_lvs=$3
-	local num_snaps=$4
-
-	check_vg_field_ $local_vg pv_count $num_pvs &&
-	check_vg_field_ $local_vg lv_count $num_lvs &&
-	check_vg_field_ $local_vg snap_count $num_snaps
-}
-
 test_expect_success \
   'set up temp files, loopback devices, PVs, vgnames' \
   'f1=$(pwd)/1 && d1=$(loop_setup_ "$f1") &&
@@ -81,10 +69,10 @@ test_expect_success \
    lvcreate -l 16 -n $lv1 $vg1 &&
    lvcreate -l 4 -s -n $lv2 $vg1/$lv1 &&
    vgchange -an $vg1 &&
-   vg_validate_pvlv_counts_ $vg1 2 1 1 &&
+   vg_validate_pvlv_counts_ $vg1 2 2 1 &&
    vg_validate_pvlv_counts_ $vg2 2 0 0 &&
    vgmerge $vg2 $vg1 &&
-   vg_validate_pvlv_counts_ $vg2 4 1 1 &&
+   vg_validate_pvlv_counts_ $vg2 4 2 1 &&
    lvremove -f $vg2/$lv2 &&
    vgremove -f $vg2'
 
@@ -94,10 +82,10 @@ test_expect_success \
    vgcreate $vg2 $d4 &&
    lvcreate -l 4 -n $lv1 -m1 $vg1 &&
    vgchange -an $vg1 &&
-   vg_validate_pvlv_counts_ $vg1 3 4 0 &&
+   vg_validate_pvlv_counts_ $vg1 3 1 0 &&
    vg_validate_pvlv_counts_ $vg2 1 0 0 &&
    vgmerge $vg2 $vg1 &&
-   vg_validate_pvlv_counts_ $vg2 4 4 0 &&
+   vg_validate_pvlv_counts_ $vg2 4 1 0 &&
    lvremove -f $vg2/$lv1 &&
    vgremove -f $vg2'
 

test/t-vgsplit-operation.sh

@@ -24,18 +24,6 @@ cleanup_()
   rm -f "$f1" "$f2" "$f3" "$f4" "$f5"
 }
 
-vg_validate_pvlv_counts_()
-{
-	local local_vg=$1
-	local num_pvs=$2
-	local num_lvs=$3
-	local num_snaps=$4
-
-	check_vg_field_ $local_vg pv_count $num_pvs &&
-	check_vg_field_ $local_vg lv_count $num_lvs &&
-	check_vg_field_ $local_vg snap_count $num_snaps
-}
-
 # FIXME: paramaterize lvm1 vs lvm2 metadata; most of these tests should run
 # fine with lvm1 metadata as well; for now, just add disks 5 and 6 as lvm1
 # metadata
@@ -121,9 +109,9 @@ test_expect_success \
      vgsplit -n $lv1 $vg1 $vg2
    fi &&
    if [ $i == existing ]; then
-     vg_validate_pvlv_counts_ $vg2 4 4 0
+     vg_validate_pvlv_counts_ $vg2 4 1 0
    else
-     vg_validate_pvlv_counts_ $vg2 3 4 0
+     vg_validate_pvlv_counts_ $vg2 3 1 0
    fi &&
    lvremove -f $vg2/$lv1 &&
    vgremove -f $vg2'
@@ -143,9 +131,9 @@ test_expect_success \
      vgsplit -n $lv1 $vg1 $vg2
    fi &&
    if [ $i == existing ]; then
-     vg_validate_pvlv_counts_ $vg2 4 1 1
+     vg_validate_pvlv_counts_ $vg2 4 2 1
    else
-     vg_validate_pvlv_counts_ $vg2 2 1 1
+     vg_validate_pvlv_counts_ $vg2 2 2 1
    fi &&
    lvremove -f $vg2/$lv2 &&
    lvremove -f $vg2/$lv1 &&
@@ -169,10 +157,10 @@ test_expect_success \
    fi &&
    if [ $i == existing ]; then
      vg_validate_pvlv_counts_ $vg2 2 1 0
-     vg_validate_pvlv_counts_ $vg1 2 1 1
+     vg_validate_pvlv_counts_ $vg1 2 2 1
    else
      vg_validate_pvlv_counts_ $vg2 1 1 0
-     vg_validate_pvlv_counts_ $vg1 2 1 1
+     vg_validate_pvlv_counts_ $vg1 2 2 1
    fi &&
    lvremove -f $vg1/$lv2 &&
    lvremove -f $vg1/$lv1 &&
@@ -196,10 +184,10 @@ test_expect_success \
      vgsplit -n $lv2 $vg1 $vg2
    fi &&
    if [ $i == existing ]; then
-     vg_validate_pvlv_counts_ $vg1 3 4 0
+     vg_validate_pvlv_counts_ $vg1 3 2 0
      vg_validate_pvlv_counts_ $vg2 2 1 0
    else
-     vg_validate_pvlv_counts_ $vg1 3 4 0
+     vg_validate_pvlv_counts_ $vg1 3 2 0
      vg_validate_pvlv_counts_ $vg2 1 1 0
    fi &&
    lvremove -f $vg1/$lv1 &&
@@ -231,7 +219,7 @@ test_expect_success \
    lvcreate -l 16 -n $lv1 $vg1 $d1 $d2 &&
    lvcreate -l 4 -n $lv2 -s $vg1/$lv1 &&
    lvcreate -l 4 -n $lv3 -s $vg1/$lv1 &&
-   vg_validate_pvlv_counts_ $vg1 4 1 2 &&
+   vg_validate_pvlv_counts_ $vg1 4 3 2 &&
    vgchange -an $vg1 &&
    vgsplit -n $lv1 $vg1 $vg2;
    status=$?; echo status=$status; test $status = 5 &&
@@ -246,7 +234,7 @@ test_expect_success \
    lvcreate -l 16 -n $lv1 $vg1 $d1 $d2 &&
    lvcreate -l 4 -n $lv2 -s $vg1/$lv1 &&
    lvcreate -l 4 -n $lv3 -s $vg1/$lv1 &&
-   vg_validate_pvlv_counts_ $vg1 4 1 2 &&
+   vg_validate_pvlv_counts_ $vg1 4 3 2 &&
    vgchange -an $vg1 &&
    vgsplit -n $lv2 $vg1 $vg2;
    status=$?; echo status=$status; test $status = 5 &&
@@ -270,12 +258,71 @@ test_expect_success \
   'vgcreate $vg1 $d1 $d2 $d3 $d4 &&
    lvcreate -l 16 -n $lv1 -m1 $vg1 $d1 $d2 $d3 &&
    lvcreate -l 16 -n $lv2 -i 2 $vg1 $d3 $d4 &&
-   vg_validate_pvlv_counts_ $vg1 4 5 0 &&
+   vg_validate_pvlv_counts_ $vg1 4 2 0 &&
    vgchange -an $vg1 &&
    vgsplit -n $lv2 $vg1 $vg2 2>err;
    status=$?; echo status=$status; test $status = 5 &&
    vgremove -ff $vg1'
 
+#
+# Verify vgsplit rejects active LVs only when active LVs involved in split
+#
+test_expect_success \
+  "vgsplit fails, active mirror involved in split" \
+  'vgcreate $vg1 $d1 $d2 $d3 $d4 &&
+   lvcreate -l 16 -n $lv1 -m1 $vg1 $d1 $d2 $d3 &&
+   lvcreate -l 16 -n $lv2 $vg1 $d4 &&
+   lvchange -an $vg1/$lv2 &&
+   vg_validate_pvlv_counts_ $vg1 4 2 0 &&
+   vgsplit -n $lv1 $vg1 $vg2;
+   status=$?; echo status=$status; test $status = 5 &&
+   vg_validate_pvlv_counts_ $vg1 4 2 0 &&
+   vgremove -ff $vg1'
+
+test_expect_success \
+  "vgsplit succeeds, active mirror not involved in split" \
+  'vgcreate $vg1 $d1 $d2 $d3 $d4 &&
+   lvcreate -l 16 -n $lv1 -m1 $vg1 $d1 $d2 $d3 &&
+   lvcreate -l 16 -n $lv2 $vg1 $d4 &&
+   lvchange -an $vg1/$lv2 &&
+   vg_validate_pvlv_counts_ $vg1 4 2 0 &&
+   vgsplit -n $lv2 $vg1 $vg2 &&
+   vg_validate_pvlv_counts_ $vg1 3 1 0 &&
+   vg_validate_pvlv_counts_ $vg2 1 1 0 &&
+   vgremove -ff $vg1 &&
+   vgremove -ff $vg2'
+
+test_expect_success \
+  "vgsplit fails, active snapshot involved in split" \
+  'vgcreate $vg1 $d1 $d2 $d3 $d4 &&
+   lvcreate -l 64 -i 2 -n $lv1 $vg1 $d1 $d2 &&
+   lvcreate -l 4 -i 2 -s -n $lv2 $vg1/$lv1 &&
+   lvcreate -l 64 -i 2 -n $lv3 $vg1 $d3 $d4 &&
+   lvchange -an $vg1/$lv3 &&
+   vg_validate_pvlv_counts_ $vg1 4 3 1 &&
+   vgsplit -n $lv2 $vg1 $vg2;
+   status=$?; echo status=$status; test $status = 5 &&
+   vg_validate_pvlv_counts_ $vg1 4 3 1 &&
+   lvremove -f $vg1/$lv2 &&
+   vgremove -ff $vg1'
+
+test_expect_success \
+  "vgsplit succeeds, active snapshot not involved in split" \
+  'vgcreate $vg1 $d1 $d2 $d3 &&
+   lvcreate -l 64 -i 2 -n $lv1 $vg1 $d1 $d2 &&
+   lvcreate -l 4 -s -n $lv2 $vg1/$lv1 &&
+   vgextend $vg1 $d4 &&
+   lvcreate -l 64 -n $lv3 $vg1 $d4 &&
+   lvchange -an $vg1/$lv3 &&
+   vg_validate_pvlv_counts_ $vg1 4 3 1 &&
+   vgsplit -n $lv3 $vg1 $vg2 &&
+   vg_validate_pvlv_counts_ $vg1 3 2 1 &&
+   vg_validate_pvlv_counts_ $vg2 1 1 0 &&
+   vgchange -an $vg1 &&
+   lvremove -f $vg1/$lv2 &&
+   vgremove -ff $vg1 &&
+   vgremove -ff $vg2'
+
 
 test_done
 

test/t-vgsplit-usage.sh

@@ -1,5 +1,5 @@
 #!/bin/sh
-# Copyright (C) 2007 Red Hat, Inc. All rights reserved.
+# Copyright (C) 2007-2008 Red Hat, Inc. All rights reserved.
 #
 # This copyrighted material is made available to anyone wishing to use,
 # modify, copy, or redistribute it subject to the terms and conditions

tools/lvresize.c

@@ -229,8 +229,8 @@ static int _lvresize_params(struct cmd_context *cmd, int argc, char **argv,
 	argv++;
 	argc--;
 
-	if (!(lp->lv_name = skip_dev_dir(cmd, lp->lv_name, &dev_dir_found))
-	    || (!(lp->vg_name = extract_vgname(cmd, lp->lv_name)))) {
+	if (!(lp->lv_name = skip_dev_dir(cmd, lp->lv_name, &dev_dir_found)) ||
+	    !(lp->vg_name = extract_vgname(cmd, lp->lv_name))) {
 		log_error("Please provide a volume group name");
 		return 0;
 	}
@@ -269,6 +269,7 @@ static int _lvresize(struct cmd_context *cmd, struct volume_group *vg,
 	uint32_t sz, str;
 	struct list *pvh = NULL;
 	char size_buf[SIZE_BUF];
+	char lv_path[PATH_MAX];
 
 	/* does LV exist? */
 	if (!(lvl = find_lv_in_vg(vg, lp->lv_name))) {
@@ -623,8 +624,6 @@ static int _lvresize(struct cmd_context *cmd, struct volume_group *vg,
 	log_print("Logical volume %s successfully resized", lp->lv_name);
 
 	if (lp->resizefs && (lp->resize == LV_EXTEND)) {
-		char lv_path[PATH_MAX];
-
 		if (dm_snprintf(lv_path, PATH_MAX, "%s%s/%s", cmd->dev_dir,
 				lp->vg_name, lp->lv_name) < 0) {
 			log_error("Couldn't create LV path for %s",

tools/toollib.c

@@ -1245,8 +1245,6 @@ int is_reserved_lvname(const char *name)
 	return rc;
 }
 
-
-
 /*
  * Set members of struct vgcreate_params from cmdline.
  * Do preliminary validation with arg_*() interface.

tools/vgmerge.c

@@ -54,7 +54,7 @@ static int _vgmerge_single(struct cmd_context *cmd, const char *vg_name_to,
 		struct list *pvh = vg_from->pvs.n;
 		struct physical_volume *pv;
 
-		list_move(pvh, &vg_to->pvs);
+		list_move(&vg_to->pvs, pvh);
 
 		pv = list_item(pvh, struct pv_list)->pv;
 		pv->vg_name = dm_pool_strdup(cmd->mem, vg_to->name);
@@ -89,13 +89,13 @@ static int _vgmerge_single(struct cmd_context *cmd, const char *vg_name_to,
 	while (!list_empty(&vg_from->lvs)) {
 		struct list *lvh = vg_from->lvs.n;
 
-		list_move(lvh, &vg_to->lvs);
+		list_move(&vg_to->lvs, lvh);
 	}
 
 	while (!list_empty(&vg_from->fid->metadata_areas)) {
 		struct list *mdah = vg_from->fid->metadata_areas.n;
 
-		list_move(mdah, &vg_to->fid->metadata_areas);
+		list_move(&vg_to->fid->metadata_areas, mdah);
 	}
 
 	vg_to->lv_count += vg_from->lv_count;

tools/vgsplit.c

@@ -28,7 +28,7 @@ static int _move_pv(struct volume_group *vg_from, struct volume_group *vg_to,
 		return 0;
 	}
 
-	list_move(&pvl->list, &vg_to->pvs);
+	list_move(&vg_to->pvs, &pvl->list);
 
 	vg_from->pv_count--;
 	vg_to->pv_count++;
@@ -94,14 +94,18 @@ static int _lv_is_in_vg(struct volume_group *vg, struct logical_volume *lv)
 }
 
 static int _move_one_lv(struct volume_group *vg_from,
-			struct volume_group *vg_to,
-			struct list *lvh)
+			 struct volume_group *vg_to,
+			 struct list *lvh)
 {
-	struct logical_volume *lv;
+	struct logical_volume *lv = list_item(lvh, struct lv_list)->lv;
 
-	lv = list_item(lvh, struct lv_list)->lv;
-	list_move(lvh, &vg_to->lvs);
+	list_move(&vg_to->lvs, lvh);
 	
+	if (lv_is_active(lv)) {
+		log_error("Logical volume \"%s\" must be inactive", lv->name);
+		return 0;
+	}
+
 	if (lv->status & SNAPSHOT) {
 		vg_from->snapshot_count--;
 		vg_to->snapshot_count++;
@@ -171,7 +175,7 @@ static int _move_lvs(struct volume_group *vg_from, struct volume_group *vg_to)
 
 		/* Move this LV */
 		if (!_move_one_lv(vg_from, vg_to, lvh))
-			return 0;
+			return_0;
 	}
 
 	/* FIXME Ensure no LVs contain segs pointing at LVs in the other VG */
@@ -217,7 +221,7 @@ static int _move_snapshots(struct volume_group *vg_from,
 			if (_lv_is_in_vg(vg_to, seg->cow) &&
 			    _lv_is_in_vg(vg_to, seg->origin)) {
 				if (!_move_one_lv(vg_from, vg_to, lvh))
-					return 0;
+					return_0;
 			}
 		}
 
@@ -259,7 +263,7 @@ static int _move_mirrors(struct volume_group *vg_from,
 
 		if (seg_in == seg->area_count && log_in) {
 			if (!_move_one_lv(vg_from, vg_to, lvh))
-				return 0;
+				return_0;
 		}
 	}
 
@@ -284,7 +288,6 @@ int vgsplit(struct cmd_context *cmd, int argc, char **argv)
 	char *vg_name_from, *vg_name_to;
 	struct volume_group *vg_to, *vg_from;
 	int opt;
-	int active;
 	int existing_vg;
 	int consistent;
 	const char *lv_name;
@@ -323,14 +326,6 @@ int vgsplit(struct cmd_context *cmd, int argc, char **argv)
 				       CORRECT_INCONSISTENT | FAIL_INCONSISTENT)))
 		 return ECMD_FAILED;
 
-	if ((active = lvs_in_vg_activated(vg_from))) {
-		/* FIXME Remove this restriction */
-		log_error("Logical volumes in \"%s\" must be inactive",
-			  vg_name_from);
-		unlock_vg(cmd, vg_name_from);
-		return ECMD_FAILED;
-	}
-
 	log_verbose("Checking for new volume group \"%s\"", vg_name_to);
 	if (!lock_vol(cmd, vg_name_to, LCK_VG_WRITE | LCK_NONBLOCK)) {
 		log_error("Can't get lock for %s", vg_name_to);
@@ -344,10 +339,10 @@ int vgsplit(struct cmd_context *cmd, int argc, char **argv)
 		if (new_vg_option_specified(cmd)) {
 			log_error("Volume group \"%s\" exists, but new VG "
 				    "option specified", vg_name_to);
-			goto bad;
+			goto_bad;
 		}
 		if (!vgs_are_compatible(cmd, vg_from,vg_to))
-			goto bad;
+			goto_bad;
 	} else {
 		existing_vg = 0;
 
@@ -377,7 +372,7 @@ int vgsplit(struct cmd_context *cmd, int argc, char **argv)
 		if (!(vg_to = vg_create(cmd, vg_name_to, vp_new.extent_size,
 					vp_new.max_pv, vp_new.max_lv,
 					vp_new.alloc, 0, NULL)))
-			goto bad;
+			goto_bad;
 
 		if (vg_is_clustered(vg_from))
 			vg_to->status |= CLUSTERED;
@@ -385,41 +380,39 @@ int vgsplit(struct cmd_context *cmd, int argc, char **argv)
 
 	/* Archive vg_from before changing it */
 	if (!archive(vg_from))
-		goto bad;
+		goto_bad;
 
 	/* Move PVs across to new structure */
 	for (opt = 0; opt < argc; opt++) {
 		if (!_move_pv(vg_from, vg_to, argv[opt]))
-			goto bad;
+			goto_bad;
 	}
 
 	/* If an LV given on the cmdline, move used_by PVs */
-	if (lv_name) {
-		if (!_move_pvs_used_by_lv(vg_from, vg_to, lv_name))
-			goto bad;
-	}
+	if (lv_name && !_move_pvs_used_by_lv(vg_from, vg_to, lv_name))
+		goto_bad;
 
 	/* Move required LVs across, checking consistency */
 	if (!(_move_lvs(vg_from, vg_to)))
-		goto bad;
+		goto_bad;
 
 	/* Move required snapshots across */
 	if (!(_move_snapshots(vg_from, vg_to)))
-		goto bad;
+		goto_bad;
 
 	/* Move required mirrors across */
 	if (!(_move_mirrors(vg_from, vg_to)))
-		goto bad;
+		goto_bad;
 
 	/* Split metadata areas and check if both vgs have at least one area */
 	if (!(vg_split_mdas(cmd, vg_from, vg_to)) && vg_from->pv_count) {
 		log_error("Cannot split: Nowhere to store metadata for new Volume Group");
-		goto bad;
+		goto_bad;
 	}
 
 	/* Set proper name for all PVs in new VG */
 	if (!vg_rename(cmd, vg_to, vg_name_to))
-		goto bad;
+		goto_bad;
 
 	/* store it on disks */
 	log_verbose("Writing out updated volume groups");
@@ -434,21 +427,21 @@ int vgsplit(struct cmd_context *cmd, int argc, char **argv)
 	vg_to->status |= EXPORTED_VG;
 
 	if (!archive(vg_to))
-		goto bad;
+		goto_bad;
 
 	if (!vg_write(vg_to) || !vg_commit(vg_to))
-		goto bad;
+		goto_bad;
 
 	backup(vg_to);
 
 	/*
 	 * Next, write out the updated old VG.  If we crash after this point,
 	 * recovery is a vgimport on the new VG.
-	 * FIXME: recover automatically or instruct the user the user?
+	 * FIXME: recover automatically or instruct the user?
 	 */
 	if (vg_from->pv_count) {
 		if (!vg_write(vg_from) || !vg_commit(vg_from))
-			goto bad;
+			goto_bad;
 
 		backup(vg_from);
 	}
@@ -458,17 +451,17 @@ int vgsplit(struct cmd_context *cmd, int argc, char **argv)
 	 */
 	consistent = 1;
 	if (!test_mode() &&
-	    (!(vg_to = vg_read(cmd, vg_name_to, NULL, &consistent))
-	     || !consistent)) {
+	    (!(vg_to = vg_read(cmd, vg_name_to, NULL, &consistent)) ||
+	     !consistent)) {
 		log_error("Volume group \"%s\" became inconsistent: please "
 			  "fix manually", vg_name_to);
-		goto bad;
+		goto_bad;
 	}
 
 	vg_to->status &= ~EXPORTED_VG;
 
 	if (!vg_write(vg_to) || !vg_commit(vg_to))
-		goto bad;
+		goto_bad;
 
 	backup(vg_to);