[PATCH 0/4]: Remove unnecessary flags from new vg_read() APIs and tools

[PATCH 0/4]: Remove unnecessary flags from new vg_read() APIs and tools

[Dave Wysochanski]

The following 4 patches remove various flags from the new vg_read() interface.
Many of these flags provided a useful stepping stone from where we were,
but now it would be good to simplify the API as much as possible.

[PATCH] Remove LOCK_KEEP and READ_CHECK_EXISTENCE from vgsplit.

[Dave Wysochanski]

Remove LOCK_KEEP and READ_CHECK_EXISTENCE from vgsplit.
These flags are no longer necessary.  We now check for existence
in a differnet function, and it is not necessary to keep the lock.
Removing these flags simplifies the new vg_read() interface.
After this patch, we can fully remove LOCK_KEEP.
READ_CHECK_EXISTENCE needs a bit more work before full removal.

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 tools/vgsplit.c |   12 +++++++-----
 1 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/tools/vgsplit.c b/tools/vgsplit.c
index 9cd90b8..0dc5bc4 100644
--- a/tools/vgsplit.c
+++ b/tools/vgsplit.c
@@ -342,11 +342,12 @@ int vgsplit(struct cmd_context *cmd, int argc, char **argv)
 		existing_vg = 1;
 		vg_to = vg_read_for_update(cmd, vg_name_to, NULL,
 					   READ_REQUIRE_RESIZEABLE |
-					   LOCK_NONBLOCKING | LOCK_KEEP |
-					   READ_CHECK_EXISTENCE);
+					   LOCK_NONBLOCKING);
 
-		if (vg_read_error(vg_to))
-			goto_bad;
+		if (vg_read_error(vg_to)) {
+			vg_release(vg_to);
+			goto bad2;
+		}
 
 		if (new_vg_option_specified(cmd)) {
 			log_error("Volume group \"%s\" exists, but new VG "
@@ -483,7 +484,8 @@ int vgsplit(struct cmd_context *cmd, int argc, char **argv)
 	r = ECMD_PROCESSED;
 
 bad:
-	unlock_and_release_vg(cmd, vg_from, vg_name_from);
 	unlock_and_release_vg(cmd, vg_to, vg_name_to);
+bad2:
+	unlock_and_release_vg(cmd, vg_from, vg_name_from);
 	return r;
 }
-- 
1.6.0.6

[PATCH] Remove unneeded LOCK_KEEP from vg_read() interface.

[Dave Wysochanski]

Remove unneeded LOCK_KEEP from vg_read() interface.
Update comment to clarify cases where _vg_lock_and_read() may return
with an error but the lock held.  Would be nice to make the vg_read()
interface consistent with regards to lock held and error behavior.

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 lib/metadata/metadata-exported.h |    1 -
 lib/metadata/metadata.c          |    9 ++++++---
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/lib/metadata/metadata-exported.h b/lib/metadata/metadata-exported.h
index b9e9591..6e45698 100644
--- a/lib/metadata/metadata-exported.h
+++ b/lib/metadata/metadata-exported.h
@@ -113,7 +113,6 @@ struct pv_segment;
 
 /* FIXME Deduce these next requirements internally instead of having caller specify. */
 #define LOCK_NONBLOCKING	0x00000100U	/* Fail if not available immediately. */
-#define LOCK_KEEP		0x00000200U	/* Do not unlock upon read failure. */
 
 /* A meta-flag, useful with toollib for_each_* functions. */
 #define READ_FOR_UPDATE 	0x00100000U
diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
index fda07be..3c25847 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -2793,7 +2793,6 @@ static vg_t *_vg_lock_and_read(struct cmd_context *cmd, const char *vg_name,
 			goto_bad;
 		}
 	}
-	
 
 	failure |= _vg_bad_status_bits(vg, status_flags);
 	if (failure)
@@ -2803,7 +2802,7 @@ static vg_t *_vg_lock_and_read(struct cmd_context *cmd, const char *vg_name,
 
 bad:
 	if (failure != (FAILED_NOTFOUND | READ_CHECK_EXISTENCE) &&
-	    !(misc_flags & LOCK_KEEP) && !already_locked)
+	    !already_locked)
 		unlock_vg(cmd, lock_name);
 
 	return _vg_make_handle(cmd, vg, failure);
