[PATCH 00/10] Fixes related to mirror repair in clustered environment

[PATCH 00/10] Fixes related to mirror repair in clustered environment

[Milan Broz]

These are patches I created when trying to reproduce fails with
recent mirror repair changes.

Because it touch the core cluster locking parts, I separated it
to several simple patches to review it separately.

Please review and comment it - it is curretnly test blocker for clustering.

(If there is better way how to fix described problems - please let me know,
I am quite desperate after debugging this code ;-)

Milan


Milan Broz (10):
  Remove newly created log volume if initial deactivation fails.
  Get rid of hardcoded 0xffdf cluster lock flag.
  Get rid of magic masks in cluster locking code.
  Get rid of magic masks in cluster locking code - clvmd part.
  Allow implicit "convert" to the same lock mode.
  Allow manipulation with precommited metadata even when a PV is
    missing.
  Call explicitly suspend for temporary mirror layer.
  Allow implicit lock conversion for pre/post callbacks.
  Never ever use distributed lock for LV in non-clustered VG.
  Add memlock information to do_lock_lv debug output.

 daemons/clvmd/clvmd-command.c |   10 ++++----
 daemons/clvmd/lvm-functions.c |   38 +++++++++++++++++++++---------------
 lib/locking/cluster_locking.c |    7 +++--
 lib/locking/locking.h         |    3 ++
 lib/metadata/metadata.c       |    4 +-
 lib/metadata/mirror.c         |   42 +++++++++++++++++++++-------------------
 6 files changed, 58 insertions(+), 46 deletions(-)

[PATCH 01/10] Remove newly created log volume if initial deactivation fails.

[Milan Broz]

If there is problem deactivate LV and
_init_mirror_log is called with remove_on_failure = 1,
remove the newly created log LV from metadata.

(This can happen if there is active device with the same name
but different UUID.)

The main reason for this "workaround" patch is to
 - do not keep _mlog volume in metadata, so user can repeat the action
 - print better error message describing the real problem

# lvcreate -m 2 -n lv1 -l 1 --nosync vg_bar
  WARNING: New mirror won't be synchronised. Don't read what you didn't write!
  /dev/vg_bar/lv1_mlog: not found: device not cleared
  Aborting. Failed to wipe mirror log.
  Error locking on node bar-01: Input/output error
  Unable to deactivate mirror log LV. Manual intervention required.
  Failed to create mirror log.

# lvcreate -m 2 -n lv1 -l 1 --nosync vg_bar
  WARNING: New mirror won't be synchronised. Don't read what you didn't write!
  Aborting. Unable to deactivate mirror log.
  Failed to initialise mirror log.

Signed-off-by: Milan Broz <mbroz@redhat.com>
---
 lib/metadata/mirror.c |   14 +++++++++++---
 1 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/lib/metadata/mirror.c b/lib/metadata/mirror.c
