Skip to contentSkip to conversation

bz#251990: Move logical volume from one VG to another

PATCH New [email protected] 2 participants 7 messages Mar 24, 2008 – Mar 25, 2008

lore
Share & tools
lore

[email protected]

author
#0 lore raw

cover bz#251990: Move logical volume from one VG to another

The following patches update vgsplit to allow LVs on the cmdline.

Original vgsplit code only allowed PVs on the cmdline, and would fail with an error message in many cases indicating that the split could not occur because it would violate an internal LVM rule of splitting a LV across a volume group.

This code enhances vgsplit in the following ways:
1) Allow LVs on the commandline. This should be very useful as the user does not have to figure out which PVs to move.
2) Figures out underlying dependencies and queries the user if more volumes would be moved than specified on the commandline. This seemed to be a natural progression once LVs were considered. There are many scenarios where moving one LV would result in moving other LVs/PVs as a result of the internal LVM rules where an LV/PV must be contained within a single VG.
3) No longer fails with error messages indicating we can't split a PV/LV across volume groups

The following patches should also allow a simple fix to another bz:
https://bugzilla.redhat.com/show_bug.cgi?id=252041
"vgsplit an active VG where the split involves only inactive LVs"

I am not entirely happy with the code and there are some shortcomings but I figured it is close enough to post. Comments welcome.

#2 lore raw

PATCH 1/4 Add is_reserved_name() helper function.

Hi,

Quoted 1 line

Very similar to apply_lvname_restrictions but without the error messages.

...

Quoted 6 lines
(quoted diff) +6 -0
+int is_reserved_name(const char *name)
+{
+	if (!strncmp(name, "snapshot", 8))
+		return 1;
+
+	if (!strncmp(name, "pvmove", 6))

Review

Quoted 10 lines
(quoted diff) +10 -0
+		return 1;
+
+	if (strstr(name, "_mlog"))
+		return 1;
+
+	if (strstr(name, "_mimage"))
+		return 1;
+
+	return 0;
+}

Having this function separately from apply_lvname_restrictions() is error prone.
e.g. in future, adding a restriction to one but not for the other.

If only log_error() matters, how about adding 'verbose' parameter to apply_lvname_restrictions() (or to this function) to control it?

Thanks,

Signature · 1 line
Jun'ichi Nomura, NEC Corporation of America
#4 lore raw

PATCH 2/4 Update vgsplit to allow a list of LVs or PVs on the commandline.

Hi Dave,

I quickly looked through the patch.
Please see comments below.

