handle transient errors in lvconvert --repair

I have rebased the patch against current CVS. Also, I have added a guard to _mirrored_transient_status to ensure that we alloca() only very modest (and, more importantly, bounded) amount of memory.

Quoted 3 lines

I have rebased the patch against current CVS. Also, I have added a guard to _mirrored_transient_status to ensure that we alloca() only very modest (and, more importantly, bounded) amount of memory.

This patch is just a rebase against current CVS, so it doesn't support mirrored log, yet. Do you have a plan to support mirrored log?

I also tested this patch for a lvm mirror with core/disk log. When a mirror log failed, the mirror log was removed from a mirror volume, but a log voluem is not removed from its volume group. This always happens both on a transient and persistent error.

Quoted 4 lines · Takahiro Yasui

I also tested this patch for a lvm mirror with core/disk log. When a mirror log failed, the mirror log was removed from a mirror volume, but a log voluem is not removed from its volume group. This always happens both on a transient and persistent error.

This issue seems related to this part of your patch.

(quoted diff) +8 -6

@@ -1139,6 +1163,8 @@ static int _lvconvert_mirrors_repair(str
...
-	/*
-	 * Remove all failed_pvs
-	 */
-	if (!_lvconvert_mirrors_aux(cmd, lv, lp, failed_pvs,
-				    lp->mirrors, new_log_count))
-		return 0;
+	if (failed_mirrors) {
+		if (!lv_remove_mirrors(cmd, lv, failed_mirrors, new_log_count,
+				       _is_partial_lv, NULL, 0))
+			return 0;
+
+		if (!_reload_lv(cmd, lv))
+			return 0;
+	}

When I removed this modification, a log volume was removed as expected. And also other my test cases also passed.

I hope this information would be helpful.

Takahiro Yasui <[email protected]> writes:
The catch is that this won't work correctly in other cases, especially with transient errors. I suspect the real problem is in not calling _lv_update_log_type in the new code path -- but see below: I cannot reliably fix this without having a reproducer. Also, I would very much like to have the tests you had failing on our regression suite, to avoid similar problem in the future.
...
Unfortunately, I still cannot reproduce the problem -- I have written a few testcases that only fail the log, or fail a log and some other things and I can't seem to trigger the bug. I have tried with both normal and cluster locking.

It would be very useful if you could provide more specific instructions on how to trigger this.

aux prepare_vg 5
lvcreate -m 1 --ig -L 1 -n 2way $vg $dev1 $dev2 $dev3:0 disable_dev $dev3
echo n | lvconvert --repair $vg/2way
check mirror $vg 2way core
lvs -a -o +devices | not grep unknown
lvs -a -o +devices | not grep mlog
vgreduce --removemissing $vg
enable_dev $dev3

During a call to lv_remove_mirrors above, we call through to _remove_mirror_images, with remove_log = 1. We have this:

	... if (remove_log)
		detached_log_lv = detach_mirror_log(mirrored_seg);

        ...

	if (detached_log_lv && !_delete_lv(lv, detached_log_lv))
		return_0;

So the log *should* be gone after this is finished. Since you see the log hanging around, I suspect that this code has some bugs (this part of the code is known to be problematic, unfortunately). Apart from actual steps to reproduce the problem, the output from lvconvert doing the repair would be helpful. It should be printing things like "Mirror status" and "Mirror log status", please paste these.

3. run 'lvconvert --repair'

# lvconvert --config devices{ignore_suspended_devices=1} --repair --use-policies vg00/lv00
  Mirrored transient status: "2 253:1 253:2 24/24 1 AA 3 disk 253:0 D"
  Mirror log status: 1 of 1 images failed - switching to core
  WARNING: Failed to replace 1 of 1 logs in volume lv00

4. check logical volumes

# lvs
  LV        VG   Attr   LSize  Origin Snap%  Move Log Copy%  Convert
  lv00      vg00 mwi-a- 12.00M                        100.00
  lv00_mlog vg00 -wi---  4.00M

Review line 1227

Quoted 5 lines

+++ cvs-upstream/tools/lvconvert.c 2010-05-19 20:18:40.000000000 +0200

(quoted diff) +0 -0

@@ -1225,11 +1225,11 @@
 		if (!lv_remove_mirrors(cmd, lv, failed_mirrors, new_log_count,
 				       _is_partial_lv, NULL, 0))
 			return 0;

As I posted in the previous mail, the argument, new_log_count, is correct here? The argument is used to check if a log should be removed. I think that failed_log comes here instead of new_log_count.

Or the value such as nlc in _lvconvert_mirrors_aux() comes.

                uint32_t nlc = (!new_log_count || lp->mirrors == 1) ? 1U : 0U;
		...
		} else if (!lv_remove_mirrors(cmd, lv, nmc, nlc,
                                              is_mirror_image_removable, operable_pvs, 0))

