[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

[lvm-devel] [PATCH 06/11] Refactor pv_setup to not include the initialisation only code.



We'll use new pv_initialise to initialise a PV. For any other use where
we need to recalculate the parameters for an existing PV that is part of
a VG, we'll use pv_setup code.

Also, since we're using the cache now to hold any metadata changes,
remove the mdas parameter from pv_setup/pv_write.

This patch also uses new pv_add_metadata_area interface when creating a
new PV.

Signed-off-by: Peter Rajnoha <prajnoha redhat com>
---
 lib/format1/format1.c            |   11 +--
 lib/format_pool/format_pool.c    |    9 --
 lib/format_text/archiver.c       |    5 +-
 lib/format_text/format-text.c    |  219 ++++++++++++++++----------------------
 lib/metadata/metadata-exported.h |    7 +-
 lib/metadata/metadata.c          |   53 ++++-----
 lib/metadata/metadata.h          |    8 +-
 tools/vgconvert.c                |    6 +-
 8 files changed, 127 insertions(+), 191 deletions(-)

diff --git a/lib/format1/format1.c b/lib/format1/format1.c
index 4904a59..b2e4aaf 100644
--- a/lib/format1/format1.c
+++ b/lib/format1/format1.c
@@ -424,15 +424,8 @@ static int _format1_pv_initialise(const struct format_type * fmt,
 }
 
 static int _format1_pv_setup(const struct format_type *fmt,
-			     uint64_t pe_start, uint32_t extent_count,
-			     uint32_t extent_size,
-			     unsigned long data_alignment __attribute__((unused)),
-			     unsigned long data_alignment_offset __attribute__((unused)),
-			     int pvmetadatacopies __attribute__((unused)),
-			     uint64_t pvmetadatasize __attribute__((unused)),
-			     unsigned metadataignore __attribute__((unused)),
-			     struct dm_list *mdas __attribute__((unused)),
-			     struct physical_volume *pv, struct volume_group *vg __attribute__((unused)))
+			     struct physical_volume *pv,
+			     struct volume_group *vg __attribute__((unused)))
 {
 	return _format1_pv_initialise(fmt, -1, 0, 0, vg->extent_size, 0, 0, pv);
 }
