[lvm-devel] [PATCH 06/11] Refactor pv_setup to not include the initialisation only code.
Peter Rajnoha
prajnoha at redhat.com
Thu Nov 18 21:32:20 UTC 2010
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 at 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
More information about the lvm-devel
mailing list