Notice how the parameter is named "log_count" -- I have assumed, given that elsewhere it is called "remove_log" that this has a reverse meaning. Wrong, it actually means the same thing as remove_log.

Ugh, nlc sounds like short for new_log_count to me, which is *extremely

Quoted 9 lines · Takahiro Yasui

Review line 1227

Quoted 5 lines

+++ cvs-upstream/tools/lvconvert.c 2010-05-19 20:18:40.000000000 +0200

(quoted diff) +0 -0

@@ -1225,11 +1225,11 @@
 		if (!lv_remove_mirrors(cmd, lv, failed_mirrors, new_log_count,
 				       _is_partial_lv, NULL, 0))
 			return 0;

As I posted in the previous mail, the argument, new_log_count, is correct here? The argument is used to check if a log should be removed. I think that failed_log comes here instead of new_log_count.

... now this was probably not being noticed, since in most cases, the mistakenly removed log would be re-created right away by later code. I'll fix this in my local patch.

Quoted 6 lines

Or the value such as nlc in _lvconvert_mirrors_aux() comes.

                uint32_t nlc = (!new_log_count || lp->mirrors == 1) ? 1U : 0U;
		...
		} else if (!lv_remove_mirrors(cmd, lv, nmc, nlc,
                                              is_mirror_image_removable, operable_pvs, 0))

Ugh, nlc sounds like short for new_log_count to me, which is *extremely confusing*, since it's actually quite opposite: nlc = 1 means remove log, nlc = 0 means retain the log. Is that right?

The attached patch is on top of the previous one -- it should apply cleanly on top of current CVS + the lvconvert-repair-transient.diff I have sent previously. It also fixes a bug in messages, where it would claim that it failed to replace things even if it never tried to in the first place.

Please test and let me know if you run into any other issues.

Quoted 1 line

Please test and let me know if you run into any other issues.

I tested your two patches and all my test cases for 'disk' log passed. As for 'mirrored' log, we need to check the state for log_lv. Here is the sample code.

Quoted 4 lines · Takahiro Yasui

Quoted 1 line

Please test and let me know if you run into any other issues.

I tested your two patches and all my test cases for 'disk' log passed. As for 'mirrored' log, we need to check the state for log_lv. Here is the sample code.

So, do you think it is reasonable to check this in in its current form, and work on --repair support for mirrored logs in a separate patch?

I think there are currently other things broken wrt. mirrored log and --repair, other than just the transient support, and I'll have to sit down and write tests first to see what works and what doesn't and then fix things up.

Quoted 25 lines · Takahiro Yasui

Quoted 4 lines · Takahiro Yasui

I also tested this patch for a lvm mirror with core/disk log. When a mirror log failed, the mirror log was removed from a mirror volume, but a log voluem is not removed from its volume group. This always happens both on a transient and persistent error.

This issue seems related to this part of your patch.

(quoted diff) +8 -6