diff --git a/lib/format_pool/format_pool.c b/lib/format_pool/format_pool.c
index dbb73dd..a3719b2 100644
--- a/lib/format_pool/format_pool.c
+++ b/lib/format_pool/format_pool.c
@@ -201,15 +201,6 @@ static int _pool_pv_initialise(const struct format_type *fmt __attribute__((unus
 }
 
 static int _pool_pv_setup(const struct format_type *fmt __attribute__((unused)),
-			  uint64_t pe_start __attribute__((unused)),
-			  uint32_t extent_count __attribute__((unused)),
-			  uint32_t extent_size __attribute__((unused)),
-			  unsigned long data_alignment __attribute__((unused)),
-			  unsigned long data_alignment_offset __attribute__((unused)),
-			  int pvmetadatacopies __attribute__((unused)),
-			  uint64_t pvmetadatasize __attribute__((unused)),
-			  unsigned metadataignore __attribute__((unused)),
-			  struct dm_list *mdas __attribute__((unused)),
 			  struct physical_volume *pv __attribute__((unused)),
 			  struct volume_group *vg __attribute__((unused)))
 {
diff --git a/lib/format_text/archiver.c b/lib/format_text/archiver.c
index 3ace628..e215170 100644
--- a/lib/format_text/archiver.c
+++ b/lib/format_text/archiver.c
@@ -329,10 +329,7 @@ int backup_restore_vg(struct cmd_context *cmd, struct volume_group *vg)
 				  pv_dev_name(pv), info->fmt->name);
 			return 0;
 		}
-		if (!vg->fid->fmt->ops->
-		    pv_setup(vg->fid->fmt, UINT64_C(0), 0, 0, 0, 0, 0UL,
-			     UINT64_C(0), 0,
-			     &vg->fid->metadata_areas_in_use, pv, vg)) {
+		if (!vg->fid->fmt->ops->pv_setup(vg->fid->fmt, pv, vg)) {
 			log_error("Format-specific setup for %s failed",
 				  pv_dev_name(pv));
 			return 0;
diff --git a/lib/format_text/format-text.c b/lib/format_text/format-text.c
index e47cb81..f32e347 100644
--- a/lib/format_text/format-text.c
+++ b/lib/format_text/format-text.c
@@ -1849,153 +1849,116 @@ static struct metadata_area_ops _metadata_text_raw_ops = {
 	.mda_locns_match = _mda_locns_match_raw
 };
 
-/* pvmetadatasize in sectors */
-/*
- * pe_start goal: FIXME -- reality of .pv_write complexity undermines this goal
- * - In cases where a pre-existing pe_start is provided (pvcreate --restorefile
- *   and vgconvert): pe_start must not be changed (so pv->pe_start = pe_start).
- * - In cases where pe_start is 0: leave pv->pe_start as 0 and defer the
- *   setting of pv->pe_start to .pv_write
- */
 static int _text_pv_setup(const struct format_type *fmt,
-			  uint64_t pe_start, uint32_t extent_count,
-			  uint32_t extent_size, unsigned long data_alignment,
-			  unsigned long data_alignment_offset,
-			  int pvmetadatacopies, uint64_t pvmetadatasize,
-			  unsigned metadataignore, struct dm_list *mdas,
-			  struct physical_volume *pv, struct volume_group *vg)
+			  struct physical_volume *pv,
+			  struct volume_group *vg)
 {
-	struct metadata_area *mda, *mda_new, *mda2;
-	struct mda_context *mdac, *mdac2;
-	struct dm_list *pvmdas;
+	struct metadata_area *pv_mda, *vg_mda, *mda;
+	struct mda_context *pv_mdac, *vg_mdac, *mdac;
+	struct dm_list *vg_mdas;
 	struct lvmcache_info *info;
-	int found;
-	uint64_t pe_end = 0;
-	unsigned mda_count = 0;
-	uint64_t mda_size2 = 0;
 	uint64_t pe_count;
+	uint64_t size_reduction = 0;
+	int found;
 
-	/* FIXME Cope with pvchange */
-	/* FIXME Merge code with _text_create_text_instance */
-
-	/* If new vg, add any further mdas on this PV to the fid's mda list */
-	if (vg) {
-		/* Iterate through all mdas on this PV */
-		if ((info = info_from_pvid(pv->dev->pvid, 0))) {
-			pvmdas = &info->mdas;
-			dm_list_iterate_items(mda, pvmdas) {
-				mda_count++;
-				mdac =
-				    (struct mda_context *) mda->metadata_locn;
-
-				/* FIXME Check it isn't already in use */
-
-				/* Reduce usable device size */
-				if (mda_count > 1)
-					mda_size2 = mdac->area.size >> SECTOR_SHIFT;
-
-				/* Ensure it isn't already on list */
-				found = 0;
-				dm_list_iterate_items(mda2, mdas) {
-					if (mda2->ops !=
-					    &_metadata_text_raw_ops) continue;
-					mdac2 =
-					    (struct mda_context *)
-					    mda2->metadata_locn;
-					if (!memcmp
-					    (&mdac2->area, &mdac->area,
-					     sizeof(mdac->area))) {
-						found = 1;
-						break;
-					}
-				}
-				if (found)
+	/* FIXME Cope with pvchange
+	 * FIXME: What's wrong with pvchange here exactly???
+	 * 	 ...despite the fact that pvchange itself is
+	 * 	 a hack with respect to pv_write... :)
+	 */
+
+	/*
+	 * FIXME: this does not work entirely correctly in the case where a PV
+	 * has 2 mdas and only one is ignored; ideally all non-ignored mdas
+	 * should be placed on metadata_areas list and ignored on the
+	 * metadata_areas_ignored list; however this requires another
+	 * fairly complex refactoring to remove the 'mdas' parameter from both
+	 * pv_setup and pv_write.  For now, we only put ignored mdas on the
+	 * metadata_areas_ignored list if all mdas in the PV are ignored;
+	 * otherwise, we use the non-ignored list.
+	*/
+	/*
+	 * FIXME: Well, what was meant here exactly with that fixme before?
+	 *        Do we really want to support having N mdas on one PV and
+	 *        only some of them to be ignored??? Why? Is it feasible???
+	 *        If yes, then we need to extend "mda ignore" functionality
+	 *        to be able to select the mdas to ignore on that PV, not
+	 *        just the PV itself as a whole... (Probably only cmdline
+	 *        interface needed for this + a few changes inside, dunno...)
+	 */
+	if (!pv_mda_used_count(pv))
+		vg_mdas = &vg->fid->metadata_areas_ignored;
+	else
+		vg_mdas = &vg->fid->metadata_areas_in_use;
+
+	/* Iterate through all mdas on this PV */
+	if ((info = info_from_pvid(pv->dev->pvid, 0))) {
+		dm_list_iterate_items(pv_mda, &info->mdas) {
+			pv_mdac = (struct mda_context *) pv_mda->metadata_locn;
+			/* FIXME Check it isn't already in use */
+
+			/* Ensure it isn't already on list */
+			found = 0;
+			dm_list_iterate_items(vg_mda, vg_mdas) {
+				/* FIXME: Try to remove this kind of check.
+				 *        E.g. provide a clean way how to
+				 *        distinguish different subtypes of
+				 *        the format using mda separation,
+				 *        maybe a tree structure with each
+				 *        branch representing a subtype,
+				 *        indexed somehow...
+				 */
+				if (vg_mda->ops != &_metadata_text_raw_ops)
 					continue;
 
-				mda_new = mda_copy(fmt->cmd->mem, mda);
-				if (!mda_new)
-					return_0;
-				dm_list_add(mdas, &mda_new->list);
-				/* FIXME multiple dev_areas inside area */
+				vg_mdac = (struct mda_context *) vg_mda->metadata_locn;
+				if (!memcmp (&vg_mdac->area, &pv_mdac->area, sizeof(mdac->area))) {
+					found = 1;
+					break;
+				}
 			}
-		}
 
-		/* FIXME Cope with genuine pe_count 0 */
-
-		/* If missing, estimate pv->size from file-based metadata */
-		if (!pv->size && pv->pe_count)
-			pv->size = pv->pe_count * (uint64_t) vg->extent_size +
-				   pv->pe_start + mda_size2;
-
-		/* Recalculate number of extents that will fit */
-		if (!pv->pe_count) {
-			pe_count = (pv->size - pv->pe_start - mda_size2) /
-				   vg->extent_size;
-			if (pe_count > UINT32_MAX) {
-				log_error("PV %s too large for extent size %s.",
-					  pv_dev_name(pv),
-					  display_size(vg->cmd, (uint64_t) vg->extent_size));
-				return 0;
-			}
-			pv->pe_count = (uint32_t) pe_count;
-		}
+			if (found)
+				continue;
 
-		/* Unlike LVM1, we don't store this outside a VG */
-		/* FIXME Default from config file? vgextend cmdline flag? */
-		pv->status |= ALLOCATABLE_PV;
-	} else {
-		if (pe_start) {
-			pv->pe_start_locked = 1;
-			pv->pe_start = pe_start;
+			if (!(mda = mda_copy(fmt->cmd->mem, pv_mda)))
+				return_0;
+
+			dm_list_add(vg_mdas, &mda->list);
+			/* FIXME multiple dev_areas inside area */
 		}
 
-		if (!data_alignment)
-			data_alignment = find_config_tree_int(pv->fmt->cmd,
-						      "devices/data_alignment",
-						      0) * 2;
+		/* If there's the 2nd mda, we need to reduce
+		 * usable size for further pe_count calculation! */
+		if ((mda = info->mda_slots[1]) &&
+		    (mdac = mda->metadata_locn))
+			size_reduction = mdac->area.size >> SECTOR_SHIFT;
+	}
 
-		if (set_pe_align(pv, data_alignment) != data_alignment &&
-		    data_alignment) {
-			log_error("%s: invalid data alignment of "
-				  "%lu sectors (requested %lu sectors)",
-				  pv_dev_name(pv), pv->pe_align, data_alignment);
-			return 0;
-		}
+	/* FIXME Cope with genuine pe_count 0 */
 
-		if (set_pe_align_offset(pv, data_alignment_offset) != data_alignment_offset &&
-		    data_alignment_offset) {
-			log_error("%s: invalid data alignment offset of "
-				  "%lu sectors (requested %lu sectors)",
-				  pv_dev_name(pv), pv->pe_align_offset, data_alignment_offset);
-			return 0;
-		}
+	/* If missing, estimate pv->size from file-based metadata */
+	if (!pv->size && pv->pe_count)
+		pv->size = pv->pe_count * (uint64_t) vg->extent_size +
+			   pv->pe_start + size_reduction;
 
-		if (pv->pe_align < pv->pe_align_offset) {
-			log_error("%s: pe_align (%lu sectors) must not be less "
-				  "than pe_align_offset (%lu sectors)",
-				  pv_dev_name(pv), pv->pe_align, pv->pe_align_offset);
+	/* Recalculate number of extents that will fit */
+	if (!pv->pe_count) {
+		pe_count = (pv->size - pv->pe_start - size_reduction) /
+			   vg->extent_size;
+		if (pe_count > UINT32_MAX) {
+			log_error("PV %s too large for extent size %s.",
+				  pv_dev_name(pv),
+				  display_size(vg->cmd, (uint64_t) vg->extent_size));
 			return 0;
 		}
-
-		/*
-		 * This initialization has a side-effect of allowing
-		 * orphaned PVs to be created with the proper alignment.
-		 * Setting pv->pe_start here circumvents .pv_write's
-		 * "pvcreate on PV without prior pvremove" retreival of
-		 * the PV's previous pe_start.
-		 * - Without this you get actual != expected pe_start
-		 *   failures in the testsuite.
-		 */
-		if (!pv->pe_start_locked && pv->pe_start < pv->pe_align)
-			pv->pe_start = pv->pe_align;
-
-		if (extent_count)
-			pe_end = pe_start + extent_count * extent_size - 1;
-		if (!_mda_setup(fmt, pe_start, pe_end, pvmetadatacopies,
-				pvmetadatasize, metadataignore,  mdas, pv, vg))
-			return_0;
+		pv->pe_count = (uint32_t) pe_count;
 	}
 
+	/* Unlike LVM1, we don't store this outside a VG */
+	/* FIXME Default from config file? vgextend cmdline flag? */
+	pv->status |= ALLOCATABLE_PV;
+
 	return 1;
 }
 
diff --git a/lib/metadata/metadata-exported.h b/lib/metadata/metadata-exported.h
index fe7813f..dab953d 100644
--- a/lib/metadata/metadata-exported.h
+++ b/lib/metadata/metadata-exported.h
@@ -398,9 +398,10 @@ struct physical_volume *pv_create(const struct cmd_context *cmd,
 				  uint64_t pe_start,
 				  uint32_t existing_extent_count,
 				  uint32_t existing_extent_size,
-				  int pvmetadatacopies, uint64_t pvmetadatasize,
-				  unsigned metadataignore,
-				  struct dm_list *mdas);
+				  uint64_t label_sector,
+				  int pvmetadatacopies,
+				  uint64_t pvmetadatasize,
+				  unsigned metadataignore);
 int pv_resize(struct physical_volume *pv, struct volume_group *vg,
              uint32_t new_pe_count);
 int pv_analyze(struct cmd_context *cmd, const char *pv_name,
diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
index 9ff9607..30b79f8 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -194,7 +194,6 @@ int add_pv_to_vg(struct volume_group *vg, const char *pv_name,
 	struct format_instance *fid = vg->fid;
 	struct dm_pool *mem = vg->vgmem;
 	char uuid[64] __attribute__((aligned(8)));
-	struct dm_list *mdas;
 
 	log_verbose("Adding physical volume '%s' to volume group '%s'",
 		    pv_name, vg->name);
@@ -238,24 +237,7 @@ int add_pv_to_vg(struct volume_group *vg, const char *pv_name,
 	 */
 	pv->pe_alloc_count = 0;
 
-	/*
-	 * FIXME: this does not work entirely correctly in the case where a PV
-	 * has 2 mdas and only one is ignored; ideally all non-ignored mdas
-	 * should be placed on metadata_areas list and ignored on the
-	 * metadata_areas_ignored list; however this requires another
-	 * fairly complex refactoring to remove the 'mdas' parameter from both
-	 * pv_setup and pv_write.  For now, we only put ignored mdas on the
-	 * metadata_areas_ignored list if all mdas in the PV are ignored;
-	 * otherwise, we use the non-ignored list.
-	 */
-	if (!pv_mda_used_count(pv))
-		mdas = &fid->metadata_areas_ignored;
-	else
-		mdas = &fid->metadata_areas_in_use;
-
-	if (!fid->fmt->ops->pv_setup(fid->fmt, UINT64_C(0), 0,
-				     vg->extent_size, 0, 0, 0UL, UINT64_C(0),
-				     0, mdas, pv, vg)) {
+	if (!fid->fmt->ops->pv_setup(fid->fmt, pv, vg)) {
 		log_error("Format-specific setup of physical volume '%s' "
 			  "failed.", pv_name);
 		return 0;
@@ -1488,8 +1470,8 @@ struct physical_volume * pvcreate_single(struct cmd_context *cmd,
 	if (!(pv = pv_create(cmd, dev, pp->idp, pp->size,
 			     pp->data_alignment, pp->data_alignment_offset,
 			     pp->pe_start, pp->extent_count, pp->extent_size,
-			     pp->pvmetadatacopies, pp->pvmetadatasize,
-			     pp->metadataignore, &mdas))) {
+			     pp->labelsector, pp->pvmetadatacopies,
+			     pp->pvmetadatasize, pp->metadataignore))) {
 		log_error("Failed to setup physical volume \"%s\"", pv_name);
 		goto error;
 	}
@@ -1593,12 +1575,15 @@ struct physical_volume *pv_create(const struct cmd_context *cmd,
 				  uint64_t pe_start,
 				  uint32_t existing_extent_count,
 				  uint32_t existing_extent_size,
-				  int pvmetadatacopies, uint64_t pvmetadatasize,
-				  unsigned metadataignore, struct dm_list *mdas)
+				  uint64_t label_sector,
+				  int pvmetadatacopies,
+				  uint64_t pvmetadatasize,
+				  unsigned metadataignore)
 {
 	const struct format_type *fmt = cmd->fmt;
 	struct dm_pool *mem = fmt->cmd->mem;
 	struct physical_volume *pv = _alloc_pv(mem, dev);
+	unsigned mda_index;
 
 	if (!pv)
 		return NULL;
@@ -1640,16 +1625,24 @@ struct physical_volume *pv_create(const struct cmd_context *cmd,
 	pv->fmt = fmt;
 	pv->vg_name = fmt->orphan_vg_name;
 
-	if (!fmt->ops->pv_setup(fmt, pe_start, existing_extent_count,
-				existing_extent_size, data_alignment,
-				data_alignment_offset,
-				pvmetadatacopies, pvmetadatasize,
-				metadataignore, mdas, pv, NULL)) {
-		log_error("%s: Format-specific setup of physical volume "
-			  "failed.", pv_dev_name(pv));
+	if (!fmt->ops->pv_initialise(fmt, label_sector, pe_start,
+				     existing_extent_count, existing_extent_size,
+				     data_alignment, data_alignment_offset, pv)) {
+		log_error("Format-specific initialisation of physical "
+			  "volume %s failed.", pv_dev_name(pv));
 		goto bad;
 	}
 
+	for (mda_index = 0; mda_index < pvmetadatacopies; mda_index++) {
+		if (pv->fmt->ops->pv_add_metadata_area &&
+		    !pv->fmt->ops->pv_add_metadata_area(pv->fmt, pv, mda_index,
+					pvmetadatasize, metadataignore)) {
+			log_error("Failed to add metadata area for "
+				  "new physical volume %s", pv_dev_name(pv));
+			goto bad;
+		}
+	}
+
 	return pv;
 
       bad:
diff --git a/lib/metadata/metadata.h b/lib/metadata/metadata.h
index b69c9ee..6e01b17 100644
--- a/lib/metadata/metadata.h
+++ b/lib/metadata/metadata.h
@@ -255,12 +255,8 @@ struct format_handler {
 	 * vg.  eg. pe_count is format specific.
 	 */
 	int (*pv_setup) (const struct format_type * fmt,
-			 uint64_t pe_start, uint32_t extent_count,
-			 uint32_t extent_size, unsigned long data_alignment,
-			 unsigned long data_alignment_offset,
-			 int pvmetadatacopies, uint64_t pvmetadatasize,
-			 unsigned metadataignore, struct dm_list * mdas,
-			 struct physical_volume * pv, struct volume_group * vg);
+			 struct physical_volume * pv,
+			 struct volume_group * vg);
 
 	/*
 	 * Add metadata area to a PV. Changes will take effect on pv_write.
diff --git a/tools/vgconvert.c b/tools/vgconvert.c
index acae0fc..db28710 100644
--- a/tools/vgconvert.c
+++ b/tools/vgconvert.c
@@ -126,8 +126,10 @@ static int vgconvert_single(struct cmd_context *cmd, const char *vg_name,
 		if (!(pv = pv_create(cmd, pv_dev(existing_pv),
 				     &existing_pv->id, size, 0, 0,
 				     pe_start, pv_pe_count(existing_pv),
-				     pv_pe_size(existing_pv), pvmetadatacopies,
-				     pvmetadatasize, 0, &mdas))) {
+				     pv_pe_size(existing_pv),
+				     arg_int64_value(cmd, labelsector_ARG,
+						     DEFAULT_LABELSECTOR),
+				     pvmetadatacopies, pvmetadatasize, 0))) {
 			log_error("Failed to setup physical volume \"%s\"",
 				  pv_dev_name(existing_pv));
 			if (change_made)
-- 
1.7.3.2


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]