* [PATCH 0/4]: Remove unnecessary flags from new vg_read() APIs and tools @ 2009-07-07 4:24 Dave Wysochanski 2009-07-07 4:24 ` [PATCH] Remove LOCK_KEEP and READ_CHECK_EXISTENCE from vgsplit Dave Wysochanski 2009-07-07 21:17 ` [PATCH 0/4]: Remove unnecessary flags from new vg_read() APIs and tools Alasdair G Kergon 0 siblings, 2 replies; 8+ messages in thread From: Dave Wysochanski @ 2009-07-07 4:24 UTC (permalink / raw) To: lvm-devel 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] Remove LOCK_KEEP and READ_CHECK_EXISTENCE from vgsplit. 2009-07-07 4:24 [PATCH 0/4]: Remove unnecessary flags from new vg_read() APIs and tools Dave Wysochanski @ 2009-07-07 4:24 ` Dave Wysochanski 2009-07-07 4:24 ` [PATCH] Remove unneeded LOCK_KEEP from vg_read() interface Dave Wysochanski 2009-07-07 9:01 ` [PATCH] Remove LOCK_KEEP and READ_CHECK_EXISTENCE from vgsplit Milan Broz 2009-07-07 21:17 ` [PATCH 0/4]: Remove unnecessary flags from new vg_read() APIs and tools Alasdair G Kergon 1 sibling, 2 replies; 8+ messages in thread From: Dave Wysochanski @ 2009-07-07 4:24 UTC (permalink / raw) To: lvm-devel 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH] Remove unneeded LOCK_KEEP from vg_read() interface. 2009-07-07 4:24 ` [PATCH] Remove LOCK_KEEP and READ_CHECK_EXISTENCE from vgsplit Dave Wysochanski @ 2009-07-07 4:24 ` Dave Wysochanski 2009-07-07 4:24 ` [PATCH] Remove READ_CHECK_EXISTENCE and vg_might_exist() Dave Wysochanski 2009-07-07 9:01 ` [PATCH] Remove LOCK_KEEP and READ_CHECK_EXISTENCE from vgsplit Milan Broz 1 sibling, 1 reply; 8+ messages in thread From: Dave Wysochanski @ 2009-07-07 4:24 UTC (permalink / raw) To: lvm-devel 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH] Remove READ_CHECK_EXISTENCE and vg_might_exist(). 2009-07-07 4:24 ` [PATCH] Remove unneeded LOCK_KEEP from vg_read() interface Dave Wysochanski @ 2009-07-07 4:24 ` Dave Wysochanski 2009-07-07 4:24 ` [PATCH] Remove unneeded LOCK_NONBLOCKING from vg_read() API Dave Wysochanski 0 siblings, 1 reply; 8+ messages in thread From: Dave Wysochanski @ 2009-07-07 4:24 UTC (permalink / raw) To: lvm-devel 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH] Remove unneeded LOCK_NONBLOCKING from vg_read() API. 2009-07-07 4:24 ` [PATCH] Remove READ_CHECK_EXISTENCE and vg_might_exist() Dave Wysochanski @ 2009-07-07 4:24 ` Dave Wysochanski 0 siblings, 0 replies; 8+ messages in thread From: Dave Wysochanski @ 2009-07-07 4:24 UTC (permalink / raw) To: lvm-devel 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH] Remove LOCK_KEEP and READ_CHECK_EXISTENCE from vgsplit. 2009-07-07 4:24 ` [PATCH] Remove LOCK_KEEP and READ_CHECK_EXISTENCE from vgsplit Dave Wysochanski 2009-07-07 4:24 ` [PATCH] Remove unneeded LOCK_KEEP from vg_read() interface Dave Wysochanski @ 2009-07-07 9:01 ` Milan Broz 2009-07-07 13:11 ` Dave Wysochanski 1 sibling, 1 reply; 8+ messages in thread From: Milan Broz @ 2009-07-07 9:01 UTC (permalink / raw) To: lvm-devel 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] Remove LOCK_KEEP and READ_CHECK_EXISTENCE from vgsplit. 2009-07-07 9:01 ` [PATCH] Remove LOCK_KEEP and READ_CHECK_EXISTENCE from vgsplit Milan Broz @ 2009-07-07 13:11 ` Dave Wysochanski 0 siblings, 0 replies; 8+ messages in thread From: Dave Wysochanski @ 2009-07-07 13:11 UTC (permalink / raw) To: lvm-devel 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> ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 0/4]: Remove unnecessary flags from new vg_read() APIs and tools 2009-07-07 4:24 [PATCH 0/4]: Remove unnecessary flags from new vg_read() APIs and tools Dave Wysochanski 2009-07-07 4:24 ` [PATCH] Remove LOCK_KEEP and READ_CHECK_EXISTENCE from vgsplit Dave Wysochanski @ 2009-07-07 21:17 ` Alasdair G Kergon 1 sibling, 0 replies; 8+ messages in thread From: Alasdair G Kergon @ 2009-07-07 21:17 UTC (permalink / raw) To: lvm-devel 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2009-07-07 21:17 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-07-07 4:24 [PATCH 0/4]: Remove unnecessary flags from new vg_read() APIs and tools Dave Wysochanski 2009-07-07 4:24 ` [PATCH] Remove LOCK_KEEP and READ_CHECK_EXISTENCE from vgsplit Dave Wysochanski 2009-07-07 4:24 ` [PATCH] Remove unneeded LOCK_KEEP from vg_read() interface Dave Wysochanski 2009-07-07 4:24 ` [PATCH] Remove READ_CHECK_EXISTENCE and vg_might_exist() Dave Wysochanski 2009-07-07 4:24 ` [PATCH] Remove unneeded LOCK_NONBLOCKING from vg_read() API Dave Wysochanski 2009-07-07 9:01 ` [PATCH] Remove LOCK_KEEP and READ_CHECK_EXISTENCE from vgsplit Milan Broz 2009-07-07 13:11 ` Dave Wysochanski 2009-07-07 21:17 ` [PATCH 0/4]: Remove unnecessary flags from new vg_read() APIs and tools Alasdair G Kergon
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.