[PATCH 2/3] Add find_vgname_from_pvname() function using lvmcache functions.

[PATCH 2/3] Add find_vgname_from_pvname() function using lvmcache functions.

[Dave Wysochanski]

Some commands start with a pvname, but we'd like to force users to
start with a vg handle to obtain a pv handle.  Our best option seems
to be providing a way to look up the vgname from the pvname, and then
requiring them to use vg_read/vg_open.  This find function first
does an intial scan, then asks lvmcache for the vgname.  If the
vgname is an orphan, we then check to see if there's metadata areas,
and if not, we scan every PV on the system by calling
scan_vgs_for_pvs().  In most cases we should not need to do this,
and by using the info->mdas count, we avoid calling pv_read() as
prior code did.  So this patch is a bit cleaner IMO and should
allow us to refactor more of the pv code.

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

diff --git a/lib/metadata/metadata-exported.h b/lib/metadata/metadata-exported.h
index f8329d5..597b913 100644
--- a/lib/metadata/metadata-exported.h
+++ b/lib/metadata/metadata-exported.h
@@ -605,6 +605,7 @@ struct logical_volume *find_lv(const struct volume_group *vg,
 struct physical_volume *find_pv_by_name(struct cmd_context *cmd,
 					const char *pv_name);
 
+char *find_vgname_from_pvname(struct cmd_context *cmd, const char *pvname);
 /* Find LV segment containing given LE */
 struct lv_segment *first_seg(const struct logical_volume *lv);
 
diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
index 28c7c47..cec0b7e 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -2971,6 +2971,48 @@ out:
 	return NULL;
 }
 
