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

[lvm-devel] [PATCH] lvchange -a y does not need a write lock



Hi,

this is a trivial patch in itself, but it depends on my version of "[PATCH] Fix
segfault when using vgsplit in stacked environment" (attached again for
completeness) which in turn depends on the vg_read patches that are stuck
somewhere in the middle of review/application.

This patch is mostly useful for the ignorelockingfailed re-implementation that
can be found yet elsewhere in the list (for explanation, see below). I am also
attaching that patch for reference.

Unfortunately, as things are, all this blocks on the vg_read patches. As for
merit of the ignorelockingfailed patches (with or without this addition), we
can probably discuss that when vg_read is sorted somehow.

Either way, this mini-patch fixes Dave's concern about lvchange:

Dave Wysochanski <dwysocha redhat com> writes:
> * Then again, lvchange and vgchange seem to have different code for
> changing availability of an LV (?) so there may be a subtle reason
> lvchange always takes a WRITE lock.

Since for the segfault fix mentioned earlier, we need to let lvchange/vgchange
set handles_missing_pvs (since it really does), we already know that we are
only going to do activation (and nothing else) in lvchange, in which case, we
can as well ask for a read-only lock (since we aren't going to do any metadata
writes).

Yours,
   Petr.

Tue Mar 17 09:14:05 CET 2009  Petr Rockai <me mornfall net>
  * We don't need a write lock for activation alone in lvchange.
diff -rN -u -p old-lvmlib/tools/lvchange.c new-lvmlib/tools/lvchange.c
--- old-lvmlib/tools/lvchange.c	2009-03-17 09:19:23.488957079 +0100
+++ new-lvmlib/tools/lvchange.c	2009-03-17 09:19:23.628956367 +0100
@@ -727,6 +727,7 @@ int lvchange(struct cmd_context *cmd, in
 		return EINVALID_CMD_LINE;
 	}
 
-	return process_each_lv(cmd, argc, argv, READ_FOR_UPDATE, NULL,
+	return process_each_lv(cmd, argc, argv,
+			       avail_only ? 0 : READ_FOR_UPDATE, NULL,
 			       &lvchange_single);
 }
 
This is the "Fix segfault when using vgsplit in stacked environment" one:
Mon Feb 16 23:09:02 CET 2009  Petr Rockai <me mornfall net>
  * Refuse to open VG with MISSING_PVs for update unless handles_missing_pvs is set.
diff -rN -u -p old-lvmlib/lib/metadata/metadata.c new-lvmlib/lib/metadata/metadata.c
--- old-lvmlib/lib/metadata/metadata.c	2009-02-16 23:09:25.741274395 +0100
+++ new-lvmlib/lib/metadata/metadata.c	2009-02-16 23:09:25.797274040 +0100
@@ -2546,6 +2546,14 @@ static vg_t *_vg_lock_and_read(struct cm
 		}
 	}
 
+	if (!cmd->handles_missing_pvs && vg_missing_pv_count(vg) &&
+	    (lock_flags & LCK_WRITE)) {
+		log_error("Cannot change VG %s while PVs are missing!",
+			  vg->name);
+		failure |= FAILED_INCONSISTENT; // FIXME
+		goto_bad;
+	}
+
 	failure |= _vg_check_status(vg, status_flags & ~CLUSTERED);
 	if (failure)
 		goto_bad;
diff -rN -u -p old-lvmlib/test/t-partial-activate.sh new-lvmlib/test/t-partial-activate.sh
--- old-lvmlib/test/t-partial-activate.sh	2009-02-16 23:09:25.741274395 +0100
+++ new-lvmlib/test/t-partial-activate.sh	2009-02-16 23:09:25.833271562 +0100
@@ -10,3 +10,7 @@ disable_dev $dev1
 vgreduce --removemissing $vg
 not lvchange -v -a y $vg/mirror
 lvchange -v --partial -a y $vg/mirror
