[PATCH] Fix zombie mirror poll daemon (bug 585328)

[PATCH] Fix zombie mirror poll daemon (bug 585328)

[Jonathan Brassow]

This patch fixes a problem where the mirror polling process
may never complete.

If you convert from a linear to a mirror and then convert that
mirror back to linear /while/ the previous (up)convert is
taking place, the mirror polling process will never complete.
This is because the function that polls the mirror for
completion doesn't check if it is still polling a mirror and
the copy_percent that it gets back from the linear device is
certainly never 100%.

The fix is simply to check if the daemon is still looking at
a mirror device - if not, return PROGRESS_CHECK_FAILED.

The user sees the following output from the first (up)convert
if someone else sneaks in and does a down-convert shortly
after their convert:
[root at bp-01 ~]# lvconvert -m1 vg/lv
  vg/lv: Converted: 43.4%
  ABORTING: Mirror percentage check failed.

Signed-off-by: Jonathan Brassow <jbrassow@redhat.com>

Index: LVM2/tools/polldaemon.c
===================================================================
--- LVM2.orig/tools/polldaemon.c
+++ LVM2/tools/polldaemon.c
@@ -78,7 +78,8 @@ progress_t poll_mirror_progress(struct c
 	percent_range_t percent_range, overall_percent_range;
 	uint32_t event_nr = 0;
 
-	if (!lv_mirror_percent(cmd, lv, !parms->interval, &segment_percent,
+	if (!(lv->status & MIRRORED) ||
+	    !lv_mirror_percent(cmd, lv, !parms->interval, &segment_percent,
 			       &percent_range, &event_nr) ||
 	    (percent_range == PERCENT_INVALID)) {
 		log_error("ABORTING: Mirror percentage check failed.");

[PATCH] Fix zombie mirror poll daemon (bug 585328)

[Milan Broz]

On 08/25/2010 11:33 PM, Jonathan Brassow wrote:
> -	if (!lv_mirror_percent(cmd, lv, !parms->interval, &segment_percent,
> +	if (!(lv->status & MIRRORED) ||
> +	    !lv_mirror_percent(cmd, lv, !parms->interval, &segment_percent,
>  			       &percent_range, &event_nr) ||
>  	    (percent_range == PERCENT_INVALID)) {
>  		log_error("ABORTING: Mirror percentage check failed.");

why not move this check inside lv_mirror_percent?
(or is this only place where the bug can appear?)

(and it should probably use !lv_is_mirrored(lv) instead of mask)

otherwise ack

Milan

[PATCH] Fix zombie mirror poll daemon (bug 585328)

[Petr Rockai]

Milan Broz <mbroz@redhat.com> writes:

> On 08/25/2010 11:33 PM, Jonathan Brassow wrote:
>> -	if (!lv_mirror_percent(cmd, lv, !parms->interval, &segment_percent,
>> +	if (!(lv->status & MIRRORED) ||
>> +	    !lv_mirror_percent(cmd, lv, !parms->interval, &segment_percent,
>>  			       &percent_range, &event_nr) ||
>>  	    (percent_range == PERCENT_INVALID)) {
>>  		log_error("ABORTING: Mirror percentage check failed.");
>
> why not move this check inside lv_mirror_percent?
> (or is this only place where the bug can appear?)

I think it's actually safer to fix this here at this point in
time. Otherwise, lv_mirror_percent could start failing unexpectedly on
some mis-behaving code and introduce new bugs.

> (and it should probably use !lv_is_mirrored(lv) instead of mask)
Indeed. Please fix.

I would personally also change the code to print a different error if
the mirror is no longer a mirror, which would be likely less confusing
to the user.

Anyway, assuming the lv_is_mirrored change, OK.

Reviewed-By: Petr Rockai <prockai@redhat.com>

Yours,
   Petr.