+
+char *find_vgname_from_pvname(struct cmd_context *cmd, const char *pvname)
+{
+	char *vgname;
+	struct lvmcache_info *info;
+
+	/*
+	 * Initially, all we have is a pv name - we don't know the VG name.
+	 * Try to look the name up in lvmcache.
+	 */
+	lvmcache_label_scan(cmd, 0);
+	vgname = lvmcache_vgname_from_pvname(cmd, pvname);
+
+	if (is_orphan_vg(vgname)) {
+		/*
+		 * If it's an orphan though, we need to do more work,
+		 * if there's no mdas.
+		 */
+		if (!(info = info_from_pvname(pvname, 1))) {
+			return_NULL;
+		}
+		/*
+		 * If a PV has no MDAs it may appear to be an
+		 * orphan until the metadata is read off
+		 * another PV in the same VG.  Detecting this
+		 * means checking every VG by scanning every
+		 * PV on the system.
+		 */
+		if (!dm_list_size(&info->mdas)) {
+			if (!scan_vgs_for_pvs(cmd)) {
+				log_error("Rescan for PVs without "
+					  "metadata areas failed.");
+				return NULL;
+			}
+		}
+		/* Ask lvmcache again - we may have a non-orphan name now */
+		vgname = lvmcache_vgname_from_pvname(cmd, pvname);
+	}
+
+	return vgname;
+}
+
 /**
  * pv_read - read and return a handle to a physical volume
  * @cmd: LVM command initiating the pv_read
-- 
1.6.0.6

[PATCH 0/3] Add infrastructure for cleaner pv handling, v2

[Dave Wysochanski]

This is the second version of the patchset that adds infrastructure
to move us more towards the goal of removing the standalone PV object.

>From now on, a get/set of a PV property from the pvname requires
a lookup of the vgname, then a vg handle from the vgname, a
pv from the vg handle, and finally using the pv handle to get/set
the property.

The last patch reworks pvchange to use this new functionality.
It passes the testsuite but does have at least one bug.  A
setup with a VG containing a PV with metadatacopies == 0 and
pvchange -u --all end up failing because the PV is on both the
orphan VG and the real VG.  I tried a few ways of working around
this, including processing the non-orphan VGs first, but I believe
the root problem lies in the vg locking and lvmcache.
The lvmcache gets invalidated after each vg lock change, so even
if a PV with metadata copies was known to be in a real VG, once
that lock is dropped the cache is invalidated and the PV shows up
in the vg->pvs list for the orphan VG.

[PATCH 1/3] Add a couple of lvmcache functions to aid in lookups by pvname.

[Dave Wysochanski]

It would be nice to clean up some of the pv handling and move away from
the pv as a standalone object.  To this end, I introduce a couple helper
functions from lvmcache which allow us to lookup a vgname from a pvname
and an info struct from a pvname.  These functions will be used later
in a library function that we will export and will aid in cleaning
up the pv handling.

Note: Inside lvmcache, we cannot handle the case of the PV with no mdas
that is in a VG.  The reason is the lvmcache 'info' only gets updated
by calling higher-level _vg_read() function, which calls
lvmcache_update_vg().  I did not think it was a good design to parse
per-format metadata inside lvmcache, or call vg_read() from inside
lvmcache.  It seems clear to me the proper place for the additional
scanning calls is at an upper layer, which will be the next patch.

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 lib/cache/lvmcache.c |   40 ++++++++++++++++++++++++++++++++++++++++
 lib/cache/lvmcache.h |    3 +++
 2 files changed, 43 insertions(+), 0 deletions(-)

diff --git a/lib/cache/lvmcache.c b/lib/cache/lvmcache.c
index 35ee05b..40dc2a1 100644
--- a/lib/cache/lvmcache.c
+++ b/lib/cache/lvmcache.c
@@ -499,6 +499,46 @@ struct lvmcache_info *info_from_pvid(const char *pvid, int valid_only)
 	return info;
 }
 
+struct lvmcache_info *info_from_pvname(struct cmd_context *cmd,
+				       const char *pvname)
+{
+	struct lvmcache_info *info;
+	struct device *dev;
+	struct label *label;
+
+	if (!(dev = dev_cache_get(pvname, cmd->filter))) {
+		log_error("%s: Couldn't find device.  "
+			  "Check your filters?", pvname);
+		return NULL;
+	}
+	if (dev->pvid[0] == '\0')
+		label_read(dev, &label, UINT64_C(0));
+
+	info = info_from_pvid(dev->pvid, 0);
+	if (!info || dev != info->dev) {
+		log_error("Couldn't find %s in lvmcache.", pvname);
+		return NULL;
+	}
+	return info;
+}
+
+char *lvmcache_vgname_from_pvname(struct cmd_context *cmd, const char *pvname)
+{
+	struct lvmcache_info *info;
+	char *vgname;
+
+	info = info_from_pvname(cmd, pvname);
+	if (!info)
+		return_NULL;
+
+	if (!(vgname = dm_pool_alloc(cmd->mem, NAME_LEN+1))) {
+		log_errno(ENOMEM, "vgname allocation failed");
+		return NULL;
+	}
+	strncpy(vgname, info->vginfo->vgname, NAME_LEN);
+	return vgname;
+}
+
 static void _rescan_entry(struct lvmcache_info *info)
 {
 	struct label *label;
diff --git a/lib/cache/lvmcache.h b/lib/cache/lvmcache.h
index 2c14b6c..ba620ba 100644
--- a/lib/cache/lvmcache.h
+++ b/lib/cache/lvmcache.h
@@ -94,6 +94,9 @@ struct lvmcache_vginfo *vginfo_from_vgid(const char *vgid);
 struct lvmcache_info *info_from_pvid(const char *pvid, int valid_only);
 const char *vgname_from_vgid(struct dm_pool *mem, const char *vgid);
 struct device *device_from_pvid(struct cmd_context *cmd, struct id *pvid);
+char *lvmcache_vgname_from_pvname(struct cmd_context *cmd, const char *pvname);
+struct lvmcache_info *info_from_pvname(struct cmd_context *cmd,
+				       const char *pvname);
 int vgs_locked(void);
 int vgname_is_locked(const char *vgname);
 
-- 
1.6.0.6

[PATCH 2/3] Add find_vgname_from_pvname() function using lvmcache functions.

[Dave Wysochanski]

Some commands start with a pvname, but we'd like to force users to
start with a vg handle to obtain a pv handle.  Our best option seems
to be providing a way to look up the vgname from the pvname, and then
requiring them to use vg_read/vg_open.  Similarly, we could hide the
vgname lookup function inside a higher level API such as 'pv_open()',
which would take a pvname or pvid, and do the necessary lookups to
find the vgname, then do a vg_open(), and return the proper pv handle
attached to the VG.  In any case, we need this supporting function.

This find function first does an intial scan, then asks lvmcache for
the vgname.  If the vgname is an orphan, we then check to see if
there are metadata areas, and if not, we scan every PV on the system by
calling scan_vgs_for_pvs().  In most cases we should not need to do
this, and by using the info->mdas count, we avoid calling pv_read()
as prior code did.  So this patch is a bit cleaner IMO and should
allow us to refactor more of the pv code.

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

diff --git a/lib/metadata/metadata-exported.h b/lib/metadata/metadata-exported.h
index 9540285..eb2576c 100644
--- a/lib/metadata/metadata-exported.h
+++ b/lib/metadata/metadata-exported.h
@@ -609,6 +609,7 @@ struct logical_volume *find_lv(const struct volume_group *vg,
 struct physical_volume *find_pv_by_name(struct cmd_context *cmd,
 					const char *pv_name);
 
+char *find_vgname_from_pvname(struct cmd_context *cmd, const char *pvname);
 /* Find LV segment containing given LE */
 struct lv_segment *first_seg(const struct logical_volume *lv);
 
diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
index f1b90a7..5d98091 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -3031,6 +3031,48 @@ out:
 	return NULL;
 }
 
+
+char *find_vgname_from_pvname(struct cmd_context *cmd, const char *pvname)
+{
+	char *vgname;
+	struct lvmcache_info *info;
+
+	/*
+	 * Initially, all we have is a pv name - we don't know the VG name.
+	 * Try to look the name up in lvmcache.
+	 */
+	lvmcache_label_scan(cmd, 0);
+	vgname = lvmcache_vgname_from_pvname(cmd, pvname);
+
+	if (is_orphan_vg(vgname)) {
+		/*
+		 * If it's an orphan though, we need to do more work,
+		 * if there's no mdas.
+		 */
+		if (!(info = info_from_pvname(cmd, pvname))) {
+			return_NULL;
+		}
+		/*
+		 * If a PV has no MDAs it may appear to be an
+		 * orphan until the metadata is read off
+		 * another PV in the same VG.  Detecting this
+		 * means checking every VG by scanning every
+		 * PV on the system.
+		 */
+		if (!dm_list_size(&info->mdas)) {
+			if (!scan_vgs_for_pvs(cmd)) {
+				log_error("Rescan for PVs without "
+					  "metadata areas failed.");
+				return NULL;
+			}
+		}
+		/* Ask lvmcache again - we may have a non-orphan name now */
+		vgname = lvmcache_vgname_from_pvname(cmd, pvname);
+	}
+
+	return vgname;
+}
+
 /**
  * pv_read - read and return a handle to a physical volume
  * @cmd: LVM command initiating the pv_read
-- 
1.6.0.6

[PATCH 3/3] Update pvchange to always obtain a vg handle for each pv to process.

[Dave Wysochanski]

Earlier patches added some infrastructure to lookup a vgname from
a pvname.  We now can cleanup some of the pvchange and other code
by requiring callers that want to modify some pv property:
1) lookup the vgname by the pvname
2) use the vgname to obtain a vg handle
3) get the pv handle from the vg handle

This should work going forward and be a much cleaner interface,
as we move away from pvs as standalone objects.

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 tools/pvchange.c |  116 +++++++++++++++++++++--------------------------------
 1 files changed, 46 insertions(+), 70 deletions(-)

diff --git a/tools/pvchange.c b/tools/pvchange.c
index cba9dd2..0cf1c98 100644
--- a/tools/pvchange.c
+++ b/tools/pvchange.c
@@ -17,13 +17,10 @@
 
 /* FIXME Locking.  PVs in VG. */
 
