[linux-lvm] Re: [LVM2 PATCH] mirror force resync option

[Jonathan E Brassow]

On Oct 11, 2006, at 5:39 PM, Alasdair G Kergon wrote:

> On Wed, Oct 11, 2006 at 10:20:40AM -0500, Jon Brassow wrote:
>> I view
>> this as a bug in the 'deactivate_lv' code, since it should fail if
>> it is unable to deactivate the lv on all nodes.
>
> Correct, if that's true.  But the code doesn't show any deactivation
> attempt!  (see below)

Good spot.  I was incorrectly understanding the lv_info... line to 
identify active lv's on other machines.

>> +Enforce resynching the mirrored logical volumes.
>
> Needs explaining better here for someone who doesn't know
> what that means/involves:-)

Explanation expanded.

>
>> +	if (!(lv->status & MIRRORED))
>> +		return 1;
>
> Are you sure this shouldn't be an error?

I haven't changed this.  So, if tried on a linear volume, there would 
be no error (but nothing would be done).  This is easily changed if you 
like.

>
>> +	if (!lv_info(cmd, lv, &info, 0) || !info.exists)
>> +		active = 0;
>
> This is currently defined as telling you if the device is active on
> the local node.  (Patrick's looking at extending it to the cluster.)

right.

>
>> +	if (active) {
>> +		if (!deactivate_lv(cmd, lv)) {
>> +			log_error("Unable to deactivate, %s, for sync status change", 
>> lv->name);
>> +			log_error("Hint: %s may be in-use", lv->name);
>> +			return 0;
>> +		}
>
> On a cluster, you need to call deactivate_lv *even if* it's not active 
> on the
> local node.  So test for the VG clustered flag and use that to decide
> whether to do the lv_info/active test or not.  (That's only a local 
> optimisation
> normally to assist with the error messages.)

right

>
>> +		/* Must activate log, or we can't zero it */
>> +		if (!activate_lv(cmd, first_seg(lv)->log_lv)) {
>> +			log_error("Unable to activate, %s, for sync status change",
>> +				  first_seg(lv)->log_lv->name);
>> +			activate_lv(cmd, lv);
>> +			return 0;
>> +		}
>
> Interesting.   That extension to the interface needs more comment: you 
> need to
> demonstrate that the operation is either atomic (which it doesn't 
> appear to be
> at first sight), or else if it's not atomic, there must be no failure 
> modes
> that lead to corruption, or people thinking a complete resync got 
> forced when
> it actually didn't.
>
> Normally you're not allowed to activate only part of a device tree - 
> you can't
> activate a mirror log except as part of the mirror.   Notice how 
> lvcreate
> and lvconvert manipulate the log independently before attaching it to 
> the
> mirror.  However the recent changes to prepare for stacked devices 
> should be
> sufficient for that particular activation to work, so that's probably 
> acceptable
> now.
>
> A breakdown into atomic operations might be:
> Detach the log (leaving the [inactive] mirror with a core log), wipe 
> the log,
> then reattach it.
> Or you could introduce an additional metadata state to indicate the log
> needs to be wiped, wipe it, then revert to the previous metadata state.
> Or have a combination of both (so that a future lvchange -ay can 
> complete
> the process if interrupted).
>
>> +		/*
>> +		 * Note: there is no need to deactivate the log before
>> +		 * we activate the mirror
>> +		 */
>
> Because of the stacking preparation now, yes, so long as set_lv() 
> syncs the
> block device correctly.
>

Basically, I only added more to the man page and a check for CLUSTERED 
in tools/lvchange.c.  I've also added some comments in the patch as to 
why I believe the operation is atomic - posing no inconsistencies 
should the operation/machine fail while part way through.

  brassow

-------------- next part --------------
A non-text attachment was scrubbed...
Name: forcesync_mirror_option.patch
Type: application/octet-stream
Size: 7505 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/linux-lvm/attachments/20061018/0e010ccc/attachment.obj>
-------------- next part --------------