@@ -1139,6 +1163,8 @@ static int _lvconvert_mirrors_repair(str
...
-	/*
-	 * Remove all failed_pvs
-	 */
-	if (!_lvconvert_mirrors_aux(cmd, lv, lp, failed_pvs,
-				    lp->mirrors, new_log_count))
-		return 0;
+	if (failed_mirrors) {
+		if (!lv_remove_mirrors(cmd, lv, failed_mirrors, new_log_count,
+				       _is_partial_lv, NULL, 0))
+			return 0;
+
+		if (!_reload_lv(cmd, lv))
+			return 0;
+	}

Quoted 2 lines

When I removed this modification, a log volume was removed as expected. And also other my test cases also passed.

The catch is that this won't work correctly in other cases, especially with transient errors. I suspect the real problem is in not calling _lv_update_log_type in the new code path -- but see below: I cannot reliably fix this without having a reproducer. Also, I would very much like to have the tests you had failing on our regression suite, to avoid similar problem in the future.

Review

Quoted 6 lines

(quoted diff) +1 -5

-	 * Remove all failed_pvs
-	 */
-	if (!_lvconvert_mirrors_aux(cmd, lv, lp, failed_pvs,
-				    lp->mirrors, new_log_count))
-		return 0;
+	if (failed_mirrors) {

Quoted 3 lines

Review

Quoted 3 lines

(quoted diff) +3 -0

+	if (failed_mirrors) {
+		if (!lv_remove_mirrors(cmd, lv, failed_mirrors, new_log_count,
+				       _is_partial_lv, NULL, 0))

Hi Petr,

Quoted 26 lines · Petr Rockai

Quoted 25 lines · Takahiro Yasui

Quoted 4 lines · Takahiro Yasui

I also tested this patch for a lvm mirror with core/disk log. When a mirror log failed, the mirror log was removed from a mirror volume, but a log voluem is not removed from its volume group. This always happens both on a transient and persistent error.

This issue seems related to this part of your patch.

(quoted diff) +8 -6

@@ -1139,6 +1163,8 @@ static int _lvconvert_mirrors_repair(str
...
-	/*
-	 * Remove all failed_pvs
-	 */
-	if (!_lvconvert_mirrors_aux(cmd, lv, lp, failed_pvs,
-				    lp->mirrors, new_log_count))
-		return 0;
+	if (failed_mirrors) {
+		if (!lv_remove_mirrors(cmd, lv, failed_mirrors, new_log_count,
+				       _is_partial_lv, NULL, 0))
+			return 0;
+
+		if (!_reload_lv(cmd, lv))
+			return 0;
+	}

Quoted 9 lines

Quoted 2 lines

When I removed this modification, a log volume was removed as expected. And also other my test cases also passed.

The catch is that this won't work correctly in other cases, especially with transient errors. I suspect the real problem is in not calling _lv_update_log_type in the new code path -- but see below: I cannot reliably fix this without having a reproducer. Also, I would very much like to have the tests you had failing on our regression suite, to avoid similar problem in the future.

I checked this code, but the value of failed_mirrors is '0' when no mirror leg has error. So this code path was not executed in my test. This is the reason why log was not removed with mirror_log_fault_policy = "remove".

mirror_log_fault_policy = "allocate," on the other hand, the log was removed in the following path.

        while (replace_mirrors || replace_log) {
                ...
                if (_lvconvert_mirrors_aux(cmd, lv, lp, NULL,
                                           lp->mirrors, log_count))
                        break;

We need to change the condition so that lv_remove_mirrors() is executed.

Quoted 6 lines

Review

Quoted 6 lines

(quoted diff) +1 -5

-	 * Remove all failed_pvs
-	 */
-	if (!_lvconvert_mirrors_aux(cmd, lv, lp, failed_pvs,
-				    lp->mirrors, new_log_count))
-		return 0;
+	if (failed_mirrors) {

Thanks,
Taka

--
lvm-devel mailing list
lvm-devel at redhat.com
https://www.redhat.com/mailman/listinfo/lvm-devel

this is a first iteration of the transient-error handling in lvconvert. This works by marking LVs as partial even though there are no PVs missing -- whenever a mirror marks a leg or log as failed (in the status string). The downside of this approach is that we never figure which PV is failing, but that would require kernel-level support for IO error tracking.

It is very grad that the patch to fix this issue was being developed. I have not thoroughly read it, but I found one issue. It doesn't work for a "mirrored" log, while it looks good for a "core" or "disk" log.

The current code calls lv_check_transient() at the beginning of _lvconvert_mirrors_repair() but I think that lv_check_transient() needs to be called in functions which repairs a mirror log, such as _lv_update_log_type(), too.

Also this patch works well not only for a transient error but also a medium error, therefore, function names with '_transient' could be '_status' or '_status_string'. (e.g. lv_check_status())

Quoted 3 lines · Takahiro Yasui

It is very grad that the patch to fix this issue was being developed. I have not thoroughly read it, but I found one issue. It doesn't work for a "mirrored" log, while it looks good for a "core" or "disk" log.

[snip]

this is not really an issue with the new code per se. What needs to be done is that lvconvert --repair itself needs to be changed to handle mirrored logs correctly.

What needs to be done is to call the repair recursively on the log device if it is a mirror.

Quoted 4 lines

The current code calls lv_check_transient() at the beginning of _lvconvert_mirrors_repair() but I think that lv_check_transient() needs to be called in functions which repairs a mirror log, such as _lv_update_log_type(), too.

(see above)

Quoted 3 lines

Also this patch works well not only for a transient error but also a medium error, therefore, function names with '_transient' could be '_status' or '_status_string'. (e.g. lv_check_status())

I chose transient not because of "transient error", but because of the relation to the transient, in-kernel, status of the device -- as opposed to persistent, i.e. in metadata. Maybe something with "active" or "activation" in it would be better though...