index c331e0e..3d757c2 100644
--- a/lib/metadata/mirror.c
+++ b/lib/metadata/mirror.c
@@ -255,11 +255,19 @@ static int _init_mirror_log(struct cmd_context *cmd,
 
 	/* If the LV is active, deactivate it first. */
 	if (lv_info(cmd, log_lv, &info, 0, 0) && info.exists) {
-		if (!deactivate_lv(cmd, log_lv))
-			return_0;
+		(void)deactivate_lv(cmd, log_lv);
 		was_active = 1;
 	}
 
+	/* FIXME: workaround to fail early
+	 * Ensure that log is really deactivated because deactivate_lv
+	 * on cluster do not fail if there is log_lv with different UUID.
+	 */
+	if (was_active && lv_info(cmd, log_lv, &info, 0, 0) && info.exists) {
+		log_error("Aborting. Unable to deactivate mirror log.");
+		goto revert_new_lv;
+	}
+
 	/* Temporary make it visible for set_lv() */
 	lv_set_visible(log_lv);
 
@@ -1354,7 +1362,7 @@ static struct logical_volume *_set_up_mirror_log(struct cmd_context *cmd,
 	}
 
 	if (!_init_mirror_log(cmd, log_lv, in_sync, &lv->tags, 1)) {
-		log_error("Failed to create mirror log.");
+		log_error("Failed to initialise mirror log.");
 		return NULL;
 	}
 
-- 
1.6.5.4

[PATCH 02/10] Get rid of hardcoded 0xffdf cluster lock flag.

[Milan Broz]

There is hidded change - the upper flags (0xffff0000)
and now not cleared, but there are unused anyway.

Signed-off-by: Milan Broz <mbroz@redhat.com>
---
 lib/locking/cluster_locking.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/lib/locking/cluster_locking.c b/lib/locking/cluster_locking.c
index eb09241..b031f12 100644
--- a/lib/locking/cluster_locking.c
+++ b/lib/locking/cluster_locking.c
@@ -415,7 +415,7 @@ int lock_resource(struct cmd_context *cmd, const char *resource, uint32_t flags)
 		clvmd_cmd = CLVMD_CMD_LOCK_LV;
 		strcpy(lockname, resource);
 		lock_scope = "LV";
-		flags &= 0xffdf;	/* Mask off HOLD flag */
+		flags &= ~LCK_HOLD;	/* Mask off HOLD flag */
 		break;
 
 	default:
-- 
1.6.5.4

[PATCH 03/10] Get rid of magic masks in cluster locking code.

[Milan Broz]

Patch should not cause any problems, only real change is
removing LCK_LOCAL bit from lock type flag, it is never used there.
(LCK_LOCAL is part arg[1] bits anyway.)

Signed-off-by: Milan Broz <mbroz@redhat.com>
---
 lib/locking/cluster_locking.c |    5 +++--
 lib/locking/locking.h         |    3 +++
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/lib/locking/cluster_locking.c b/lib/locking/cluster_locking.c
index b031f12..b079195 100644
--- a/lib/locking/cluster_locking.c
+++ b/lib/locking/cluster_locking.c
@@ -317,8 +317,9 @@ static int _lock_for_cluster(struct cmd_context *cmd, unsigned char clvmd_cmd,
 	args = alloca(len);
 	strcpy(args + 2, name);
 
-	args[0] = flags & 0x7F; /* Maskoff lock flags */
-	args[1] = flags & 0xC0; /* Bitmap flags */
+	/* Maskoff lock flags */
+	args[0] = flags & (LCK_SCOPE_MASK | LCK_TYPE_MASK | LCK_HOLD_MASK); 
+	args[1] = flags & LCK_AREA_MASK;
 
 	if (mirror_in_sync())
 		args[1] |= LCK_MIRROR_NOSYNC_MODE;
diff --git a/lib/locking/locking.h b/lib/locking/locking.h
index 50101d1..0dce827 100644
--- a/lib/locking/locking.h
+++ b/lib/locking/locking.h
@@ -78,6 +78,9 @@ int check_lvm1_vg_inactive(struct cmd_context *cmd, const char *vgname);
 /*
  * Lock bits
  */
+#define LCK_HOLD_MASK	0x00000030U	/* NONBLOCK + HOLD bits */
+#define LCK_AREA_MASK	0x000000C0U	/* LOCAL + CLUSTER_VG bits */
+
 #define LCK_NONBLOCK	0x00000010U	/* Don't block waiting for lock? */
 #define LCK_HOLD	0x00000020U	/* Hold lock when lock_vol returns? */
 #define LCK_LOCAL	0x00000040U	/* Don't propagate to other nodes */
-- 
1.6.5.4

[PATCH 04/10] Get rid of magic masks in cluster locking code - clvmd part.

[Milan Broz]

- do_command and lock_vg expect flags including HOLD_MASK (no change here)

Bug fixes:
- lock_vg should check for NONBLOCK on lock_cmd, flgas have this bit masked-out

- do_pre/post_command expect do not mask flag at all, this causes that
the code inside is never run! (see following patches, thes functions
expect plain command without HOLD flags)

Signed-off-by: Milan Broz <mbroz@redhat.com>
---
 daemons/clvmd/clvmd-command.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/daemons/clvmd/clvmd-command.c b/daemons/clvmd/clvmd-command.c
index 2266a2b..e11f864 100644
--- a/daemons/clvmd/clvmd-command.c
+++ b/daemons/clvmd/clvmd-command.c
@@ -132,7 +132,7 @@ int do_command(struct local_client *client, struct clvm_header *msg, int msglen,
 
 	case CLVMD_CMD_LOCK_LV:
 		/* This is the biggie */
-		lock_cmd = args[0] & 0x3F;
+		lock_cmd = args[0] & (LCK_HOLD_MASK | LCK_SCOPE_MASK | LCK_TYPE_MASK);
 		lock_flags = args[1];
 		lockname = &args[2];
 		status = do_lock_lv(lock_cmd, lock_flags, lockname);
@@ -214,7 +214,7 @@ static int lock_vg(struct local_client *client)
 	client->bits.localsock.private = (void *)lock_hash;
     }
 
-    lock_cmd = args[0] & 0x3F;
+    lock_cmd = args[0] & (LCK_HOLD_MASK | LCK_SCOPE_MASK | LCK_TYPE_MASK);
     lock_flags = args[1];
     lockname = &args[2];
     DEBUGLOG("doing PRE command LOCK_VG '%s' at %x (client=%p)\n", lockname, lock_cmd, client);
@@ -237,7 +237,7 @@ static int lock_vg(struct local_client *client)
 	    lock_cmd &= ~LCK_TYPE_MASK;
 	    lock_cmd |= LCK_PREAD;
 	}
-	status = sync_lock(lockname, (int)lock_cmd, (lock_flags & LCK_NONBLOCK) ? LKF_NOQUEUE : 0, &lkid);
+	status = sync_lock(lockname, (int)lock_cmd, (lock_cmd & LCK_NONBLOCK) ? LKF_NOQUEUE : 0, &lkid);
 	if (status)
 	    status = errno;
 	else
@@ -277,7 +277,7 @@ int do_pre_command(struct local_client *client)
 		break;
 
 	case CLVMD_CMD_LOCK_LV:
-		lock_cmd = args[0];
+		lock_cmd = args[0] & (LCK_SCOPE_MASK | LCK_TYPE_MASK);
 		lock_flags = args[1];
 		lockname = &args[2];
 		status = pre_lock_lv(lock_cmd, lock_flags, lockname);
@@ -323,7 +323,7 @@ int do_post_command(struct local_client *client)
 		break;
 
 	case CLVMD_CMD_LOCK_LV:
-		lock_cmd = args[0];
+		lock_cmd = args[0] & (LCK_SCOPE_MASK | LCK_TYPE_MASK);
 		lock_flags = args[1];
 		lockname = &args[2];
 		status = post_lock_lv(lock_cmd, lock_flags, lockname);
-- 
1.6.5.4

[PATCH 05/10] Allow implicit "convert" to the same lock mode.

[Milan Broz]

(Code already not fauils if unlocking not locked resource.)

This is needed in pre/post lock_lv call, where we can
request the same lock on local node becuase of suspend call.

Signed-off-by: Milan Broz <mbroz@redhat.com>
---
 daemons/clvmd/lvm-functions.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/daemons/clvmd/lvm-functions.c b/daemons/clvmd/lvm-functions.c
index e56d11b..6f89cc3 100644
--- a/daemons/clvmd/lvm-functions.c
+++ b/daemons/clvmd/lvm-functions.c
@@ -245,6 +245,11 @@ int hold_lock(char *resource, int mode, int flags)
 
 	lvi = lookup_info(resource);
 
+	if (lvi && lvi->lock_mode == mode) {
+		DEBUGLOG("hold_lock, lock mode %d already held\n", mode);
+		return 0;
+	}
+
 	/* Only allow explicit conversions */
 	if (lvi && !(flags & LKF_CONVERT)) {
 		errno = EBUSY;
-- 
1.6.5.4

[PATCH 06/10] Allow manipulation with precommited metadata even when a PV is missing.

[Milan Broz]

The new recovery code first tries to repair LV and then removes failed PV
from VG. It means that during operation there can be VG with PV missing,
and vg_read code handles it like not consistent VG.

We already allows returning "inconsistent" commited metadata,
for mirror repair we need this for precommited too.
(The suspend call prepares precommited metadata to inactive table on
other cluster nodes.)

"Inconsistent" here means - correct metadata, just with some metadata areas
not found (obviously on missing or failed PVs).

Signed-off-by: Milan Broz <mbroz@redhat.com>
---
 lib/metadata/metadata.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
index 9d5b1ae..c8099b5 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -2706,8 +2706,8 @@ static struct volume_group *_vg_read(struct cmd_context *cmd,
 		if (use_precommitted) {
 			log_error("Inconsistent pre-commit metadata copies "
 				  "for volume group %s", vgname);
-			vg_release(correct_vg);
-			return NULL;
+			*consistent = 0;
+			return correct_vg;
 		}
 
 		if (!*consistent)
-- 
1.6.5.4

[PATCH 07/10] Call explicitly suspend for temporary mirror layer.

[Milan Broz]

The memlock_inc() fix is wrong, memlock count is not
propagated to long living process (clvmd) and just
it underflow there.
Also suspend is needed to pre-load precommited metadata
on other nodes (remapping to error taget in this case).

With explicit suspend we generate lock request and code
can update memlock count.

(Infinitely "locked" memory caused that fs_unlock() was not
called properly and on cluster nodes remains
old links in /dev/mapper for not active devices.)

(N.B. failing of suspend call here is not handled as fatal
error - the LV is going to be removed later anyway.)

Signed-off-by: Milan Broz <mbroz@redhat.com>
---
 lib/metadata/mirror.c |   28 +++++++++++-----------------
 1 files changed, 11 insertions(+), 17 deletions(-)

diff --git a/lib/metadata/mirror.c b/lib/metadata/mirror.c
index 3d757c2..3591df3 100644
--- a/lib/metadata/mirror.c
+++ b/lib/metadata/mirror.c
@@ -603,6 +603,14 @@ static int _remove_mirror_images(struct logical_volume *lv,
 		return 0;
 	}
 
+	/*
+	 * Explicitly suspend temporary LV
+	 * This balance memlock_inc() calls and also causes cluster
+	 * lock to properly propagate precommited metadata into dm table.
+	 */
+	if (lv1 && !suspend_lv(lv1->vg->cmd, lv1))
+		log_error("Problem suspending temporary LV %s", lv1->name);
+
 	if (!vg_commit(mirrored_seg->lv->vg)) {
 		resume_lv(mirrored_seg->lv->vg->cmd, mirrored_seg->lv);
 		return 0;
@@ -616,23 +624,9 @@ static int _remove_mirror_images(struct logical_volume *lv,
 	 * As it's now detached from mirrored_seg->lv we must resume it
 	 * explicitly.
 	 */
-	if (lv1) {
-		if (!resume_lv(lv1->vg->cmd, lv1)) {
-			log_error("Problem resuming temporary LV, %s", lv1->name);
-			return 0;
-		}
-
-		/*
-		 * The code above calls a suspend_lv once, however we now need
-		 * to resume 2 LVs, due to image removal: the mirror image
-		 * itself here, and now the remaining mirror LV. Since
-		 * suspend_lv/resume_lv call memlock_inc/memlock_dec and these
-		 * need to be balanced, we need to call an extra memlock_inc()
-		 * here to balance for the this extra resume -- the following
-		 * one could otherwise either deadlock due to suspended
-		 * devices, or alternatively drop memlock_count below 0.
-		 */
-		memlock_inc();
+	if (lv1 && !resume_lv(lv1->vg->cmd, lv1)) {
+		log_error("Problem resuming temporary LV, %s", lv1->name);
+		return 0;
 	}
 
 	if (!resume_lv(mirrored_seg->lv->vg->cmd, mirrored_seg->lv)) {
-- 
1.6.5.4

[PATCH 08/10] Allow implicit lock conversion for pre/post callbacks.

[Milan Broz]

This is unnoticed (because of bug mentioned in previous patch)
regression from commit 31672ff60e405795cad70d6d7888ac011f5373ce

The pre/post callback need to convert lock always, local node
is going to modify metadata in this case, it it fails conversion,
the call is ignored.

Also it fixes bug when the lock is not yet held, we cannot set LKF_CONVERT
in this case, it will fail because this lock do not exist.

p.s. I am really not sure if this is correct solution to problem,
but the regression here is obvious. Maybe we should separate the real calls
which _request_ conversion and explicitly set LCK_CONVERT flag.

Note that the automatic conversion is still disabled in activate
call, so the original fix (reactivation of exlusive LV) should
be still in place.

Signed-off-by: Milan Broz <mbroz@redhat.com>
---
 daemons/clvmd/lvm-functions.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/daemons/clvmd/lvm-functions.c b/daemons/clvmd/lvm-functions.c
index 6f89cc3..bd340fd 100644
--- a/daemons/clvmd/lvm-functions.c
+++ b/daemons/clvmd/lvm-functions.c
@@ -274,7 +274,7 @@ int hold_lock(char *resource, int mode, int flags)
 			return -1;
 
 		lvi->lock_mode = mode;
-		status = sync_lock(resource, mode, flags, &lvi->lock_id);
+		status = sync_lock(resource, mode, flags & ~LKF_CONVERT, &lvi->lock_id);
 		saved_errno = errno;
 		if (status) {
 			free(lvi);
@@ -551,7 +551,7 @@ int pre_lock_lv(unsigned char command, unsigned char lock_flags, char *resource)
 		DEBUGLOG("pre_lock_lv: resource '%s', cmd = %s, flags = %s\n",
 			 resource, decode_locking_cmd(command), decode_flags(lock_flags));
 
-		if (hold_lock(resource, LKM_PWMODE, LKF_NOQUEUE| (lock_flags & LCK_CONVERT?LKF_CONVERT:0)))
+		if (hold_lock(resource, LKM_PWMODE, LKF_NOQUEUE | LKF_CONVERT))
 			return errno;
 	}
 	return 0;
@@ -583,7 +583,7 @@ int post_lock_lv(unsigned char command, unsigned char lock_flags,
 				return EIO;
 
 			if (lvi.exists) {
-				if (hold_lock(resource, LKM_CRMODE, lock_flags & LCK_CONVERT?LKF_CONVERT:0))
+				if (hold_lock(resource, LKM_CRMODE, LKF_CONVERT))
 					return errno;
 			} else {
 				if (hold_unlock(resource))
-- 
1.6.5.4

[PATCH 09/10] Never ever use distributed lock for LV in non-clustered VG.

[Milan Broz]

The LV locks make sense only for clustered LVs.

Properly check cluster flag and never issue cluster lock here.

There are several places in code, where it is already checked, this
patch add this check to all needed calls.

In previous code the lock behaviour was inconsistent,
for example, the pre/post callback can take lock even for local volume,
but deactivate call do not released this lock and it remains held forever.

The local LV lock request now just let run the underlying activation code
on local node, the same process like in local locking.

(Again, this is important for new mirror repair calls, here for local
mirrors but with cluster locking enabled.)

Signed-off-by: Milan Broz <mbroz@redhat.com>
---
 daemons/clvmd/lvm-functions.c |   21 +++++++++++----------
 1 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/daemons/clvmd/lvm-functions.c b/daemons/clvmd/lvm-functions.c
index bd340fd..be160c3 100644
--- a/daemons/clvmd/lvm-functions.c
+++ b/daemons/clvmd/lvm-functions.c
@@ -331,7 +331,8 @@ static int do_activate_lv(char *resource, unsigned char lock_flags, int mode)
 
 	/* Is it already open ? */
 	oldmode = get_current_lock(resource);
-	if (oldmode == mode) {
+	if (oldmode == mode && (lock_flags & LCK_CLUSTER_VG)) {
+		DEBUGLOG("do_activate_lv, lock already held at %d\n", oldmode);
 		return 0;	/* Nothing to do */
 	}
 
@@ -380,13 +381,13 @@ static int do_activate_lv(char *resource, unsigned char lock_flags, int mode)
 }
 
 /* Resume the LV if it was active */
-static int do_resume_lv(char *resource)
+static int do_resume_lv(char *resource, unsigned char lock_flags)
 {
 	int oldmode;
 
 	/* Is it open ? */
 	oldmode = get_current_lock(resource);
-	if (oldmode == -1) {
+	if (oldmode == -1 && (lock_flags & LCK_CLUSTER_VG)) {
 		DEBUGLOG("do_resume_lv, lock not already held\n");
 		return 0;	/* We don't need to do anything */
 	}
@@ -398,15 +399,15 @@ static int do_resume_lv(char *resource)
 }
 
 /* Suspend the device if active */
-static int do_suspend_lv(char *resource)
+static int do_suspend_lv(char *resource, unsigned char lock_flags)
 {
 	int oldmode;
 	struct lvinfo lvi;
 
 	/* Is it open ? */
 	oldmode = get_current_lock(resource);
-	if (oldmode == -1) {
-		DEBUGLOG("do_suspend_lv, lock held at %d\n", oldmode);
+	if (oldmode == -1 && (lock_flags & LCK_CLUSTER_VG)) {
+		DEBUGLOG("do_suspend_lv, lock not already held\n");
 		return 0; /* Not active, so it's OK */
 	}
 
@@ -498,14 +499,14 @@ int do_lock_lv(unsigned char command, unsigned char lock_flags, char *resource)
 		break;
 
 	case LCK_LV_SUSPEND:
-		status = do_suspend_lv(resource);
+		status = do_suspend_lv(resource, lock_flags);
 		if (!status)
 			suspended++;
 		break;
 
 	case LCK_UNLOCK:
 	case LCK_LV_RESUME:	/* if active */
-		status = do_resume_lv(resource);
+		status = do_resume_lv(resource, lock_flags);
 		if (!status)
 			suspended--;
 		break;
@@ -547,7 +548,7 @@ int pre_lock_lv(unsigned char command, unsigned char lock_flags, char *resource)
 	   lock out on this node (because we are the node modifying the metadata)
 	   before suspending cluster-wide.
 	 */
-	if (command == LCK_LV_SUSPEND) {
+	if (command == LCK_LV_SUSPEND && (lock_flags & LCK_CLUSTER_VG)) {
 		DEBUGLOG("pre_lock_lv: resource '%s', cmd = %s, flags = %s\n",
 			 resource, decode_locking_cmd(command), decode_flags(lock_flags));
 
@@ -564,7 +565,7 @@ int post_lock_lv(unsigned char command, unsigned char lock_flags,
 	int status;
 
 	/* Opposite of above, done on resume after a metadata update */
-	if (command == LCK_LV_RESUME) {
+	if (command == LCK_LV_RESUME && (lock_flags & LCK_CLUSTER_VG)) {
 		int oldmode;
 
 		DEBUGLOG
-- 
1.6.5.4

[PATCH 10/10] Add memlock information to do_lock_lv debug output.

[Milan Broz]

This helps a lot to detect that something strange happened.

Signed-off-by: Milan Broz <mbroz@redhat.com>
---
 daemons/clvmd/lvm-functions.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/daemons/clvmd/lvm-functions.c b/daemons/clvmd/lvm-functions.c
index be160c3..72b5b25 100644
--- a/daemons/clvmd/lvm-functions.c
+++ b/daemons/clvmd/lvm-functions.c
@@ -473,8 +473,8 @@ int do_lock_lv(unsigned char command, unsigned char lock_flags, char *resource)
 {
 	int status = 0;
 
-	DEBUGLOG("do_lock_lv: resource '%s', cmd = %s, flags = %s\n",
-		 resource, decode_locking_cmd(command), decode_flags(lock_flags));
+	DEBUGLOG("do_lock_lv: resource '%s', cmd = %s, flags = %s, memlock = %d\n",
+		 resource, decode_locking_cmd(command), decode_flags(lock_flags), memlock());
 
 	if (!cmd->config_valid || config_files_changed(cmd)) {
 		/* Reinitialise various settings inc. logging, filters */
@@ -537,7 +537,7 @@ int do_lock_lv(unsigned char command, unsigned char lock_flags, char *resource)
 	dm_pool_empty(cmd->mem);
 	pthread_mutex_unlock(&lvm_lock);
 
-	DEBUGLOG("Command return is %d\n", status);
+	DEBUGLOG("Command return is %d, memlock is %d\n", status, memlock());
 	return status;
 }
 
-- 
1.6.5.4

[PATCH 03/10] Get rid of magic masks in cluster locking code.

[Heinz Mauelshagen]

On Tue, 2009-12-08 at 16:39 +0100, Milan Broz wrote:
> Patch should not cause any problems, only real change is
> removing LCK_LOCAL bit from lock type flag, it is never used there.
> (LCK_LOCAL is part arg[1] bits anyway.)
> 
> Signed-off-by: Milan Broz <mbroz@redhat.com>
> ---
>  lib/locking/cluster_locking.c |    5 +++--
>  lib/locking/locking.h         |    3 +++
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/locking/cluster_locking.c b/lib/locking/cluster_locking.c
> index b031f12..b079195 100644
> --- a/lib/locking/cluster_locking.c
> +++ b/lib/locking/cluster_locking.c
> @@ -317,8 +317,9 @@ static int _lock_for_cluster(struct cmd_context *cmd, unsigned char clvmd_cmd,
>  	args = alloca(len);
>  	strcpy(args + 2, name);
>  
> -	args[0] = flags & 0x7F; /* Maskoff lock flags */
> -	args[1] = flags & 0xC0; /* Bitmap flags */
> +	/* Maskoff lock flags */
> +	args[0] = flags & (LCK_SCOPE_MASK | LCK_TYPE_MASK | LCK_HOLD_MASK); 
> +	args[1] = flags & LCK_AREA_MASK;
>  
>  	if (mirror_in_sync())
>  		args[1] |= LCK_MIRROR_NOSYNC_MODE;
> diff --git a/lib/locking/locking.h b/lib/locking/locking.h
> index 50101d1..0dce827 100644
> --- a/lib/locking/locking.h
> +++ b/lib/locking/locking.h
> @@ -78,6 +78,9 @@ int check_lvm1_vg_inactive(struct cmd_context *cmd, const char *vgname);
>  /*
>   * Lock bits
>   */
> +#define LCK_HOLD_MASK	0x00000030U	/* NONBLOCK + HOLD bits */
> +#define LCK_AREA_MASK	0x000000C0U	/* LOCAL + CLUSTER_VG bits */

Or

#define LCK_HOLD_MASK	(LCK_NONBLOCK | LCK_HOLD)
#define	LCK_AREA_MASK	(LCK_LOCAL | LCK_CLUSTER_VG)

rather.

We should use given definitions.

Heinz

> +
>  #define LCK_NONBLOCK	0x00000010U	/* Don't block waiting for lock? */
>  #define LCK_HOLD	0x00000020U	/* Hold lock when lock_vol returns? */
>  #define LCK_LOCAL	0x00000040U	/* Don't propagate to other nodes */