[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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




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

Attachment: forcesync_mirror_option.patch
Description: Binary data



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]