+
+# also check that vgchange works
+vgchange -a n --partial $vg
+vgchange -a y --partial $vg
diff -rN -u -p old-lvmlib/test/t-vgsplit-stacked.sh new-lvmlib/test/t-vgsplit-stacked.sh
--- old-lvmlib/test/t-vgsplit-stacked.sh	1970-01-01 01:00:00.000000000 +0100
+++ new-lvmlib/test/t-vgsplit-stacked.sh	2009-02-16 23:09:25.837272211 +0100
@@ -0,0 +1,17 @@
+. ./test-utils.sh
+
+aux prepare_devs 3
+
+pvcreate $devs
+vgcreate $vg1 $dev1 $dev2
+lvcreate -n $lv1 -l 100%FREE $vg1
+
+#top VG
+pvcreate $G_dev_/$vg1/$lv1
+vgcreate $vg $G_dev_/$vg1/$lv1 $dev3
+
+vgchange -a n $vg
+vgchange -a n $vg1
+
+# this should fail but not segfault
+not vgsplit $vg $vg1 $dev3
diff -rN -u -p old-lvmlib/tools/lvchange.c new-lvmlib/tools/lvchange.c
--- old-lvmlib/tools/lvchange.c	2009-02-16 23:09:25.741274395 +0100
+++ new-lvmlib/tools/lvchange.c	2009-02-16 23:09:25.837272211 +0100
@@ -692,15 +692,20 @@ int lvchange(struct cmd_context *cmd, in
 		return EINVALID_CMD_LINE;
 	}
 
