All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] Fix vg_write/vg_commit to not reset position in metadata ring buffer on vgrename
@ 2010-03-26 13:56 Peter Rajnoha
  2010-03-26 16:15 ` Dave Wysochanski
  0 siblings, 1 reply; 6+ messages in thread
From: Peter Rajnoha @ 2010-03-26 13:56 UTC (permalink / raw)
  To: lvm-devel

We should write metadata into next position in the ring buffer while calling vgrename.
At this code level (_vg_write_raw), we're not able to determine if this is a rename
or not. If yes, then accompanying VG structure passed here has a new name set,
not the old one.

When looking for a location where to put metadata next, we're given a NULL value because
of failed VG name comparison (in _find_vg_rlocn) between the name in existing metadata
and metadata we're just about to write.

This resets the position in the ring buffer, overwriting any existing metadata
(and also incorrectly updates the cache to "orphan" afterwards).

This patch just adds old_name item in struct volume_group that we can check and use
if necessary and detect renames at lower layers as well.

Peter

 lib/format_text/format-text.c    |   17 +++++++++++++----
 lib/metadata/metadata-exported.h |    1 +
 lib/metadata/metadata.c          |    2 ++
 3 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/lib/format_text/format-text.c b/lib/format_text/format-text.c
index ad4db34..4856d07 100644
--- a/lib/format_text/format-text.c
+++ b/lib/format_text/format-text.c
@@ -380,6 +380,10 @@ static struct raw_locn *_find_vg_rlocn(struct device_area *dev_area,
 	} else
 		*precommitted = 0;
 
+	/* Do not check non-existent metadata. */
+	if (!rlocn->offset && !rlocn->size)
+		return NULL;
+
 	/* FIXME Loop through rlocns two-at-a-time.  List null-terminated. */
 	/* FIXME Ignore if checksum incorrect!!! */
 	if (!dev_read(dev_area->dev, dev_area->start + rlocn->offset,
@@ -387,9 +391,11 @@ static struct raw_locn *_find_vg_rlocn(struct device_area *dev_area,
 		goto_bad;
 
 	if (!strncmp(vgnamebuf, vgname, len = strlen(vgname)) &&
-	    (isspace(vgnamebuf[len]) || vgnamebuf[len] == '{')) {
+	    (isspace(vgnamebuf[len]) || vgnamebuf[len] == '{'))
 		return rlocn;
-	}
+	else
+		log_debug("Volume group name found in metadata does "
+			  "not match expected name %s.", vgname);
 
       bad:
 	if ((info = info_from_pvid(dev_area->dev->pvid, 0)))
@@ -542,7 +548,8 @@ static int _vg_write_raw(struct format_instance *fid, struct volume_group *vg,
 	if (!(mdah = _raw_read_mda_header(fid->fmt, &mdac->area)))
 		goto_out;
 
-	rlocn = _find_vg_rlocn(&mdac->area, mdah, vg->name, &noprecommit);
+	rlocn = _find_vg_rlocn(&mdac->area, mdah,
+			vg->old_name ? vg->old_name : vg->name, &noprecommit);
 	mdac->rlocn.offset = _next_rlocn_offset(rlocn, mdah);
 
 	if (!fidtc->raw_metadata_buf &&
@@ -647,7 +654,9 @@ static int _vg_commit_raw_rlocn(struct format_instance *fid,
 	if (!(mdah = _raw_read_mda_header(fid->fmt, &mdac->area)))
 		goto_out;
 
-	if (!(rlocn = _find_vg_rlocn(&mdac->area, mdah, vg->name, &noprecommit))) {
+	if (!(rlocn = _find_vg_rlocn(&mdac->area, mdah,
+				     vg->old_name ? vg->old_name : vg->name,
+				     &noprecommit))) {
 		mdah->raw_locns[0].offset = 0;
 		mdah->raw_locns[0].size = 0;
 		mdah->raw_locns[0].checksum = 0;
diff --git a/lib/metadata/metadata-exported.h b/lib/metadata/metadata-exported.h
index 2f1405f..855ab47 100644
--- a/lib/metadata/metadata-exported.h
+++ b/lib/metadata/metadata-exported.h
@@ -216,6 +216,7 @@ struct volume_group {
 
 	struct id id;
 	char *name;
+	char *old_name;
 	char *system_id;
 
 	uint32_t extent_size;
diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
index 020e897..867e004 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -437,6 +437,8 @@ int vg_rename(struct cmd_context *cmd, struct volume_group *vg,
 	struct dm_pool *mem = vg->vgmem;
 	struct pv_list *pvl;
 
+	vg->old_name = vg->name;
+
 	if (!(vg->name = dm_pool_strdup(mem, new_name))) {
 		log_error("vg->name allocation failed for '%s'", new_name);
 		return 0;



^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH] Fix vg_write/vg_commit to not reset position in metadata ring buffer on vgrename
  2010-03-26 13:56 [PATCH] Fix vg_write/vg_commit to not reset position in metadata ring buffer on vgrename Peter Rajnoha
@ 2010-03-26 16:15 ` Dave Wysochanski
  2010-03-26 17:38   ` [PATCH] Remove vgname check from _find_vg_rlocn and just return the rlocn slot Dave Wysochanski
  2010-03-26 23:01   ` [PATCH] Fix vg_write/vg_commit to not reset position in metadata ring buffer on vgrename Peter Rajnoha
  0 siblings, 2 replies; 6+ messages in thread