lib/metadata/metadata-exported.h struct pv_segment { · line 155

Quoted 12 lines

Add some helper functions to allow vgsplit to continue in more cases. Introduce "related volumes" concept, as defined in the comments below.

lib/metadata/metadata-exported.h +2 -0
@@ -151,6 +151,9 @@ struct pv_segment {
 	uint32_t lv_area;	/* Index to area in LV segment */
 };
 
+#define pvseg_is_allocated(pvseg) (pvseg->lvseg)
+#define pvseg_is_free(pvseg) (!pvseg_is_allocated(pvseg))

Macro parameters should be surrounded by (). #define pvseg_is_allocated(pvseg) ((pvseg)->lvseg) #define pvseg_is_free(pvseg) (!pvseg_is_allocated((pvseg)))

lib/metadata/metadata-exported.h struct pv_segment {

Quoted 5 lines
lib/metadata/metadata-exported.h +5 -0
+struct related_volumes {
+	struct volume_group *vg;
+	struct list ll; /* struct lv_list */
+	struct list pl; /* struct pv_list */
+};

I think 'lvs' and 'pvs' are more descriptive.

struct related_volumes {
struct volume_group *vg;
struct list lvs; /* struct lv_list */
struct list pvs; /* struct pv_list */
};

lib/metadata/metadata-exported.h

Quoted 2 lines
lib/metadata/metadata-exported.h +2 -0
+void pv_add_related_volume(struct related_volumes *rvs, struct pv_list *pvl);
+void lv_add_related_volume(struct related_volumes *rvs, struct lv_list *lvl);

On the analogy of other function names in LVM2, these sound like 'adding "related volume" to pv/lv'.

What these functions actually do is collecting related volumes for given LV/PV and adding them to rvs.
How about collect_related_volumes_for_{pv,lv} or collect_rvs_for_{pv,lv}?

Or IOW, the functions add given PV/LV (and its relatives) to rvs. So add_{pv,lv}_to_related_volumes() might be good as well.

lib/metadata/metadata-exported.h

Quoted 7 lines
lib/metadata/metadata-exported.h +7 -0
+static void _move_pv(struct volume_group *vg_from, struct volume_group *vg_to,
+		     struct pv_list *pvl)
+{
+	struct physical_volume *pv;
+
+	list_del(&pvl->list);
+	list_add(&vg_to->pvs, &pvl->list);

Since this del-and-add logic appears many times through this patch, how about adding list_move() to lib/datastruct/list.h?

static inline list_move(struct list *item, struct list *head)

{
	list_del(item);
	list_add(head, item);
}

lib/metadata/metadata-exported.h

Quoted 1 line
lib/metadata/metadata-exported.h +1 -0
+	pv = list_item(pvl, struct pv_list)->pv;

This is a bug, though it works with luck by struct pv_list layout. It should be:
pv = pvl->pv;

Not a problem of this patch, as it exists in the original code.

lib/metadata/metadata-exported.h

Quoted 2 lines
lib/metadata/metadata-exported.h +2 -0
+static int find_in_pv_list(const struct list *pl,
+			   const struct physical_volume *pv)

I think other find_* functions in LVM2 returns what it finds. Also, names of static functions tend to start with '_'.

So how about _find_pv_in_pv_list() and return a pointer to the matched pvl otherwise NULL?
Or _pv_is_in_pv_list(), if the matched pvl has no use.

lib/metadata/metadata-exported.h

Quoted 8 lines
lib/metadata/metadata-exported.h +8 -0
+{
+	const struct pv_list *pvl;
+
+	list_iterate_items(pvl, pl)
+		if (pvl->pv == pv)
+			return 1;
+	return 0;
+}

_add_pvs() in lib/metadata/lv_manip.c can use this function, too.

lib/metadata/metadata-exported.h

Quoted 2 lines
lib/metadata/metadata-exported.h +2 -0
+static int find_in_lv_list(const struct list *ll,
+			   const struct logical_volume *lv)

Ditto about the naming.

Quoted 2 lines
lib/metadata/metadata-exported.h +2 -0
+static void add_related_pv(struct related_volumes *rvs,
+			   const struct physical_volume *pv)

...

Quoted 2 lines
lib/metadata/metadata-exported.h +2 -0
+static void add_related_lv(struct related_volumes *rvs,
+			   const struct logical_volume *lv)

...

lib/metadata/metadata-exported.h

Quoted 2 lines
lib/metadata/metadata-exported.h +2 -0
+static void add_related_segment(struct related_volumes *rvs,
+				const struct lv_segment *lvseg)

These names sound like 'adding given pv/lv/lvseg to rvs'. but actually are 'adding related volumes for pv/lv/lvseg to rvs'. So similar renaming as pv_add_related_volume() would be nice, IMO.

lib/metadata/metadata-exported.h

Quoted 5 lines
lib/metadata/metadata-exported.h +5 -0
+	if (!list_empty(&lvseg->lv->segs_using_this_lv)) {
+		lvseg2 = get_only_segment_using_this_lv(lvseg->lv);
+		if (lvseg2)
+			add_related_segment(rvs, lvseg2);
+	}

First, I think this part should be moved to lv_find_related_volumes() because segs_using_this_lv is a property of lv, not lvseg.

Second, you should walk through segs_using_this_lv, instead of using get_only_segment_using_this_lv(). e.g. "pvmove" LV may be used by multiple lvsegs.

Quoted 1 line
lib/metadata/metadata-exported.h +0 -0
@@ -302,28 +146,64 @@ int vgsplit(struct cmd_context *cmd, int argc, char **argv)

...

Quoted 19 lines
lib/metadata/metadata-exported.h +14 -3
+	init_related_volumes(&rvs, vg_from);
+	lv_count_cmdline = 0;
+	pv_count_cmdline = 0;
+	for (opt = 0; opt < argc; opt++) {
+		/* FIXME: handle tags */
+		if ((lvl = find_lv_in_vg(vg_from, argv[opt]))) {
+			lv_count_cmdline++;
+			lv_add_related_volume(&rvs, lvl);
+		} else if ((pvl = find_pv_in_vg(vg_from, argv[opt]))) {
+			pv_count_cmdline++;
+			pv_add_related_volume(&rvs, pvl);
+		}
 	}
 
-	/* Move required LVs across, checking consistency */
-	if (!(_move_lvs(vg_from, vg_to)))
-		goto error;
+	/*
+	 * Now that we have the lists of all PVs and LVs involved in the

lib/metadata/metadata-exported.h

Quoted 10 lines
lib/metadata/metadata-exported.h +10 -0
+	 * split, check if this list contains PVs or LVs above what the user
+	 * requested.  If so, ask for confirmation before continuing, as this
+	 * is most likely what the user would expect.
+	 */
+	if ( ((pv_count_cmdline && list_size(&rvs.pl) > pv_count_cmdline) ||
+	      (lv_count_cmdline && list_size(&rvs.ll) > lv_count_cmdline)) &&
+	     !arg_count(cmd, force_ARG) ) {
+		if (!confirm_vgsplit(&rvs.ll, &rvs.pl))
+			goto error;
+	}

The above condition won't work as expected if specified LV is composed of some hidden LVs, e.g. mirror?

Thanks,

Signature · 1 line
Jun'ichi Nomura, NEC Corporation of America