@@ -2821,7 +2820,11 @@ bad:
  *    FAILED_RESIZEABLE
  *  - locking failed: FAILED_LOCKING
  *
- * On failures, all locks are released, unless LOCK_KEEP has been supplied.
+ * On failures, all locks are released, unless one of the following applies:
+ *  - failure == (FAILED_NOTFOUND | READ_CHECK_EXISTENCE)
+ *  - vgname_is_locked(lock_name) is true
+ * FIXME: remove the above 2 conditions if possible and make an error always
+ * release the lock.
  *
  * Volume groups are opened read-only unless flags contains READ_FOR_UPDATE.
  *
-- 
1.6.0.6

[PATCH] Remove READ_CHECK_EXISTENCE and vg_might_exist().

[Dave Wysochanski]

Remove READ_CHECK_EXISTENCE and vg_might_exist().
This flag and API is no longer used now that we have a separate
API to check for existence.

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 lib/metadata/metadata-exported.h |    2 -
 lib/metadata/metadata.c          |   47 +------------------------------------
 2 files changed, 2 insertions(+), 47 deletions(-)

diff --git a/lib/metadata/metadata-exported.h b/lib/metadata/metadata-exported.h
index 6e45698..33eeae0 100644
--- a/lib/metadata/metadata-exported.h
+++ b/lib/metadata/metadata-exported.h
@@ -109,7 +109,6 @@ struct pv_segment;
 #define READ_ALLOW_INCONSISTENT	0x00010000U
 #define READ_ALLOW_EXPORTED	0x00020000U
 #define READ_REQUIRE_RESIZEABLE	0x00040000U
-#define READ_CHECK_EXISTENCE	0x00080000U	/* Also used in vg->read_status */
 
 /* FIXME Deduce these next requirements internally instead of having caller specify. */
 #define LOCK_NONBLOCKING	0x00000100U	/* Fail if not available immediately. */