From: Dave Wysochanski @ 2010-03-26 16:15 UTC (permalink / raw)
  To: lvm-devel

On Fri, 2010-03-26 at 14:56 +0100, Peter Rajnoha wrote:
> We should write metadata into next position in the ring buffer while calling vgrename.
> At this code level (_vg_write_raw), we're not able to determine if this is a rename
> or not. If yes, then accompanying VG structure passed here has a new name set,
> not the old one.
> 
> When looking for a location where to put metadata next, we're given a NULL value because
> of failed VG name comparison (in _find_vg_rlocn) between the name in existing metadata
> and metadata we're just about to write.
> 
> This resets the position in the ring buffer, overwriting any existing metadata
> (and also incorrectly updates the cache to "orphan" afterwards).
> 
> This patch just adds old_name item in struct volume_group that we can check and use
> if necessary and detect renames at lower layers as well.
> 
> Peter
> 
>  lib/format_text/format-text.c    |   17 +++++++++++++----
>  lib/metadata/metadata-exported.h |    1 +
>  lib/metadata/metadata.c          |    2 ++
>  3 files changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/format_text/format-text.c b/lib/format_text/format-text.c
> index ad4db34..4856d07 100644
> --- a/lib/format_text/format-text.c
> +++ b/lib/format_text/format-text.c
> @@ -380,6 +380,10 @@ static struct raw_locn *_find_vg_rlocn(struct device_area *dev_area,
>  	} else
>  		*precommitted = 0;
>  
> +	/* Do not check non-existent metadata. */
> +	if (!rlocn->offset && !rlocn->size)
> +		return NULL;
> +
>  	/* FIXME Loop through rlocns two-at-a-time.  List null-terminated. */
>  	/* FIXME Ignore if checksum incorrect!!! */
>  	if (!dev_read(dev_area->dev, dev_area->start + rlocn->offset,
> @@ -387,9 +391,11 @@ static struct raw_locn *_find_vg_rlocn(struct device_area *dev_area,
>  		goto_bad;
>  
>  	if (!strncmp(vgnamebuf, vgname, len = strlen(vgname)) &&
> -	    (isspace(vgnamebuf[len]) || vgnamebuf[len] == '{')) {
> +	    (isspace(vgnamebuf[len]) || vgnamebuf[len] == '{'))
>  		return rlocn;
> -	}
> +	else
> +		log_debug("Volume group name found in metadata does "
> +			  "not match expected name %s.", vgname);
>  
>        bad:
>  	if ((info = info_from_pvid(dev_area->dev->pvid, 0)))
> @@ -542,7 +548,8 @@ static int _vg_write_raw(struct format_instance *fid, struct volume_group *vg,
>  	if (!(mdah = _raw_read_mda_header(fid->fmt, &mdac->area)))
>  		goto_out;
>  
> -	rlocn = _find_vg_rlocn(&mdac->area, mdah, vg->name, &noprecommit);
> +	rlocn = _find_vg_rlocn(&mdac->area, mdah,
> +			vg->old_name ? vg->old_name : vg->name, &noprecommit);
>  	mdac->rlocn.offset = _next_rlocn_offset(rlocn, mdah);
>  
>  	if (!fidtc->raw_metadata_buf &&
> @@ -647,7 +654,9 @@ static int _vg_commit_raw_rlocn(struct format_instance *fid,
>  	if (!(mdah = _raw_read_mda_header(fid->fmt, &mdac->area)))
>  		goto_out;
>  
> -	if (!(rlocn = _find_vg_rlocn(&mdac->area, mdah, vg->name, &noprecommit))) {
> +	if (!(rlocn = _find_vg_rlocn(&mdac->area, mdah,
> +				     vg->old_name ? vg->old_name : vg->name,
> +				     &noprecommit))) {
>  		mdah->raw_locns[0].offset = 0;
>  		mdah->raw_locns[0].size = 0;
>  		mdah->raw_locns[0].checksum = 0;
> diff --git a/lib/metadata/metadata-exported.h b/lib/metadata/metadata-exported.h
> index 2f1405f..855ab47 100644
> --- a/lib/metadata/metadata-exported.h
> +++ b/lib/metadata/metadata-exported.h
> @@ -216,6 +216,7 @@ struct volume_group {
>  
>  	struct id id;
>  	char *name;
> +	char *old_name;
>  	char *system_id;
>  
>  	uint32_t extent_size;
> diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
> index 020e897..867e004 100644
> --- a/lib/metadata/metadata.c
> +++ b/lib/metadata/metadata.c
> @@ -437,6 +437,8 @@ int vg_rename(struct cmd_context *cmd, struct volume_group *vg,
>  	struct dm_pool *mem = vg->vgmem;
>  	struct pv_list *pvl;
>  
> +	vg->old_name = vg->name;
> +
>  	if (!(vg->name = dm_pool_strdup(mem, new_name))) {
>  		log_error("vg->name allocation failed for '%s'", new_name);
>  		return 0;
> 

Seems reasonable.

But while we're on this topic, can you explain why _find_vg_rlocn() is
necessary at all?

Why are we not just using a precommit or non-precommit area here.  This
whole function looks like a hack and it makes some things harder.  IMO
if we could get rid of it we'd be better off.



^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH] Remove vgname check from _find_vg_rlocn and just return the rlocn slot.
  2010-03-26 16:15 ` Dave Wysochanski
@ 2010-03-26 17:38   ` Dave Wysochanski
  2010-03-26 18:31     ` Alasdair G Kergon
  2010-03-26 23:01   ` [PATCH] Fix vg_write/vg_commit to not reset position in metadata ring buffer on vgrename Peter Rajnoha
  1 sibling, 1 reply; 6+ messages in thread
From: Dave Wysochanski @ 2010-03-26 17:38 UTC (permalink / raw)
  To: lvm-devel

Unclear what the purpose of this check was/is.  Removing it allows for easier
refactoring, fixes at least one bug, and at least breaks no tests.  If it
covers and important case, we should add a comment and/or testcase.

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 lib/format_text/format-text.c |   35 +++++++----------------------------
 1 files changed, 7 insertions(+), 28 deletions(-)

diff --git a/lib/format_text/format-text.c b/lib/format_text/format-text.c
index ad4db34..7a3c4ef 100644
--- a/lib/format_text/format-text.c
+++ b/lib/format_text/format-text.c
@@ -360,15 +360,10 @@ static int _raw_write_mda_header(const struct format_type *fmt,
 	return 1;
 }
 
-static struct raw_locn *_find_vg_rlocn(struct device_area *dev_area,
-				       struct mda_header *mdah,
-				       const char *vgname,
+static struct raw_locn *_find_vg_rlocn(struct mda_header *mdah,
 				       int *precommitted)
 {
-	size_t len;
-	char vgnamebuf[NAME_LEN + 2] __attribute((aligned(8)));
 	struct raw_locn *rlocn, *rlocn_precommitted;
-	struct lvmcache_info *info;
 
 	rlocn = mdah->raw_locns;	/* Slot 0 */
 	rlocn_precommitted = rlocn + 1;	/* Slot 1 */
@@ -380,23 +375,7 @@ static struct raw_locn *_find_vg_rlocn(struct device_area *dev_area,
 	} else
 		*precommitted = 0;
 
-	/* FIXME Loop through rlocns two-at-a-time.  List null-terminated. */
-	/* FIXME Ignore if checksum incorrect!!! */
-	if (!dev_read(dev_area->dev, dev_area->start + rlocn->offset,
-		      sizeof(vgnamebuf), vgnamebuf))
-		goto_bad;
-
-	if (!strncmp(vgnamebuf, vgname, len = strlen(vgname)) &&
-	    (isspace(vgnamebuf[len]) || vgnamebuf[len] == '{')) {
-		return rlocn;
-	}
-
-      bad:
-	if ((info = info_from_pvid(dev_area->dev->pvid, 0)))
-		lvmcache_update_vgname_and_id(info, FMT_TEXT_ORPHAN_VG_NAME,
-					      FMT_TEXT_ORPHAN_VG_NAME, 0, NULL);
-
-	return NULL;
+	return rlocn;
 }
 
 /*
@@ -430,7 +409,7 @@ static int _raw_holds_vgname(struct format_instance *fid,
 	if (!(mdah = _raw_read_mda_header(fid->fmt, dev_area)))
 		return_0;
 
-	if (_find_vg_rlocn(dev_area, mdah, vgname, &noprecommit))
+	if (_find_vg_rlocn(mdah, &noprecommit))
 		r = 1;
 
 	if (!dev_close(dev_area->dev))
@@ -457,7 +436,7 @@ static struct volume_group *_vg_read_raw_area(struct format_instance *fid,
 	if (!(mdah = _raw_read_mda_header(fid->fmt, area)))
 		goto_out;
 
-	if (!(rlocn = _find_vg_rlocn(area, mdah, vgname, &precommitted))) {
+	if (!(rlocn = _find_vg_rlocn(mdah, &precommitted))) {
 		log_debug("VG %s not found on %s", vgname, dev_name(area->dev));
 		goto out;
 	}
@@ -542,7 +521,7 @@ static int _vg_write_raw(struct format_instance *fid, struct volume_group *vg,
 	if (!(mdah = _raw_read_mda_header(fid->fmt, &mdac->area)))
 		goto_out;
 
-	rlocn = _find_vg_rlocn(&mdac->area, mdah, vg->name, &noprecommit);
+	rlocn = _find_vg_rlocn(mdah, &noprecommit);
 	mdac->rlocn.offset = _next_rlocn_offset(rlocn, mdah);
 
 	if (!fidtc->raw_metadata_buf &&
@@ -647,7 +626,7 @@ static int _vg_commit_raw_rlocn(struct format_instance *fid,
 	if (!(mdah = _raw_read_mda_header(fid->fmt, &mdac->area)))
 		goto_out;
 
-	if (!(rlocn = _find_vg_rlocn(&mdac->area, mdah, vg->name, &noprecommit))) {
+	if (!(rlocn = _find_vg_rlocn(mdah, &noprecommit))) {
 		mdah->raw_locns[0].offset = 0;
 		mdah->raw_locns[0].size = 0;
 		mdah->raw_locns[0].checksum = 0;
@@ -756,7 +735,7 @@ static int _vg_remove_raw(struct format_instance *fid, struct volume_group *vg,
 	if (!(mdah = _raw_read_mda_header(fid->fmt, &mdac->area)))
 		goto_out;
 
-	if (!(rlocn = _find_vg_rlocn(&mdac->area, mdah, vg->name, &noprecommit))) {
+	if (!(rlocn = _find_vg_rlocn(mdah, &noprecommit))) {
 		rlocn = &mdah->raw_locns[0];
 		mdah->raw_locns[1].offset = 0;
 	}
-- 
1.6.0.6



^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH] Remove vgname check from _find_vg_rlocn and just return the rlocn slot.
  2010-03-26 17:38   ` [PATCH] Remove vgname check from _find_vg_rlocn and just return the rlocn slot Dave Wysochanski
@ 2010-03-26 18:31     ` Alasdair G Kergon
  0 siblings, 0 replies; 6+ messages in thread
From: Alasdair G Kergon @ 2010-03-26 18:31 UTC (permalink / raw)
  To: lvm-devel

On Fri, Mar 26, 2010 at 01:38:46PM -0400, Dave Wysochanski wrote:
> Unclear what the purpose of this check was/is.  Removing it allows for easier
> refactoring, fixes at least one bug, and at least breaks no tests.  If it
> covers and important case, we should add a comment and/or testcase.
 
The on-disk layout supports multiple mdas for different VGs stored on the same
disk.  But at the moment we only use one.
[Create a test case: PV with 2 mdas, and different vgname in each...current
code should ignore the 2nd one, maybe overwriting it with copy of first, I'm
not sure.]

> -	/* FIXME Loop through rlocns two-at-a-time.  List null-terminated. */

> -	/* FIXME Ignore if checksum incorrect!!! */

FIXME removed, so does the patch now validate the checksum?

I think the vgname validation is still needed on almost all occasions.
Only the 'tell me which VG is in this mda' case doesn't want it.

Alasdair



^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH] Fix vg_write/vg_commit to not reset position in metadata ring buffer on vgrename
  2010-03-26 16:15 ` Dave Wysochanski
  2010-03-26 17:38   ` [PATCH] Remove vgname check from _find_vg_rlocn and just return the rlocn slot Dave Wysochanski
@ 2010-03-26 23:01   ` Peter Rajnoha
  2010-03-27  0:18     ` Alasdair G Kergon
  1 sibling, 1 reply; 6+ messages in thread
From: Peter Rajnoha @ 2010-03-26 23:01 UTC (permalink / raw)
  To: lvm-devel

On 03/26/2010 05:15 PM, Dave Wysochanski wrote:
>> When looking for a location where to put metadata next, we're given a NULL value because
>> of failed VG name comparison (in _find_vg_rlocn) between the name in existing metadata
>> and metadata we're just about to write.
>>
>> This resets the position in the ring buffer, overwriting any existing metadata
>> (and also incorrectly updates the cache to "orphan" afterwards).
>>
>> This patch just adds old_name item in struct volume_group that we can check and use
>> if necessary and detect renames at lower layers as well.
> 
> Seems reasonable.
> 
> But while we're on this topic, can you explain why _find_vg_rlocn() is
> necessary at all?
> 
> Why are we not just using a precommit or non-precommit area here.  This
> whole function looks like a hack and it makes some things harder.  IMO
> if we could get rid of it we'd be better off.
> 

Hmm, there's also the vgcfgrestore that has a very similar problem like the vgrename.
But for the vgcfgrestore to write metadata correctly too, we would probably need
to disable this VG name check (as discussed with Alasdair today), so actually we have
3 situations:

 - normal vg_write/vg_commit while changing VG
 - vgrename
 - vgcfgrestore

For the check to be correct we have vg->name for the first case. For the second one,
we need that vg->old_name I tried to add. But I was still thinking about the third one.
If we were to support that, we would need to disable that check (we don't do full vg_read
on vgcfgrestore). But how would _find_vg_rlocn know this is the situation? How to pass
that information from the "layer" above through vg_write/vg_commit? Adding another
item into struct volume_group (or elsewhere)? That would be messy.

So yes, your question is probably right. If that check was not so essential and its
removal didn't break anything, it would probably make things much more simple and clean.

Whether the check we do in that _find_vg_rlocn is really worth it... Hmm...

Peter



^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH] Fix vg_write/vg_commit to not reset position in metadata ring buffer on vgrename
  2010-03-26 23:01   ` [PATCH] Fix vg_write/vg_commit to not reset position in metadata ring buffer on vgrename Peter Rajnoha
@ 2010-03-27  0:18     ` Alasdair G Kergon
  0 siblings, 0 replies; 6+ messages in thread
From: Alasdair G Kergon @ 2010-03-27  0:18 UTC (permalink / raw)
  To: lvm-devel

On Sat, Mar 27, 2010 at 12:01:35AM +0100, Peter Rajnoha wrote:
> How to pass
> that information from the "layer" above through vg_write/vg_commit?

A special value of 'old_name'?

Alasdair



^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2010-03-27  0:18 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-03-26 13:56 [PATCH] Fix vg_write/vg_commit to not reset position in metadata ring buffer on vgrename Peter Rajnoha
2010-03-26 16:15 ` Dave Wysochanski
2010-03-26 17:38   ` [PATCH] Remove vgname check from _find_vg_rlocn and just return the rlocn slot Dave Wysochanski
2010-03-26 18:31     ` Alasdair G Kergon
2010-03-26 23:01   ` [PATCH] Fix vg_write/vg_commit to not reset position in metadata ring buffer on vgrename Peter Rajnoha
2010-03-27  0:18     ` 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.