[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 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 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 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 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 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.

[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 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.

[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