-	if (arg_count(cmd, ignorelockingfailure_ARG) &&
-	    (arg_count(cmd, contiguous_ARG) || arg_count(cmd, permission_ARG) ||
+	int avail_only =
+	    !(arg_count(cmd, contiguous_ARG) || arg_count(cmd, permission_ARG) ||
 	     arg_count(cmd, readahead_ARG) || arg_count(cmd, persistent_ARG) ||
 	     arg_count(cmd, addtag_ARG) || arg_count(cmd, deltag_ARG) ||
-	     arg_count(cmd, refresh_ARG) || arg_count(cmd, alloc_ARG))) {
+	     arg_count(cmd, refresh_ARG) || arg_count(cmd, alloc_ARG));
+
+	if (arg_count(cmd, ignorelockingfailure_ARG) && !avail_only) {
 		log_error("Only -a permitted with --ignorelockingfailure");
 		return EINVALID_CMD_LINE;
 	}
 
+	if (avail_only)
+		cmd->handles_missing_pvs = 1;
+
 	if (!argc) {
 		log_error("Please give logical volume path(s)");
 		return EINVALID_CMD_LINE;
diff -rN -u -p old-lvmlib/tools/vgchange.c new-lvmlib/tools/vgchange.c
--- old-lvmlib/tools/vgchange.c	2009-02-16 23:09:25.741274395 +0100
+++ new-lvmlib/tools/vgchange.c	2009-02-16 23:09:25.841274186 +0100
@@ -126,6 +126,12 @@ static int _vgchange_available(struct cm
 	int available;
 	int activate = 1;
 
+	/*
+	 * Safe, since we never write out new metadata here. Required for
+	 * partial activation to work.
+	 */
+	cmd->handles_missing_pvs = 1;
+
 	available = arg_uint_value(cmd, available_ARG, 0);
 
 	if ((available == CHANGE_AN) || (available == CHANGE_ALN))
diff -rN -u -p old-lvmlib/tools/vgreduce.c new-lvmlib/tools/vgreduce.c
--- old-lvmlib/tools/vgreduce.c	2009-02-16 23:09:25.741274395 +0100
+++ new-lvmlib/tools/vgreduce.c	2009-02-16 23:09:25.845274135 +0100
@@ -506,6 +506,9 @@ int vgreduce(struct cmd_context *cmd, in
 
 	log_verbose("Finding volume group \"%s\"", vg_name);
 
+	if (repairing)
+		cmd->handles_missing_pvs = 1;
+
 	vg = vg_read_for_update(cmd, vg_name, NULL, ALLOW_EXPORTED);
 	if (vg_read_error(vg) == FAILED_ALLOCATION ||
 	    vg_read_error(vg) == FAILED_NOTFOUND)
And this is the ignorelockingfailure re-implementation:
Thu Jan 22 11:12:30 CET 2009  Petr Rockai <me mornfall net>
  * Remove lockingfailed().
  
  We provide a lock type that behaves like no_locking, but is not
  clustered. Moreover, it also forbids any write locks. This magically (and
  consistently) prevents use of clustered VGs, or changing local VGs with
  --ignorelockingfailure. As a bonus, we can remove the special hacks in a few
  places. Of course, people looking for trouble can always set their locking_type
  to 0 to override.
diff -rN -u -p old-lvmlib/lib/locking/locking.c new-lvmlib/lib/locking/locking.c
--- old-lvmlib/lib/locking/locking.c	2009-01-22 11:14:00.158779503 +0100
+++ new-lvmlib/lib/locking/locking.c	2009-01-22 11:14:00.202779927 +0100
@@ -211,8 +211,6 @@ static void _update_vg_lock_count(uint32
  */
 int init_locking(int type, struct cmd_context *cmd)
 {
-	init_lockingfailed(0);
-
 	switch (type) {
 	case 0:
 		init_no_locking(&_locking, cmd);
@@ -271,9 +269,7 @@ int init_locking(int type, struct cmd_co
 
 	/* FIXME Ensure only read ops are permitted */
 	log_verbose("Locking disabled - only read operations permitted.");
-
-	init_no_locking(&_locking, cmd);
-	init_lockingfailed(1);
+	init_boottime_locking(&_locking, cmd);
 
 	return 1;
 }
diff -rN -u -p old-lvmlib/lib/locking/locking_types.h new-lvmlib/lib/locking/locking_types.h
--- old-lvmlib/lib/locking/locking_types.h	2009-01-22 11:14:00.158779503 +0100
+++ new-lvmlib/lib/locking/locking_types.h	2009-01-22 11:14:00.202779927 +0100
@@ -38,6 +38,8 @@ struct locking_type {
  */
 int init_no_locking(struct locking_type *locking, struct cmd_context *cmd);
 
+int init_boottime_locking(struct locking_type *locking, struct cmd_context *cmd);
+
 int init_file_locking(struct locking_type *locking, struct cmd_context *cmd);
 
 int init_external_locking(struct locking_type *locking, struct cmd_context *cmd);
diff -rN -u -p old-lvmlib/lib/locking/no_locking.c new-lvmlib/lib/locking/no_locking.c
--- old-lvmlib/lib/locking/no_locking.c	2009-01-22 11:14:00.158779503 +0100
+++ new-lvmlib/lib/locking/no_locking.c	2009-01-22 11:14:00.202779927 +0100
@@ -66,6 +66,17 @@ static int _no_lock_resource(struct cmd_
 	return 1;
 }
 
+static int _boottime_lock_resource(struct cmd_context *cmd,
+				   const char *resource,
+				   uint32_t flags)
+{
+	if (flags & LCK_TYPE_MASK == LCK_WRITE) {
+		log_error("Write locks are prohibited with --ignorelockingfailure.");
+		return 0;
+	}
+	return _no_lock_resource(cmd, resource, flags);
+}
+
 int init_no_locking(struct locking_type *locking, struct cmd_context *cmd __attribute((unused)))
 {
 	locking->lock_resource = _no_lock_resource;
@@ -75,3 +86,13 @@ int init_no_locking(struct locking_type 
 
 	return 1;
 }
+
+int init_boottime_locking(struct locking_type *locking, struct cmd_context *cmd __attribute((unused)))
+{
+	locking->lock_resource = _boottime_lock_resource;
+	locking->reset_locking = _no_reset_locking;
+	locking->fin_locking = _no_fin_locking;
+	locking->flags = 0;
+
+	return 1;
+}
diff -rN -u -p old-lvmlib/lib/metadata/metadata.c new-lvmlib/lib/metadata/metadata.c
--- old-lvmlib/lib/metadata/metadata.c	2009-01-22 11:14:00.154779134 +0100
+++ new-lvmlib/lib/metadata/metadata.c	2009-01-22 11:14:00.206780576 +0100
@@ -2386,8 +2386,7 @@ static uint32_t _vg_check_status(const s
 	uint32_t failure = 0;
 
 	if ((status & CLUSTERED) &&
-	    (vg_is_clustered(vg)) && !locking_is_clustered() &&
-	    !lockingfailed()) {
+	    (vg_is_clustered(vg)) && !locking_is_clustered()) {
 		log_error("Skipping clustered volume group %s", vg->name);
 		return FAILED_CLUSTERED;
 	}
diff -rN -u -p old-lvmlib/lib/misc/lvm-globals.c new-lvmlib/lib/misc/lvm-globals.c
--- old-lvmlib/lib/misc/lvm-globals.c	2009-01-22 11:14:00.154779134 +0100
+++ new-lvmlib/lib/misc/lvm-globals.c	2009-01-22 11:14:00.206780576 +0100
@@ -31,7 +31,6 @@ static int _trust_cache = 0; /* Don't sc
 static int _debug_level = 0;
 static int _log_cmd_name = 0;
 static int _ignorelockingfailure = 0;
-static int _lockingfailed = 0;
 static int _security_level = SECURITY_LEVEL;
 static char _cmd_name[30] = "";
 static int _mirror_in_sync = 0;
@@ -77,11 +76,6 @@ void init_ignorelockingfailure(int level
 	_ignorelockingfailure = level;
 }
 
-void init_lockingfailed(int level)
-{
-	_lockingfailed = level;
-}
-
 void init_security_level(int level)
 {
 	_security_level = level;
@@ -161,11 +155,6 @@ int trust_cache()
 	return _trust_cache;
 }
 
-int lockingfailed()
-{
-	return _lockingfailed;
-}
-
 int ignorelockingfailure()
 {
 	return _ignorelockingfailure;
diff -rN -u -p old-lvmlib/man/lvm.conf.5 new-lvmlib/man/lvm.conf.5
--- old-lvmlib/man/lvm.conf.5	2009-01-22 11:14:00.054777458 +0100
+++ new-lvmlib/man/lvm.conf.5	2009-01-22 11:14:00.218780704 +0100
@@ -1,4 +1,4 @@
-.TH LVM.CONF 5 "LVM TOOLS" "Sistina Software UK" \" -*- nroff -*-
+.TH LVM.CONF 5 "LVM TOOLS 2.02.44-cvs (2008-11-19)" "Sistina Software UK" \" -*- nroff -*-
 .SH NAME
 lvm.conf \- Configuration file for LVM2
 .SH SYNOPSIS
diff -rN -u -p old-lvmlib/tools/lvchange.c new-lvmlib/tools/lvchange.c
--- old-lvmlib/tools/lvchange.c	2009-01-22 11:14:00.154779134 +0100
+++ new-lvmlib/tools/lvchange.c	2009-01-22 11:14:00.242779145 +0100
@@ -119,12 +119,6 @@ static int lvchange_availability(struct 
 		if (!deactivate_lv(cmd, lv))
 			return_0;
 	} else {
-		if (lockingfailed() && (vg_is_clustered(lv->vg))) {
-			log_verbose("Locking failed: ignoring clustered "
-				    "logical volume %s", lv->name);
-			return 0;
-		}
-
 		if (lv_is_origin(lv) || (activate == CHANGE_AE)) {
 			log_verbose("Activating logical volume \"%s\" "
 				    "exclusively", lv->name);
diff -rN -u -p old-lvmlib/tools/vgchange.c new-lvmlib/tools/vgchange.c
--- old-lvmlib/tools/vgchange.c	2009-01-22 11:14:00.154779134 +0100
+++ new-lvmlib/tools/vgchange.c	2009-01-22 11:14:00.246780631 +0100
@@ -138,14 +138,8 @@ static int _vgchange_available(struct cm
 		return ECMD_FAILED;
 	}
 
-	if (activate && lockingfailed() && (vg_is_clustered(vg))) {
-		log_error("Locking inactive: ignoring clustered "
-			  "volume group %s", vg->name);
-		return ECMD_FAILED;
-	}
-
 	/* FIXME Move into library where clvmd can use it */
-	if (activate && !lockingfailed())
+	if (activate)
 		check_current_backup(vg);
 
 	if (activate && (active = lvs_in_vg_activated(vg))) {


-- 
Peter Rockai | me()mornfall!net | prockai()redhat!com
 http://blog.mornfall.net | http://web.mornfall.net

"In My Egotistical Opinion, most people's C programs should be
 indented six feet downward and covered with dirt."
     -- Blair P. Houghton on the subject of C program indentation

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