-static int _pvchange_single(struct cmd_context *cmd, struct physical_volume *pv,
+static int _pvchange_single(struct cmd_context *cmd, struct volume_group *vg,
+			    struct physical_volume *pv,
 			    void *handle __attribute((unused)))
 {
-	struct volume_group *vg = NULL;
-	const char *vg_name = NULL;
-	struct pv_list *pvl;
-	uint64_t sector;
 	uint32_t orig_pe_alloc_count;
 	/* FIXME Next three only required for format1. */
 	uint32_t orig_pe_count, orig_pe_size;
@@ -53,21 +50,6 @@ static int _pvchange_single(struct cmd_context *cmd, struct physical_volume *pv,
 
 	/* If in a VG, must change using volume group. */
 	if (!is_orphan(pv)) {
-		vg_name = pv_vg_name(pv);
-
-		log_verbose("Finding volume group %s of physical volume %s",
-			    vg_name, pv_name);
-		vg = vg_read_for_update(cmd, vg_name, NULL, 0);
-		if (vg_read_error(vg)) {
-			vg_release(vg);
-			return_0;
-		}
-
-		if (!(pvl = find_pv_in_vg(vg, pv_name))) {
-			log_error("Unable to find \"%s\" in volume group \"%s\"",
-				  pv_name, vg->name);
-			goto out;
-		}
 		if (tagarg && !(vg->fid->fmt->features & FMT_TAGS)) {
 			log_error("Volume group containing %s does not "
 				  "support tags", pv_name);
@@ -78,7 +60,6 @@ static int _pvchange_single(struct cmd_context *cmd, struct physical_volume *pv,
 				  "logical volumes", pv_name);
 			goto out;
 		}
-		pv = pvl->pv;
 		if (!archive(vg))
 			goto out;
 	} else {
@@ -87,19 +68,6 @@ static int _pvchange_single(struct cmd_context *cmd, struct physical_volume *pv,
 				  "in volume group", pv_name);
 			return 0;
 		}
-
-		vg_name = VG_ORPHANS;
-
-		if (!lock_vol(cmd, vg_name, LCK_VG_WRITE)) {
-			log_error("Can't get lock for orphans");
-			return 0;
-		}
-
-		if (!(pv = pv_read(cmd, pv_name, NULL, &sector, 1, 0))) {
-			unlock_vg(cmd, vg_name);
-			log_error("Unable to read PV \"%s\"", pv_name);
-			return 0;
-		}
 	}
 
 	if (arg_count(cmd, allocatable_ARG)) {
@@ -201,7 +169,6 @@ static int _pvchange_single(struct cmd_context *cmd, struct physical_volume *pv,
 	log_print("Physical volume \"%s\" changed", pv_name);
 	r = 1;
 out:
-	unlock_and_release_vg(cmd, vg, vg_name);
 	return r;
 
 }
@@ -212,12 +179,12 @@ int pvchange(struct cmd_context *cmd, int argc, char **argv)
 	int done = 0;
 	int total = 0;
 
-	struct physical_volume *pv;
-	char *pv_name;
+	struct volume_group *vg;
+	const char *pv_name, *vg_name;
 
 	struct pv_list *pvl;
-	struct dm_list *pvslist;
-	struct dm_list mdas;
+	struct dm_list *vgnames;
+	struct str_list *sll;
 
 	if (arg_count(cmd, allocatable_ARG) + arg_count(cmd, addtag_ARG) +
 	    arg_count(cmd, deltag_ARG) + arg_count(cmd, uuid_ARG) != 1) {
@@ -240,47 +207,56 @@ int pvchange(struct cmd_context *cmd, int argc, char **argv)
 		log_verbose("Using physical volume(s) on command line");
 		for (; opt < argc; opt++) {
 			pv_name = argv[opt];
-			dm_list_init(&mdas);
-			if (!(pv = pv_read(cmd, pv_name, &mdas, NULL, 1, 0))) {
+			vg_name = find_vgname_from_pvname(cmd, pv_name);
+			if (!vg_name) {
 				log_error("Failed to read physical volume %s",
 					  pv_name);
 				continue;
 			}
-			/*
-			 * If a PV has no MDAs it may appear to be an
-			 * orphan until the metadata is read off
-			 * another PV in the same VG.  Detecting this
-			 * means checking every VG by scanning every
-			 * PV on the system.
-			 */
-			if (is_orphan(pv) && !dm_list_size(&mdas)) {
-				if (!scan_vgs_for_pvs(cmd)) {
-					log_error("Rescan for PVs without "
-						  "metadata areas failed.");
-					continue;
-				}
-				if (!(pv = pv_read(cmd, pv_name,
-						   NULL, NULL, 1, 0))) {
-					log_error("Failed to read "
-						  "physical volume %s",
-						  pv_name);
-					continue;
-				}
+			vg = vg_read_for_update(cmd, vg_name, NULL, 0);
+			if (vg_read_error(vg)) {
+				vg_release(vg);
+				stack;
+				continue;
+			}
+			pvl = find_pv_in_vg(vg, pv_name);
+			if (!pvl || !pvl->pv) {
+				log_error("Unable to find %s in %s",
+					  pv_name, vg_name);
+				continue;
 			}
 
 			total++;
-			done += _pvchange_single(cmd, pv, NULL);
+			done += _pvchange_single(cmd, vg,
+						 pvl->pv, NULL);
+			/* FIXME: we should be using #orphans_lvm2 everwhere */
+			if (is_orphan_vg(vg->name))
+				vg_name = VG_ORPHANS;
+			unlock_and_release_vg(cmd, vg, vg_name);
 		}
 	} else {
 		log_verbose("Scanning for physical volume names");
-		if (!(pvslist = get_pvs(cmd))) {
-			stack;
-			return ECMD_FAILED;
-		}
-
-		dm_list_iterate_items(pvl, pvslist) {
-			total++;
-			done += _pvchange_single(cmd, pvl->pv, NULL);
+		/* FIXME: share code with toollib */
+		if ((vgnames = get_vgnames(cmd, 0, 1)) &&
+		    !dm_list_empty(vgnames)) {
+			dm_list_iterate_items(sll, vgnames) {
+				vg = vg_read_for_update(cmd, sll->str, NULL, 0);
+				if (vg_read_error(vg)) {
+					vg_release(vg);
+					stack;
+					continue;
+				}
+				dm_list_iterate_items(pvl, &vg->pvs) {
+					total++;
+					done += _pvchange_single(cmd, vg,
+								 pvl->pv,
+								 NULL);
+				}
+				/* FIXME: we should be using #orphans_lvm2 everwhere */
+				if (is_orphan_vg(vg->name))
+					vg_name = VG_ORPHANS;
+				unlock_and_release_vg(cmd, vg, vg_name);
+			}
 		}
 	}
 
-- 
1.6.0.6

[PATCH 2/3] Add find_vgname_from_pvname() function using lvmcache functions.

[Dave Wysochanski]

On Wed, 2010-03-17 at 11:51 -0400, Dave Wysochanski wrote:
> +char *find_vgname_from_pvname(struct cmd_context *cmd, const char
> *pvname)
> +{
> +       char *vgname;
> +       struct lvmcache_info *info;
> +
> +       /*
> +        * Initially, all we have is a pv name - we don't know the VG
> name.
> +        * Try to look the name up in lvmcache.
> +        */
> +       lvmcache_label_scan(cmd, 0);
> +       vgname = lvmcache_vgname_from_pvname(cmd, pvname);
> +

Actually the lvmcache_label_scan() should not be necessary here.

We read the label for the device later and the expensive scans if
necessary (orphan and no mdas).