@@ -406,7 +405,6 @@ vg_t *vg_read_for_update(struct cmd_context *cmd, const char *vg_name,
  * Test validity of a VG handle.
  */
 uint32_t vg_read_error(vg_t *vg_handle);
-uint32_t vg_might_exist(vg_t *vg_handle);
 
 /* pe_start and pe_end relate to any existing data so that new metadata
 * areas can avoid overlap */
diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
index 3c25847..25c2795 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -2754,9 +2754,6 @@ static vg_t *_vg_lock_and_read(struct cmd_context *cmd, const char *vg_name,
 	if (is_orphan_vg(vg_name))
 		status_flags &= ~LVM_WRITE;
 
-	if (misc_flags & READ_CHECK_EXISTENCE)
-		consistent = 0;
-
 	consistent_in = consistent;
 
 	/* If consistent == 1, we get NULL here if correction fails. */
@@ -2767,10 +2764,7 @@ static vg_t *_vg_lock_and_read(struct cmd_context *cmd, const char *vg_name,
 			goto_bad;
 		}
 
-		if (!(misc_flags & READ_CHECK_EXISTENCE))
-			log_error("Volume group \"%s\" not found", vg_name);
-		else
-			failure |= READ_CHECK_EXISTENCE;
+		log_error("Volume group \"%s\" not found", vg_name);
 
 		failure |= FAILED_NOTFOUND;
 		goto_bad;
@@ -2801,8 +2795,7 @@ static vg_t *_vg_lock_and_read(struct cmd_context *cmd, const char *vg_name,
 	return _vg_make_handle(cmd, vg, failure);
 
 bad:
-	if (failure != (FAILED_NOTFOUND | READ_CHECK_EXISTENCE) &&
-	    !already_locked)
+	if (!already_locked)
 		unlock_vg(cmd, lock_name);
 
 	return _vg_make_handle(cmd, vg, failure);
@@ -2821,7 +2814,6 @@ bad:
  *  - locking failed: FAILED_LOCKING
  *
  * On failures, all locks are released, unless one of the following applies:
- *  - failure == (FAILED_NOTFOUND | READ_CHECK_EXISTENCE)
  *  - vgname_is_locked(lock_name) is true
  * FIXME: remove the above 2 conditions if possible and make an error always
  * release the lock.
@@ -2830,12 +2822,6 @@ bad:
  *
  * Checking for VG existence:
  *
- * If READ_CHECK_EXISTENCE is set in flags, if the VG exists, a non-NULL struct
- * volume_group will be returned every time, but if it has INCONSISTENT_VG set,
- * the other fields will be uninitialized.  You must check for INCONSISTENT_VG
- * if passing READ_CHECK_EXISTENCE.  You also must not use it if it has
- * INCONSISTENT_VG set.
- *
  * FIXME: We want vg_read to attempt automatic recovery after acquiring a
  * temporary write lock: if that fails, we bail out as usual, with failed &
  * FAILED_INCONSISTENT. If it works, we are good to go. Code that's been in
@@ -2878,45 +2864,16 @@ vg_t *vg_read_for_update(struct cmd_context *cmd, const char *vg_name,
 
 /*
  * Test the validity of a VG handle returned by vg_read() or vg_read_for_update().
- *
- * If READ_CHECK_EXISTENCE was supplied the non-existence of the volume group
- * is not considered an error.
- *
- * !vg_read_error() && vg_might_exist() => valid handle to VG.
- * vg_read_error() && vg_might_exist() => handle invalid, but VG might
- *					  exist but cannot be read.
- * !vg_read_error() && !vg_might_exist() => the VG does not exist
- * vg_read_error() && !vg_might_exist() is impossible.
  */
 uint32_t vg_read_error(vg_t *vg_handle)
 {
 	if (!vg_handle)
 		return FAILED_ALLOCATION;
 
-	if (vg_handle->read_status & READ_CHECK_EXISTENCE)
-		return vg_handle->read_status &
-		       ~(READ_CHECK_EXISTENCE | FAILED_NOTFOUND);
-
 	return vg_handle->read_status;
 }
 
 /*
- * Returns true if the volume group already exists.
- * If unsure, it will return true. It might exist but the read failed
- * for some other reason.
- */
-uint32_t vg_might_exist(vg_t *vg_handle)
-{
-	if (!vg_handle)
-		return 1;
-
-	if (vg_handle->read_status == (FAILED_NOTFOUND | READ_CHECK_EXISTENCE))
-		return 0;
-
-	return 1;
-}
-
-/*
  * Lock a vgname and/or check for existence.
  * Takes a WRITE lock on the vgname before scanning.
  * If scanning fails or vgname found, release the lock.
-- 
1.6.0.6

[PATCH] Remove unneeded LOCK_NONBLOCKING from vg_read() API.

[Dave Wysochanski]

Remove unneeded LOCK_NONBLOCKING from vg_read() API and tools that
use it.  We no longer need this flag anywhere since we now automatically
set LCK_NONBLOCK inside lock_vol() if vgs_locked().
For further details, see:
commit d52b3fd3fe2006e2d13e42f8518b6512bff03710
Author: Dave Wysochanski <dwysocha@redhat.com>
Date:   Wed May 13 13:02:52 2009 +0000

    Remove NON_BLOCKING lock flag from tools and set a policy to auto-set.

    As a simplification to the tools and further liblvm, this patch pushes
    the setting of NON_BLOCKING lock flag inside the lock_vol() call.
    The policy we set is if any existing VGs are currently locked, we
    set the NON_BLOCKING flag.

At some point it may make sense to add this flag back if we get an
RFE from a liblvm user, but for now let's keep it as simple as
possible.

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 lib/metadata/metadata-exported.h |    3 ---
 lib/metadata/metadata.c          |    3 ---
 tools/vgextend.c                 |    2 +-
 tools/vgmerge.c                  |    3 +--
 tools/vgreduce.c                 |    2 +-
 tools/vgremove.c                 |    2 +-
 tools/vgsplit.c                  |    3 +--
 7 files changed, 5 insertions(+), 13 deletions(-)

diff --git a/lib/metadata/metadata-exported.h b/lib/metadata/metadata-exported.h
index 33eeae0..e49aa9c 100644
--- a/lib/metadata/metadata-exported.h
+++ b/lib/metadata/metadata-exported.h
@@ -110,9 +110,6 @@ struct pv_segment;
 #define READ_ALLOW_EXPORTED	0x00020000U
 #define READ_REQUIRE_RESIZEABLE	0x00040000U
 
-/* FIXME Deduce these next requirements internally instead of having caller specify. */
-#define LOCK_NONBLOCKING	0x00000100U	/* Fail if not available immediately. */
-
 /* A meta-flag, useful with toollib for_each_* functions. */
 #define READ_FOR_UPDATE 	0x00100000U
 
diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
index 25c2795..449ac4c 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -2845,9 +2845,6 @@ vg_t *vg_read(struct cmd_context *cmd, const char *vg_name,
 	if (flags & READ_REQUIRE_RESIZEABLE)
 		status |= RESIZEABLE_VG;
 
-	if (flags & LOCK_NONBLOCKING)
-		lock_flags |= LCK_NONBLOCK;
-
 	return _vg_lock_and_read(cmd, vg_name, vgid, lock_flags, status, flags);
 }
 
diff --git a/tools/vgextend.c b/tools/vgextend.c
index 1000dc2..a08d4db 100644
--- a/tools/vgextend.c
+++ b/tools/vgextend.c
@@ -43,7 +43,7 @@ int vgextend(struct cmd_context *cmd, int argc, char **argv)
 
 	log_verbose("Checking for volume group \"%s\"", vg_name);
 	vg = vg_read_for_update(cmd, vg_name, NULL,
-				READ_REQUIRE_RESIZEABLE | LOCK_NONBLOCKING);
+				READ_REQUIRE_RESIZEABLE);
 	if (vg_read_error(vg)) {
 		vg_release(vg);
 		unlock_vg(cmd, VG_ORPHANS);
diff --git a/tools/vgmerge.c b/tools/vgmerge.c
index 4a9e377..37f6bd7 100644
--- a/tools/vgmerge.c
+++ b/tools/vgmerge.c
@@ -35,8 +35,7 @@ static int _vgmerge_single(struct cmd_context *cmd, const char *vg_name_to,
 	}
 
 	log_verbose("Checking for volume group \"%s\"", vg_name_from);
-	vg_from = vg_read_for_update(cmd, vg_name_from, NULL,
-				     LOCK_NONBLOCKING);
+	vg_from = vg_read_for_update(cmd, vg_name_from, NULL, 0);
 	if (vg_read_error(vg_from)) {
 		vg_release(vg_from);
 		unlock_and_release_vg(cmd, vg_to, vg_name_to);
diff --git a/tools/vgreduce.c b/tools/vgreduce.c
index 44010e3..b3b6dba 100644
--- a/tools/vgreduce.c
+++ b/tools/vgreduce.c
@@ -424,7 +424,7 @@ static int _vgreduce_single(struct cmd_context *cmd, struct volume_group *vg,
 	vg->extent_count -= pv_pe_count(pv);
 
 	orphan_vg = vg_read_for_update(cmd, vg->fid->fmt->orphan_vg_name,
-				       NULL, LOCK_NONBLOCKING);
+				       NULL, 0);
 
 	if (vg_read_error(orphan_vg))
 		goto bad;
diff --git a/tools/vgremove.c b/tools/vgremove.c
index 5956075..ff35720 100644
--- a/tools/vgremove.c
+++ b/tools/vgremove.c
@@ -41,7 +41,7 @@ int vgremove(struct cmd_context *cmd, int argc, char **argv)
 	}
 
 	ret = process_each_vg(cmd, argc, argv,
-			      READ_FOR_UPDATE | LOCK_NONBLOCKING,
+			      READ_FOR_UPDATE,
 			      NULL, &vgremove_single);
 
 	unlock_vg(cmd, VG_ORPHANS);
diff --git a/tools/vgsplit.c b/tools/vgsplit.c
index 0dc5bc4..6d6090b 100644
--- a/tools/vgsplit.c
+++ b/tools/vgsplit.c
@@ -341,8 +341,7 @@ int vgsplit(struct cmd_context *cmd, int argc, char **argv)
 	if (rc == FAILED_EXIST) {
 		existing_vg = 1;
 		vg_to = vg_read_for_update(cmd, vg_name_to, NULL,
-					   READ_REQUIRE_RESIZEABLE |
-					   LOCK_NONBLOCKING);
+					   READ_REQUIRE_RESIZEABLE);
 
 		if (vg_read_error(vg_to)) {
 			vg_release(vg_to);
-- 
1.6.0.6

[PATCH] Remove LOCK_KEEP and READ_CHECK_EXISTENCE from vgsplit.

[Milan Broz]

Dave Wysochanski wrote:
> diff --git a/tools/vgsplit.c b/tools/vgsplit.c
> index 9cd90b8..0dc5bc4 100644
> --- a/tools/vgsplit.c
> +++ b/tools/vgsplit.c
> @@ -342,11 +342,12 @@ int vgsplit(struct cmd_context *cmd, int argc, char **argv)
>  		existing_vg = 1;
>  		vg_to = vg_read_for_update(cmd, vg_name_to, NULL,
>  					   READ_REQUIRE_RESIZEABLE |
> -					   LOCK_NONBLOCKING | LOCK_KEEP |
> -					   READ_CHECK_EXISTENCE);
> +					   LOCK_NONBLOCKING);
>  
> -		if (vg_read_error(vg_to))
> -			goto_bad;
> +		if (vg_read_error(vg_to)) {

	but vg_name_to is locked by vg_lock_newname() previously?
	so unlock_and_release here?

	also now code missing stack; (note goto_bad before)

> +			vg_release(vg_to);
> +			goto bad2;
> +		}
>  


Milan

[PATCH] Remove LOCK_KEEP and READ_CHECK_EXISTENCE from vgsplit.

[Dave Wysochanski]

On Tue, 2009-07-07 at 11:01 +0200, Milan Broz wrote:
> Dave Wysochanski wrote:
> > diff --git a/tools/vgsplit.c b/tools/vgsplit.c
> > index 9cd90b8..0dc5bc4 100644
> > --- a/tools/vgsplit.c
> > +++ b/tools/vgsplit.c
> > @@ -342,11 +342,12 @@ int vgsplit(struct cmd_context *cmd, int argc, char **argv)
> >  		existing_vg = 1;
> >  		vg_to = vg_read_for_update(cmd, vg_name_to, NULL,
> >  					   READ_REQUIRE_RESIZEABLE |
> > -					   LOCK_NONBLOCKING | LOCK_KEEP |
> > -					   READ_CHECK_EXISTENCE);
> > +					   LOCK_NONBLOCKING);
> >  
> > -		if (vg_read_error(vg_to))
> > -			goto_bad;
> > +		if (vg_read_error(vg_to)) {
> 
> 	but vg_name_to is locked by vg_lock_newname() previously?
> 	so unlock_and_release here?
> 

No - if vg_lock_newname() returns any failure code, it releases the
lock.  In this case it is FAILED_EXIST.


> 	also now code missing stack; (note goto_bad before)
> 

Right - good catch.


> > +			vg_release(vg_to);
> > +			goto bad2;
> > +		}
> >  
> 
> 
> Milan
> 
> --
> lvm-devel mailing list
> lvm-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/lvm-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Remove-LOCK_KEEP-and-READ_CHECK_EXISTENCE-from-vgspl.patch
Type: text/x-patch
Size: 1676 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20090707/261d7dbd/attachment.bin>

[PATCH 0/4]: Remove unnecessary flags from new vg_read() APIs and tools

[Alasdair G Kergon]

On Tue, Jul 07, 2009 at 12:24:20AM -0400, Dave Wysochanski wrote:
> The following 4 patches remove various flags from the new vg_read() interface.
> Many of these flags provided a useful stepping stone from where we were,
> but now it would be good to simplify the API as much as possible.
 
Ack.

Alasdair.