[PATCH] lvconvert --repair

[PATCH] lvconvert --repair

[Petr Rockai]

Hi,

this has fallen through the cracks again. I have dusted the patch and fixed
some bugs in it. Since it is prerequisite to automatic hotspare migration of
LVM mirrors (bz 186437), Tom has asked me to give priority to this.

The first patch is just a few minor bits in the testing infrastructure, not
overly important (but it is needed for the new t-lvconvent-repair.sh testcase
to pass, due to vgreduce --removemissing return code).

The second patch does the actual implementation. It should be up to date with
CVS as of today. It should be relatively straightforward, although it did
require adding MISSING_PV checks to some places that were missing them (it's
been harmless till now, since no allocation was ever done on VGs with PVs
missing, but it's crucial for lvconvert --repair).

Yours,
   Petr.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: lvm2-minor-fixes.diff
Type: text/x-diff
Size: 3353 bytes
Desc: lvm2-minor-tests-fixes.diff
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20090130/a9737603/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: lvm2-lvconvert-repair.diff
Type: text/x-diff
Size: 15543 bytes
Desc: lvm2-lvconvert-repair.diff
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20090130/a9737603/attachment-0001.bin>
-------------- next part --------------

-- 
Peter Rockai | me()mornfall!net | prockai()redhat!com
 http://blog.mornfall.net | http://web.mornfall.net

"In My Egotistical Opinion, most people's C programs should be
 indented six feet downward and covered with dirt."
     -- Blair P. Houghton on the subject of C program indentation

[PATCH] lvconvert --repair

[Milan Broz]

Petr Rockai wrote:
> The first patch is just a few minor bits in the testing infrastructure, not
> overly important (but it is needed for the new t-lvconvent-repair.sh testcase
> to pass, due to vgreduce --removemissing return code).

This is the only important chunk in this patch, simple bugfix

--- old-lvconvert-reorder-2/tools/vgreduce.c	2009-01-30 11:03:12.589282389 +0100
+++ new-lvconvert-reorder-2/tools/vgreduce.c	2009-01-30 11:03:12.685281881 +0100
@@ -575,6 +575,8 @@ int vgreduce(struct cmd_context *cmd, in
 		if (fixed)
 			log_print("Wrote out consistent volume group %s",
 				  vg_name);
+		else
+			ret = ECMD_FAILED;
 
 	} else {
 		if (!vg_check_status(vg, EXPORTED_VG | LVM_WRITE | RESIZEABLE_VG)) {

ACK, but as a separate fix/commit

Also there should be
-	int ret = 1;
+	int ret = ECMD_PROCESSED;

to be consistent enough...

Acked-by: Milan Broz <mbroz@redhat.com>


-------
> The second patch does the actual implementation. It should be up to date with
> CVS as of today. It should be relatively straightforward, although it did
> require adding MISSING_PV checks to some places that were missing them (it's
> been harmless till now, since no allocation was ever done on VGs with PVs
> missing, but it's crucial for lvconvert --repair).


- missing man page update

--- old-lvconvert-reorder-2/lib/metadata/pv_map.c	2009-01-30 11:03:20.569284247 +0100
+++ new-lvconvert-reorder-2/lib/metadata/pv_map.c	2009-01-30 11:03:20.625285639 +0100
@@ -129,6 +129,11 @@ static int _create_maps(struct dm_pool *
 	dm_list_iterate_items(pvl, pvs) {
 		if (!(pvl->pv->status & ALLOCATABLE_PV))
 			continue;
+		if (pvl->pv->status & MISSING_PV) {
+			log_very_verbose("skipping missing pv ...");

Do we have name of PV (hint in metadata?) or something to print here?


diff -rN -u -p old-lvconvert-reorder-2/tools/lvconvert.c new-lvconvert-reorder-2/tools/lvconvert.c
--- old-lvconvert-reorder-2/tools/lvconvert.c	2009-01-30 11:03:20.573279378 +0100
+++ new-lvconvert-reorder-2/tools/lvconvert.c	2009-01-30 11:03:20.661284767 +0100
@@ -364,6 +364,61 @@ static int _insert_lvconvert_layer(struc
 	return 1;
 }
 
+static int _area_missing(struct lv_segment *lvseg, int s)
+{
+	if (seg_type(lvseg, s) == AREA_LV) {
+		if (seg_lv(lvseg, s)->status & PARTIAL_LV)
+			return 1;
+	} else if (seg_type(lvseg, s) == AREA_PV) {
+		if (seg_pv(lvseg, s)->status & MISSING_PV)
+			return 1;
+	}
+	return 0;
+}
+
+/* FIXME we want to handle mirror stacks here... */
+static int _count_failed_mirrors(struct logical_volume *lv)
+{
+	struct lv_segment *lvseg;
+	int ret = 0;
+	int s;
+	dm_list_iterate_items(lvseg, &lv->segments) {
+		if (!seg_is_mirrored(lvseg))
+			return -1;

why -1 here and not simply continue or return_0 ?

If this can really happen, it adds mirror lp->mirrors...

		int failed_mirrors = 0;
		...
		failed_mirrors = _count_failed_mirrors(lv);
		lp->mirrors -= failed_mirrors;
		log_error("Mirror status: %d/%d legs failed.",
			  failed_mirrors, existing_mirrors);
...

+		for(s = 0; s < lvseg->area_count; ++s) {
+			if (_area_missing(lvseg, s))
+				++ ret;
+		}
+	}
+	return ret;
+}


+static struct dm_list *_failed_pv_list(struct cmd_context *cmd,
+				       struct volume_group *vg)
+{
+	struct dm_list *r;
+	struct pv_list *pvl, *new_pvl;
+
+	if (!(r = dm_pool_alloc(cmd->mem, sizeof(*r)))) {
+		log_error("Allocation of list failed");
+		return_0;
+	}
+
+	dm_list_init(r);
+	dm_list_iterate_items(pvl, &vg->pvs) {
+		if (!(pvl->pv->status & MISSING_PV))
+			continue;
+
+		if (!(new_pvl = dm_pool_alloc(cmd->mem, sizeof(*new_pvl)))) {
+			log_err("Unable to allocate physical volume list.");
+			return 0;

s/log_err/log_error, return_0 :)

also it should check for 0 othewwise it expects empty list (not NULL) later
I mean this

		- lp->pvh = _failed_pv_list(cmd, lv->vg);

		+ if (!(lp->pvh = ...)
		+	return_0; 


+	if (arg_count(cmd,repair_ARG)) {
+		cmd->handles_missing_pvs = 1;
+		cmd->partial_activation = 1;
+		lp->need_polling = 0;
+		if (!(lv->status & PARTIAL_LV)) {
+			log_error("The mirror is consistent, nothing to repair.");
+			return_0;

why return_0? PEBKAC :)

+		}
+		failed_mirrors = _count_failed_mirrors(lv);
see above

+		lp->mirrors -= failed_mirrors;
+		log_error("Mirror status: %d/%d legs failed.",
+			  failed_mirrors, existing_mirrors);
+		old_pvh = lp->pvh;
+		lp->pvh = _failed_pv_list(cmd, lv->vg);
ditto

+		log_lv=first_seg(lv)->log_lv;

Someone could say that there should be space before and after equal sign :-]


@@ -455,6 +538,18 @@ static int lvconvert_mirrors(struct cmd_
 	}
 
 	/*
+	 * FIXME This check used to precede mirror->mirror conversion
+	 * but didn't affect mirror->linear or linear->mirror. I do
+	 * not understand what is its intention, in fact.
+	 */
+	if (dm_list_size(&lv->segments) != 1) {
+		log_error("Logical volume %s has multiple "
+			  "mirror segments.", lv->name);
+		return 0;
+	}

I think we temporarily restrict mirrors to only one segment mirrors.
Linear can have more segments of course.

There is probably gap in logic, if combined with --alloc anywhere...
(But trying to test that I found another error:-)

----
Why this testcase doesn't work (resp. doesn't finish without
manual intervention and hangs in lvconvert?)
It seems that some devices are left suspended...

prepare_vg 3

lvcreate --alloc anywhere -m1 -L 1 -n mirror $vg

dd if=/dev/zero of=$dev1 bs=1M count=1

#not vgreduce -v --removemissing $vg
lvconvert --repair $vg/mirror
vgreduce --removemissing $vg


Milan
--
mbroz at redhat.com

[PATCH] lvconvert --repair

[Petr Rockai]

Milan Broz <mbroz@redhat.com> writes:
> Also there should be
> -	int ret = 1;
> +	int ret = ECMD_PROCESSED;
>
> to be consistent enough...
will fix

> - missing man page update
indeed

> --- old-lvconvert-reorder-2/lib/metadata/pv_map.c	2009-01-30 11:03:20.569284247 +0100
> +++ new-lvconvert-reorder-2/lib/metadata/pv_map.c	2009-01-30 11:03:20.625285639 +0100
> @@ -129,6 +129,11 @@ static int _create_maps(struct dm_pool *
>  	dm_list_iterate_items(pvl, pvs) {
>  		if (!(pvl->pv->status & ALLOCATABLE_PV))
>  			continue;
> +		if (pvl->pv->status & MISSING_PV) {
> +			log_very_verbose("skipping missing pv ...");
>
> Do we have name of PV (hint in metadata?) or something to print here?
I don't think (the hint tends to be empty for missing PVs). Maybe remove that
log message altogether, it was more useful for debugging.

> +/* FIXME we want to handle mirror stacks here... */
> +static int _count_failed_mirrors(struct logical_volume *lv)
> +{
> +	struct lv_segment *lvseg;
> +	int ret = 0;
> +	int s;
> +	dm_list_iterate_items(lvseg, &lv->segments) {
> +		if (!seg_is_mirrored(lvseg))
> +			return -1;
>
> why -1 here and not simply continue or return_0 ?
Because 0 would mean that there are no failed legs, not an error. Not continue
because it'd probably break anyway. I'll fix the callers to check for -1.

> If this can really happen, it adds mirror lp->mirrors...
>
> 		int failed_mirrors = 0;
> 		...
> 		failed_mirrors = _count_failed_mirrors(lv);

                if (failed_mirrors < 0)
                           error out...

> +		if (!(new_pvl = dm_pool_alloc(cmd->mem, sizeof(*new_pvl)))) {
> +			log_err("Unable to allocate physical volume list.");
> +			return 0;
>
> s/log_err/log_error, return_0 :)
Ok (dunno how log_err crept in...).

> also it should check for 0 othewwise it expects empty list (not NULL) later
> I mean this
>
> 		- lp->pvh = _failed_pv_list(cmd, lv->vg);
>
> 		+ if (!(lp->pvh = ...)
> 		+	return_0; 
>
>
> +	if (arg_count(cmd,repair_ARG)) {
> +		cmd->handles_missing_pvs = 1;
> +		cmd->partial_activation = 1;
> +		lp->need_polling = 0;
> +		if (!(lv->status & PARTIAL_LV)) {
> +			log_error("The mirror is consistent, nothing to repair.");
> +			return_0;
>
> why return_0? PEBKAC :)
Ah ah.

> +		log_lv=first_seg(lv)->log_lv;
> Someone could say that there should be space before and after equal sign :-]
I'll fix this (and any other formatting problems I spot). Will send a patch
amended with the above (real) fixes and cosmetic fixes in a bit.

> Why this testcase doesn't work (resp. doesn't finish without
> manual intervention and hangs in lvconvert?)
> It seems that some devices are left suspended...
>
> prepare_vg 3
>
> lvcreate --alloc anywhere -m1 -L 1 -n mirror $vg
>
> dd if=/dev/zero of=$dev1 bs=1M count=1
>
> #not vgreduce -v --removemissing $vg
> lvconvert --repair $vg/mirror
> vgreduce --removemissing $vg
We have tried to reproduce this without any success, on either my laptop or
Milan's test machine. Let's assume it was a problem with loop devices or
similar that went away in the meantime.

Yours,
   Petr.

-- 
Peter Rockai | me()mornfall!net | prockai()redhat!com
 http://blog.mornfall.net | http://web.mornfall.net

"In My Egotistical Opinion, most people's C programs should be
 indented six feet downward and covered with dirt."
     -- Blair P. Houghton on the subject of C program indentation

[PATCH] lvconvert --repair

[Petr Rockai]

Petr Rockai <prockai@redhat.com> writes:
>> - missing man page update
> indeed
The manpage update is still missing, but all the rest should be addressed in
the attached patch. (I will chop it up for commit, just sending in all the bits
together). I'll trow the manpage together later (this should not interfere with
review or acceptance, I hope).

Yours,
   Petr.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: lvconvert-repair.diff
Type: text/x-diff
Size: 19249 bytes
Desc: lvconvert-repair.diff
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20090212/683e0cba/attachment.bin>
-------------- next part --------------

-- 
Peter Rockai | me()mornfall!net | prockai()redhat!com
 http://blog.mornfall.net | http://web.mornfall.net

"In My Egotistical Opinion, most people's C programs should be
 indented six feet downward and covered with dirt."
     -- Blair P. Houghton on the subject of C program indentation

[PATCH] lvconvert --repair

[Milan Broz]

Petr Rockai wrote:

> The manpage update is still missing, but all the rest should be addressed in
> the attached patch. (I will chop it up for commit, just sending in all the bits
> together). I'll trow the manpage together later (this should not interfere with
> review or acceptance, I hope).
please try this test - it tries to allocate new log - not sure if it is what we want,
but lvconvert fails but log lv is later visible, vg is still inconsistent
and I am confused what the correct state should be:-)

Milan

diff --git a/test/t-lvconvert-repair.sh b/test/t-lvconvert-repair.sh
index 2b72495..98a5ede 100644
--- a/test/t-lvconvert-repair.sh
+++ b/test/t-lvconvert-repair.sh
@@ -37,3 +37,10 @@ disable_dev $dev3
 lvconvert --repair $vg/mirror
 vgreduce --removemissing $vg
 
+enable_dev $dev3
+vgextend $vg $dev3
+lvcreate -m 2 -l 1 -n mirror2 $vg $dev1 $dev2 $dev3 $dev4
+vgchange -a n $vg
+pvremove -ff -y $dev4
+not echo 'y' | lvconvert --repair $vg/mirror2
+vgs

[PATCH] lvconvert --repair

[Petr Rockai]

Milan Broz <mbroz@redhat.com> writes:
> please try this test - it tries to allocate new log - not sure if it is what we want,
> but lvconvert fails but log lv is later visible, vg is still inconsistent
> and I am confused what the correct state should be:-)
The problem was that the code neglected to write out the consistent volume
before trying to add new bits (which could obviously fail). The attached
version of the patch fixes that (it also contains your test now).

-------------- next part --------------
A non-text attachment was scrubbed...
Name: lvconvert-repair.diff
Type: text/x-diff
Size: 19931 bytes
Desc: lvconvert-repair.diff
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20090330/04afeba8/attachment.bin>
-------------- next part --------------

Yours,
   Petr.

-- 
Peter Rockai | me()mornfall!net | prockai()redhat!com
 http://blog.mornfall.net | http://web.mornfall.net

"In My Egotistical Opinion, most people's C programs should be
 indented six feet downward and covered with dirt."
     -- Blair P. Houghton on the subject of C program indentation

[PATCH] lvconvert --repair

[Milan Broz]

Petr Rockai wrote:
> Milan Broz <mbroz@redhat.com> writes:
>> please try this test - it tries to allocate new log - not sure if it is what we want,
>> but lvconvert fails but log lv is later visible, vg is still inconsistent
>> and I am confused what the correct state should be:-)
> The problem was that the code neglected to write out the consistent volume
> before trying to add new bits (which could obviously fail). The attached
> version of the patch fixes that (it also contains your test now).

Just minor things:
- some chunks (repairing vgreduce return code etc) are now already upstream
- there is also trivial merge problem, I am sure darcs merge code handle it:-)

- please check and remove some trailing whitespaces

- consider using vg->vgmem instead of cmd->mem - VG is locked
for the whole operation and failed PV list make sense only for that VG

- please add -i parameter to lvconvert in test script - it waits too long in tests

- add man page entry ;-)

Otherwise

Acked-by: Milan Broz <mbroz@redhat.com>

Milan