[PATCH 00/11] Add interface to support adding and removing metadata areas on demand

[PATCH 00/11] Add interface to support adding and removing metadata areas on demand

[Peter Rajnoha]

This patchset provides a possibility to add new metadata areas on demand
as well as removing them. Now, we can add a new metadata area independently
of the PV initialisation so if there's a place we can even add metadata area
additionally. The same applies for metadata removal functionality. This is
only internal interface for now, but we should be able to provide external
hooks in lvm2 commands if desired.

The patchset also tries to cleanup existing pv_setup/pv_write code. It also
uses the new interface to allow resizing a PV with more copies of metadata
areas on disk which was not allowed before because of missing metadata
handling part in the interface.

(This patchset should be applicable to 4f2136c6545532afe149657f923fb982d2660415.
There's still one thing that is failing which I need to inspect - it's lvm1
format handling and failing t-pv-create-usage.sh: "PV segment pe_count mismatch: 33 != 32".
But anyway, here it is... I'll provide an update if I find anything.)

Peter Rajnoha (11):
  Add mda_slots field for PV info in the cache.
  Add pe_start_locked flag in struct physical_volume to retain
    pe_start.
  Add pv_add_metadata_area to format_handler interface.
  Add pv_remove_metadata_area to format_handler interface.
  Add pv_initialise to format_handler interface.
  Refactor pv_setup to not include the initialisation only code.
  Remove unused _mda_setup.
  Cleanup pv_write to use recent changes in metadata handling
    interface.
  Remove useless mdas parameter in code creating new PVs.
  Make existing lvmcache_update_pvid public.
  Use new metadata interface to provide better support for PV resize.

 lib/cache/lvmcache.c             |   18 +-
 lib/cache/lvmcache.h             |    3 +
 lib/format1/disk-rep.c           |    7 +
 lib/format1/disk-rep.h           |    1 +
 lib/format1/format1.c            |   64 ++-
 lib/format1/format1.h            |    1 +
 lib/format1/lvm1-label.c         |    5 +
 lib/format_pool/disk_rep.c       |    4 +
 lib/format_pool/disk_rep.h       |    1 +
 lib/format_pool/format_pool.c    |   22 +-
 lib/format_pool/format_pool.h    |    1 +
 lib/format_text/archiver.c       |    5 +-
 lib/format_text/format-text.c    |  912 ++++++++++++++++++++++----------------
 lib/format_text/format-text.h    |   11 +-
 lib/format_text/text_label.c     |   26 +-
 lib/metadata/metadata-exported.h |   12 +-
 lib/metadata/metadata.c          |   80 ++--
 lib/metadata/metadata.h          |   49 ++-
 lib/metadata/pv.h                |    1 +
 lib/metadata/pv_manip.c          |   70 +++-
 tools/pvchange.c                 |   10 +-
 tools/pvresize.c                 |   57 +---
 tools/vgconvert.c                |   12 +-
 tools/vgreduce.c                 |    2 +-
 24 files changed, 803 insertions(+), 571 deletions(-)

-- 
1.7.3.2

[PATCH 01/11] Add mda_slots field for PV info in the cache.

[Peter Rajnoha]

The code further needs to access the information about metadata areas
directly quite often without checking the relative position to other
metadata areas or data areas. IOW, we need to know directly whether
there is any metadata area at the start of the disk or at the end of the
disk or any other position we can think of in the future. This way, I
know exactly which slots are free, which are occupied and I know the
relative position to other metadata areas without checking any offsets
etc.

Using the original list would be quite painful here - either I'd need to
store the position somewhere or I'll just use this array which seems
better. Though, the list itself is still there - this is something I'd
like to discuss... (So I haven't tried to remove the mda_list completely
yet).

Signed-off-by: Peter Rajnoha <prajnoha@redhat.com>
---
 lib/cache/lvmcache.c |    1 +
 lib/cache/lvmcache.h |    2 ++
 2 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/lib/cache/lvmcache.c b/lib/cache/lvmcache.c
index 60edcec..d1b6dea 100644
--- a/lib/cache/lvmcache.c
+++ b/lib/cache/lvmcache.c
@@ -1357,6 +1357,7 @@ static void _lvmcache_destroy_entry(struct lvmcache_info *info)
 	_vginfo_detach_info(info);
 	strcpy(info->dev->pvid, "");
 	label_destroy(info->label);
+	dm_free(info->mda_slots);
 	dm_free(info);
 }
 
diff --git a/lib/cache/lvmcache.h b/lib/cache/lvmcache.h
index 28f8541..32b4579 100644
--- a/lib/cache/lvmcache.h
+++ b/lib/cache/lvmcache.h
@@ -33,6 +33,7 @@
 struct cmd_context;
 struct format_type;
 struct volume_group;
