[PATCH 0/5] Add pv->vg link, v2

[PATCH 0/5] Add pv->vg link, v2

[Dave Wysochanski]

This patchset refactors and adds a pv->vg link.  The previous patchset
missed a few cases, namely vgsplit, vgmerge, vgrename, and format1 code.
This patcheset I believe addresses all cases.  I dropped the lvm2app
patches that utilize this link for pv/vg/lv properties - will be submitted
in another patchset.

It would be nice to refactor further if possible, in particular
the pv->vgname should probably be set inside the add_pvl_to_vgs()
function or something similar, but for now this refactoring is
a step in the right direction.

Although the testsuite is still failing on the latest commits,
these patches passed the testsuite when rebased on the following commit:
commit f9554c7d27930d2ad404b4473ec13aa77f0e53d0
Author: Mike Snitzer <snitzer@redhat.com>
Date:   Wed Mar 24 22:25:11 2010 +0000

    Revert having clvmd consult the lvm.conf "activation/monitoring".
    Correctly implementing the intent of this change requires more
    analysis.

[PATCH 1/5] Add pv to vg->pvs after check for maximum value of vg->extent_count.

[Dave Wysochanski]

In add_pv_to_vg(), we should only add the pv to vg->pvs after all
internal checks have passed.  The check for vg->extent_count exeeding
maximum was after we added the pv to the list, so this function could
return a state of vg->pvs that did not reflect other parameters such
as vg->pv_count.

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

diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
index a12a8c8..9e6d9ba 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -220,9 +220,6 @@ int add_pv_to_vg(struct volume_group *vg, const char *pv_name,
 	if (!alloc_pv_segment_whole_pv(mem, pv))
 		return_0;
 
-	pvl->pv = pv;
-	dm_list_add(&vg->pvs, &pvl->list);
-
 	if ((uint64_t) vg->extent_count + pv->pe_count > UINT32_MAX) {
 		log_error("Unable to add %s to %s: new extent count (%"
 			  PRIu64 ") exceeds limit (%" PRIu32 ").",
@@ -232,6 +229,9 @@ int add_pv_to_vg(struct volume_group *vg, const char *pv_name,
 		return 0;
 	}
 
+	pvl->pv = pv;
+	dm_list_add(&vg->pvs, &pvl->list);
+
 	vg->pv_count++;
 	vg->extent_count += pv->pe_count;
 	vg->free_count += pv->pe_count;
-- 
1.6.0.6

[PATCH 2/5] Refactor _read_pv() code that updates vg->extent_count and vg->free_count.

[Dave Wysochanski]

Simple refactor to mov code that updates the vg extent counts from a
single pv's counts close to the code that adds a pv to vg->pvs and
updates vg->pv_count.  No functional change.

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

diff --git a/lib/format_text/import_vsn1.c b/lib/format_text/import_vsn1.c
index ae9742a..cf2c9df 100644
--- a/lib/format_text/import_vsn1.c
+++ b/lib/format_text/import_vsn1.c
@@ -241,10 +241,6 @@ static int _read_pv(struct format_instance *fid, struct dm_pool *mem,
 		return 0;
 	}
 
-	/* adjust the volume group. */
-	vg->extent_count += pv->pe_count;
-	vg->free_count += pv->pe_count;
-
 	pv->pe_size = vg->extent_size;
 
 	pv->pe_alloc_count = 0;
@@ -273,6 +269,8 @@ static int _read_pv(struct format_instance *fid, struct dm_pool *mem,
 	if (!alloc_pv_segment_whole_pv(mem, pv))
 		return_0;
 
+	vg->extent_count += pv->pe_count;
+	vg->free_count += pv->pe_count;
 	vg->pv_count++;
 	dm_list_add(&vg->pvs, &pvl->list);
 
-- 
1.6.0.6

[PATCH 3/5] Refactor format1 vg->pvs list add and vg->pv_count.

[Dave Wysochanski]

Refactor adding to the vg->pvs list and incrementing the count, which
will allow further refactoring.  Should be no functional change.

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 lib/format1/disk-rep.h      |    3 +--
 lib/format1/format1.c       |    2 +-
 lib/format1/import-export.c |    9 ++++-----
 3 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/lib/format1/disk-rep.h b/lib/format1/disk-rep.h
index 138794e..b91be39 100644
--- a/lib/format1/disk-rep.h
+++ b/lib/format1/disk-rep.h
@@ -224,8 +224,7 @@ int export_extents(struct disk_list *dl, uint32_t lv_num,
 		   struct logical_volume *lv, struct physical_volume *pv);
 
 int import_pvs(const struct format_type *fmt, struct dm_pool *mem,
-	       struct volume_group *vg,
-	       struct dm_list *pvds, struct dm_list *results, uint32_t *count);
+	       struct volume_group *vg, struct dm_list *pvds);
 
 int import_lvs(struct dm_pool *mem, struct volume_group *vg, struct dm_list *pvds);
 int export_lvs(struct disk_list *dl, struct volume_group *vg,
diff --git a/lib/format1/format1.c b/lib/format1/format1.c
index b6a2c15..1167e04 100644
--- a/lib/format1/format1.c
+++ b/lib/format1/format1.c
@@ -142,7 +142,7 @@ static struct volume_group *_build_vg(struct format_instance *fid,
 	if (!import_vg(mem, vg, dl))
 		goto_bad;
 
-	if (!import_pvs(fid->fmt, mem, vg, pvs, &vg->pvs, &vg->pv_count))
+	if (!import_pvs(fid->fmt, mem, vg, pvs))
 		goto_bad;
 
 	if (!import_lvs(mem, vg, pvs))
diff --git a/lib/format1/import-export.c b/lib/format1/import-export.c
index cd7cc7f..2ffc19e 100644
--- a/lib/format1/import-export.c
+++ b/lib/format1/import-export.c
@@ -422,13 +422,12 @@ int export_extents(struct disk_list *dl, uint32_t lv_num,
 }
 
 int import_pvs(const struct format_type *fmt, struct dm_pool *mem,
-	       struct volume_group *vg,
-	       struct dm_list *pvds, struct dm_list *results, uint32_t *count)
+	       struct volume_group *vg, struct dm_list *pvds)
 {
 	struct disk_list *dl;
 	struct pv_list *pvl;
 
-	*count = 0;
+	vg->pv_count = 0;
 	dm_list_iterate_items(dl, pvds) {
 		if (!(pvl = dm_pool_zalloc(mem, sizeof(*pvl))) ||
 		    !(pvl->pv = dm_pool_alloc(mem, sizeof(*pvl->pv))))
@@ -438,8 +437,8 @@ int import_pvs(const struct format_type *fmt, struct dm_pool *mem,
 			return_0;
 
 		pvl->pv->fmt = fmt;
-		dm_list_add(results, &pvl->list);
-		(*count)++;
+		dm_list_add(&vg->pvs, &pvl->list);
+		vg->pv_count++;
 	}
 
 	return 1;
-- 
1.6.0.6

[PATCH 4/5] Add add_pvl_to_vgs() - helper function to add a pv to a vg list.

[Dave Wysochanski]

Small refactor of main places in the code where a pv is added to a
vg into a small function which adds the pv to the list and updates
the vg counts.

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 lib/format1/import-export.c   |    3 +--
 lib/format_text/import_vsn1.c |    3 +--
 lib/metadata/metadata.c       |   11 ++++++++---
 lib/metadata/metadata.h       |    1 +
 4 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/lib/format1/import-export.c b/lib/format1/import-export.c
index 2ffc19e..e37d59e 100644
--- a/lib/format1/import-export.c
+++ b/lib/format1/import-export.c
@@ -437,8 +437,7 @@ int import_pvs(const struct format_type *fmt, struct dm_pool *mem,
 			return_0;
 
 		pvl->pv->fmt = fmt;
-		dm_list_add(&vg->pvs, &pvl->list);
-		vg->pv_count++;
+		add_pvl_to_vgs(vg, pvl);
 	}
 
 	return 1;
diff --git a/lib/format_text/import_vsn1.c b/lib/format_text/import_vsn1.c
index cf2c9df..ed508cf 100644
--- a/lib/format_text/import_vsn1.c
+++ b/lib/format_text/import_vsn1.c
@@ -271,8 +271,7 @@ static int _read_pv(struct format_instance *fid, struct dm_pool *mem,
 
 	vg->extent_count += pv->pe_count;
 	vg->free_count += pv->pe_count;
-	vg->pv_count++;
-	dm_list_add(&vg->pvs, &pvl->list);
+	add_pvl_to_vgs(vg, pvl);
 
 	return 1;
 }
diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
index 9e6d9ba..6ceb793 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -136,6 +136,13 @@ out:
 	return pv->pe_align_offset;
 }
 
+void add_pvl_to_vgs(struct volume_group *vg, struct pv_list *pvl)
+{
+	dm_list_add(&vg->pvs, &pvl->list);
+	vg->pv_count++;
+}
+
+
 /**
  * add_pv_to_vg - Add a physical volume to a volume group
  * @vg - volume group to add to
@@ -230,9 +237,7 @@ int add_pv_to_vg(struct volume_group *vg, const char *pv_name,
 	}
 
 	pvl->pv = pv;
-	dm_list_add(&vg->pvs, &pvl->list);
-
-	vg->pv_count++;
+	add_pvl_to_vgs(vg, pvl);
 	vg->extent_count += pv->pe_count;
 	vg->free_count += pv->pe_count;
 
diff --git a/lib/metadata/metadata.h b/lib/metadata/metadata.h
index 782d300..1496252 100644
--- a/lib/metadata/metadata.h
+++ b/lib/metadata/metadata.h
@@ -377,5 +377,6 @@ struct id pv_vgid(const struct physical_volume *pv);
 struct physical_volume *pv_by_path(struct cmd_context *cmd, const char *pv_name);
 int add_pv_to_vg(struct volume_group *vg, const char *pv_name,
 		 struct physical_volume *pv);
+void add_pvl_to_vgs(struct volume_group *vg, struct pv_list *pvl);
 
 #endif
-- 
1.6.0.6

[PATCH 5/5] Add pv->vg to solidify link between a pv and a vg.

[Dave Wysochanski]

lvm2app needs a link back to the vg in order to use the vg handle for
memory allocations as well as other things.  This patch adds the field
to struct physical_volume, and sets pv->vg when reading a vg from disk or
extending a vg by using the helper function previously added,
add_pvl_to_vgs().  Moves and renames are handled with separate code
inside move_pv() and vgmerge().  Add pv->vg check to vg_validate().

NOTE: Unfortunately in the case of pv_read() callers, we cannot set
this pv->vg field properly, since the vg it belongs to may not be
known by just reading a single device, and no VG is in existence at
the time of pv_read().  Significant refactorings related to
lvmcache, PVs with 0 mdas, etc, are required to set this field
for callers of pv_read().  For now we assume that if the field is
NULL, the pv is not associated with a complete VG (as returned
by vg_read()).

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 lib/metadata/metadata-exported.h |    1 +
 lib/metadata/metadata.c          |    8 ++++++++
 tools/vgmerge.c                  |    1 +
 3 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/lib/metadata/metadata-exported.h b/lib/metadata/metadata-exported.h
index b02a22c..ef6d1ef 100644
--- a/lib/metadata/metadata-exported.h
+++ b/lib/metadata/metadata-exported.h
@@ -184,6 +184,7 @@ struct physical_volume {
 	const char *vg_name;
 	struct id vgid;
 
+	struct volume_group *vg;
 	uint64_t status;
 	uint64_t size;
 
diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
index 6ceb793..70debf4 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -140,6 +140,7 @@ void add_pvl_to_vgs(struct volume_group *vg, struct pv_list *pvl)
 {
 	dm_list_add(&vg->pvs, &pvl->list);
 	vg->pv_count++;
+	pvl->pv->vg = vg;
 }
 
 
@@ -336,6 +337,7 @@ int move_pv(struct volume_group *vg_from, struct volume_group *vg_to,
 
 	vg_from->pv_count--;
 	vg_to->pv_count++;
+	pvl->pv->vg = vg_to;
 
 	pv = pvl->pv;
 
@@ -2146,6 +2148,12 @@ int vg_validate(struct volume_group *vg)
 			/* FIXME Dump list structure? */
 			r = 0;
 		}
+		if (pvl->pv->vg != vg) {
+			log_error(INTERNAL_ERROR "VG %s PV list entry points "
+				  "to different VG %s", vg->name,
+				  pvl->pv->vg ? pvl->pv->vg->name : "NULL");
+			r = 0;
+		}
 	}
 
 	loop_counter1 = loop_counter2 = 0;
diff --git a/tools/vgmerge.c b/tools/vgmerge.c
index 4a6721d..0acdbb9 100644
--- a/tools/vgmerge.c
+++ b/tools/vgmerge.c
@@ -90,6 +90,7 @@ static int _vgmerge_single(struct cmd_context *cmd, const char *vg_name_to,
 
 		pv = dm_list_item(pvh, struct pv_list)->pv;
 		pv->vg_name = dm_pool_strdup(cmd->mem, vg_to->name);
+		pv->vg = vg_to;
 	}
 	vg_to->pv_count += vg_from->pv_count;
 
-- 
1.6.0.6

[PATCH 5/5] Add pv->vg to solidify link between a pv and a vg.

[Alasdair G Kergon]

On Mon, Apr 05, 2010 at 03:51:39PM -0400, Dave Wysochanski wrote:
> +++ b/lib/metadata/metadata-exported.h
> @@ -184,6 +184,7 @@ struct physical_volume {
>  	const char *vg_name;
>  	struct id vgid;
>  
> +	struct volume_group *vg;
>  	uint64_t status;
>  	uint64_t size;
 
Put it at the end of the other group i.e. move the blank line.

There are now *three* entries in the PV struct referring to the VG.
- vg_name
- vgid
- vg

Why do we need 3?
- Historically, only the name was used.
- We added vgid to distinguish between different VGs with the same name plugged
into a system.
- The new 'vg' field as proposed is *independent* of those two existing ones, and
is set and maintained when the PV belongs to a 'pvs' list in a parent VG struct.
Please make that clear in a comment by the field.

Alasdair

[PATCH 5/5] Add pv->vg to solidify link between a pv and a vg.

[Dave Wysochanski]

On Tue, 2010-04-06 at 14:27 +0100, Alasdair G Kergon wrote:
> On Mon, Apr 05, 2010 at 03:51:39PM -0400, Dave Wysochanski wrote:
> > +++ b/lib/metadata/metadata-exported.h
> > @@ -184,6 +184,7 @@ struct physical_volume {
> >  	const char *vg_name;
> >  	struct id vgid;
> >  
> > +	struct volume_group *vg;
> >  	uint64_t status;
> >  	uint64_t size;
>  
> Put it at the end of the other group i.e. move the blank line.
> 
> There are now *three* entries in the PV struct referring to the VG.
> - vg_name
> - vgid
> - vg
> 
> Why do we need 3?
> - Historically, only the name was used.
> - We added vgid to distinguish between different VGs with the same name plugged
> into a system.
> - The new 'vg' field as proposed is *independent* of those two existing ones, and
> is set and maintained when the PV belongs to a 'pvs' list in a parent VG struct.
> Please make that clear in a comment by the field.
> 

Ok yes, I will add the comment/explanation.

I started to see if I could refactor such that I remove vgname and vgid
from struct physical_volume and just reference vg->name and vg->id, but
it may be more trouble than it is worth.  Ideally all we need is 'vg',
but this assumes the VG is constructed at all times when the vgname and
vgid fields are needed.  When we read labels though, and dip into the vg
metadata for the vgname, etc, we don't really construct the vg at this
point, so it might not be something that could be easily/safely
refactored.  Then again, longer term we may want to sub-divide the vg
into a "vg header" which contains just what we read at label_read()
time, and then we could conceivably refactor this I think.

[PATCH 5/5] Add pv->vg to solidify link between a pv and a vg.

[Alasdair G Kergon]

I'm not convinced this patch catches everything yet.

E.g. vg_remove_check has a:
                dm_list_del(&pvl->list);
and I don't spot the matching pv->vg change.
If it's there, it's well-hidden.

Every place in the code where vg->pvs changes should have a corresponding
pv->vg change next to it.

Alasdair

[PATCH 4/5] Add add_pvl_to_vgs() - helper function to add a pv to a vg list.

[Alasdair G Kergon]

On Mon, Apr 05, 2010 at 03:51:38PM -0400, Dave Wysochanski wrote:
> Small refactor of main places in the code where a pv is added to a
> vg into a small function which adds the pv to the list and updates
> the vg counts.
 
Ack.

Alasdair

[PATCH 3/5] Refactor format1 vg->pvs list add and vg->pv_count.

[Alasdair G Kergon]

On Mon, Apr 05, 2010 at 03:51:37PM -0400, Dave Wysochanski wrote:
> Refactor adding to the vg->pvs list and incrementing the count, which
> will allow further refactoring.  Should be no functional change.
 
Ack.

Alasdair

[PATCH 2/5] Refactor _read_pv() code that updates vg->extent_count and vg->free_count.

[Alasdair G Kergon]

On Mon, Apr 05, 2010 at 03:51:36PM -0400, Dave Wysochanski wrote:
> Simple refactor to mov code that updates the vg extent counts from a
> single pv's counts close to the code that adds a pv to vg->pvs and
> updates vg->pv_count.  No functional change.
 
Ack.

Alasdair

[PATCH 1/5] Add pv to vg->pvs after check for maximum value of vg->extent_count.

[Alasdair G Kergon]

On Mon, Apr 05, 2010 at 03:51:35PM -0400, Dave Wysochanski wrote:
> In add_pv_to_vg(), we should only add the pv to vg->pvs after all
> internal checks have passed.  The check for vg->extent_count exeeding
> maximum was after we added the pv to the list, so this function could
> return a state of vg->pvs that did not reflect other parameters such
> as vg->pv_count.
 
Ack.

Alasdair

[PATCH 0/5] Add pv->vg link, v3

[Dave Wysochanski]

This is a third revision of the patchset that adds a pv->vg link.
This patchset hopefully covers all remaining cases.

There's plenty of room for future cleanup/refactorings but I stop
here for now.

Passes testsuite.

[PATCH 1/5] Remove unnecessary parameter from import_pool_pvs().

[Dave Wysochanski]

The dm_list * parameter is unnecessary since we are passing in 'vg'
and the only caller of import_pool_pvs() passes '&vg->pvs' in the
dm_list * parameter.  Just use vg->pvs directly in the function.

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 lib/format_pool/disk_rep.h      |    2 +-
 lib/format_pool/format_pool.c   |    2 +-
 lib/format_pool/import_export.c |    4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/lib/format_pool/disk_rep.h b/lib/format_pool/disk_rep.h
index fb33cd8..fff7343 100644
--- a/lib/format_pool/disk_rep.h
+++ b/lib/format_pool/disk_rep.h
@@ -141,7 +141,7 @@ int import_pool_vg(struct volume_group *vg, struct dm_pool *mem, struct dm_list
 int import_pool_lvs(struct volume_group *vg, struct dm_pool *mem,
 		    struct dm_list *pls);
 int import_pool_pvs(const struct format_type *fmt, struct volume_group *vg,
-		    struct dm_list *pvs, struct dm_pool *mem, struct dm_list *pls);
+		    struct dm_pool *mem, struct dm_list *pls);
 int import_pool_pv(const struct format_type *fmt, struct dm_pool *mem,
 		   struct volume_group *vg, struct physical_volume *pv,
 		   struct pool_list *pl);
diff --git a/lib/format_pool/format_pool.c b/lib/format_pool/format_pool.c
index 26321c1..d8a09ec 100644
--- a/lib/format_pool/format_pool.c
+++ b/lib/format_pool/format_pool.c
@@ -129,7 +129,7 @@ static struct volume_group *_build_vg_from_pds(struct format_instance
 	if (!import_pool_vg(vg, smem, pds))
 		return_NULL;
 
-	if (!import_pool_pvs(fid->fmt, vg, &vg->pvs, smem, pds))
+	if (!import_pool_pvs(fid->fmt, vg, smem, pds))
 		return_NULL;
 
 	if (!import_pool_lvs(vg, smem, pds))
diff --git a/lib/format_pool/import_export.c b/lib/format_pool/import_export.c
index b383557..032d428 100644
--- a/lib/format_pool/import_export.c
+++ b/lib/format_pool/import_export.c
@@ -100,7 +100,7 @@ int import_pool_lvs(struct volume_group *vg, struct dm_pool *mem, struct dm_list
 }
 
 int import_pool_pvs(const struct format_type *fmt, struct volume_group *vg,
-		    struct dm_list *pvs, struct dm_pool *mem, struct dm_list *pls)
+		    struct dm_pool *mem, struct dm_list *pls)
 {
 	struct pv_list *pvl;
 	struct pool_list *pl;
@@ -120,7 +120,7 @@ int import_pool_pvs(const struct format_type *fmt, struct volume_group *vg,
 		pl->pv = pvl->pv;
 		pvl->mdas = NULL;
 		pvl->pe_ranges = NULL;
-		dm_list_add(pvs, &pvl->list);
+		dm_list_add(&vg->pvs, &pvl->list);
 	}
 
 	return 1;
-- 
1.6.0.6

[PATCH 2/5] Move increment of vg->pv_count from import_pool_vg() to import_pool_pvs().

[Dave Wysochanski]

Move the increment of vg->pv_count next to the place where we add to
vg->pvs.  It looks safe to do this since the only caller of import_pool_vg()
calls import_pool_pvs() immediately afterward, and there is no way
import_pool_vg() can fail (always returns 1).  However, if there's a
memory allocation failure inside import_pool_pvs(), we will end up with
a different count in vg->pv_count that with the original code.  In any
case, vg->pv_count should be as close to dm_list_size(&vg->pvs) as
possible, as is the case everywhere else in the code.

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 lib/format_pool/import_export.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/lib/format_pool/import_export.c b/lib/format_pool/import_export.c
index 032d428..d464364 100644
--- a/lib/format_pool/import_export.c
+++ b/lib/format_pool/import_export.c
@@ -37,7 +37,6 @@ int import_pool_vg(struct volume_group *vg, struct dm_pool *mem, struct dm_list
 		    ((pl->pd.pl_blocks) / POOL_PE_SIZE);
 
 		vg->free_count = vg->extent_count;
-		vg->pv_count++;
 
 		if (vg->name)
 			continue;
@@ -120,6 +119,7 @@ int import_pool_pvs(const struct format_type *fmt, struct volume_group *vg,
 		pl->pv = pvl->pv;
 		pvl->mdas = NULL;
 		pvl->pe_ranges = NULL;
+		vg->pv_count++;
 		dm_list_add(&vg->pvs, &pvl->list);
 	}
 
-- 
1.6.0.6

[PATCH 3/5] Add del_pvl_from_vgs() and move prototypes into metadata-exported.h

[Dave Wysochanski]

Add a delete function to manage the vg->pvs list.

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

diff --git a/lib/metadata/metadata-exported.h b/lib/metadata/metadata-exported.h
index b02a22c..865cb3b 100644
--- a/lib/metadata/metadata-exported.h
+++ b/lib/metadata/metadata-exported.h
@@ -469,6 +469,13 @@ int vg_set_alloc_policy(struct volume_group *vg, alloc_policy_t alloc);
 int vg_set_clustered(struct volume_group *vg, int clustered);
 int vg_split_mdas(struct cmd_context *cmd, struct volume_group *vg_from,
 		  struct volume_group *vg_to);
+/*
+ * FIXME: These functions should probably take a 'pv' as parameter. To
+ * accomplish this, add dm_list * to struct physical_volume and refactor all
+ * the callers.
+ */
+void add_pvl_to_vgs(struct volume_group *vg, struct pv_list *pvl);
+void del_pvl_from_vgs(struct volume_group *vg, struct pv_list *pvl);
 
 /* FIXME: refactor / unexport when lvremove liblvm refactoring dones */
 int remove_lvs_in_vg(struct cmd_context *cmd,
diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
index 6ceb793..dd986a1 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -142,6 +142,12 @@ void add_pvl_to_vgs(struct volume_group *vg, struct pv_list *pvl)
 	vg->pv_count++;
 }
 
+void del_pvl_from_vgs(struct volume_group *vg, struct pv_list *pvl)
+{
+	vg->pv_count--;
+	dm_list_del(&pvl->list);
+}
+
 
 /**
  * add_pv_to_vg - Add a physical volume to a volume group
diff --git a/lib/metadata/metadata.h b/lib/metadata/metadata.h
index 1496252..782d300 100644
--- a/lib/metadata/metadata.h
+++ b/lib/metadata/metadata.h
@@ -377,6 +377,5 @@ struct id pv_vgid(const struct physical_volume *pv);
 struct physical_volume *pv_by_path(struct cmd_context *cmd, const char *pv_name);
 int add_pv_to_vg(struct volume_group *vg, const char *pv_name,
 		 struct physical_volume *pv);
-void add_pvl_to_vgs(struct volume_group *vg, struct pv_list *pvl);
 
 #endif
-- 
1.6.0.6

[PATCH 4/5] Call add_pvl_to_vgs() and del_pvl_from_vgs() from more places.

[Dave Wysochanski]

Now that we have library functions to add/delete a pv from the vg->pvs
list, call them from everywhere.

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 lib/format_pool/import_export.c |    3 +--
 lib/metadata/metadata.c         |   11 ++++-------
 tools/lvconvert.c               |    3 +--
 tools/vgmerge.c                 |   14 +++++---------
 tools/vgreduce.c                |    4 +---
 5 files changed, 12 insertions(+), 23 deletions(-)

diff --git a/lib/format_pool/import_export.c b/lib/format_pool/import_export.c
index d464364..7e04895 100644
--- a/lib/format_pool/import_export.c
+++ b/lib/format_pool/import_export.c
@@ -119,8 +119,7 @@ int import_pool_pvs(const struct format_type *fmt, struct volume_group *vg,
 		pl->pv = pvl->pv;
 		pvl->mdas = NULL;
 		pvl->pe_ranges = NULL;
-		vg->pv_count++;
-		dm_list_add(&vg->pvs, &pvl->list);
+		add_pvl_to_vgs(vg, pvl);
 	}
 
 	return 1;
diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
index dd986a1..85fa097 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -338,10 +338,8 @@ int move_pv(struct volume_group *vg_from, struct volume_group *vg_to,
 	    _vg_bad_status_bits(vg_to, RESIZEABLE_VG))
 		return 0;
 
-	dm_list_move(&vg_to->pvs, &pvl->list);
-
-	vg_from->pv_count--;
-	vg_to->pv_count++;
+	del_pvl_from_vgs(vg_from, pvl);
+	add_pvl_to_vgs(vg_to, pvl);
 
 	pv = pvl->pv;
 
@@ -508,7 +506,7 @@ int vg_remove_check(struct volume_group *vg)
 		return 0;
 
 	dm_list_iterate_items_safe(pvl, tpvl, &vg->pvs) {
-		dm_list_del(&pvl->list);
+		del_pvl_from_vgs(vg, pvl);
 		dm_list_add(&vg->removed_pvs, &pvl->list);
 	}
 	return 1;
@@ -2501,8 +2499,7 @@ static struct volume_group *_vg_read_orphans(struct cmd_context *cmd,
 			goto bad;
 		}
 		pvl->pv = pv;
-		dm_list_add(&vg->pvs, &pvl->list);
-		vg->pv_count++;
+		add_pvl_to_vgs(vg, pvl);
 	}
 
 	return vg;
diff --git a/tools/lvconvert.c b/tools/lvconvert.c
index 35ffac2..57581c8 100644
--- a/tools/lvconvert.c
+++ b/tools/lvconvert.c
@@ -736,8 +736,7 @@ static void _remove_missing_empty_pv(struct volume_group *vg, struct dm_list *re
 			/* FIXME: duplication of vgreduce code, move this to library */
 			vg->free_count -= pvl_vg->pv->pe_count;
 			vg->extent_count -= pvl_vg->pv->pe_count;
-			vg->pv_count--;
-			dm_list_del(&pvl_vg->list);
+			del_pvl_from_vgs(vg, pvl_vg);
 
 			removed++;
 		}
diff --git a/tools/vgmerge.c b/tools/vgmerge.c
index 4a6721d..299bed7 100644
--- a/tools/vgmerge.c
+++ b/tools/vgmerge.c
@@ -31,6 +31,7 @@ static struct volume_group *_vgmerge_vg_read(struct cmd_context *cmd,
 static int _vgmerge_single(struct cmd_context *cmd, const char *vg_name_to,
 			   const char *vg_name_from)
 {
+	struct pv_list *pvl, *tpvl;
 	struct volume_group *vg_to, *vg_from;
 	struct lv_list *lvl1, *lvl2;
 	int r = ECMD_FAILED;
@@ -82,16 +83,11 @@ static int _vgmerge_single(struct cmd_context *cmd, const char *vg_name_to,
 	drop_cached_metadata(vg_from);
 
 	/* Merge volume groups */
-	while (!dm_list_empty(&vg_from->pvs)) {
-		struct dm_list *pvh = vg_from->pvs.n;
-		struct physical_volume *pv;
-
-		dm_list_move(&vg_to->pvs, pvh);
-
-		pv = dm_list_item(pvh, struct pv_list)->pv;
-		pv->vg_name = dm_pool_strdup(cmd->mem, vg_to->name);
+	dm_list_iterate_items_safe(pvl, tpvl, &vg_from->pvs) {
+		del_pvl_from_vgs(vg_from, pvl);
+		add_pvl_to_vgs(vg_to, pvl);
+		pvl->pv->vg_name = dm_pool_strdup(cmd->mem, vg_to->name);
 	}
-	vg_to->pv_count += vg_from->pv_count;
 
 	/* Fix up LVIDs */
 	dm_list_iterate_items(lvl1, &vg_to->lvs) {
diff --git a/tools/vgreduce.c b/tools/vgreduce.c
index eb228a3..287db41 100644
--- a/tools/vgreduce.c
+++ b/tools/vgreduce.c
@@ -39,9 +39,7 @@ static int _remove_pv(struct volume_group *vg, struct pv_list *pvl, int silent)
 
 	vg->free_count -= pvl->pv->pe_count;
 	vg->extent_count -= pvl->pv->pe_count;
-	vg->pv_count--;
-
-	dm_list_del(&pvl->list);
+	del_pvl_from_vgs(vg, pvl);
 
 	return 1;
 }
-- 
1.6.0.6

[PATCH 5/5] Add pv->vg to solidify link between a pv and a vg.

[Dave Wysochanski]

lvm2app needs a link back to the vg in order to use the vg handle for
memory allocations as well as other things.  This patch adds the field
to struct physical_volume, and sets pv->vg when reading a vg from disk or
extending a vg by using the helper function previously added,
add_pvl_to_vgs().  Moves and renames are handled with separate code
inside move_pv() and vgmerge().  Add pv->vg check to vg_validate().

NOTE: Unfortunately in the case of pv_read() callers, we cannot set
this pv->vg field properly, since the vg it belongs to may not be
known by just reading a single device, and no VG is in existence at
the time of pv_read().  Significant refactorings related to
lvmcache, PVs with 0 mdas, etc, are required to set this field
for callers of pv_read().  For now we assume that if the field is
NULL, the pv is not associated with a complete VG (as returned
by vg_read()).

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

diff --git a/lib/metadata/metadata-exported.h b/lib/metadata/metadata-exported.h
index 865cb3b..20dd669 100644
--- a/lib/metadata/metadata-exported.h
+++ b/lib/metadata/metadata-exported.h
@@ -181,9 +181,20 @@ struct physical_volume {
 	struct id id;
 	struct device *dev;
 	const struct format_type *fmt;
+
+	/*
+	 * vg_name and vgid are used before the parent VG struct exists.
+	 * FIXME: remove these in favor of using 'vg' below.
+	 */
 	const char *vg_name;
 	struct id vgid;
 
+	/*
+	 * 'vg' is set and maintained when the PV belongs to a 'pvs'
+	 * list in a parent VG struct.
+	 */
+	struct volume_group *vg;
+
 	uint64_t status;
 	uint64_t size;
 
diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
index 85fa097..7369a3a 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -140,12 +140,14 @@ void add_pvl_to_vgs(struct volume_group *vg, struct pv_list *pvl)
 {
 	dm_list_add(&vg->pvs, &pvl->list);
 	vg->pv_count++;
+	pvl->pv->vg = vg;
 }
 
 void del_pvl_from_vgs(struct volume_group *vg, struct pv_list *pvl)
 {
 	vg->pv_count--;
 	dm_list_del(&pvl->list);
+	pvl->pv->vg = NULL;
 }
 
 
@@ -2150,6 +2152,12 @@ int vg_validate(struct volume_group *vg)
 			/* FIXME Dump list structure? */
 			r = 0;
 		}
+		if (pvl->pv->vg != vg) {
+			log_error(INTERNAL_ERROR "VG %s PV list entry points "
+				  "to different VG %s", vg->name,
+				  pvl->pv->vg ? pvl->pv->vg->name : "NULL");
+			r = 0;
+		}
 	}
 
 	loop_counter1 = loop_counter2 = 0;
-- 
1.6.0.6