simplify vg_read_orphans

All this function does is create a 'pv' struct, setting
the pv fields from the lvmcache 'info' struct, which was
collected by label scan.  The previous implementation was
very complicated for no apparent reason, perhaps the
various parts were overdesigned in expectation of using
them for other things beyond reading orphans.

7 files changed, 40 insertions, 230 deletions

diff --git a/lib/cache/lvmcache.c b/lib/cache/lvmcache.c
index 8253fd237..01177e601 100644
--- a/lib/cache/lvmcache.c
+++ b/lib/cache/lvmcache.c
@@ -2459,56 +2459,29 @@ int lvmcache_fid_add_mdas_vg(struct lvmcache_vginfo *vginfo, struct format_insta
 	return 1;
 }
 
-static int _get_pv_if_in_vg(struct lvmcache_info *info,
-			    struct physical_volume *pv)
-{
-	char vgname[NAME_LEN + 1];
-	char vgid[ID_LEN + 1];
-
-	if (info->vginfo && info->vginfo->vgname &&
-	    !is_orphan_vg(info->vginfo->vgname)) {
-		/*
-		 * get_pv_from_vg_by_id() may call
-		 * lvmcache_label_scan() and drop cached
-		 * vginfo so make a local copy of string.
-		 */
-		(void) dm_strncpy(vgname, info->vginfo->vgname, sizeof(vgname));
-		memcpy(vgid, info->vginfo->vgid, sizeof(vgid));
-
-		if (get_pv_from_vg_by_id(info->fmt, vgname, vgid,
-					 info->dev->pvid, pv))
-			return 1;
-	}
-
-	return 0;
-}
-
 int lvmcache_populate_pv_fields(struct lvmcache_info *info,
-				struct physical_volume *pv,
-				int scan_label_only)
+				struct volume_group *vg,
+				struct physical_volume *pv)
 {
 	struct data_area_list *da;
-
-	/* Have we already cached vgname? */
-	if (!scan_label_only && _get_pv_if_in_vg(info, pv))
-		return 1;
-
-	/* Perform full scan (just the first time) and try again */
-	if (!scan_label_only && !critical_section() && !full_scan_done()) {
-		lvmcache_force_next_label_scan();
-		lvmcache_label_scan(info->fmt->cmd);
-
-		if (_get_pv_if_in_vg(info, pv))
-			return 1;
+	
+	if (!info->label) {
+		log_error("No cached label for orphan PV %s", pv_dev_name(pv));
+		return 0;
 	}
 
-	/* Orphan */
+	pv->label_sector = info->label->sector;
 	pv->dev = info->dev;
 	pv->fmt = info->fmt;
 	pv->size = info->device_size >> SECTOR_SHIFT;
-	pv->vg_name = FMT_TEXT_ORPHAN_VG_NAME;
+	pv->vg_name = vg->name;
 	memcpy(&pv->id, &info->dev->pvid, sizeof(pv->id));
 
+	if (!pv->size) {
+		log_error("PV %s size is zero.", dev_name(info->dev));
+		return 0;
+	}
+
 	/* Currently only support exactly one data area */
 	if (dm_list_size(&info->das) != 1) {
 		log_error("Must be exactly one data area (found %d) on PV %s",
diff --git a/lib/cache/lvmcache.h b/lib/cache/lvmcache.h
index abdea30b2..676071f5c 100644
--- a/lib/cache/lvmcache.h
+++ b/lib/cache/lvmcache.h
@@ -145,8 +145,8 @@ int lvmcache_fid_add_mdas(struct lvmcache_info *info, struct format_instance *fi
 int lvmcache_fid_add_mdas_pv(struct lvmcache_info *info, struct format_instance *fid);
 int lvmcache_fid_add_mdas_vg(struct lvmcache_vginfo *vginfo, struct format_instance *fid);
 int lvmcache_populate_pv_fields(struct lvmcache_info *info,
-				struct physical_volume *pv,
-				int scan_label_only);
+				struct volume_group *vg,
+				struct physical_volume *pv);
 int lvmcache_check_format(struct lvmcache_info *info, const struct format_type *fmt);
 void lvmcache_del_mdas(struct lvmcache_info *info);
 void lvmcache_del_das(struct lvmcache_info *info);
diff --git a/lib/format1/format1.c b/lib/format1/format1.c
index 816e2580f..db685be92 100644
--- a/lib/format1/format1.c
+++ b/lib/format1/format1.c
@@ -313,37 +313,6 @@ static int _format1_vg_write(struct format_instance *fid, struct volume_group *v
 	return r;
 }
 
-static int _format1_pv_read(const struct format_type *fmt, const char *pv_name,
-		    struct physical_volume *pv, int scan_label_only __attribute__((unused)))
-{
-	struct dm_pool *mem = dm_pool_create("lvm1 pv_read", 1024);
-	struct disk_list *dl;
-	struct device *dev;
-	int r = 0;
-
-	log_very_verbose("Reading physical volume data %s from disk", pv_name);
-
-	if (!mem)
-		return_0;
-
-	if (!(dev = dev_cache_get(pv_name, fmt->cmd->filter)))
-		goto_out;
-
-	if (!(dl = read_disk(fmt, dev, mem, NULL)))
-		goto_out;
-
-	if (!import_pv(fmt, fmt->cmd->mem, dl->dev, NULL, pv, &dl->pvd, &dl->vgd))
-		goto_out;
-
-	pv->fmt = fmt;
-
-	r = 1;
-
-      out:
-	dm_pool_destroy(mem);
-	return r;
-}
-
 static int _format1_pv_initialise(const struct format_type * fmt,
 				  struct pv_create_args *pva,
 				  struct physical_volume * pv)
@@ -555,7 +524,6 @@ static void _format1_destroy(struct format_type *fmt)
 }
 
 static struct format_handler _format1_ops = {
-	.pv_read = _format1_pv_read,
 	.pv_initialise = _format1_pv_initialise,
 	.pv_setup = _format1_pv_setup,
 	.pv_write = _format1_pv_write,
diff --git a/lib/format_pool/format_pool.c b/lib/format_pool/format_pool.c
index 93214a179..0e6cf78e1 100644
--- a/lib/format_pool/format_pool.c
+++ b/lib/format_pool/format_pool.c
@@ -179,43 +179,6 @@ static int _pool_pv_setup(const struct format_type *fmt __attribute__((unused)),
 	return 1;
 }
 
-static int _pool_pv_read(const struct format_type *fmt, const char *pv_name,
-			 struct physical_volume *pv,
-			 int scan_label_only __attribute__((unused)))
-{
-	struct dm_pool *mem = dm_pool_create("pool pv_read", 1024);
-	struct pool_list *pl;
-	struct device *dev;
-	int r = 0;
-
-	log_very_verbose("Reading physical volume data %s from disk", pv_name);
-
-	if (!mem)
-		return_0;
-
-	if (!(dev = dev_cache_get(pv_name, fmt->cmd->filter)))
-		goto_out;
-
-	/*
-	 * I need to read the disk and populate a pv structure here
-	 * I'll probably need to abstract some of this later for the
-	 * vg_read code
-	 */
-	if (!(pl = read_pool_disk(fmt, dev, mem, NULL)))
-		goto_out;
-
-	if (!import_pool_pv(fmt, fmt->cmd->mem, NULL, pv, pl))
-		goto_out;
-
-	pv->fmt = fmt;
-
-	r = 1;
-
-      out:
-	dm_pool_destroy(mem);
-	return r;
-}
-
 /* *INDENT-OFF* */
 static struct metadata_area_ops _metadata_format_pool_ops = {
 	.vg_read = _pool_vg_read,
@@ -266,7 +229,6 @@ static void _pool_destroy(struct format_type *fmt)
 
 /* *INDENT-OFF* */
 static struct format_handler _format_pool_ops = {
-	.pv_read = _pool_pv_read,
 	.pv_initialise = _pool_pv_initialise,
 	.pv_setup = _pool_pv_setup,
 	.create_instance = _pool_create_instance,
diff --git a/lib/format_text/format-text.c b/lib/format_text/format-text.c
index c7fe20896..66f6bab86 100644
--- a/lib/format_text/format-text.c
+++ b/lib/format_text/format-text.c
@@ -1584,36 +1584,6 @@ static uint64_t _metadata_locn_offset_raw(void *metadata_locn)
 	return mdac->area.start;
 }
 
-static int _text_pv_read(const struct format_type *fmt, const char *pv_name,
-		    struct physical_volume *pv, int scan_label_only)
-{
-	struct lvmcache_info *info;
-	struct device *dev;
-
-	if (!(dev = dev_cache_get(pv_name, fmt->cmd->filter)))
-		return_0;
-
-	if (lvmetad_used()) {
-		info = lvmcache_info_from_pvid(dev->pvid, dev, 0);
-		if (!info && !lvmetad_pv_lookup_by_dev(fmt->cmd, dev, NULL))
-			return 0;
-		info = lvmcache_info_from_pvid(dev->pvid, dev, 0);
-	} else {
-		struct label *label;
-		if (!(label_read(dev, &label, UINT64_C(0))))
-			return_0;
-		info = label->info;
-	}
-
-	if (!info)
-		return_0;
-
-	if (!lvmcache_populate_pv_fields(info, pv, scan_label_only))
-		return 0;
-
-	return 1;
-}
-
 static int _text_pv_initialise(const struct format_type *fmt,
 			       struct pv_create_args *pva,
 			       struct physical_volume *pv)
@@ -2462,7 +2432,6 @@ static struct format_instance *_text_create_text_instance(const struct format_ty
 
 static struct format_handler _text_handler = {
 	.scan = _text_scan,
-	.pv_read = _text_pv_read,
 	.pv_initialise = _text_pv_initialise,
 	.pv_setup = _text_pv_setup,
 	.pv_add_metadata_area = _text_pv_add_metadata_area,
diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
index ff22277fc..0e1396e7a 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -40,10 +40,8 @@
 #include <sys/param.h>
 
 static struct physical_volume *_pv_read(struct cmd_context *cmd,
-					struct dm_pool *pvmem,
-					const char *pv_name,
-					struct format_instance *fid,
-					uint32_t warn_flags, int scan_label_only);
+					struct volume_group *vg,
+					struct lvmcache_info *info);
 
 static int _alignment_overrides_default(unsigned long data_alignment,
 					unsigned long default_pe_align)
@@ -3254,9 +3252,6 @@ static int _check_mda_in_use(struct metadata_area *mda, void *_in_use)
 struct _vg_read_orphan_baton {
 	struct cmd_context *cmd;
 	struct volume_group *vg;
-	uint32_t warn_flags;
-	int consistent;
-	int repair;
 };
 
 /*
@@ -3353,8 +3348,7 @@ static int _vg_read_orphan_pv(struct lvmcache_info *info, void *baton)
 	uint32_t ext_version;
 	uint32_t ext_flags;
 
-	if (!(pv = _pv_read(b->vg->cmd, b->vg->vgmem, dev_name(lvmcache_device(info)),
-			    b->vg->fid, b->warn_flags, 0))) {
+	if (!(pv = _pv_read(b->cmd, b->vg, info))) {
 		stack;
 		return 1;
 	}
@@ -3461,10 +3455,21 @@ static struct volume_group *_vg_read_orphans(struct cmd_context *cmd,
 	vg->free_count = 0;
 
 	baton.cmd = cmd;
-	baton.warn_flags = warn_flags;
 	baton.vg = vg;
-	baton.consistent = 1;
-	baton.repair = *consistent;
+
+	/*
+	 * vg_read for a normal VG will rescan labels for all the devices
+	 * in the VG, in case something changed on disk between the initial
+	 * label scan and acquiring the VG lock.  We don't rescan labels
+	 * here because this is only called in two ways:
+	 *
+	 * 1. for reporting, in which case it doesn't matter if something
+	 *    changed between the label scan and printing the PVs here
+	 *
+	 * 2. pvcreate_each_device() for pvcreate//vgcreate/vgextend,
+	 *    which already does the label rescan after taking the
+	 *    orphan lock.
+	 */
 
 	while ((pvl = (struct pv_list *) dm_list_first(&head.list))) {
 		dm_list_del(&pvl->list);
@@ -3476,7 +3481,6 @@ static struct volume_group *_vg_read_orphans(struct cmd_context *cmd,
 	if (!lvmcache_foreach_pv(vginfo, _vg_read_orphan_pv, &baton))
 		return_NULL;
 
-	*consistent = baton.consistent;
 	return vg;
 }
 
@@ -4689,86 +4693,26 @@ const char *find_vgname_from_pvname(struct cmd_context *cmd,
 	return find_vgname_from_pvid(cmd, pvid);
 }
 
-/* FIXME Use label functions instead of PV functions */
 static struct physical_volume *_pv_read(struct cmd_context *cmd,
-					struct dm_pool *pvmem,
-					const char *pv_name,
-					struct format_instance *fid,
-					uint32_t warn_flags, int scan_label_only)
+					struct volume_group *vg,
+					struct lvmcache_info *info)
 {
 	struct physical_volume *pv;
-	struct label *label;
-	struct lvmcache_info *info;
-	struct device *dev;
-	const struct format_type *fmt;
-	int found;
-
-	if (!(dev = dev_cache_get(pv_name, cmd->filter)))
-		return_NULL;
-
-	if (lvmetad_used()) {
-		info = lvmcache_info_from_pvid(dev->pvid, dev, 0);
-		if (!info) {
-			if (!lvmetad_pv_lookup_by_dev(cmd, dev, &found))
-				return_NULL;
-			if (!found) {
-				if (warn_flags & WARN_PV_READ)
-					log_error("No physical volume found in lvmetad cache for %s",
-						  pv_name);
-				return NULL;
-			}
-			if (!(info = lvmcache_info_from_pvid(dev->pvid, dev, 0))) {
-				if (warn_flags & WARN_PV_READ)
-					log_error("No cache info in lvmetad cache for %s.",
-						  pv_name);
-				return NULL;
-			}
-		}
-		label = lvmcache_get_label(info);
-	} else {
-		if (!(label_read(dev, &label, UINT64_C(0)))) {
-			if (warn_flags & WARN_PV_READ)
-				log_error("No physical volume label read from %s",
-					  pv_name);
-			return NULL;
-		}
-		info = (struct lvmcache_info *) label->info;
-	}
-
-	fmt = lvmcache_fmt(info);
 
-	pv = _alloc_pv(pvmem, dev);
-	if (!pv) {
-		log_error("pv allocation for '%s' failed", pv_name);
+	if (!(pv = _alloc_pv(vg->vgmem, NULL))) {
+		log_error("pv allocation failed");
 		return NULL;
 	}
 
-	pv->label_sector = label->sector;
-
-	/* FIXME Move more common code up here */
-	if (!(lvmcache_fmt(info)->ops->pv_read(lvmcache_fmt(info), pv_name, pv, scan_label_only))) {
-		log_error("Failed to read existing physical volume '%s'",
-			  pv_name);
-		goto bad;
-	}
-
-	if (!pv->size)
-		goto bad;
-
-	if (!alloc_pv_segment_whole_pv(pvmem, pv))
+	if (!lvmcache_populate_pv_fields(info, vg, pv))
 		goto_bad;
 
-	if (fid)
-		lvmcache_fid_add_mdas(info, fid, (const char *) &pv->id, ID_LEN);
-	else {
-		lvmcache_fid_add_mdas(info, fmt->orphan_vg->fid, (const char *) &pv->id, ID_LEN);
-		pv_set_fid(pv, fmt->orphan_vg->fid);
-	}
-
+	lvmcache_fid_add_mdas(info, vg->fid, (const char *) &pv->id, ID_LEN);
+	pv_set_fid(pv, vg->fid);
 	return pv;
 bad:
 	free_pv_fid(pv);
-	dm_pool_free(pvmem, pv);
+	dm_pool_free(vg->vgmem, pv);
 	return NULL;
 }
 
diff --git a/lib/metadata/metadata.h b/lib/metadata/metadata.h
index 145d0428a..1a569a611 100644
--- a/lib/metadata/metadata.h
+++ b/lib/metadata/metadata.h
@@ -255,12 +255,6 @@ struct format_handler {
 	int (*scan) (const struct format_type * fmt, const char *vgname);
 
 	/*
-	 * Return PV with given path.
-	 */
-	int (*pv_read) (const struct format_type * fmt, const char *pv_name,
-			struct physical_volume * pv, int scan_label_only);
-
-	/*
 	 * Initialise a new PV.
 	 */
 	int (*pv_initialise) (const struct format_type * fmt,