+struct metadata_area;
 
 /* One per VG */
 struct lvmcache_vginfo {
@@ -53,6 +54,7 @@ struct lvmcache_vginfo {
 struct lvmcache_info {
 	struct dm_list list;	/* Join VG members together */
 	struct dm_list mdas;	/* list head for metadata areas */
+	void **mda_slots;	/* Direct indexed access for metadata areas */
 	struct dm_list das;	/* list head for data areas */
 	struct lvmcache_vginfo *vginfo;	/* NULL == unknown */
 	struct label *label;
-- 
1.7.3.2

[PATCH 02/11] Add pe_start_locked flag in struct physical_volume to retain pe_start.

[Peter Rajnoha]

Normally, pe_start value is changing throughout the calculation of the
alignment and/or adding and removing metadata areas. But sometimes we
really need to retain the old pe_start value, not trying to change it.
When this flag is set, the code will try to fit new metadata areas
within this limit and it will adjust the calculation so that the
pe_start value is not changed at all if possible (so for example, we
don't overwrite any existing data areas - a restore from backups etc.).

Signed-off-by: Peter Rajnoha <prajnoha@redhat.com>
---
 lib/format_text/format-text.c |    6 ++++--
 lib/metadata/pv.h             |    1 +
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/lib/format_text/format-text.c b/lib/format_text/format-text.c
index 0102a15..af95cb5 100644
--- a/lib/format_text/format-text.c
+++ b/lib/format_text/format-text.c
@@ -1854,8 +1854,10 @@ static int _text_pv_setup(const struct format_type *fmt,
 		/* FIXME Default from config file? vgextend cmdline flag? */
 		pv->status |= ALLOCATABLE_PV;
 	} else {
-		if (pe_start)
+		if (pe_start) {
+			pv->pe_start_locked = 1;
 			pv->pe_start = pe_start;
+		}
 
 		if (!data_alignment)
 			data_alignment = find_config_tree_int(pv->fmt->cmd,
@@ -1894,7 +1896,7 @@ static int _text_pv_setup(const struct format_type *fmt,
 		 * - Without this you get actual != expected pe_start
 		 *   failures in the testsuite.
 		 */
-		if (!pe_start && pv->pe_start < pv->pe_align)
+		if (!pv->pe_start_locked && pv->pe_start < pv->pe_align)
 			pv->pe_start = pv->pe_align;
 
 		if (extent_count)
diff --git a/lib/metadata/pv.h b/lib/metadata/pv.h
index a02f6f8..7cdcaf9 100644
--- a/lib/metadata/pv.h
+++ b/lib/metadata/pv.h
@@ -44,6 +44,7 @@ struct physical_volume {
 	/* physical extents */
 	uint32_t pe_size;
 	uint64_t pe_start;
+	int pe_start_locked;		/* If set, try to retain the PE start value. */
 	uint32_t pe_count;
 	uint32_t pe_alloc_count;
 	unsigned long pe_align;
-- 
1.7.3.2

[PATCH 03/11] Add pv_add_metadata_area to format_handler interface.

[Peter Rajnoha]

This provides a way to add new metadata area for a PV on demand.
Adding any new metadata area will update the cache directly (just as it
is in the situation while scanning and reading existing PVs). Why cache?
Because when I add a new metadata area, I need to know about other
metadata areas (together with any newly add metadata areas!) So I had
two choices - either I copy the list of existing metadata areas to my
own list and proceed with this one (adding/removing other mdas as
needed) or I can use the cache directly (which we update anyway).
Now, we update it a little bit earlier than we did before.

So there's a question probably - what is our cache policy?

 - read info from disk, cache it, use it and if anything changes,
   throw the cache away immediately and read new info from the disk
   again,

 - read info from disk, cache it, use it, modify it together with any
   modifications we're just doing, use the new cache later on, don't
   read from the disk again (unless really necessary),

 - read info form disk, cache it, use it, modify it only at certain
   points where it is allowed.

The thing is that I really want to avoid making *another* copy of the
mda_list. We already have one in the format_instance, one in the cache,
of course... Another one just for mda add/remove interface? No. So if
this cache modification is no-go, I'd lean to changing the
format_instance itself so it could be used for this mda add/remove
interface as well (but that would require much more changes, I think).

But here's the solution with the cache... It's doing its job so it's
just about the question what's our policy when working with the cache.

Signed-off-by: Peter Rajnoha <prajnoha@redhat.com>
---
 lib/format_text/format-text.c |  249 +++++++++++++++++++++++++++++++++++++++++
 lib/format_text/format-text.h |    7 +
 lib/metadata/metadata.h       |   10 ++
 3 files changed, 266 insertions(+), 0 deletions(-)

diff --git a/lib/format_text/format-text.c b/lib/format_text/format-text.c
index af95cb5..0c1c4c7 100644
--- a/lib/format_text/format-text.c
+++ b/lib/format_text/format-text.c
@@ -1909,6 +1909,254 @@ static int _text_pv_setup(const struct format_type *fmt,
 	return 1;
 }
 
+int add_metadata_area_to_cache(struct lvmcache_info *info,
+			       unsigned mda_index,
+			       uint64_t mda_start,
+			       uint64_t mda_size,
+			       unsigned mda_ignored)
+{
+	struct metadata_area *mda;
+	struct mda_context *mdac;
+	struct mda_lists *mda_lists = (struct mda_lists *) info->fmt->private;
+
+	if (mda_index >= FMT_TEXT_MAX_MDAS_PER_PV) {
+		log_error(INTERNAL_ERROR "can't add metadata with index %u to"
+					 "cache for PV %s. Metadata layout "
+					 "not supported by %s format.",
+					  mda_index, dev_name(info->dev),
+					  info->fmt->name);
+	}
+
+	if (!(mda = dm_malloc(sizeof(struct metadata_area)))) {
+		log_error("struct metadata_area allocation failed");
+		return 0;
+	}
+
+	if (!(mdac = dm_malloc(sizeof(struct mda_context)))) {
+		log_error("struct mda_context allocation failed");
+		dm_free(mda);
+		return 0;
+	}
+
+	mda->ops = mda_lists->raw_ops;
+	mda->metadata_locn = mdac;
+	mda->status = 0;
+
+	mdac->area.dev = info->dev;
+	mdac->area.start = mda_start;
+	mdac->area.size = mda_size;
+	mdac->free_sectors = UINT64_C(0);
+	memset(&mdac->rlocn, 0, sizeof(mdac->rlocn));
+	mda_set_ignored(mda, mda_ignored);
+
+	dm_list_add(&info->mdas, &mda->list);
+	info->mda_slots[mda_index] = mda;
+
+	return 1;
+}
+
+static int _text_pv_add_metadata_area(const struct format_type *fmt,
+				      struct physical_volume *pv,
+				      unsigned mda_index,
+				      uint64_t mda_size,
+				      unsigned mda_ignored)
+{
+	uint64_t pe_start, pe_end;
+	uint64_t alignment, alignment_offset;
+	uint64_t disk_size;
+	uint64_t mda_start;
+	uint64_t adjustment, limit;
+	uint64_t wipe_size = 8 << SECTOR_SHIFT;
+	size_t page_size = lvm_getpagesize();
+	struct lvmcache_info *info;
+	struct metadata_area *mda;
+	struct mda_context *mdac;
+
+	if (mda_index >= FMT_TEXT_MAX_MDAS_PER_PV) {
+		log_error(INTERNAL_ERROR "invalid index of value %u used "
+			      "while trying to add metadata area on PV %s. "
+			      "Metadata layout not supported by %s format.",
+			       mda_index, pv_dev_name(pv), fmt->name);
+		return 0;
+	}
+
+	pe_start = pv->pe_start << SECTOR_SHIFT;
+	alignment = pv->pe_align << SECTOR_SHIFT;
+	alignment_offset = pv->pe_align_offset << SECTOR_SHIFT;
+	disk_size = pv->size << SECTOR_SHIFT;
+	mda_size = mda_size << SECTOR_SHIFT;
+
+	if (!(info = info_from_pvid(pv->dev->pvid, 0)))
+		return_0;
+
+	if (info->mda_slots[mda_index]) {
+		log_error(INTERNAL_ERROR "metadata area with index %u already "
+			"exists on PV %s.", mda_index, pv_dev_name(pv));
+		return 0;
+	}
+
+	/* First metadata area at the start of the device. */
+	if (mda_index == 0) {
+		/*
+		 * Try to fit MDA0 end within given pe_start limit if its value
+		 * is locked. If it's not locked, count with any existing MDA1.
+		 * If there's no MDA1, just use disk size as the limit.
+		 */
+		if (pv->pe_start_locked)
+			limit = pe_start;
+		else if ((mda = (struct metadata_area *) info->mda_slots[1]) &&
+			 (mdac = mda->metadata_locn))
+			limit = mdac->area.start;
+		else
+			limit = disk_size;
+
+		if (limit > disk_size)
+			goto bad;
+
+		mda_start = LABEL_SCAN_SIZE;
+
+		/* Align MDA0 start with page size if possible. */
+		if (limit - mda_start >= MDA_SIZE_MIN) {
+			if ((adjustment = mda_start % page_size))
+				mda_start += (page_size - adjustment);
+		}
+
+		/* Align MDA0 end position with given alignment if possible. */
+		if (alignment) {
+			if ((adjustment = (mda_start + mda_size) % alignment)) {
+				mda_size += (alignment - adjustment);
+				if (mda_start + mda_size > limit)
+					mda_size -= (alignment - adjustment);
+			}
+		}
+
+		/* Align MDA0 end position with given alignment offset if possible. */
+		if (alignment_offset &&
+		    (((mda_start + mda_size) % alignment) == 0)) {
+			mda_size += alignment_offset;
+			if (mda_start + mda_size > limit)
+				mda_size -= alignment_offset;
+		}
+
+		if (mda_start + mda_size > limit) {
+			log_warn("WARNING: metadata area size outreaches "
+				 "a limit on PV %s specified by its %s. "
+				 "Trying to adjust metadata area size.",
+				  pv_dev_name(pv),
+				  pv->pe_start_locked ? "PE start"
+						      : "disk size");
+
+			/*
+			 * Try to decrease the MDA0 size with twice the
+			 * alignment and then align with given alignment.
+			 * If pe_start is locked, skip this type of
+			 * alignment since it would be useless.
+			 * Check first whether we can apply that!
+			 */
+			if (!pv->pe_start_locked &&
+			    ((limit - mda_start) > alignment * 2)) {
+				mda_size = limit - mda_start - alignment * 2;
+
+				if ((adjustment = (mda_start + mda_size) % alignment))
+					mda_size += (alignment - adjustment);
+
+				/* Still too much? Then there's nothing else to do. */
+				if (mda_start + mda_size > limit)
+					goto bad;
+			}
+			/* Otherwise, give up and take any usable space. */
+			/* FIXME: We should probably check for some minimum MDA size here. */
+			else
+				mda_size = limit - mda_start;
+		}
+
+		/*
+		 * If PV's pe_start is not locked, update pe_start value with the
+		 * start of the area that follows the MDA0 we've just calculated.
+		 */
+		if (!pv->pe_start_locked) {
+			pe_start = mda_start + mda_size;
+			pv->pe_start = pe_start >> SECTOR_SHIFT;
+		}
+	}
+	/* Second metadata area at the end of the device. */
+	else {
+		/*
+		 * Try to fit MDA1 start within given pe_end or pe_start limit
+		 * if it's locked. If pe_start and pe_end are not defined yet,
+		 * count with any existing MDA0 and pe_start. If MDA0 does not
+		 * exist, just use LABEL_SCAN_SIZE.
+		 */
+		pe_end = pv->pe_count ? (pv->pe_start +
+					 pv->pe_count * pv->pe_size - 1) << SECTOR_SHIFT
+				      : 0;
+		if (pv->pe_start_locked)
+			limit = pe_end ? pe_end : pe_start;
+		else if (pe_start)
+			limit = pe_start;
+		/*
+		 * Normally MDA0's start + size should be pe_start.
+		 * The statemet here is probably useless since the
+		 * situation is covered by previous statement.
+		 */
+		else if ((mda = (struct metadata_area *) info->mda_slots[0]) &&
+			 (mdac = mda->metadata_locn))
+			limit = mdac->area.start + mdac->area.size;
+		else
+			limit = LABEL_SCAN_SIZE;
+
+		if (limit > disk_size || mda_size > disk_size)
+			goto bad;
+
+		mda_start = disk_size - mda_size;
+
+		if (alignment) {
+			adjustment = mda_start % alignment;
+			if (adjustment)
+				mda_size += adjustment;
+		}
+
+		/* FIXME: mda_size should be sanity-checked somewhere before.
+		 *	  I'm not sure whether it really is --> !(mda_size > disk_size). */
+		if (disk_size - mda_size < limit)
+			mda_size = disk_size - limit;
+
+		/*
+		 * If PV's pe_end not set yet, set it to the end of the
+		 * area that precedes the MDA1 we've just calculated.
+		 * FIXME: do we need to set this? Isn't it always set before?
+		 */
+		/*if (!pe_end) {
+			pe_end = mda_start;
+			pv->pe_end = pe_end >> SECTOR_SHIFT;
+		}*/
+	}
+
+	if (mda_size) {
+		/* Wipe metadata area with zeroes. */
+		if (!dev_set((struct device *) pv->dev, mda_start,
+			(size_t) (mda_size > wipe_size ? : mda_size), 0)) {
+				log_error("Failed to wipe new metadata area "
+					  "at the %s of the %s",
+					   mda_index ? "end" : "start",
+					   pv_dev_name(pv));
+				return 0;
+		}
+
+		/* Finally, add new metadata area to cache. */
+		if (!add_metadata_area_to_cache(info, mda_index,
+				mda_start, mda_size, mda_ignored))
+			return_0;
+	}
+
+	return 1;
+
+bad:
+	log_error("Not enough space available for metadata area "
+		  "with index %u on PV %s.", mda_index, pv_dev_name(pv));
+	return 0;
+}
+
 /* NULL vgname means use only the supplied context e.g. an archive file */
 static struct format_instance *_text_create_text_instance(const struct format_type
 						     *fmt, const char *vgname,
@@ -2049,6 +2297,7 @@ static struct format_handler _text_handler = {
 	.scan = _text_scan,
 	.pv_read = _text_pv_read,
 	.pv_setup = _text_pv_setup,
+	.pv_add_metadata_area = _text_pv_add_metadata_area,
 	.pv_write = _text_pv_write,
 	.vg_setup = _text_vg_setup,
 	.lv_setup = _text_lv_setup,
diff --git a/lib/format_text/format-text.h b/lib/format_text/format-text.h
index 79365ea..75d69bf 100644
--- a/lib/format_text/format-text.h
+++ b/lib/format_text/format-text.h
@@ -22,6 +22,7 @@
 #define FMT_TEXT_NAME "lvm2"
 #define FMT_TEXT_ALIAS "text"
 #define FMT_TEXT_ORPHAN_VG_NAME ORPHAN_VG_NAME(FMT_TEXT_NAME)
+#define FMT_TEXT_MAX_MDAS_PER_PV 2
 
 /*
  * Archives a vg config.  'retain_days' is the minimum number of
@@ -55,6 +56,12 @@ int add_da(struct dm_pool *mem, struct dm_list *das,
 	   uint64_t start, uint64_t size);
 void del_das(struct dm_list *das);
 
+int add_metadata_area_to_cache(struct lvmcache_info *info,
+			       unsigned mda_index,
+			       uint64_t mda_start,
+			       uint64_t mda_size,
+			       unsigned mda_ignored);
+
 int add_mda(const struct format_type *fmt, struct dm_pool *mem, struct dm_list *mdas,
 	    struct device *dev, uint64_t start, uint64_t size, unsigned ignored);
 void del_mdas(struct dm_list *mdas);
diff --git a/lib/metadata/metadata.h b/lib/metadata/metadata.h
index 5aa9c19..ca2ec0b 100644
--- a/lib/metadata/metadata.h
+++ b/lib/metadata/metadata.h
@@ -23,6 +23,7 @@
 
 #include "ctype.h"
 #include "dev-cache.h"
+#include "lvmcache.h"
 #include "lvm-string.h"
 #include "metadata-exported.h"
 
@@ -249,6 +250,15 @@ struct format_handler {
 			 struct physical_volume * pv, struct volume_group * vg);
 
 	/*
+	 * Add metadata area to a PV. Changes will take effect on pv_write.
+	 */
+	int (*pv_add_metadata_area) (const struct format_type * fmt,
+				     struct physical_volume * pv,
+				     unsigned metadata_index,
+				     uint64_t metadata_size,
+				     unsigned metadata_ignored);
+
+	/*
 	 * Write a PV structure to disk. Fails if the PV is in a VG ie
 	 * pv->vg_name must be a valid orphan VG name
 	 */
-- 
1.7.3.2

[PATCH 04/11] Add pv_remove_metadata_area to format_handler interface.

[Peter Rajnoha]

The same as for pv_add_metadata_area, but removing...

Signed-off-by: Peter Rajnoha <prajnoha@redhat.com>
---
 lib/format_text/format-text.c |   50 +++++++++++++++++++++++++++++++++++++++++
 lib/format_text/format-text.h |    3 ++
 lib/metadata/metadata.h       |    7 +++++
 3 files changed, 60 insertions(+), 0 deletions(-)

diff --git a/lib/format_text/format-text.c b/lib/format_text/format-text.c
index 0c1c4c7..db39c80 100644
--- a/lib/format_text/format-text.c
+++ b/lib/format_text/format-text.c
@@ -2157,6 +2157,55 @@ bad:
 	return 0;
 }
 
+int remove_metadata_area_from_cache(struct lvmcache_info *info,
+					    unsigned mda_index)
+{
+	struct metadata_area *mda;
+
+	if (mda_index >= FMT_TEXT_MAX_MDAS_PER_PV) {
+		log_error(INTERNAL_ERROR "can't remove metadata with index %u "
+					 "from cache for PV %s. Metadata layout "
+					 "not supported by %s format.",
+					  mda_index, dev_name(info->dev),
+					  info->fmt->name);
+		return 0;
+	}
+
+	mda = info->mda_slots[mda_index];
+
+	dm_list_del(&mda->list);
+	info->mda_slots[mda_index] = NULL;
+	dm_free(mda->metadata_locn);
+	dm_free(mda);
+
+	return 1;
+}
+
+static int _text_pv_remove_metadata_area(const struct format_type *fmt,
+					 struct physical_volume *pv,
+					 unsigned mda_index)
+{
+	struct lvmcache_info *info;
+
+	if (mda_index >= FMT_TEXT_MAX_MDAS_PER_PV) {
+		log_error(INTERNAL_ERROR "invalid index of value %u used "
+			   "while trying to remove metadata area on PV %s. "
+			   "Metadata layout not supported by %s format.",
+			    mda_index, pv_dev_name(pv), fmt->name);
+		return 0;
+	}
+
+	if (!(info = info_from_pvid(pv->dev->pvid, 0)))
+		return_0;
+
+	/* FIXME: Also wipe the mda area with zeroes? */
+
+	if (!remove_metadata_area_from_cache(info, mda_index))
+		return_0;
+
+	return 1;
+}
+
 /* NULL vgname means use only the supplied context e.g. an archive file */
 static struct format_instance *_text_create_text_instance(const struct format_type
 						     *fmt, const char *vgname,
@@ -2298,6 +2347,7 @@ static struct format_handler _text_handler = {
 	.pv_read = _text_pv_read,
 	.pv_setup = _text_pv_setup,
 	.pv_add_metadata_area = _text_pv_add_metadata_area,
+	.pv_remove_metadata_area = _text_pv_remove_metadata_area,
 	.pv_write = _text_pv_write,
 	.vg_setup = _text_vg_setup,
 	.lv_setup = _text_lv_setup,
diff --git a/lib/format_text/format-text.h b/lib/format_text/format-text.h
index 75d69bf..e172e3b 100644
--- a/lib/format_text/format-text.h
+++ b/lib/format_text/format-text.h
@@ -62,6 +62,9 @@ int add_metadata_area_to_cache(struct lvmcache_info *info,
 			       uint64_t mda_size,
 			       unsigned mda_ignored);
 
+int remove_metadata_area_from_cache(struct lvmcache_info *info,
+				    unsigned mda_index);
+
 int add_mda(const struct format_type *fmt, struct dm_pool *mem, struct dm_list *mdas,
 	    struct device *dev, uint64_t start, uint64_t size, unsigned ignored);
 void del_mdas(struct dm_list *mdas);
diff --git a/lib/metadata/metadata.h b/lib/metadata/metadata.h
index ca2ec0b..a78f3a2 100644
--- a/lib/metadata/metadata.h
+++ b/lib/metadata/metadata.h
@@ -259,6 +259,13 @@ struct format_handler {
 				     unsigned metadata_ignored);
 
 	/*
+	 * Remove metadata area from a PV. Changes will take effect on pv_write.
+	 */
+	int (*pv_remove_metadata_area) (const struct format_type *fmt,
+					struct physical_volume *pv,
+					unsigned metadata_index);
+
+	/*
 	 * Write a PV structure to disk. Fails if the PV is in a VG ie
 	 * pv->vg_name must be a valid orphan VG name
 	 */
-- 
1.7.3.2

[PATCH 05/11] Add pv_initialise to format_handler interface.

[Peter Rajnoha]

This is a split-off from the original pv_setup code, a simple refactor.
It makes the code easier to read and cleans up the interface a bit. So
now we will call pv_initialise when we only initialise a PV, creating a
brand new PV.

This patch also adjusts the code reading a PV to fill the mda_slots with
existing mdas so it's prepared for use in new mda add/removal interface.
We allocate the mda_slots based on the value of the maximum allowed
metadata areas for each format which is defined by FMT_<format>_MAX_MDAS_PER_PV.

Signed-off-by: Peter Rajnoha <prajnoha@redhat.com>
---
 lib/format1/disk-rep.c        |    7 +++
 lib/format1/disk-rep.h        |    1 +
 lib/format1/format1.c         |   57 +++++++++++++++++++++-----
 lib/format1/format1.h         |    1 +
 lib/format1/lvm1-label.c      |    5 ++
 lib/format_pool/disk_rep.c    |    4 ++
 lib/format_pool/disk_rep.h    |    1 +
 lib/format_pool/format_pool.c |   13 ++++++
 lib/format_pool/format_pool.h |    1 +
 lib/format_text/format-text.c |   91 +++++++++++++++++++++++++++++++++++++++++
 lib/format_text/format-text.h |    1 -
 lib/format_text/text_label.c  |   26 +++++-------
 lib/metadata/metadata.c       |   14 ++++++
 lib/metadata/metadata.h       |   13 ++++++
 14 files changed, 209 insertions(+), 26 deletions(-)

diff --git a/lib/format1/disk-rep.c b/lib/format1/disk-rep.c
index bc58744..38cb34d 100644
--- a/lib/format1/disk-rep.c
+++ b/lib/format1/disk-rep.c
@@ -337,6 +337,13 @@ static void __update_lvmcache(const struct format_type *fmt,
 
 	info->device_size = xlate32(dl->pvd.pv_size) << SECTOR_SHIFT;
 	dm_list_init(&info->mdas);
+	if (info->mda_slots)
+		dm_free(info->mda_slots);
+	if (!(info->mda_slots = dm_zalloc(FMT_LVM1_MAX_MDAS_PER_PV * sizeof(struct metadata_area *)))) {
+		/* FIXME: Add revert of cache info! */
+		stack;
+		return;
+	}
 	info->status &= ~CACHE_INVALID;
 }
 
diff --git a/lib/format1/disk-rep.h b/lib/format1/disk-rep.h
index 0d54438..65fad1b 100644
--- a/lib/format1/disk-rep.h
+++ b/lib/format1/disk-rep.h
@@ -18,6 +18,7 @@
 
 #include "lvm-types.h"
 #include "metadata.h"
+#include "format1.h"
 #include "toolcontext.h"
 
 #define MAX_PV 256
diff --git a/lib/format1/format1.c b/lib/format1/format1.c
index 0dcec05..4904a59 100644
--- a/lib/format1/format1.c
+++ b/lib/format1/format1.c
@@ -362,17 +362,17 @@ static int _format1_pv_read(const struct format_type *fmt, const char *pv_name,
 	return r;
 }
 
-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)))
+static int _format1_pv_initialise(const struct format_type * fmt,
+				  int64_t label_sector,
+				  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)),
+				  struct physical_volume * pv)
 {
+	struct lvmcache_info *info;
+
 	if (pv->size > MAX_PV_SIZE)
 		pv->size--;
 	if (pv->size > MAX_PV_SIZE) {
@@ -398,9 +398,45 @@ static int _format1_pv_setup(const struct format_type *fmt,
 		return 0;
 	}
 
+	/* Add newly initialised PV to cache. */
+        if (!(info = lvmcache_add(fmt->labeller, (char *) &pv->id, pv->dev,
+                                        fmt->orphan_vg_name, NULL, 0)))
+                return_0;
+
+        if (label_sector != -1)
+                info->label->sector = label_sector;
+
+        info->device_size = pv->size << SECTOR_SHIFT;
+        info->fmt = fmt;
+
+        if (info->mdas.n)
+                del_mdas(&info->mdas);
+        else
+                dm_list_init(&info->mdas);
+
+	dm_free(info->mda_slots);
+	if (!(info->mda_slots = dm_zalloc(FMT_LVM1_MAX_MDAS_PER_PV * sizeof(struct metadata_area *)))) {
+		/* FIXME: Add revert of cache info! */
+		return 0;
+	}
+
 	return 1;
 }
 
+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)))
+{
+	return _format1_pv_initialise(fmt, -1, 0, 0, vg->extent_size, 0, 0, pv);
+}
+
 static int _format1_lv_setup(struct format_instance *fid, struct logical_volume *lv)
 {
 	uint64_t max_size = UINT_MAX;
@@ -562,6 +598,7 @@ static void _format1_destroy(const 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,
 	.lv_setup = _format1_lv_setup,
diff --git a/lib/format1/format1.h b/lib/format1/format1.h
index c76ba62..ab7b571 100644
--- a/lib/format1/format1.h
+++ b/lib/format1/format1.h
@@ -21,6 +21,7 @@
 
 #define FMT_LVM1_NAME "lvm1"
 #define FMT_LVM1_ORPHAN_VG_NAME ORPHAN_VG_NAME(FMT_LVM1_NAME)
+#define FMT_LVM1_MAX_MDAS_PER_PV 1
 
 #ifdef LVM1_INTERNAL
 struct format_type *init_lvm1_format(struct cmd_context *cmd);
diff --git a/lib/format1/lvm1-label.c b/lib/format1/lvm1-label.c
index 07596a5..dbab874 100644
--- a/lib/format1/lvm1-label.c
+++ b/lib/format1/lvm1-label.c
@@ -82,6 +82,11 @@ static int _lvm1_read(struct labeller *l, struct device *dev, void *buf,
 	info->device_size = xlate32(pvd->pv_size) << SECTOR_SHIFT;
 	dm_list_init(&info->mdas);
 
+	if (!(info->mda_slots = dm_zalloc(FMT_LVM1_MAX_MDAS_PER_PV * sizeof(struct metadata_area *)))) {
+		/* FIXME: Add revert of cache info! */
+		return 0;
+	}
+
 	info->status &= ~CACHE_INVALID;
 
 	return 1;
diff --git a/lib/format_pool/disk_rep.c b/lib/format_pool/disk_rep.c
index 2c16f50..9bd8604 100644
--- a/lib/format_pool/disk_rep.c
+++ b/lib/format_pool/disk_rep.c
@@ -105,6 +105,10 @@ int read_pool_label(struct pool_list *pl, struct labeller *l,
 
 	info->device_size = xlate32_be(pd->pl_blocks) << SECTOR_SHIFT;
 	dm_list_init(&info->mdas);
+	if (!(info->mda_slots = dm_zalloc(FMT_POOL_MAX_MDAS_PER_PV * sizeof(struct metadata_area *)))) {
+		/* FIXME: Add revert of cache info! */
+		return 0;
+	}
 
 	info->status &= ~CACHE_INVALID;
 
diff --git a/lib/format_pool/disk_rep.h b/lib/format_pool/disk_rep.h
index fff7343..af620c9 100644
--- a/lib/format_pool/disk_rep.h
+++ b/lib/format_pool/disk_rep.h
@@ -18,6 +18,7 @@
 
 #include "label.h"
 #include "metadata.h"
+#include "format_pool.h"
 
 #define MINOR_OFFSET 65536
 
diff --git a/lib/format_pool/format_pool.c b/lib/format_pool/format_pool.c
index 04b5611..dbb73dd 100644
--- a/lib/format_pool/format_pool.c
+++ b/lib/format_pool/format_pool.c
@@ -188,6 +188,18 @@ out:
 	return NULL;
 }
 
+static int _pool_pv_initialise(const struct format_type *fmt __attribute__((unused)),
+			       int64_t label_sector __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)),
+			       struct physical_volume *pv __attribute__((unused)))
+{
+	return 1;
+}
+
 static int _pool_pv_setup(const struct format_type *fmt __attribute__((unused)),
 			  uint64_t pe_start __attribute__((unused)),
 			  uint32_t extent_count __attribute__((unused)),
@@ -294,6 +306,7 @@ static void _pool_destroy(const 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,
 	.destroy_instance = _pool_destroy_instance,
diff --git a/lib/format_pool/format_pool.h b/lib/format_pool/format_pool.h
index 0d4b973..afe90cc 100644
--- a/lib/format_pool/format_pool.h
+++ b/lib/format_pool/format_pool.h
@@ -20,6 +20,7 @@
 
 #define FMT_POOL_NAME "pool"
 #define FMT_POOL_ORPHAN_VG_NAME ORPHAN_VG_NAME(FMT_POOL_NAME)
+#define FMT_POOL_MAX_MDAS_PER_PV 1
 
 #ifdef POOL_INTERNAL
 struct format_type *init_pool_format(struct cmd_context *cmd);
diff --git a/lib/format_text/format-text.c b/lib/format_text/format-text.c
index db39c80..e47cb81 100644
--- a/lib/format_text/format-text.c
+++ b/lib/format_text/format-text.c
@@ -1691,6 +1691,96 @@ static int _text_pv_read(const struct format_type *fmt, const char *pv_name,
 	return 1;
 }
 
+static int _text_pv_initialise(const struct format_type *fmt,
+			       const int64_t label_sector,
+			       uint64_t pe_start,
+			       uint32_t extent_count,
+			       uint32_t extent_size,
+			       unsigned long data_alignment,
+			       unsigned long data_alignment_offset,
+			       struct physical_volume *pv)
+{
+	struct lvmcache_info *info;
+
+	pv->fmt = fmt;
+	pv->vg_name = fmt->orphan_vg_name;
+
+	/*
+	 * Try to keep the value of PE start set to a firm value if requested.
+	 * This is usefull when restoring existing PE start value (backups etc.).
+	 */
+	if (pe_start) {
+		pv->pe_start_locked = 1;
+		pv->pe_start = pe_start;
+	}
+
+	if (!data_alignment)
+		data_alignment = find_config_tree_int(pv->fmt->cmd,
+					      "devices/data_alignment",
+					      0) * 2;
+
+	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;
+	}
+
+	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 (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);
+		return 0;
+	}
+
+	if (!pv->pe_start_locked && pv->pe_start < pv->pe_align)
+		pv->pe_start = pv->pe_align;
+
+	if (extent_size)
+		pv->pe_size = extent_size;
+
+	if (extent_count)
+		pv->pe_count = extent_count;
+
+	if ((pv->pe_start + pv->pe_count * pv->pe_size - 1) > (pv->size << SECTOR_SHIFT)) {
+		log_error("Physical extents end beyond end of device %s.",
+			   pv_dev_name(pv));
+		return 0;
+	}
+
+	/* Add newly initialised PV to cache. */
+	if (!(info = lvmcache_add(fmt->labeller, (char *) &pv->id, pv->dev,
+					fmt->orphan_vg_name, NULL, 0)))
+		return_0;
+
+	if (label_sector != -1)
+		info->label->sector = label_sector;
+
+	info->device_size = pv->size << SECTOR_SHIFT;
+	info->fmt = fmt;
+
+	if (info->mdas.n)
+		del_mdas(&info->mdas);
+	else
+		dm_list_init(&info->mdas);
+
+	if (!(info->mda_slots = dm_zalloc(FMT_TEXT_MAX_MDAS_PER_PV * sizeof(struct metadata_area *)))) {
+		/* FIXME: Add revert of cache info! */
+		return 0;
+	}
+
+	return 1;
+}
+
 static void _text_destroy_instance(struct format_instance *fid __attribute__((unused)))
 {
 }
@@ -2345,6 +2435,7 @@ void *create_text_context(struct cmd_context *cmd, const char *path,
 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,
 	.pv_remove_metadata_area = _text_pv_remove_metadata_area,
diff --git a/lib/format_text/format-text.h b/lib/format_text/format-text.h
index e172e3b..7459c0e 100644
--- a/lib/format_text/format-text.h
+++ b/lib/format_text/format-text.h
@@ -67,6 +67,5 @@ int remove_metadata_area_from_cache(struct lvmcache_info *info,
 
 int add_mda(const struct format_type *fmt, struct dm_pool *mem, struct dm_list *mdas,
 	    struct device *dev, uint64_t start, uint64_t size, unsigned ignored);
-void del_mdas(struct dm_list *mdas);
 
 #endif
diff --git a/lib/format_text/text_label.c b/lib/format_text/text_label.c
index e459cde..192b702 100644
--- a/lib/format_text/text_label.c
+++ b/lib/format_text/text_label.c
@@ -229,19 +229,6 @@ int add_mda(const struct format_type *fmt, struct dm_pool *mem, struct dm_list *
 	return 1;
 }
 
-void del_mdas(struct dm_list *mdas)
-{
-	struct dm_list *mdah, *tmp;
-	struct metadata_area *mda;
-
-	dm_list_iterate_safe(mdah, tmp, mdas) {
-		mda = dm_list_item(mdah, struct metadata_area);
-		dm_free(mda->metadata_locn);
-		dm_list_del(&mda->list);
-		dm_free(mda);
-	}
-}
-
 static int _text_initialise_label(struct labeller *l __attribute__((unused)),
 				  struct label *label)
 {
@@ -257,6 +244,7 @@ static int _text_read(struct labeller *l, struct device *dev, void *buf,
 	struct pv_header *pvhdr;
 	struct lvmcache_info *info;
 	struct disk_locn *dlocn_xl;
+	unsigned dlocn_index = 0;
 	uint64_t offset;
 	struct metadata_area *mda;
 	struct id vgid;
@@ -284,6 +272,11 @@ static int _text_read(struct labeller *l, struct device *dev, void *buf,
 		del_mdas(&info->mdas);
 	dm_list_init(&info->mdas);
 
+	if (!(info->mda_slots = dm_zalloc(FMT_TEXT_MAX_MDAS_PER_PV * sizeof(struct metadata_area *)))) {
+		/* FIXME: Add revert of cache info! */
+		return 0;
+	}
+
 	/* Data areas holding the PEs */
 	dlocn_xl = pvhdr->disk_areas_xl;
 	while ((offset = xlate64(dlocn_xl->offset))) {
@@ -295,9 +288,12 @@ static int _text_read(struct labeller *l, struct device *dev, void *buf,
 	/* Metadata area headers */
 	dlocn_xl++;
 	while ((offset = xlate64(dlocn_xl->offset))) {
-		add_mda(info->fmt, NULL, &info->mdas, dev, offset,
-			xlate64(dlocn_xl->size), 0);
+		/* FIXME: Check order! We could possibly have mda at the end only! */
+		/* 	  E.g. check relative position to data area etc. */
+		add_metadata_area_to_cache(info, dlocn_index, offset,
+					   xlate64(dlocn_xl->size), 0);
 		dlocn_xl++;
+		dlocn_index++;
 	}
 
 	dm_list_iterate_items(mda, &info->mdas) {
diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
index 61b4600..9ff9607 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -3855,6 +3855,20 @@ struct metadata_area *mda_copy(struct dm_pool *mem,
 
 	return mda_new;
 }
+
+void del_mdas(struct dm_list *mdas)
+{
+	struct dm_list *mdah, *tmp;
+	struct metadata_area *mda;
+
+	dm_list_iterate_safe(mdah, tmp, mdas) {
+		mda = dm_list_item(mdah, struct metadata_area);
+		dm_free(mda->metadata_locn);
+		dm_list_del(&mda->list);
+		dm_free(mda);
+	}
+}
+
 /*
  * This function provides a way to answer the question on a format specific
  * basis - does the format specfic context of these two metadata areas
diff --git a/lib/metadata/metadata.h b/lib/metadata/metadata.h
index a78f3a2..b69c9ee 100644
--- a/lib/metadata/metadata.h
+++ b/lib/metadata/metadata.h
@@ -187,6 +187,7 @@ struct metadata_area {
 };
 struct metadata_area *mda_copy(struct dm_pool *mem,
 			       struct metadata_area *mda);
+void del_mdas(struct dm_list *mdas);
 
 unsigned mda_is_ignored(struct metadata_area *mda);
 void mda_set_ignored(struct metadata_area *mda, unsigned ignored);
@@ -238,6 +239,18 @@ struct format_handler {
 			int scan_label_only);
 
 	/*
+	 * Initialise a new PV.
+	 */
+	int (*pv_initialise) (const struct format_type * fmt,
+			      int64_t label_sector,
+			      uint64_t pe_start,
+			      uint32_t extent_count,
+			      uint32_t extent_size,
+			      unsigned long data_alignment,
+			      unsigned long data_alignment_offset,
+			      struct physical_volume * pv);
+
+	/*
 	 * Tweak an already filled out a pv ready for importing into a
 	 * vg.  eg. pe_count is format specific.
 	 */
-- 
1.7.3.2

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

[Peter Rajnoha]

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

[PATCH 07/11] Remove unused _mda_setup.

[Peter Rajnoha]

The functionality of former _mda_setup is now in new
pv_add_metadata_area fn (though it's changed considerably).

Signed-off-by: Peter Rajnoha <prajnoha@redhat.com>
---
 lib/format_text/format-text.c |  153 -----------------------------------------
 1 files changed, 0 insertions(+), 153 deletions(-)

diff --git a/lib/format_text/format-text.c b/lib/format_text/format-text.c
index f32e347..c7f5d5a 100644
--- a/lib/format_text/format-text.c
+++ b/lib/format_text/format-text.c
@@ -1246,159 +1246,6 @@ static int _text_scan(const struct format_type *fmt)
 	return (_scan_file(fmt) & _scan_raw(fmt));
 }
 
-/* For orphan, creates new mdas according to policy.
-   Always have an mda between end-of-label and pe_align() boundary */
-static int _mda_setup(const struct format_type *fmt,
-		      uint64_t pe_start, uint64_t pe_end,
-		      int pvmetadatacopies, uint64_t pvmetadatasize,
-		      unsigned metadataignore, struct dm_list *mdas,
-		      struct physical_volume *pv,
-		      struct volume_group *vg __attribute__((unused)))
-{
-	uint64_t mda_adjustment, disk_size, alignment, alignment_offset;
-	uint64_t start1, mda_size1;	/* First area - start of disk */
-	uint64_t start2, mda_size2;	/* Second area - end of disk */
-	uint64_t wipe_size = 8 << SECTOR_SHIFT;
-	size_t pagesize = lvm_getpagesize();
-
-	if (!pvmetadatacopies)
-		return 1;
-
-	alignment = pv->pe_align << SECTOR_SHIFT;
-	alignment_offset = pv->pe_align_offset << SECTOR_SHIFT;
-	disk_size = pv->size << SECTOR_SHIFT;
-	pe_start <<= SECTOR_SHIFT;
-	pe_end <<= SECTOR_SHIFT;
-
-	if (pe_end > disk_size) {
-		log_error("Physical extents end beyond end of device %s!",
-			  pv_dev_name(pv));
-		return 0;
-	}
-
-	/* Requested metadatasize */
-	mda_size1 = pvmetadatasize << SECTOR_SHIFT;
-
-	/* Place mda straight after label area@start of disk */
-	start1 = LABEL_SCAN_SIZE;
-
-	/* Unless the space available is tiny, round to PAGE_SIZE boundary */
-	if ((!pe_start && !pe_end) ||
-	    ((pe_start > start1) && (pe_start - start1 >= MDA_SIZE_MIN))) {
-		mda_adjustment = start1 % pagesize;
-		if (mda_adjustment)
-			start1 += (pagesize - mda_adjustment);
-	}
-
-	/* Round up to pe_align boundary */
-	mda_adjustment = (mda_size1 + start1) % alignment;
-	if (mda_adjustment) {
-		mda_size1 += (alignment - mda_adjustment);
-		/* Revert if it's now too large */
-		if (start1 + mda_size1 > disk_size)
-			mda_size1 -= (alignment - mda_adjustment);
-	}
-
-	/* Add pe_align_offset if on pe_align boundary */
-	if (alignment_offset &&
-	    (((start1 + mda_size1) % alignment) == 0)) {
-		mda_size1 += alignment_offset;
-		/* Revert if it's now too large */
-		if (start1 + mda_size1 > disk_size)
-			mda_size1 -= alignment_offset;
-	}
-
-	/* Ensure it's not going to be bigger than the disk! */
-	if (start1 + mda_size1 > disk_size) {
-		log_warn("WARNING: metadata area fills disk leaving no "
-			 "space for data on %s.", pv_dev_name(pv));
-		/* Leave some free space for rounding */
-		/* Avoid empty data area as could cause tools problems */
-		mda_size1 = disk_size - start1 - alignment * 2;
-		if (start1 + mda_size1 > disk_size) {
-			log_error("Insufficient space for first mda on %s",
-				  pv_dev_name(pv));
-			return 0;
-		}
-		/* Round up to pe_align boundary */
-		mda_adjustment = (mda_size1 + start1) % alignment;
-		if (mda_adjustment)
-			mda_size1 += (alignment - mda_adjustment);
-		/* Only have 1 mda in this case */
-		pvmetadatacopies = 1;
-	}
-
-	/* If we already have PEs, avoid overlap */
-	if (pe_start || pe_end) {
-		if (pe_start <= start1)
-			mda_size1 = 0;
-		else if (start1 + mda_size1 > pe_start)
-			mda_size1 = pe_start - start1;
-	}
-
-	/* FIXME If creating new mdas, wipe them! */
-	if (mda_size1) {
-		if (!add_mda(fmt, fmt->cmd->mem, mdas, pv->dev, start1,
-			     mda_size1, metadataignore))
-			return 0;
-
-		if (!dev_set((struct device *) pv->dev, start1,
-			     (size_t) (mda_size1 >
-				       wipe_size ? : mda_size1), 0)) {
-			log_error("Failed to wipe new metadata area");
-			return 0;
-		}
-
-		if (pvmetadatacopies == 1)
-			return 1;
-	} else
-		start1 = 0;
-
-	/* A second copy at end of disk */
-	mda_size2 = pvmetadatasize << SECTOR_SHIFT;
-
-	/* Ensure it's not going to be bigger than the disk! */
-	if (mda_size2 > disk_size)
-		mda_size2 = disk_size - start1 - mda_size1;
-
-	mda_adjustment = (disk_size - mda_size2) % alignment;
-	if (mda_adjustment)
-		mda_size2 += mda_adjustment;
-
-	start2 = disk_size - mda_size2;
-
-	/* If we already have PEs, avoid overlap */
-	if (pe_start || pe_end) {
-		if (start2 < pe_end) {
-			mda_size2 -= (pe_end - start2);
-			start2 = pe_end;
-		}
-	}
-
-	/* If we already have a first mda, avoid overlap */
-	if (mda_size1) {
-		if (start2 < start1 + mda_size1) {
-			mda_size2 -= (start1 + mda_size1 - start2);
-			start2 = start1 + mda_size1;
-		}
-		/* No room for any PEs here now! */
-	}
-
-	if (mda_size2) {
-		if (!add_mda(fmt, fmt->cmd->mem, mdas, pv->dev, start2,
-			     mda_size2, metadataignore)) return 0;
-		if (!dev_set(pv->dev, start2,
-			     (size_t) (mda_size1 >
-				       wipe_size ? : mda_size1), 0)) {
-			log_error("Failed to wipe new metadata area");
-			return 0;
-		}
-	} else
-		return 0;
-
-	return 1;
-}
-
 /* Only for orphans */
 /* Set label_sector to -1 if rewriting existing label into same sector */
 /* If mdas is supplied it overwrites existing mdas e.g. used with pvcreate */
-- 
1.7.3.2

[PATCH 08/11] Cleanup pv_write to use recent changes in metadata handling interface.

[Peter Rajnoha]

Well, pv_write is now quite short and clean when using all the changes
and I hope (maybe 'pray' is a better word :)) I haven't removed anything
important. Also, I hope that everything that should be calculated was
calculated in the code before and eveything is in its place.

Please, double-check me, triple-check me, ... :)

Signed-off-by: Peter Rajnoha <prajnoha@redhat.com>
---
 lib/format1/format1.c            |   14 +----
 lib/format_text/format-text.c    |  114 +------------------------------------
 lib/metadata/metadata-exported.h |    3 +-
 lib/metadata/metadata.c          |   11 ++--
 lib/metadata/metadata.h          |    3 +-
 tools/pvchange.c                 |    4 +-
 tools/pvresize.c                 |    2 +-
 tools/vgconvert.c                |    4 +-
 tools/vgreduce.c                 |    2 +-
 9 files changed, 17 insertions(+), 140 deletions(-)

diff --git a/lib/format1/format1.c b/lib/format1/format1.c
index b2e4aaf..e8b15fe 100644
--- a/lib/format1/format1.c
+++ b/lib/format1/format1.c
@@ -451,23 +451,11 @@ static int _format1_lv_setup(struct format_instance *fid, struct logical_volume
 	return 1;
 }
 
-static int _format1_pv_write(const struct format_type *fmt, struct physical_volume *pv,
-		     struct dm_list *mdas __attribute__((unused)), int64_t sector __attribute__((unused)))
+static int _format1_pv_write(const struct format_type *fmt, struct physical_volume *pv)
 {
 	struct dm_pool *mem;
 	struct disk_list *dl;
 	struct dm_list pvs;
-	struct label *label;
-	struct lvmcache_info *info;
-
-	if (!(info = lvmcache_add(fmt->labeller, (char *) &pv->id, pv->dev,
-				  pv->vg_name, NULL, 0)))
-		return_0;
-	label = info->label;
-	info->device_size = pv->size << SECTOR_SHIFT;
-	info->fmt = fmt;
-
-	dm_list_init(&info->mdas);
 
 	dm_list_init(&pvs);
 
diff --git a/lib/format_text/format-text.c b/lib/format_text/format-text.c
index c7f5d5a..7481d80 100644
--- a/lib/format_text/format-text.c
+++ b/lib/format_text/format-text.c
@@ -1247,129 +1247,23 @@ static int _text_scan(const struct format_type *fmt)
 }
 
 /* Only for orphans */
-/* Set label_sector to -1 if rewriting existing label into same sector */
-/* If mdas is supplied it overwrites existing mdas e.g. used with pvcreate */
-static int _text_pv_write(const struct format_type *fmt, struct physical_volume *pv,
-		     struct dm_list *mdas, int64_t label_sector)
+static int _text_pv_write(const struct format_type *fmt, struct physical_volume *pv)
 {
-	struct label *label;
 	struct lvmcache_info *info;
 	struct mda_context *mdac;
 	struct metadata_area *mda;
 	char buf[MDA_HEADER_SIZE] __attribute__((aligned(8)));
 	struct mda_header *mdah = (struct mda_header *) buf;
-	uint64_t adjustment;
-	struct data_area_list *da;
 
-	/* FIXME Test mode don't update cache? */
-
-	if (!(info = lvmcache_add(fmt->labeller, (char *) &pv->id, pv->dev,
-				  FMT_TEXT_ORPHAN_VG_NAME, NULL, 0)))
+	if (!(info = info_from_pvid((char *) &pv->id, 0)))
 		return_0;
-	label = info->label;
-
-	if (label_sector != -1)
-		label->sector = label_sector;
-
-	info->device_size = pv->size << SECTOR_SHIFT;
-	info->fmt = fmt;
-
-	/* If mdas supplied, use them regardless of existing ones, */
-	/* otherwise retain existing ones */
-	if (mdas) {
-		if (info->mdas.n)
-			del_mdas(&info->mdas);
-		else
-			dm_list_init(&info->mdas);
-		dm_list_iterate_items(mda, mdas) {
-			mdac = mda->metadata_locn;
-			log_debug("Creating metadata area on %s at sector %"
-				  PRIu64 " size %" PRIu64 " sectors",
-				  dev_name(mdac->area.dev),
-				  mdac->area.start >> SECTOR_SHIFT,
-				  mdac->area.size >> SECTOR_SHIFT);
-			add_mda(fmt, NULL, &info->mdas, mdac->area.dev,
-				mdac->area.start, mdac->area.size, mda_is_ignored(mda));
-		}
-		/* FIXME Temporary until mda creation supported by tools */
-	} else if (!info->mdas.n) {
-		dm_list_init(&info->mdas);
-	}
 
-	/*
-	 * If no pe_start supplied but PV already exists,
-	 * get existing value; use-cases include:
-	 * - pvcreate on PV without prior pvremove
-	 * - vgremove on VG with PV(s) that have pe_start=0 (hacked cfg)
-	 */
 	if (info->das.n) {
-		if (!pv->pe_start)
-			dm_list_iterate_items(da, &info->das)
-				pv->pe_start = da->disk_locn.offset >> SECTOR_SHIFT;
 		del_das(&info->das);
 	} else
 		dm_list_init(&info->das);
 
-#if 0
-	/*
-	 * FIXME: ideally a pre-existing pe_start seen in .pv_write
-	 * would always be preserved BUT 'pvcreate on PV without prior pvremove'
-	 * could easily cause the pe_start to overlap with the first mda!
-	 */
-	if (pv->pe_start) {
-		log_very_verbose("%s: preserving pe_start=%lu",
-				 pv_dev_name(pv), pv->pe_start);
-		goto preserve_pe_start;
-	}
-#endif
-
-	/*
-	 * If pe_start is still unset, set it to first aligned
-	 * sector after any metadata areas that begin before pe_start.
-	 */
-	if (!pv->pe_start) {
-		pv->pe_start = pv->pe_align;
-		if (pv->pe_align_offset)
-			pv->pe_start += pv->pe_align_offset;
-	}
-	dm_list_iterate_items(mda, &info->mdas) {
-		mdac = (struct mda_context *) mda->metadata_locn;
-		if (pv->dev == mdac->area.dev &&
-		    ((mdac->area.start <= (pv->pe_start << SECTOR_SHIFT)) ||
-		    (mdac->area.start <= lvm_getpagesize() &&
-		     pv->pe_start < (lvm_getpagesize() >> SECTOR_SHIFT))) &&
-		    (mdac->area.start + mdac->area.size >
-		     (pv->pe_start << SECTOR_SHIFT))) {
-			pv->pe_start = (mdac->area.start + mdac->area.size)
-			    >> SECTOR_SHIFT;
-			/* Adjust pe_start to: (N * pe_align) + pe_align_offset */
-			if (pv->pe_align) {
-				adjustment =
-				(pv->pe_start - pv->pe_align_offset) % pv->pe_align;
-				if (adjustment)
-					pv->pe_start += (pv->pe_align - adjustment);
-
-				log_very_verbose("%s: setting pe_start=%" PRIu64
-					 " (orig_pe_start=%" PRIu64 ", "
-					 "pe_align=%lu, pe_align_offset=%lu, "
-					 "adjustment=%" PRIu64 ")",
-					 pv_dev_name(pv), pv->pe_start,
-					 (adjustment ?
-					  pv->pe_start - (pv->pe_align - adjustment) :
-					  pv->pe_start),
-					 pv->pe_align, pv->pe_align_offset, adjustment);
-			}
-		}
-	}
-	if (pv->pe_start >= pv->size) {
-		log_error("Data area is beyond end of device %s!",
-			  pv_dev_name(pv));
-		return 0;
-	}
-
-	/* FIXME: preserve_pe_start: */
-	if (!add_da
-	    (NULL, &info->das, pv->pe_start << SECTOR_SHIFT, UINT64_C(0)))
+	if (!add_da(NULL, &info->das, pv->pe_start << SECTOR_SHIFT, UINT64_C(0)))
 		return_0;
 
 	if (!dev_open(pv->dev))
@@ -1388,7 +1282,7 @@ static int _text_pv_write(const struct format_type *fmt, struct physical_volume
 		}
 	}
 
-	if (!label_write(pv->dev, label)) {
+	if (!label_write(pv->dev, info->label)) {
 		dev_close(pv->dev);
 		return_0;
 	}
diff --git a/lib/metadata/metadata-exported.h b/lib/metadata/metadata-exported.h
index dab953d..850a4ca 100644
--- a/lib/metadata/metadata-exported.h
+++ b/lib/metadata/metadata-exported.h
@@ -359,8 +359,7 @@ struct dm_list *get_vgnames(struct cmd_context *cmd, int include_internal);
 struct dm_list *get_vgids(struct cmd_context *cmd, int include_internal);
 int scan_vgs_for_pvs(struct cmd_context *cmd);
 
-int pv_write(struct cmd_context *cmd, struct physical_volume *pv,
-	     struct dm_list *mdas, int64_t label_sector);
+int pv_write(struct cmd_context *cmd, struct physical_volume *pv);
 int move_pv(struct volume_group *vg_from, struct volume_group *vg_to,
 	    const char *pv_name);
 int move_pvs_used_by_lv(struct volume_group *vg_from,
diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
index 30b79f8..be1e7c5 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -585,7 +585,7 @@ int vg_remove(struct volume_group *vg)
 		}
 
 		/* FIXME Write to same sector label was read from */
-		if (!pv_write(vg->cmd, pv, NULL, INT64_C(-1))) {
+		if (!pv_write(vg->cmd, pv)) {
 			log_error("Failed to remove physical volume \"%s\""
 				  " from volume group \"%s\"",
 				  pv_dev_name(pv), vg->name);
@@ -1503,7 +1503,7 @@ struct physical_volume * pvcreate_single(struct cmd_context *cmd,
 	log_very_verbose("Writing physical volume data to disk \"%s\"",
 			 pv_name);
 
-	if (!(pv_write(cmd, pv, &mdas, pp->labelsector))) {
+	if (!(pv_write(cmd, pv))) {
 		log_error("Failed to write physical volume \"%s\"", pv_name);
 		goto error;
 	}
@@ -3403,8 +3403,7 @@ int scan_vgs_for_pvs(struct cmd_context *cmd)
 }
 
 int pv_write(struct cmd_context *cmd __attribute__((unused)),
-	     struct physical_volume *pv,
-	     struct dm_list *mdas, int64_t label_sector)
+	     struct physical_volume *pv)
 {
 	if (!pv->fmt->ops->pv_write) {
 		log_error("Format does not support writing physical volumes");
@@ -3417,7 +3416,7 @@ int pv_write(struct cmd_context *cmd __attribute__((unused)),
 		return 0;
 	}
 
-	if (!pv->fmt->ops->pv_write(pv->fmt, pv, mdas, label_sector))
+	if (!pv->fmt->ops->pv_write(pv->fmt, pv))
 		return_0;
 
 	return 1;
@@ -3436,7 +3435,7 @@ int pv_write_orphan(struct cmd_context *cmd, struct physical_volume *pv)
 		return 0;
 	}
 
-	if (!pv_write(cmd, pv, NULL, INT64_C(-1))) {
+	if (!pv_write(cmd, pv)) {
 		log_error("Failed to clear metadata from physical "
 			  "volume \"%s\" after removal from \"%s\"",
 			  pv_dev_name(pv), old_vg_name);
diff --git a/lib/metadata/metadata.h b/lib/metadata/metadata.h
index 6e01b17..5962b38 100644
--- a/lib/metadata/metadata.h
+++ b/lib/metadata/metadata.h
@@ -279,8 +279,7 @@ struct format_handler {
 	 * pv->vg_name must be a valid orphan VG name
 	 */
 	int (*pv_write) (const struct format_type * fmt,
-			 struct physical_volume * pv, struct dm_list * mdas,
-			 int64_t label_sector);
+			 struct physical_volume * pv);
 
 	/*
 	 * Tweak an already filled out a lv eg, check there
diff --git a/tools/pvchange.c b/tools/pvchange.c
index 02d3510..2633da2 100644
--- a/tools/pvchange.c
+++ b/tools/pvchange.c
@@ -160,7 +160,7 @@ static int _pvchange_single(struct cmd_context *cmd, struct volume_group *vg,
 
 			pv->vg_name = pv->fmt->orphan_vg_name;
 			pv->pe_alloc_count = 0;
-			if (!(pv_write(cmd, pv, NULL, INT64_C(-1)))) {
+			if (!(pv_write(cmd, pv))) {
 				log_error("pv_write with new uuid failed "
 					  "for %s.", pv_name);
 				goto out;
@@ -182,7 +182,7 @@ static int _pvchange_single(struct cmd_context *cmd, struct volume_group *vg,
 			goto out;
 		}
 		backup(vg);
-	} else if (!(pv_write(cmd, pv, NULL, INT64_C(-1)))) {
+	} else if (!(pv_write(cmd, pv))) {
 		log_error("Failed to store physical volume \"%s\"",
 			  pv_name);
 		goto out;
diff --git a/tools/pvresize.c b/tools/pvresize.c
index af499e5..b39f94c 100644
--- a/tools/pvresize.c
+++ b/tools/pvresize.c
@@ -152,7 +152,7 @@ static int _pv_resize_single(struct cmd_context *cmd,
 			goto out;
 		}
 		backup(vg);
-	} else if (!(pv_write(cmd, pv, NULL, INT64_C(-1)))) {
+	} else if (!(pv_write(cmd, pv))) {
 		log_error("Failed to store physical volume \"%s\"",
 			  pv_name);
 		goto out;
diff --git a/tools/vgconvert.c b/tools/vgconvert.c
index db28710..2043500 100644
--- a/tools/vgconvert.c
+++ b/tools/vgconvert.c
@@ -155,9 +155,7 @@ static int vgconvert_single(struct cmd_context *cmd, const char *vg_name,
 
 		log_very_verbose("Writing physical volume data to disk \"%s\"",
 				 pv_dev_name(pv));
-		if (!(pv_write(cmd, pv, &mdas,
-			       arg_int64_value(cmd, labelsector_ARG,
-					       DEFAULT_LABELSECTOR)))) {
+		if (!(pv_write(cmd, pv))) {
 			log_error("Failed to write physical volume \"%s\"",
 				  pv_dev_name(pv));
 			log_error("Use pvcreate and vgcfgrestore to repair "
diff --git a/tools/vgreduce.c b/tools/vgreduce.c
index eceafcd..7e1380a 100644
--- a/tools/vgreduce.c
+++ b/tools/vgreduce.c
@@ -438,7 +438,7 @@ static int _vgreduce_single(struct cmd_context *cmd, struct volume_group *vg,
 		goto bad;
 	}
 
-	if (!pv_write(cmd, pv, NULL, INT64_C(-1))) {
+	if (!pv_write(cmd, pv)) {
 		log_error("Failed to clear metadata from physical "
 			  "volume \"%s\" "
 			  "after removal from \"%s\"", name, vg->name);
-- 
1.7.3.2

[PATCH 09/11] Remove useless mdas parameter in code creating new PVs.

[Peter Rajnoha]

Signed-off-by: Peter Rajnoha <prajnoha@redhat.com>
---
 lib/metadata/metadata.c |    2 --
 tools/vgconvert.c       |    2 --
 2 files changed, 0 insertions(+), 4 deletions(-)

diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
index be1e7c5..c9d58e2 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -1434,7 +1434,6 @@ struct physical_volume * pvcreate_single(struct cmd_context *cmd,
 {
 	struct physical_volume *pv;
 	struct device *dev;
-	struct dm_list mdas;
 	struct pvcreate_params default_pp;
 	char buffer[64] __attribute__((aligned(8)));
 
@@ -1466,7 +1465,6 @@ struct physical_volume * pvcreate_single(struct cmd_context *cmd,
 		goto error;
 	}
 
-	dm_list_init(&mdas);
 	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,
diff --git a/tools/vgconvert.c b/tools/vgconvert.c
index 2043500..d2f2d66 100644
--- a/tools/vgconvert.c
+++ b/tools/vgconvert.c
@@ -23,7 +23,6 @@ static int vgconvert_single(struct cmd_context *cmd, const char *vg_name,
 	struct logical_volume *lv;
 	struct lv_list *lvl;
 	uint64_t size = 0;
-	struct dm_list mdas;
 	int pvmetadatacopies = 0;
 	uint64_t pvmetadatasize = 0;
 	uint64_t pe_end = 0, pe_start = 0;
@@ -122,7 +121,6 @@ static int vgconvert_single(struct cmd_context *cmd, const char *vg_name,
 		pe_end = pv_pe_count(existing_pv) * pv_pe_size(existing_pv)
 		    + pe_start - 1;
 
-		dm_list_init(&mdas);
 		if (!(pv = pv_create(cmd, pv_dev(existing_pv),
 				     &existing_pv->id, size, 0, 0,
 				     pe_start, pv_pe_count(existing_pv),
-- 
1.7.3.2

[PATCH 10/11] Make existing lvmcache_update_pvid public.

[Peter Rajnoha]

When changing PV uuid (and the UUID which is important since the cache
hash is based on its value), we need to update the cache. This is
important for subsequent pv_write which tries to find the new PV with
that new UUID in the cache...

Signed-off-by: Peter Rajnoha <prajnoha@redhat.com>
---
 lib/cache/lvmcache.c |   17 +++++++++++------
 lib/cache/lvmcache.h |    1 +
 tools/pvchange.c     |    6 ++++++
 3 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/lib/cache/lvmcache.c b/lib/cache/lvmcache.c
index d1b6dea..ac871c2 100644
--- a/lib/cache/lvmcache.c
+++ b/lib/cache/lvmcache.c
@@ -872,19 +872,24 @@ void lvmcache_del(struct lvmcache_info *info)
 	return;
 } */
 
-static int _lvmcache_update_pvid(struct lvmcache_info *info, const char *pvid)
+int lvmcache_update_pvid(struct lvmcache_info *info, const char *pvid)
 {
+	char pvid_s[ID_LEN + 1] __attribute__((aligned(8)));
+
+	strncpy(pvid_s, pvid, sizeof(pvid_s));
+	pvid_s[sizeof(pvid_s) - 1] = '\0';
+
 	/*
 	 * Nothing to do if already stored with same pvid.
 	 */
-	if (((dm_hash_lookup(_pvid_hash, pvid)) == info) &&
+	if (((dm_hash_lookup(_pvid_hash, pvid_s)) == info) &&
 	    !strcmp(info->dev->pvid, pvid))
 		return 1;
 	if (*info->dev->pvid)
 		dm_hash_remove(_pvid_hash, info->dev->pvid);
-	strncpy(info->dev->pvid, pvid, sizeof(info->dev->pvid));
-	if (!dm_hash_insert(_pvid_hash, pvid, info)) {
-		log_error("_lvmcache_update: pvid insertion failed: %s", pvid);
+	strncpy(info->dev->pvid, pvid_s, sizeof(info->dev->pvid));
+	if (!dm_hash_insert(_pvid_hash, pvid_s, info)) {
+		log_error("_lvmcache_update: pvid insertion failed: %s", pvid_s);
 		return 0;
 	}
 
@@ -1331,7 +1336,7 @@ struct lvmcache_info *lvmcache_add(struct labeller *labeller, const char *pvid,
 	info->fmt = (const struct format_type *) labeller->private;
 	info->status |= CACHE_INVALID;
 
-	if (!_lvmcache_update_pvid(info, pvid_s)) {
+	if (!lvmcache_update_pvid(info, pvid_s)) {
 		if (!existing) {
 			dm_free(info);
 			label_destroy(label);
diff --git a/lib/cache/lvmcache.h b/lib/cache/lvmcache.h
index 32b4579..52f2d68 100644
--- a/lib/cache/lvmcache.h
+++ b/lib/cache/lvmcache.h
@@ -80,6 +80,7 @@ int lvmcache_add_orphan_vginfo(const char *vgname, struct format_type *fmt);
 void lvmcache_del(struct lvmcache_info *info);
 
 /* Update things */
+int lvmcache_update_pvid(struct lvmcache_info *info, const char *pvid);
 int lvmcache_update_vgname_and_id(struct lvmcache_info *info,
 				  const char *vgname, const char *vgid,
 				  uint32_t vgstatus, const char *hostname);
diff --git a/tools/pvchange.c b/tools/pvchange.c
index 2633da2..a88e113 100644
--- a/tools/pvchange.c
+++ b/tools/pvchange.c
@@ -21,6 +21,8 @@ static int _pvchange_single(struct cmd_context *cmd, struct volume_group *vg,
 			    struct physical_volume *pv,
 			    void *handle __attribute__((unused)))
 {
+	struct lvmcache_info *info;
+
 	uint32_t orig_pe_alloc_count;
 	/* FIXME Next three only required for format1. */
 	uint32_t orig_pe_count, orig_pe_size;
@@ -141,6 +143,9 @@ static int _pvchange_single(struct cmd_context *cmd, struct volume_group *vg,
 			goto out;
 	} else {
 		/* --uuid: Change PV ID randomly */
+		if (!(info = info_from_pvid((char *) &pv->id, 0)))
+			goto_out;
+
 		if (!id_create(&pv->id)) {
 			log_error("Failed to generate new random UUID for %s.",
 				  pv_name);
@@ -149,6 +154,7 @@ static int _pvchange_single(struct cmd_context *cmd, struct volume_group *vg,
 		if (!id_write_format(&pv->id, uuid, sizeof(uuid)))
 			goto_out;
 		log_verbose("Changing uuid of %s to %s.", pv_name, uuid);
+		if (!(lvmcache_update_pvid(info, (char *) &pv->id)));
 		if (!is_orphan(pv)) {
 			orig_vg_name = pv_vg_name(pv);
 			orig_pe_alloc_count = pv_pe_alloc_count(pv);
-- 
1.7.3.2

[PATCH 11/11] Use new metadata interface to provide better support for PV resize.

[Peter Rajnoha]

(rhbz #477891)

Finally, we can achieve this! Now, it's simple - when resizing a PV which
contains 2nd metadata area at the end of the disk, we'll just remove the
mda, resize and add a new mda.

We also need a format-specific resize part of the code to be called so
the format_handler interface now includes the pv_resize fn (metadata
location is format-specific as well as the number of supported metadata
area and the metadata layout itself).

The patch moves some parts of the PV resize code a bit to fit the changes
better.

Signed-off-by: Peter Rajnoha <prajnoha@redhat.com>
---
 lib/format_text/format-text.c    |   52 ++++++++++++++++++++++++++++
 lib/metadata/metadata-exported.h |    2 +-
 lib/metadata/metadata.h          |    8 ++++
 lib/metadata/pv_manip.c          |   70 +++++++++++++++++++++++++++++---------
 tools/pvresize.c                 |   55 ++----------------------------
 5 files changed, 118 insertions(+), 69 deletions(-)

diff --git a/lib/format_text/format-text.c b/lib/format_text/format-text.c
index 7481d80..5a935e6 100644
--- a/lib/format_text/format-text.c
+++ b/lib/format_text/format-text.c
@@ -2000,6 +2000,57 @@ static int _text_pv_remove_metadata_area(const struct format_type *fmt,
 	return 1;
 }
 
+static int _text_pv_resize(const struct format_type *fmt,
+			   struct physical_volume *pv,
+			   struct volume_group *vg,
+			   uint64_t size)
+{
+	struct lvmcache_info *info;
+	struct metadata_area *mda;
+	struct mda_context *mdac;
+	uint64_t size_reduction;
+	uint64_t mda_size;
+	unsigned mda_ignored;
+
+	if (!(info = info_from_pvid(pv->dev->pvid, 0)))
+		return_0;
+
+	/*
+	 * First, set the new size and update the cache and reset pe_count.
+	 * (pe_count must be reset otherwise it would be considered as
+	 * a limiting factor while moving the mda!)
+	 */
+	pv->size = size;
+	pv->pe_count = 0;
+	info->device_size = size << SECTOR_SHIFT;
+
+	/* If there's an mda@the end, move it to a new position. */
+	if ((mda = info->mda_slots[1]) &&
+	    (mdac = mda->metadata_locn)) {
+		/* FIXME: Maybe MDA0 size would be better? */
+		mda_size = mdac->area.size >> SECTOR_SHIFT;
+		mda_ignored = mda_is_ignored(mda);
+
+		if (!_text_pv_remove_metadata_area(fmt, pv, 1) ||
+		    !_text_pv_add_metadata_area(fmt, pv, 1, mda_size, mda_ignored)) {
+			log_error("Failed to move metadata area with index 1 "
+				  "while resizing PV %s.", pv_dev_name(pv));
+			return 0;
+		}
+	}
+
+	/* If there's a VG, reduce size by counting in pe_start and metadata areas. */
+	if (vg) {
+		size_reduction = pv_pe_start(pv);
+		if ((mda = info->mda_slots[1]) &&
+		    (mdac = mda->metadata_locn))
+			size_reduction += mdac->area.size >> SECTOR_SHIFT;
+		pv->size -= size_reduction;
+	}
+
+	return 1;
+}
+
 /* NULL vgname means use only the supplied context e.g. an archive file */
 static struct format_instance *_text_create_text_instance(const struct format_type
 						     *fmt, const char *vgname,
@@ -2143,6 +2194,7 @@ static struct format_handler _text_handler = {
 	.pv_setup = _text_pv_setup,
 	.pv_add_metadata_area = _text_pv_add_metadata_area,
 	.pv_remove_metadata_area = _text_pv_remove_metadata_area,
+	.pv_resize = _text_pv_resize,
 	.pv_write = _text_pv_write,
 	.vg_setup = _text_vg_setup,
 	.lv_setup = _text_lv_setup,
diff --git a/lib/metadata/metadata-exported.h b/lib/metadata/metadata-exported.h
index 850a4ca..6383985 100644
--- a/lib/metadata/metadata-exported.h
+++ b/lib/metadata/metadata-exported.h
@@ -402,7 +402,7 @@ struct physical_volume *pv_create(const struct cmd_context *cmd,
 				  uint64_t pvmetadatasize,
 				  unsigned metadataignore);
 int pv_resize(struct physical_volume *pv, struct volume_group *vg,
-             uint32_t new_pe_count);
+             uint64_t size);
 int pv_analyze(struct cmd_context *cmd, const char *pv_name,
 	       uint64_t label_sector);
 
diff --git a/lib/metadata/metadata.h b/lib/metadata/metadata.h
index 5962b38..6acaa87 100644
--- a/lib/metadata/metadata.h
+++ b/lib/metadata/metadata.h
@@ -275,6 +275,14 @@ struct format_handler {
 					unsigned metadata_index);
 
 	/*
+	 * Recalculate the PV size taking into account any existing metadata areas.
+	 */
+	int (*pv_resize) (const struct format_type *fmt,
+			  struct physical_volume *pv,
+			  struct volume_group *vg,
+			  uint64_t size);
+
+	/*
 	 * Write a PV structure to disk. Fails if the PV is in a VG ie
 	 * pv->vg_name must be a valid orphan VG name
 	 */
diff --git a/lib/metadata/pv_manip.c b/lib/metadata/pv_manip.c
index e6fe6c2..03ba090 100644
--- a/lib/metadata/pv_manip.c
+++ b/lib/metadata/pv_manip.c
@@ -357,10 +357,10 @@ int check_pv_segments(struct volume_group *vg)
 	return ret;
 }
 
-static int _reduce_pv(struct physical_volume *pv, struct volume_group *vg, uint32_t new_pe_count)
+static int _reduce_pv(struct physical_volume *pv, struct volume_group *vg,
+		      uint32_t old_pe_count, uint32_t new_pe_count)
 {
 	struct pv_segment *peg, *pegt;
-	uint32_t old_pe_count = pv->pe_count;
 
 	if (new_pe_count < pv->pe_alloc_count) {
 		log_error("%s: cannot resize to %" PRIu32 " extents "
@@ -400,10 +400,9 @@ static int _reduce_pv(struct physical_volume *pv, struct volume_group *vg, uint3
 }
 
 static int _extend_pv(struct physical_volume *pv, struct volume_group *vg,
-		      uint32_t new_pe_count)
+		      uint32_t old_pe_count, uint32_t new_pe_count)
 {
 	struct pv_segment *peg;
-	uint32_t old_pe_count = pv->pe_count;
 
 	if ((uint64_t) new_pe_count * pv->pe_size > pv->size ) {
 		log_error("%s: cannot resize to %" PRIu32 " extents as there "
@@ -432,20 +431,59 @@ static int _extend_pv(struct physical_volume *pv, struct volume_group *vg,
  */
 int pv_resize(struct physical_volume *pv,
 	      struct volume_group *vg,
-	      uint32_t new_pe_count)
+	      uint64_t size)
 {
-	if ((new_pe_count == pv->pe_count)) {
-		log_verbose("No change to size of physical volume %s.",
-			    pv_dev_name(pv));
-		return 1;
+	uint32_t old_pe_count, new_pe_count = 0;
+
+	if (size < PV_MIN_SIZE) {
+		log_error("Size must exceed minimum of %ld sectors on PV %s.",
+			   PV_MIN_SIZE, pv_dev_name(pv));
+		return 0;
 	}
 
-	log_verbose("Resizing physical volume %s from %" PRIu32
-		    " to %" PRIu32 " extents.",
-		    pv_dev_name(pv), pv->pe_count, new_pe_count);
+	if (size < pv_pe_start(pv)) {
+		log_error("Size must exceed physical extent start "
+			  "of %" PRIu64 " sectors on PV %s.",
+			  pv_pe_start(pv), pv_dev_name(pv));
+	}
 
-	if (new_pe_count > pv->pe_count)
-		return _extend_pv(pv, vg, new_pe_count);
-	else
-		return _reduce_pv(pv, vg, new_pe_count);
+	old_pe_count = pv->pe_count;
+
+	if (!pv->fmt->ops->pv_resize(pv->fmt, pv, vg, size)) {
+		log_error("Format specific resize of PV %s failed.",
+			   pv_dev_name(pv));
+		return 0;
+	}
+
+	/* pv->pe_count is 0 now! We need to recalculate! */
+
+	/* If there's a VG, calculate new PE count value. */
+	if (vg) {
+		/* FIXME: Maybe PE calculation should go into pv->fmt->resize?
+		          (like it is for pv->fmt->setup) */
+		if (!(new_pe_count = pv_size(pv) / vg->extent_size)) {
+			log_error("Size must leave space for at least one physical "
+				  "extent of %" PRIu32 " sectors on PV %s.",
+				   pv_pe_size(pv), pv_dev_name(pv));
+			return 0;
+		}
+
+		if ((new_pe_count == old_pe_count)) {
+			pv->pe_count = old_pe_count;
+			log_verbose("No change to size of physical volume %s.",
+				    pv_dev_name(pv));
+			return 1;
+		}
+
+		log_verbose("Resizing physical volume %s from %" PRIu32
+			    " to %" PRIu32 " extents.",
+			    pv_dev_name(pv), pv->pe_count, new_pe_count);
+
+		if (new_pe_count > pv->pe_count)
+			return _extend_pv(pv, vg, old_pe_count, new_pe_count);
+		else
+			return _reduce_pv(pv, vg, old_pe_count, new_pe_count);
+	}
+
+	return 1;
 }
diff --git a/tools/pvresize.c b/tools/pvresize.c
index b39f94c..5a68669 100644
--- a/tools/pvresize.c
+++ b/tools/pvresize.c
@@ -30,30 +30,22 @@ static int _pv_resize_single(struct cmd_context *cmd,
 {
 	struct pv_list *pvl;
 	uint64_t size = 0;
-	uint32_t new_pe_count = 0;
 	int r = 0;
-	struct dm_list mdas;
 	const char *pv_name = pv_dev_name(pv);
 	const char *vg_name = pv_vg_name(pv);
-	struct lvmcache_info *info;
-	int mda_count = 0;
 	struct volume_group *old_vg = vg;
 
-	dm_list_init(&mdas);
-
 	if (is_orphan_vg(vg_name)) {
 		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, &mdas, NULL, 1, 0))) {
+		if (!(pv = pv_read(cmd, pv_name, NULL, NULL, 1, 0))) {
 			unlock_vg(cmd, vg_name);
 			log_error("Unable to read PV \"%s\"", pv_name);
 			return 0;
 		}
-
-		mda_count = dm_list_size(&mdas);
 	} else {
 		vg = vg_read_for_update(cmd, vg_name, NULL, 0);
 
@@ -72,24 +64,10 @@ static int _pv_resize_single(struct cmd_context *cmd,
 
 		pv = pvl->pv;
 
-		if (!(info = info_from_pvid(pv->dev->pvid, 0))) {
-			log_error("Can't get info for PV %s in volume group %s",
-				  pv_name, vg->name);
-			goto out;
-		}
-
-		mda_count = dm_list_size(&info->mdas);
-
 		if (!archive(vg))
 			goto out;
 	}
 
-	/* FIXME Create function to test compatibility properly */
-	if (mda_count > 1) {
-		log_error("%s: too many metadata areas for pvresize", pv_name);
-		goto out;
-	}
-
 	if (!(pv->fmt->features & FMT_RESIZE_PV)) {
 		log_error("Physical volume %s format does not support resizing.",
 			  pv_name);
@@ -111,35 +89,8 @@ static int _pv_resize_single(struct cmd_context *cmd,
 		size = new_size;
 	}
 
-	if (size < PV_MIN_SIZE) {
-		log_error("%s: Size must exceed minimum of %ld sectors.",
-			  pv_name, PV_MIN_SIZE);
-		goto out;
-	}
-
-	if (size < pv_pe_start(pv)) {
-		log_error("%s: Size must exceed physical extent start of "
-			  "%" PRIu64 " sectors.", pv_name, pv_pe_start(pv));
-		goto out;
-	}
-
-	pv->size = size;
-
-	if (vg) {
-		pv->size -= pv_pe_start(pv);
-		new_pe_count = pv_size(pv) / vg->extent_size;
-
- 		if (!new_pe_count) {
-			log_error("%s: Size must leave space for at "
-				  "least one physical extent of "
-				  "%" PRIu32 " sectors.", pv_name,
-				  pv_pe_size(pv));
-			goto out;
-		}
-
-		if (!pv_resize(pv, vg, new_pe_count))
-			goto_out;
-	}
+	if (!pv_resize(pv, vg, size))
+		goto_out;
 
 	log_verbose("Resizing volume \"%s\" to %" PRIu64 " sectors.",
 		    pv_name, pv_size(pv));
-- 
1.7.3.2

[PATCH 01/11] Add mda_slots field for PV info in the cache.

[Zdenek Kabelac]

Dne 18.11.2010 22:32, Peter Rajnoha napsal(a):
> The code further needs to access the information about metadata areas
> directly quite often without checking the relative position to other
> metadata areas or data areas. IOW, we need to know directly whether
> there is any metadata area at the start of the disk or at the end of the
> disk or any other position we can think of in the future. This way, I
> know exactly which slots are free, which are occupied and I know the
> relative position to other metadata areas without checking any offsets
> etc.
> 
> Using the original list would be quite painful here - either I'd need to
> store the position somewhere or I'll just use this array which seems
> better. Though, the list itself is still there - this is something I'd
> like to discuss... (So I haven't tried to remove the mda_list completely
> yet).
> 
> Signed-off-by: Peter Rajnoha <prajnoha@redhat.com>
> ---
>  lib/cache/lvmcache.c |    1 +
>  lib/cache/lvmcache.h |    2 ++
>  2 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/lib/cache/lvmcache.c b/lib/cache/lvmcache.c
> index 60edcec..d1b6dea 100644
> --- a/lib/cache/lvmcache.c
> +++ b/lib/cache/lvmcache.c
> @@ -1357,6 +1357,7 @@ static void _lvmcache_destroy_entry(struct lvmcache_info *info)
>  	_vginfo_detach_info(info);
>  	strcpy(info->dev->pvid, "");
>  	label_destroy(info->label);
> +	dm_free(info->mda_slots);
>  	dm_free(info);
>  }
>  
> diff --git a/lib/cache/lvmcache.h b/lib/cache/lvmcache.h
> index 28f8541..32b4579 100644
> --- a/lib/cache/lvmcache.h
> +++ b/lib/cache/lvmcache.h
> @@ -33,6 +33,7 @@
>  struct cmd_context;
>  struct format_type;
>  struct volume_group;
> +struct metadata_area;


I'm not really sure where we are heading with out  lvmcache.

I'd like to see rather complete removal of this part of code - once we will
start work on thing like Petr's  lvmetad  - which would probably completely
eliminate need of any cache.

I think we should keep our lvmcache as simple as we can only to skip repeated
open & read from device -  everything else should be kept in higher level.
That could make future removal of this code much easier.

I guess we already break this concept in few place - but we should rather fix
those areas instead of add more hardwired functionality into cache level.


Zdenek

[PATCH 04/11] Add pv_remove_metadata_area to format_handler interface.

[Zdenek Kabelac]

Dne 18.11.2010 22:32, Peter Rajnoha napsal(a):
> The same as for pv_add_metadata_area, but removing...
> 
> Signed-off-by: Peter Rajnoha <prajnoha@redhat.com>
> ---
>  lib/format_text/format-text.c |   50 +++++++++++++++++++++++++++++++++++++++++
>  lib/format_text/format-text.h |    3 ++
>  lib/metadata/metadata.h       |    7 +++++
>  3 files changed, 60 insertions(+), 0 deletions(-)
> 
> diff --git a/lib/format_text/format-text.c b/lib/format_text/format-text.c
> index 0c1c4c7..db39c80 100644
> --- a/lib/format_text/format-text.c
> +++ b/lib/format_text/format-text.c
> @@ -2157,6 +2157,55 @@ bad:
>  	return 0;
>  }
>  
> +int remove_metadata_area_from_cache(struct lvmcache_info *info,
> +					    unsigned mda_index)
> +{
> +	struct metadata_area *mda;
> +
> +	if (mda_index >= FMT_TEXT_MAX_MDAS_PER_PV) {
> +		log_error(INTERNAL_ERROR "can't remove metadata with index %u "
> +					 "from cache for PV %s. Metadata layout "
> +					 "not supported by %s format.",
> +					  mda_index, dev_name(info->dev),
> +					  info->fmt->name);

align text on '(' - no reason to keep INTERNAL_ERROR as extra indent level.



Zdenek

[PATCH 05/11] Add pv_initialise to format_handler interface.

[Zdenek Kabelac]

Dne 18.11.2010 22:32, Peter Rajnoha napsal(a):

> diff --git a/lib/format1/disk-rep.c b/lib/format1/disk-rep.c
> index bc58744..38cb34d 100644
> --- a/lib/format1/disk-rep.c
> +++ b/lib/format1/disk-rep.c
> @@ -337,6 +337,13 @@ static void __update_lvmcache(const struct format_type *fmt,
>  

Interesting - we should not use  '__' as prefix in our code - though not
directly related to our code.



>  	info->device_size = xlate32(dl->pvd.pv_size) << SECTOR_SHIFT;
>  	dm_list_init(&info->mdas);
> +	if (info->mda_slots)
> +		dm_free(info->mda_slots);
> +	if (!(info->mda_slots = dm_zalloc(FMT_LVM1_MAX_MDAS_PER_PV * sizeof(struct metadata_area *)))) {
> +		/* FIXME: Add revert of cache info! */
> +		stack;
> +		return;

This is yet another reason why we should not play with cache internally - we
may introduce internal consistency problem and may get into the moment when
our cache will represent different data than does we would read from disk.


> +
> +	if (!(info->mda_slots = dm_zalloc(FMT_TEXT_MAX_MDAS_PER_PV * sizeof(struct metadata_area *)))) {
> +		/* FIXME: Add revert of cache info! */

again quite important case...

> +		return 0;
> +	}
> +
> +	return 1;
> +}
> +
>  static void _text_destroy_instance(struct format_instance *fid __attribute__((unused)))
>  {
>  }


Zdenek

[PATCH 01/11] Add mda_slots field for PV info in the cache.

[Peter Rajnoha]

On 11/25/2010 02:15 PM +0100, Zdenek Kabelac wrote:
>> +struct metadata_area;

(ah, this line slipped from the older version of this patch, this line
is not needed anymore at this place, but that's a detail...)

> 
> I'm not really sure where we are heading with out  lvmcache.
> 
> I'd like to see rather complete removal of this part of code - once we will
> start work on thing like Petr's  lvmetad  - which would probably completely
> eliminate need of any cache.

Yes, that's a very good question - where are we heading with our cache? As I read
the lvmetad concept, it seemed to me that the daemon will just replace the actual
scanning phase and the code will ask the daemon to provide any info it has already
cached instead of firing the scan. Otherwise, everything would stay as we have
today (so the concept seems to be fine - to do as less surgery as possible). Which is
not a bad idea - since all this existing cache machinery is quite a complex. So we
need to do those changes in steps (iow, I don't think that removing the cache as it
is today completely and replacing it with totally different scheme is something
that we can do in a short time).

> 
> I think we should keep our lvmcache as simple as we can only to skip repeated
> open & read from device -  everything else should be kept in higher level.
> That could make future removal of this code much easier.
> 

What I've done here is that I used the cache to keep a few info that I need
(and since I haven't seen any rules as to how the cache should be used :)), I
sticked to that. Well, I just did the same what the older code did, but I moved
the update of the cache a little bit sooner in time... That's all.

Of course, I can avoid using the cache this way, but that would mean far more
changes to be done (for this interface to work). At least, it would mean consolidating
the use of "format instance" and change the structure and its use throughout the
code.

The logic of this patches would stay of course, what I need to decide (with respect
to this patchset, not the cache itself) is where to store this information, how to
'package' the info and how to handle it internally so I can access it easily.

So we would have structures for the cached information that would be only read, not
edited nor touched in any way. If there are any changes done, we throw away the cache
completely and read it again from the raw disk - we need to state the policy clearly
and follow it!

For now, I reused the cache structure. But sure, I can add something else so we can
avoid using the cache this way (it's just about replacing the carrier
of the information). We would have a "working copy" of the cache. Though that would
mean copying part of the information we already have in cache structures. This happens
already for the internal "format_instance". Unluckily, this structure is not suitable
for this patchset, so I would need to change it considerably, I think.

But yes, it could be done, but including far far more changes...

> I guess we already break this concept in few place - but we should rather fix
> those areas instead of add more hardwired functionality into cache level.

As I said above, let's state the policy and the rules for the cache use and
let's follow it ;)

Peter

[PATCH 01/11] Add mda_slots field for PV info in the cache.

[Peter Rajnoha]

On 11/25/2010 02:39 PM +0100, Peter Rajnoha wrote:
> Of course, I can avoid using the cache this way, but that would mean far more
> changes to be done (for this interface to work). At least, it would mean consolidating
> the use of "format instance" and change the structure and its use throughout the
> code.

..I'd like to avoid making another copy of the cache. It's not about performance right
now, but it's more about keeping the code readable - having N copies of the same structures
around the code will bring confusion and bugs...

Peter