* [PATCH] Remove newly created log volume if initial deactivation fails. @ 2009-12-03 13:31 Milan Broz 2009-12-03 17:49 ` Dave Wysochanski 2009-12-04 15:03 ` Milan Broz 0 siblings, 2 replies; 4+ messages in thread From: Milan Broz @ 2009-12-03 13:31 UTC (permalink / raw) To: lvm-devel 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.) Signed-off-by: Milan Broz <mbroz@redhat.com> --- lib/metadata/mirror.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/metadata/mirror.c b/lib/metadata/mirror.c index c331e0e..4c24058 100644 --- a/lib/metadata/mirror.c +++ b/lib/metadata/mirror.c @@ -256,7 +256,7 @@ 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; + goto revert_new_lv; was_active = 1; } @@ -1354,7 +1354,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.3 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH] Remove newly created log volume if initial deactivation fails. 2009-12-03 13:31 [PATCH] Remove newly created log volume if initial deactivation fails Milan Broz @ 2009-12-03 17:49 ` Dave Wysochanski 2009-12-03 18:06 ` Milan Broz 2009-12-04 15:03 ` Milan Broz 1 sibling, 1 reply; 4+ messages in thread From: Dave Wysochanski @ 2009-12-03 17:49 UTC (permalink / raw) To: lvm-devel On Thu, 2009-12-03 at 14:31 +0100, Milan Broz wrote: > 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.) > > Signed-off-by: Milan Broz <mbroz@redhat.com> > --- > lib/metadata/mirror.c | 4 ++-- > 1 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/lib/metadata/mirror.c b/lib/metadata/mirror.c > index c331e0e..4c24058 100644 > --- a/lib/metadata/mirror.c > +++ b/lib/metadata/mirror.c > @@ -256,7 +256,7 @@ 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; > + goto revert_new_lv; > was_active = 1; > } > This doesn't look right based on my read of the function. First, if we goto revert_new_lv, we do all this recovery which is based on things that have not happened. Also, later in the function we have this: if (!deactivate_lv(cmd, log_lv)) { log_error("Aborting. Failed to deactivate mirror log. " "Manual intervention required."); return 0; } Do we just need a log_error() in your place above or is the recovery wrong later? > @@ -1354,7 +1354,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; > } > ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH] Remove newly created log volume if initial deactivation fails. 2009-12-03 17:49 ` Dave Wysochanski @ 2009-12-03 18:06 ` Milan Broz 0 siblings, 0 replies; 4+ messages in thread From: Milan Broz @ 2009-12-03 18:06 UTC (permalink / raw) To: lvm-devel On 12/03/2009 06:49 PM, Dave Wysochanski wrote: > On Thu, 2009-12-03 at 14:31 +0100, Milan Broz wrote: >> 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.) >> >> Signed-off-by: Milan Broz <mbroz@redhat.com> >> --- >> lib/metadata/mirror.c | 4 ++-- >> 1 files changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/lib/metadata/mirror.c b/lib/metadata/mirror.c >> index c331e0e..4c24058 100644 >> --- a/lib/metadata/mirror.c >> +++ b/lib/metadata/mirror.c >> @@ -256,7 +256,7 @@ 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; >> + goto revert_new_lv; >> was_active = 1; >> } >> > > This doesn't look right based on my read of the function. > First, if we goto revert_new_lv, we do all this recovery which is based > on things that have not happened. Also, later in the function we have > this: this is another case - the log LV was not active before but after creation its deactivation failed. Read the fuction one level up - _set_up_mirror_log if (!(log_lv = _create_mirror_log(lv, ah, alloc, (const char *) lv_name, suffix))) { log_error("Failed to create mirror log."); return NULL; } if (!_init_mirror_log(cmd, log_lv, in_sync, &lv->tags, 1)) { log_error("Failed to initialise mirror log."); >>> we fail here, returning NULL but with metadata containing LV create a few lines above. return NULL; } return log_lv; I am fixing the path that completely unrelated device conflicts with internally created device - so yes, manual intervention is needed too but not to fix metadata but only remove that conflicting device. > if (!deactivate_lv(cmd, log_lv)) { > log_error("Aborting. Failed to deactivate mirror log. " > "Manual intervention required."); > return 0; > } First, this is not patch to fix tests run, but part of my attempts to reproduce bug 538571. But this patch just tries to not leave metadata with _mlog device present, partially abusing this error path. I think that run backup_vg() and some related operations on LV which is going to be removed is maybe redundant here, but should not be problem. Note, that this code path run _only_ if log_lv was previously created in the function which calls this - so we can safely remove it (remove_on_failure is set). > Do we just need a log_error() in your place above or is the recovery wrong later? Code should remove the newly created log_lv if initialisation fails. Milan ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH] Remove newly created log volume if initial deactivation fails. 2009-12-03 13:31 [PATCH] Remove newly created log volume if initial deactivation fails Milan Broz 2009-12-03 17:49 ` Dave Wysochanski @ 2009-12-04 15:03 ` Milan Broz 1 sibling, 0 replies; 4+ messages in thread From: Milan Broz @ 2009-12-04 15:03 UTC (permalink / raw) To: lvm-devel (this works even on cluster - this help to debug situations like in bug 538571) Remove newly created log volume if initial deactivation fails. 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 # (old) 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; } ^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2009-12-04 15:03 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-12-03 13:31 [PATCH] Remove newly created log volume if initial deactivation fails Milan Broz 2009-12-03 17:49 ` Dave Wysochanski 2009-12-03 18:06 ` Milan Broz 2009-12-04 15:03 ` Milan Broz
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.