Add interface to support adding and removing metadata areas on demand (v2)

The format instance holds the information about current metadata areas and it is a snapshot of what we have in the cache at the moment of the instance creation. Through the command exection, we use this instance to modify, set and access modified metadata areas (like we use it to support the --metadataignore and we store the metadata areas that are in use and the ones that are ignored).

But to support modifying metadata areas fully (with add/remove metadata area support), we need to be able to create format instances for PVs as well. Hence, add the pvid parameter for the create_instance function.

The rules we will follow:

- the format instance is VG-based or PV-based, but *never both* at the same time,

- when a PV (structure) is alone, not part of any VG, we will have a PV based format_instance attached to the PV structure,

- when a PV is part of the VG, any existing PV-based format_instance is destroyed and the PV strucutre will reference VG based format instance instead.

- whatever we need change for mdas, we'll use the format_instance, we don't touch the cache directly! This will be synchronized with the format instance after pv_write automatically.

Signed-off-by Peter Rajnoha <[email protected]>

---
 lib/cache/lvmcache.c          |    2 +-
 lib/format1/format1.c         |    1 +
 lib/format_pool/format_pool.c |    1 +
 lib/format_text/archive.c     |    2 +-
 lib/format_text/archiver.c    |    8 ++++----
 lib/format_text/format-text.c |   20 +++++++++++---------
 lib/metadata/metadata.c       |   10 +++++-----
 lib/metadata/metadata.h       |    5 +++--
 8 files changed, 27 insertions(+), 22 deletions(-)

lib/cache/lvmcache.c +1 -1

index 0e9cae1..476b176 100644
@@ -653,7 +653,7 @@ struct volume_group *lvmcache_get_vg(const char *vgid, unsigned precommitted)
 		return NULL;
 
 	if (!(fid = vginfo->fmt->ops->create_instance(vginfo->fmt,
-						      vginfo->vgname,
+						      NULL, vginfo->vgname,
 						      vgid, NULL)))
 		return_NULL;
 

lib/format1/format1.c +1 -0

index fc14444..13b757d 100644
@@ -522,6 +522,7 @@ static struct metadata_area_ops _metadata_format1_ops = {
 };
 
 static struct format_instance *_format1_create_instance(const struct format_type *fmt,
+						const char *pvid __attribute__((unused)),
 						const char *vgname __attribute__((unused)),
 						const char *vgid __attribute__((unused)),
 						void *private __attribute__((unused)))

lib/format_pool/format_pool.c +1 -0

index 730da87..814d80c 100644
@@ -249,6 +249,7 @@ static struct metadata_area_ops _metadata_format_pool_ops = {
 /* *INDENT-ON* */
 
 static struct format_instance *_pool_create_instance(const struct format_type *fmt,
+						const char *pvid __attribute__((unused)),
 						const char *vgname __attribute__((unused)),
 						const char *vgid __attribute__((unused)),
 						void *private __attribute__((unused)))

lib/format_text/archive.c +1 -1

index 43425dc..760153c 100644
@@ -309,7 +309,7 @@ static void _display_archive(struct cmd_context *cmd, struct archive_file *af)
 
 	if (!(context = create_text_context(cmd, af->path, NULL)) ||
 	    !(tf = cmd->fmt_backup->ops->create_instance(cmd->fmt_backup, NULL,
-							 NULL, context))) {
+							 NULL, NULL, context))) {
 		log_error("Couldn't create text instance object.");
 		return;
 	}

lib/format_text/archiver.c +4 -4

index e0212a2..b7dcad9 100644
@@ -278,7 +278,7 @@ struct volume_group *backup_read_vg(struct cmd_context *cmd,
 	if (!(context = create_text_context(cmd, file,
 					    cmd->cmd_line)) ||
 	    !(tf = cmd->fmt_backup->ops->create_instance(cmd->fmt_backup, NULL,
-							 NULL, context))) {
+							 NULL, NULL, context))) {
 		log_error("Couldn't create text format object.");
 		return NULL;
 	}

@@ -306,8 +306,8 @@ int backup_restore_vg(struct cmd_context *cmd, struct volume_group *vg)
 	 */
 
 	/* Attempt to write out using currently active format */
-	if (!(vg->fid = cmd->fmt->ops->create_instance(cmd->fmt, vg->name,
-						       NULL, NULL))) {
+	if (!(vg->fid = cmd->fmt->ops->create_instance(cmd->fmt, NULL,
+						vg->name, NULL, NULL))) {
 		log_error("Failed to allocate format instance");
 		return 0;
 	}

@@ -398,7 +398,7 @@ int backup_to_file(const char *file, const char *desc, struct volume_group *vg)
 
 	if (!(context = create_text_context(cmd, file, desc)) ||
 	    !(tf = cmd->fmt_backup->ops->create_instance(cmd->fmt_backup, NULL,
-							 NULL, context))) {
+							 NULL, NULL, context))) {
 		log_error("Couldn't create backup object.");
 		return 0;
 	}

lib/format_text/format-text.c +11 -9

index c186757..fda8255 100644
@@ -37,10 +37,11 @@
 #include <dirent.h>
 #include <ctype.h>
 
-static struct format_instance *_text_create_text_instance(const struct format_type
-						     *fmt, const char *vgname,
-						     const char *vgid,
-						     void *context);
+static struct format_instance *_text_create_text_instance(const struct format_type *fmt,
+							  const char *pvid,
+							  const char *vgname,
+							  const char *vgid,
+							  void *context);
 
 struct text_fid_context {
 	char *raw_metadata_buf;

@@ -1085,7 +1086,7 @@ static int _scan_file(const struct format_type *fmt, const char *vgname)
 
 				/* FIXME stat file to see if it's changed */
 				fid = _text_create_text_instance(fmt, NULL, NULL,
-							    NULL);
+								 NULL, NULL);
 				if ((vg = _vg_read_file_name(fid, scanned_vgname,
 							     path))) {
 					/* FIXME Store creation host in vg */

@@ -1912,10 +1913,11 @@ static int _text_pv_setup(const struct format_type *fmt,
 }
 
 /* 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,
-						     const char *vgid,
-						     void *context)
+static struct format_instance *_text_create_text_instance(const struct format_type*fmt,
+							  const char *pvid,
+							  const char *vgname,
+							  const char *vgid,
+							  void *context)
 {
 	struct format_instance *fid;
 	struct text_fid_context *fidtc;

lib/metadata/metadata.c +5 -5

index 771cdb0..fd1948a 100644
@@ -974,8 +974,8 @@ struct volume_group *vg_create(struct cmd_context *cmd, const char *vg_name)
 	/* initialize removed_pvs list */
 	dm_list_init(&vg->removed_pvs);
 
-	if (!(vg->fid = cmd->fmt->ops->create_instance(cmd->fmt, vg_name,
-						       NULL, NULL))) {
+	if (!(vg->fid = cmd->fmt->ops->create_instance(cmd->fmt, NULL,
+						vg_name, NULL, NULL))) {
 		log_error("Failed to create format instance");
 		goto bad;
 	}

@@ -2629,7 +2629,7 @@ static struct volume_group *_vg_read_orphans(struct cmd_context *cmd,
 	}
 
 	/* create format instance with appropriate metadata area */
-	if (!(vg->fid = vginfo->fmt->ops->create_instance(vginfo->fmt,
+	if (!(vg->fid = vginfo->fmt->ops->create_instance(vginfo->fmt, NULL,
 							  orphan_vgname, NULL,
 							  NULL))) {
 		log_error("Failed to create format instance");

@@ -2808,7 +2808,7 @@ static struct volume_group *_vg_read(struct cmd_context *cmd,
 		use_precommitted = 0;
 
 	/* create format instance with appropriate metadata area */
-	if (!(fid = fmt->ops->create_instance(fmt, vgname, vgid, NULL))) {
+	if (!(fid = fmt->ops->create_instance(fmt, NULL, vgname, vgid, NULL))) {
 		log_error("Failed to create format instance");
 		return NULL;
 	}

@@ -2962,7 +2962,7 @@ static struct volume_group *_vg_read(struct cmd_context *cmd,
 			use_precommitted = 0;
 
 		/* create format instance with appropriate metadata area */
-		if (!(fid = fmt->ops->create_instance(fmt, vgname, vgid, NULL))) {
+		if (!(fid = fmt->ops->create_instance(fmt, NULL, vgname, vgid, NULL))) {
 			log_error("Failed to create format instance");
 			return NULL;
 		}

lib/metadata/metadata.h +3 -2

index c0f9148..ab7cff0 100644
@@ -279,8 +279,9 @@ struct format_handler {
 	/*
 	 * Create format instance with a particular metadata area
 	 */
-	struct format_instance *(*create_instance) (const struct format_type *
-						    fmt, const char *vgname,
+	struct format_instance *(*create_instance) (const struct format_type *fmt,
+						    const char *pvid,
+						    const char *vgname,
 						    const char *vgid,
 						    void *context);

This patch divides current create_instance functionality into PV-based and VG-based. We add a simple flag "pv_only" in struct format_instance that will help us to identify the type of the format_instance when used later on (since we will always have PV/VG separation, I think it would be useless to define an interface for this where we would attach supporting functions on the fly like we do for mda->ops as an example. In this case, a simple flag is fine to switch the functionality).

Another change is the metadata_areas_index we have in the struct format_instance. To access a certain metadata area, we need to know two keys - PV id and metadta area index within that PV. So a few new functions have been modified/added to support this:
- fid_add_mda (modified)
- fid_add_mdas (modified)
- fid_remove_mda (new)
- fid_get_mda_indexed (new)

Actually, the form of the key that will be used is up to the caller, but he needs to remember the key somehow to get the mda back :)

The key is simply made up of the base key (a string) and a subkey (an unsigned integer). The actual key used internally is then a string:

<basekey>_<subkey>

Now, the metadata_areas_index is format specific. This is because each format can have different number and types of metadata areas it can support and so we'd like to be able to use the most suitable index for the specific format. Also, we can use a different indexing scheme for PV and VG based format_instance.

For PV based format instance in current lvm2 format, we have only 2 metadata areas at maximum. So we use only a simple array. No need to use any buldozer-type indexing here :)

For VG based format instance, we can group many many mdas together from numerous PVs. So we'll use a hash here (like we use for the cache). (but maybe we can add even better indexing in the future!)

If key is not defined, we just store the mda without indexing. We'll have it in the metadata_areas_in_use/ignored list directly.

...but we can change these later if needed, of course. The implementation is internal, the interface stays the same)

Also, I kept the format instance's metadata_areas_in_use/ignored for now, but eventually, I'd like this to be accessed through the index in the future. But that is another step to take later...

Signed-off-by Peter Rajnoha <[email protected]>

---
 lib/format1/format1.c            |    1 +
 lib/format_text/format-text.c    |  137 ++++++++++++++++++++++++++++----------
 lib/format_text/format-text.h    |    1 +
 lib/metadata/metadata-exported.h |    3 +
 lib/metadata/metadata.c          |  103 +++++++++++++++++++++++++++-
 lib/metadata/metadata.h          |   10 ++-
 6 files changed, 213 insertions(+), 42 deletions(-)

lib/format1/format1.c +1 -0

index 13b757d..ff5b0f5 100644
@@ -534,6 +534,7 @@ static struct format_instance *_format1_create_instance(const struct format_type
 		return_NULL;
 
 	fid->fmt = fmt;
+	fid->metadata_areas_index = NULL;
 	dm_list_init(&fid->metadata_areas_in_use);
 	dm_list_init(&fid->metadata_areas_ignored);
 

lib/format_text/format-text.c +21 -6

index fda8255..c8c56f7 100644
@@ -48,6 +48,10 @@ struct text_fid_context {
 	uint32_t raw_metadata_buf_size;
 };
 
+struct text_fid_pv_context {
+	int64_t label_sector;
+};
+
 struct dir_list {
 	struct dm_list list;
 	char dir[0];

@@ -1912,14 +1916,38 @@ static int _text_pv_setup(const struct format_type *fmt,
 	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 *pvid,
-							  const char *vgname,
-							  const char *vgid,
-							  void *context)
+static int _create_pv_text_instance(struct format_instance *fid, const char *pvid)
+{
+	struct text_fid_pv_context *fid_pv_tc;
+	struct lvmcache_info *info;
+
+	fid->pv_only = 1;
+
+	if (!(fid_pv_tc = (struct text_fid_pv_context *)
+			  dm_pool_zalloc(fid->fmt->cmd->mem, sizeof(*fid_pv_tc)))) {
+		log_error("Couldn't allocate text_fid_pv_context.");
+		return 0;
+	}
+	fid_pv_tc->label_sector = -1;
+	fid->private = (void *) fid_pv_tc;
+
+	if (!(fid->metadata_areas_index = dm_pool_zalloc(fid->fmt->cmd->mem,
+						   FMT_TEXT_MAX_MDAS_PER_PV *

lib/format_text/format-text.c +61 -30

+
+static int _create_vg_text_instance(struct format_instance *fid,
+				    const char *vgname, const char *vgid,
+				    void *context)
 {
-	struct format_instance *fid;
 	struct text_fid_context *fidtc;
 	struct metadata_area *mda;
 	struct mda_context *mdac;

@@ -1930,85 +1958,122 @@ static struct format_instance *_text_create_text_instance(const struct format_ty
 	struct lvmcache_vginfo *vginfo;
 	struct lvmcache_info *info;
 
-	if (!(fid = dm_pool_alloc(fmt->cmd->mem, sizeof(*fid)))) {
-		log_error("Couldn't allocate format instance object.");
-		return NULL;
-	}
+	fid->pv_only = 0;
 
 	if (!(fidtc = (struct text_fid_context *)
-			dm_pool_zalloc(fmt->cmd->mem,sizeof(*fidtc)))) {
+			dm_pool_zalloc(fid->fmt->cmd->mem,sizeof(*fidtc)))) {
 		log_error("Couldn't allocate text_fid_context.");
-		return NULL;
+		return 0;
 	}
 
 	fidtc->raw_metadata_buf = NULL;
 	fid->private = (void *) fidtc;
 
-	fid->fmt = fmt;
-	dm_list_init(&fid->metadata_areas_in_use);
-	dm_list_init(&fid->metadata_areas_ignored);
-
 	if (!vgname) {
-		if (!(mda = dm_pool_zalloc(fmt->cmd->mem, sizeof(*mda))))
-			return_NULL;
+		if (!(mda = dm_pool_zalloc(fid->fmt->cmd->mem, sizeof(*mda))))
+			return_0;
 		mda->ops = &_metadata_text_file_backup_ops;
 		mda->metadata_locn = context;
 		mda->status = 0;
-		fid_add_mda(fid, mda);
+		fid_add_mda(fid, mda, NULL, 0, 0);
 	} else {
-		dir_list = &((struct mda_lists *) fmt->private)->dirs;
+		if (!(fid->metadata_areas_index = dm_hash_create(128))) {
+			log_error("Couldn't create metadata index for format "
+				  "instance of VG %s.", vgname);
+			return 0;
+		}
+
+		dir_list = &((struct mda_lists *) fid->fmt->private)->dirs;
 
 		dm_list_iterate_items(dl, dir_list) {
 			if (dm_snprintf(path, PATH_MAX, "%s/%s",
 					 dl->dir, vgname) < 0) {
 				log_error("Name too long %s/%s", dl->dir,
 					  vgname);
-				return NULL;
+				return 0;
 			}
 
-			context = create_text_context(fmt->cmd, path, NULL);
-			if (!(mda = dm_pool_zalloc(fmt->cmd->mem, sizeof(*mda))))
-				return_NULL;
+			context = create_text_context(fid->fmt->cmd, path, NULL);
+			if (!(mda = dm_pool_zalloc(fid->fmt->cmd->mem, sizeof(*mda))))
+				return_0;
 			mda->ops = &_metadata_text_file_ops;
 			mda->metadata_locn = context;
 			mda->status = 0;
-			fid_add_mda(fid, mda);
+			fid_add_mda(fid, mda, NULL, 0, 0);
 		}
 
-		raw_list = &((struct mda_lists *) fmt->private)->raws;
+		raw_list = &((struct mda_lists *) fid->fmt->private)->raws;
 
 		dm_list_iterate_items(rl, raw_list) {
 			/* FIXME Cache this; rescan below if some missing */
 			if (!_raw_holds_vgname(fid, &rl->dev_area, vgname))
 				continue;
 
-			if (!(mda = dm_pool_zalloc(fmt->cmd->mem, sizeof(*mda))))
-				return_NULL;
+			if (!(mda = dm_pool_zalloc(fid->fmt->cmd->mem, sizeof(*mda))))
+				return_0;
 
-			if (!(mdac = dm_pool_zalloc(fmt->cmd->mem, sizeof(*mdac))))
-				return_NULL;
+			if (!(mdac = dm_pool_zalloc(fid->fmt->cmd->mem, sizeof(*mdac))))
+				return_0;
 			mda->metadata_locn = mdac;
 			/* FIXME Allow multiple dev_areas inside area */
 			memcpy(&mdac->area, &rl->dev_area, sizeof(mdac->area));
 			mda->ops = &_metadata_text_raw_ops;
 			mda->status = 0;
 			/* FIXME MISTAKE? mda->metadata_locn = context; */
-			fid_add_mda(fid, mda);
+			fid_add_mda(fid, mda, NULL, 0, 0);
 		}
 
 		/* Scan PVs in VG for any further MDAs */
-		lvmcache_label_scan(fmt->cmd, 0);
+		lvmcache_label_scan(fid->fmt->cmd, 0);
 		if (!(vginfo = vginfo_from_vgname(vgname, vgid)))
 			goto_out;
 		dm_list_iterate_items(info, &vginfo->infos) {
-			if (!fid_add_mdas(fid, &info->mdas))
-				return_NULL;
+			if (!fid_add_mdas(fid, &info->mdas, info->dev->pvid,
+					  ID_LEN))
+				return_0;
 		}
 		/* FIXME Check raw metadata area count - rescan if required */
 	}
 
       out:
-	return fid;
+	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 *pvid,
+							  const char *vgname,
+							  const char *vgid,
+							  void *context)
+{
+	struct format_instance *fid;
+	int r;
+
+	if (pvid && (vgname || vgid)) {
+		log_error(INTERNAL_ERROR "Format instance must be PV "
+					 "or VG specific, not both.");
+		return NULL;
+	}
+
+	if (!(fid = dm_pool_alloc(fmt->cmd->mem, sizeof(*fid)))) {
+		log_error("Couldn't allocate format instance object.");
+		return NULL;
+	}
+
+	fid->fmt = fmt;
+	fid->metadata_areas_index = NULL;
+	dm_list_init(&fid->metadata_areas_in_use);
+	dm_list_init(&fid->metadata_areas_ignored);
+

lib/format_text/format-text.c +0 -0

 void *create_text_context(struct cmd_context *cmd, const char *path,

lib/format_text/format-text.h +1 -0

index 79365ea..f3cf4f5 100644
@@ -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

lib/metadata/metadata-exported.h +0 -0

index a9709d6..d8247dd 100644

lib/metadata/metadata-exported.h +0 -0

 	void *private;
 };
 

lib/metadata/metadata.c +3 -2

index fd1948a..c3accaf 100644
@@ -2881,7 +2881,8 @@ static struct volume_group *_vg_read(struct cmd_context *cmd,
 					break;
 				}
 				if (dm_list_size(&info->mdas)) {
-					if (!fid_add_mdas(fid, &info->mdas))
+					if (!fid_add_mdas(fid, &info->mdas,
+							info->dev->pvid, ID_LEN))
 						return_NULL;
 
 					log_debug("Empty mda found for VG %s.", vgname);

@@ -3912,22 +3913,116 @@ uint32_t vg_lock_newname(struct cmd_context *cmd, const char *vgname)
 	return FAILED_EXIST;
 }
 
-void fid_add_mda(struct format_instance *fid, struct metadata_area *mda)
+static int _convert_key_to_string(const char *key, size_t key_len,

lib/metadata/metadata.c +9 -0

+int fid_add_mda(struct format_instance *fid, struct metadata_area *mda,
+		 const char *key, size_t key_len, const unsigned subkey)
+{
+	char extended_key[PATH_MAX];
+
 	dm_list_add(mda_is_ignored(mda) ? &fid->metadata_areas_ignored :
 					  &fid->metadata_areas_in_use, &mda->list);
+
+	/*
+	 * Return if the mda is not supposed to be
+	 * indexed or the index itself is not initialised */

lib/metadata/metadata.c +20 -2

+	else
+		dm_hash_insert(fid->metadata_areas_index, extended_key, mda);
+
+	return 1;
 }
 
-int fid_add_mdas(struct format_instance *fid, struct dm_list *mdas)
+int fid_add_mdas(struct format_instance *fid, struct dm_list *mdas,
+		 const char *key, size_t key_len)
 {
 	struct metadata_area *mda, *mda_new;
+	unsigned mda_index = 0;
 
 	dm_list_iterate_items(mda, mdas) {
 		mda_new = mda_copy(fid->fmt->cmd->mem, mda);
 		if (!mda_new)
 			return_0;
-		fid_add_mda(fid, mda_new);
+
+		fid_add_mda(fid, mda_new, key, key_len, mda_index);
+		mda_index++;
 	}
+
+	return 1;
+}
+
+struct metadata_area *fid_get_mda_indexed(struct format_instance *fid,
+					  const char *key, size_t key_len,
+					  const unsigned subkey)
+{
+	char extended_key[PATH_MAX];
+	struct metadata_area *mda = NULL;

lib/metadata/metadata.c +23 -0

+	return mda;
+}
+
+int fid_remove_mda(struct format_instance *fid, struct metadata_area *mda,
+		   const char *key, size_t key_len, const unsigned subkey)
+{
+	struct metadata_area *mda_indexed = NULL;
+	char extended_key[PATH_MAX];
+
+	/* At least one of mda or key must be specified. */
+	if (!mda && !key)
+		return 1;
+
+	if (key) {
+		/*
+		 * If both mda and key specified, check given mda
+		 * with what we find using the index and return
+		 * immediately if these two do not match.
+		 */
+		if (!(mda_indexed = fid_get_mda_indexed(fid, key, key_len, subkey)) ||
+		     (mda && mda != mda_indexed))
+			return 1;
+

lib/metadata/metadata.c +4 -0

+	}
+
+	dm_list_del(&mda->list);
+
 	return 1;
 }
 

lib/metadata/metadata.h +8 -2

index ab7cff0..0fbc954 100644
@@ -191,8 +191,14 @@ struct metadata_area *mda_copy(struct dm_pool *mem,
 unsigned mda_is_ignored(struct metadata_area *mda);
 void mda_set_ignored(struct metadata_area *mda, unsigned ignored);
 unsigned mda_locns_match(struct metadata_area *mda1, struct metadata_area *mda2);
-void fid_add_mda(struct format_instance *fid, struct metadata_area *mda);
-int fid_add_mdas(struct format_instance *fid, struct dm_list *mdas);
+int fid_add_mda(struct format_instance *fid, struct metadata_area *mda,
+		 const char *key, size_t key_len, const unsigned subkey);
+int fid_add_mdas(struct format_instance *fid, struct dm_list *mdas,
+		 const char *key, size_t key_len);
+int fid_remove_mda(struct format_instance *fid, struct metadata_area *mda,
+		   const char *key, size_t key_len, const unsigned subkey);
+struct metadata_area *fid_get_mda_indexed(struct format_instance *fid,
+		const char *key, size_t key_len, const unsigned subkey);
 int mdas_empty_or_ignored(struct dm_list *mdas);
 
 #define seg_pvseg(seg, s)	(seg)->areas[(s)].u.pv.pvseg

lib/metadata/metadata-exported.h struct pv_segment { · line 176

Quoted 3 lines

struct format_instance {

lib/metadata/metadata-exported.h +1 -0

 	const struct format_type *fmt;
+	int pv_only;

Could this be changed to an enumerated "type" field, instead of pv_only? If I understand correctly, types are mutually exclusive anyway, and pv_only is rather confusing and non-obvious when reading the code.

Quoted 5 lines

lib/metadata/metadata-exported.h +1 -0

+
 	/*
 	 * Each mda in a vg is on exactly one of the below lists.
 	 * MDAs on the 'in_use' list will be read from / written to

@@ -181,6 +183,7 @@ struct format_instance {

lib/metadata/metadata-exported.h struct format_instance { · line 186

Quoted 4 lines

lib/metadata/metadata-exported.h +1 -0

 	 */
 	struct dm_list metadata_areas_in_use;
 	struct dm_list metadata_areas_ignored;
+	void *metadata_areas_index;

I don't currently expect that we would need as much genericity as void * offers (and that we could use a little extra safety). Would an union be better here, with an explicit list of types that can go in?

Quoted 6 lines

lib/metadata/metadata.c +5 -0

+static int _convert_key_to_string(const char *key, size_t key_len,
+				  unsigned subkey, char *buf, size_t buf_len)
 {
+	memcpy(buf, key, key_len);
+	buf += key_len;
+	buf_len -= key_len;

lib/metadata/metadata.c uint32_t vg_lock_newname(struct cmd_context *cmd, const char *vgname) · line 3926

Quoted 5 lines

lib/metadata/metadata.c +5 -0

+	if ((dm_snprintf(buf, buf_len, "_%u", subkey) == -1))
+		return_0;
+
+	return 1;
+}

Would it be better to require that key is NULL-terminated? It would simplify a lot of code, by not needing to pass the length as an extra

parameter everywhere. I know that currently we keep the PV/VG/... IDs in unterminated strings, but I am quite sure this is not a good idea. I wouldn't expect it to be a *lot* of work to change things to keep a NULL termination in there as well. Maybe in a followup patch?

Quoted 20 lines

lib/metadata/metadata.c +17 -0

+int fid_add_mda(struct format_instance *fid, struct metadata_area *mda,
+		 const char *key, size_t key_len, const unsigned subkey)
+{
+	char extended_key[PATH_MAX];
+
 	dm_list_add(mda_is_ignored(mda) ? &fid->metadata_areas_ignored :
 					  &fid->metadata_areas_in_use, &mda->list);
+
+	/*
+	 * Return if the mda is not supposed to be
+	 * indexed or the index itself is not initialised */
+	if (!key || !fid->metadata_areas_index)
+		return 1;
+
+	if (!_convert_key_to_string(key, key_len, subkey,
+				    extended_key, PATH_MAX))
+		return_0;
+
+	/* Add metadata area to index. */

lib/metadata/metadata.c uint32_t vg_lock_newname(struct cmd_context *cmd, const char *vgname) · line 3948

Quoted 2 lines

lib/metadata/metadata.c +2 -0

+	if (fid->pv_only)
+		((struct metadata_area **)fid->metadata_areas_index)[subkey] = mda;

In this branch, all the extended_key logic above is useless. It could be made more obvious that only the else branch needs extended_key, by restructuring the code. Also, is key, subkey and extended_key the right naming scheme? Maybe full_key instead of extended? And maybe sub_key, for consistency?

Quoted 10 lines

lib/metadata/metadata.c +10 -0

+struct metadata_area *fid_get_mda_indexed(struct format_instance *fid,
+					  const char *key, size_t key_len,
+					  const unsigned subkey)
+{
+	char extended_key[PATH_MAX];
+	struct metadata_area *mda = NULL;
+
+	if (!_convert_key_to_string(key, key_len, subkey,
+				    extended_key, PATH_MAX))
+		return_NULL;

lib/metadata/metadata.c uint32_t vg_lock_newname(struct cmd_context *cmd, const char *vgname) · line 3988

Quoted 6 lines

lib/metadata/metadata.c +6 -0

+
+	if (fid->pv_only)
+		mda = ((struct metadata_area **)fid->metadata_areas_index)[subkey];
+	else
+		mda = (struct metadata_area *) dm_hash_lookup(fid->metadata_areas_index,
+							      extended_key);

Same about extended_key as above (both path coverage and naming).

Quoted 24 lines

lib/metadata/metadata.c +24 -0

+int fid_remove_mda(struct format_instance *fid, struct metadata_area *mda,
+		   const char *key, size_t key_len, const unsigned subkey)
+{
+	struct metadata_area *mda_indexed = NULL;
+	char extended_key[PATH_MAX];
+
+	/* At least one of mda or key must be specified. */
+	if (!mda && !key)
+		return 1;
+
+	if (key) {
+		/*
+		 * If both mda and key specified, check given mda
+		 * with what we find using the index and return
+		 * immediately if these two do not match.
+		 */
+		if (!(mda_indexed = fid_get_mda_indexed(fid, key, key_len, subkey)) ||
+		     (mda && mda != mda_indexed))
+			return 1;
+
+		if (!_convert_key_to_string(key, key_len, subkey,
+					    extended_key, PATH_MAX))
+			return_0;
+

lib/metadata/metadata.c uint32_t vg_lock_newname(struct cmd_context *cmd, const char *vgname) · line 4021

Quoted 6 lines

lib/metadata/metadata.c +6 -0

+		mda = mda_indexed;
+
+		if (fid->pv_only)
+			((struct metadata_area**)fid->metadata_areas_index)[subkey] = NULL;
+		else
+			dm_hash_remove(fid->metadata_areas_index, extended_key);

Redundant code paths again.

Quoted 6 lines

lib/metadata/metadata-exported.h struct format_instance { · line 186

Quoted 1 line

lib/metadata/metadata-exported.h +1 -0

+	void *metadata_areas_index;

I don't currently expect that we would need as much genericity as void * offers (and that we could use a little extra safety). Would an union be better here, with an explicit list of types that can go in?

Well, my incentive here was to keep this for any future changes so the format-specific code can decide and use any type of index we can imagine. We have this "metadata.c" layer and then the "format-specific" layer underneath that. So adding any new format with a brand new indexing scheme that is fine-tuned and suitable for that one new format would require breaking the abstraction layer (so defining a new type of index used in new format in the "metadata.c" layer above). So my incentive was to make the index transparent for the "metadata.c" layer.

(I know, we have these "fid_{add,remove,get}_mda" functions that work directly with the index in "metadata.c" now, but that may eventually evolve into format-specific "fid" functions later. But since we have only one format that actually make real use of the format instance, I kept it in that metadata.c layer for now...)

But if we expect to use the same index scheme all the time for all possible formats, not changing it too much, then sure, we can use a union there... But I'd like to avoid too much editing in the future :)

Quoted 4 lines

parameter everywhere. I know that currently we keep the PV/VG/... IDs in unterminated strings, but I am quite sure this is not a good idea. I wouldn't expect it to be a *lot* of work to change things to keep a NULL termination in there as well. Maybe in a followup patch?

..thing with PV id is that we store that as a "struct id" which internally is an array of integeres... Hmm, so you want to put another array item with 0 at the end?

BTW, we also do this "convert_to_string" in the cache code... So yes, maybe it would be fine to clean this up somehow...

So PV has also a format instance from now on.
Also, for starters, let's create a new PV-based format_instance in pv_create code.

Signed-off-by Peter Rajnoha <[email protected]>

---
 lib/metadata/metadata.c |    7 +++++++
 lib/metadata/pv.h       |    1 +
 2 files changed, 8 insertions(+), 0 deletions(-)

lib/metadata/metadata.c +7 -0

index c3accaf..ed2084d 100644
@@ -1547,6 +1547,7 @@ static struct physical_volume *_alloc_pv(struct dm_pool *mem, struct device *dev
 	if (!pv)
 		return_NULL;
 
+	pv->fid = NULL;
 	pv->pe_size = 0;
 	pv->pe_start = 0;
 	pv->pe_count = 0;

@@ -1638,6 +1639,12 @@ struct physical_volume *pv_create(const struct cmd_context *cmd,
 		goto bad;
 	}
 
+	if (!(pv->fid = fmt->ops->create_instance(fmt, (const char *) &pv->id,
+						  NULL, NULL, NULL))) {
+		log_error("Couldn't create format instance for PV %s.", pv_dev_name(pv));
+		goto bad;
+	}
+
 	pv->fmt = fmt;
 	pv->vg_name = fmt->orphan_vg_name;
 

lib/metadata/pv.h +1 -0

index a02f6f8..a9efcfe 100644
@@ -24,6 +24,7 @@ struct physical_volume {
 	struct id id;
 	struct device *dev;
 	const struct format_type *fmt;
+	struct format_instance *fid;
 
 	/*
 	 * vg_name and vgid are used before the parent VG struct exists.

We don't need to store metadata areas in a separate "mdas" parameter since it is in the pv->fid now. If the fid is not defined yet and is NULL, pv_read code will create a new one (if there's no VG fid yet where the metadata areas read off of a PV should be attached to).

---
 lib/metadata/metadata.c |   26 +++++++++++++++++++++-----
 1 files changed, 21 insertions(+), 5 deletions(-)

lib/metadata/metadata.c +10 -4

index ed2084d..f6ea0e5 100644
@@ -37,6 +37,7 @@
 static struct physical_volume *_pv_read(struct cmd_context *cmd,
 					struct dm_pool *pvmem,
 					const char *pv_name,
+					struct format_instance *fid,
 					struct dm_list *mdas,
 					uint64_t *label_sector,
 					int warnings, int scan_label_only);

@@ -166,6 +167,7 @@ void add_pvl_to_vgs(struct volume_group *vg, struct pv_list *pvl)
 	dm_list_add(&vg->pvs, &pvl->list);
 	vg->pv_count++;
 	pvl->pv->vg = vg;
+	pvl->pv->fid = vg->fid;
 }
 
 void del_pvl_from_vgs(struct volume_group *vg, struct pv_list *pvl)

@@ -1813,7 +1815,7 @@ static struct physical_volume *_find_pv_by_name(struct cmd_context *cmd,
 	struct physical_volume *pv;
 
 	dm_list_init(&mdas);
-	if (!(pv = _pv_read(cmd, cmd->mem, pv_name, &mdas, NULL, 1, 0))) {
+	if (!(pv = _pv_read(cmd, cmd->mem, pv_name, NULL, &mdas, NULL, 1, 0))) {
 		log_error("Physical volume %s not found", pv_name);
 		return NULL;
 	}

@@ -1822,7 +1824,7 @@ static struct physical_volume *_find_pv_by_name(struct cmd_context *cmd,
 		/* If a PV has no MDAs - need to search all VGs for it */
 		if (!scan_vgs_for_pvs(cmd, 1))
 			return_NULL;
-		if (!(pv = _pv_read(cmd, cmd->mem, pv_name, NULL, NULL, 1, 0))) {
+		if (!(pv = _pv_read(cmd, cmd->mem, pv_name, NULL, NULL, NULL, 1, 0))) {
 			log_error("Physical volume %s not found", pv_name);
 			return NULL;
 		}

@@ -2644,7 +2646,8 @@ static struct volume_group *_vg_read_orphans(struct cmd_context *cmd,
 	}
 
 	dm_list_iterate_items(info, &vginfo->infos) {
-		if (!(pv = _pv_read(cmd, mem, dev_name(info->dev), NULL, NULL, warnings, 0))) {
+		if (!(pv = _pv_read(cmd, mem, dev_name(info->dev), vg->fid,
+				    NULL, NULL, warnings, 0))) {
 			continue;
 		}
 		if (!(pvl = dm_pool_zalloc(mem, sizeof(*pvl)))) {

@@ -3357,13 +3360,14 @@ struct physical_volume *pv_read(struct cmd_context *cmd, const char *pv_name,
 				struct dm_list *mdas, uint64_t *label_sector,
 				int warnings, int scan_label_only)
 {
-	return _pv_read(cmd, cmd->mem, pv_name, mdas, label_sector, warnings, scan_label_only);
+	return _pv_read(cmd, cmd->mem, pv_name, NULL, mdas, label_sector, warnings, scan_label_only);
 }
 
 /* FIXME Use label functions instead of PV functions */
 static struct physical_volume *_pv_read(struct cmd_context *cmd,
 					struct dm_pool *pvmem,
 					const char *pv_name,
+					struct format_instance *fid,
 					struct dm_list *mdas,
 					uint64_t *label_sector,
 					int warnings, int scan_label_only)

@@ -3407,6 +3411,18 @@ static struct physical_volume *_pv_read(struct cmd_context *cmd,
 	if (!alloc_pv_segment_whole_pv(pvmem, pv))
 		goto_bad;
 
+	if (fid)
+		fid_add_mdas(fid, &info->mdas, (const char *) &pv->id, ID_LEN);

Quoted 4 lines · Peter Rajnoha

We don't need to store metadata areas in a separate "mdas" parameter since it is in the pv->fid now. If the fid is not defined yet and is NULL, pv_read code will create a new one (if there's no VG fid yet where the metadata areas read off of a PV should be attached to).

This is somewhat hairy... With this patch, it's starting to seem that PV will sometimes point to a VG FID and other times to PV FID. Is there a good reason for that? Is the PV FID only used with orphan PVs?

I think this is something that really needs to be documented. If nothing

else, the format_instance type value (as suggested in an earlier comment) should read ORPHAN_PV_FID and VG_FID. It also does raise the

question, why we don't always use VG_FID even for orphans, given how we

have an orphans VG? Would simply using the orphans VG FID be more consistent overall?

Quoted 8 lines

lib/metadata/metadata.c +8 -0

+	if (fid)
+		fid_add_mdas(fid, &info->mdas, (const char *) &pv->id, ID_LEN);
+	else {
+		if (!(pv->fid = pv->fmt->ops->create_instance(pv->fmt,
+						(const char *) &pv->id,
+						NULL, NULL, NULL))) {
+			log_error("_pv_read: Couldn't create format instance "
+				  "for PV %s", pv_name);

lib/metadata/metadata.c static struct physical_volume *_pv_read(struct cmd_context *cmd, · line 3425

Quoted 4 lines

lib/metadata/metadata.c +4 -0

+			goto bad;
+		}
+	}
+

The code above is certainly confusing. A comment is needed, explaining what is going on. Please explain that non-NULL "fid" means that this PV belongs to a VG, and the fid variable points to that VG's fid.

Also, why mdas are added to the VG fid, but the (orphan) PV fid is left empty? Should the code read

if (!fid) {
    if (!(fid = pv->fid = ...
}

fid_add_mdas(fid, ...)

?

If, as I suspect, it's because create_instance adds the MDA's itself, please add a comment to that effect. (Also, is it necessary that create_instance does this? I am not sure it is entirely intuitive...)

It's because this _pv_read could be part of a vg_read (e.g. _vg_read_orphans).

If attach_existing_mdas parameter is set, we'll attach any existing metadata areas found in the cache to the format_instance. Otherwise, we will only initialise the format_instance and keep the metadata areas list empty (so we can fill that later if that's needed).

Signed-off-by Peter Rajnoha <[email protected]>

---
 lib/cache/lvmcache.c          |    2 +-
 lib/format1/format1.c         |    3 ++-
 lib/format_pool/format_pool.c |    3 ++-
 lib/format_text/archive.c     |    2 +-
 lib/format_text/archiver.c    |   10 ++++++----
 lib/format_text/format-text.c |   34 ++++++++++++++++++++--------------
 lib/metadata/metadata.c       |   27 ++++++++++++++++++++-------
 lib/metadata/metadata.h       |    4 +++-
 8 files changed, 55 insertions(+), 30 deletions(-)

lib/cache/lvmcache.c +1 -1

index 476b176..e3ac4f2 100644
@@ -654,7 +654,7 @@ struct volume_group *lvmcache_get_vg(const char *vgid, unsigned precommitted)
 
 	if (!(fid = vginfo->fmt->ops->create_instance(vginfo->fmt,
 						      NULL, vginfo->vgname,
-						      vgid, NULL)))
+						      vgid, NULL, 1)))
 		return_NULL;
 
 	/* Build config tree from vgmetadata, if not yet cached */

lib/format1/format1.c +2 -1

index ff5b0f5..b79bd56 100644
@@ -525,7 +525,8 @@ static struct format_instance *_format1_create_instance(const struct format_type
 						const char *pvid __attribute__((unused)),
 						const char *vgname __attribute__((unused)),
 						const char *vgid __attribute__((unused)),
-						void *private __attribute__((unused)))
+						void *private __attribute__((unused)),
+						int attach_existing_mdas __attribute((unused)))
 {
 	struct format_instance *fid;
 	struct metadata_area *mda;

lib/format_pool/format_pool.c +2 -1

index 814d80c..3ecbfe9 100644
@@ -252,7 +252,8 @@ static struct format_instance *_pool_create_instance(const struct format_type *f
 						const char *pvid __attribute__((unused)),
 						const char *vgname __attribute__((unused)),
 						const char *vgid __attribute__((unused)),
-						void *private __attribute__((unused)))
+						void *private __attribute__((unused)),
+						int attach_existing_mdas __attribute__((unused)))
 {
 	struct format_instance *fid;
 	struct metadata_area *mda;

lib/format_text/archive.c +1 -1

index 760153c..8d55988 100644
@@ -309,7 +309,7 @@ static void _display_archive(struct cmd_context *cmd, struct archive_file *af)
 
 	if (!(context = create_text_context(cmd, af->path, NULL)) ||
 	    !(tf = cmd->fmt_backup->ops->create_instance(cmd->fmt_backup, NULL,
-							 NULL, NULL, context))) {
+						NULL, NULL, context, 1))) {
 		log_error("Couldn't create text instance object.");
 		return;
 	}

lib/format_text/archiver.c +6 -4

index b7dcad9..ec65794 100644
@@ -278,7 +278,7 @@ struct volume_group *backup_read_vg(struct cmd_context *cmd,
 	if (!(context = create_text_context(cmd, file,
 					    cmd->cmd_line)) ||
 	    !(tf = cmd->fmt_backup->ops->create_instance(cmd->fmt_backup, NULL,
-							 NULL, NULL, context))) {
+						NULL, NULL, context, 1))) {
 		log_error("Couldn't create text format object.");
 		return NULL;
 	}

@@ -299,6 +299,7 @@ int backup_restore_vg(struct cmd_context *cmd, struct volume_group *vg)
 	struct pv_list *pvl;
 	struct physical_volume *pv;
 	struct lvmcache_info *info;
+	struct format_instance *fid;
 
 	/*
 	 * FIXME: Check that the PVs referenced in the backup are

@@ -306,11 +307,12 @@ int backup_restore_vg(struct cmd_context *cmd, struct volume_group *vg)
 	 */
 
 	/* Attempt to write out using currently active format */
-	if (!(vg->fid = cmd->fmt->ops->create_instance(cmd->fmt, NULL,
-						vg->name, NULL, NULL))) {
+	if (!(fid = cmd->fmt->ops->create_instance(cmd->fmt, NULL,
+						vg->name, NULL, NULL, 1))) {
 		log_error("Failed to allocate format instance");
 		return 0;
 	}
+	change_vg_format_instance(vg, fid);
 
 	/*
 	 * Setting vg->old_name to a blank value will explicitly

@@ -398,7 +400,7 @@ int backup_to_file(const char *file, const char *desc, struct volume_group *vg)
 
 	if (!(context = create_text_context(cmd, file, desc)) ||
 	    !(tf = cmd->fmt_backup->ops->create_instance(cmd->fmt_backup, NULL,
-							 NULL, NULL, context))) {
+						NULL, NULL, context, 1))) {
 		log_error("Couldn't create backup object.");
 		return 0;
 	}

lib/format_text/format-text.c +20 -14

index c8c56f7..a2f3d74 100644
@@ -41,7 +41,8 @@ static struct format_instance *_text_create_text_instance(const struct format_ty
 							  const char *pvid,
 							  const char *vgname,
 							  const char *vgid,
-							  void *context);
+							  void *context,
+							  int attach_existing_mdas);
 
 struct text_fid_context {
 	char *raw_metadata_buf;

@@ -1090,7 +1091,7 @@ static int _scan_file(const struct format_type *fmt, const char *vgname)
 
 				/* FIXME stat file to see if it's changed */
 				fid = _text_create_text_instance(fmt, NULL, NULL,
-								 NULL, NULL);
+								 NULL, NULL, 1);
 				if ((vg = _vg_read_file_name(fid, scanned_vgname,
 							     path))) {
 					/* FIXME Store creation host in vg */

@@ -1916,7 +1917,8 @@ static int _text_pv_setup(const struct format_type *fmt,
 	return 1;
 }
 
-static int _create_pv_text_instance(struct format_instance *fid, const char *pvid)
+static int _create_pv_text_instance(struct format_instance *fid, const char *pvid,
+				    int attach_existing_mdas)
 {
 	struct text_fid_pv_context *fid_pv_tc;
 	struct lvmcache_info *info;

@@ -1938,7 +1940,7 @@ static int _create_pv_text_instance(struct format_instance *fid, const char *pvi
 		return 0;
 	}
 
-	if ((info = info_from_pvid(pvid, 0)))
+	if (attach_existing_mdas && (info = info_from_pvid(pvid, 0)))
 		fid_add_mdas(fid, &info->mdas, pvid, ID_LEN);
 
 	return 1;

@@ -1946,7 +1948,7 @@ static int _create_pv_text_instance(struct format_instance *fid, const char *pvi
 
 static int _create_vg_text_instance(struct format_instance *fid,
 				    const char *vgname, const char *vgid,
-				    void *context)
+				    void *context, int attach_existing_mdas)
 {
 	struct text_fid_context *fidtc;
 	struct metadata_area *mda;

@@ -1969,6 +1971,15 @@ static int _create_vg_text_instance(struct format_instance *fid,
 	fidtc->raw_metadata_buf = NULL;
 	fid->private = (void *) fidtc;
 
+	if (!(fid->metadata_areas_index = dm_hash_create(128))) {
+		log_error("Couldn't create metadata index for format "
+			  "instance of VG %s.", vgname);
+		return 0;
+	}
+
+	if (!attach_existing_mdas)
+		goto out;
+
 	if (!vgname) {
 		if (!(mda = dm_pool_zalloc(fid->fmt->cmd->mem, sizeof(*mda))))
 			return_0;

@@ -1977,12 +1988,6 @@ static int _create_vg_text_instance(struct format_instance *fid,
 		mda->status = 0;
 		fid_add_mda(fid, mda, NULL, 0, 0);
 	} else {
-		if (!(fid->metadata_areas_index = dm_hash_create(128))) {
-			log_error("Couldn't create metadata index for format "
-				  "instance of VG %s.", vgname);
-			return 0;
-		}
-
 		dir_list = &((struct mda_lists *) fid->fmt->private)->dirs;
 
 		dm_list_iterate_items(dl, dir_list) {

@@ -2044,7 +2049,8 @@ static struct format_instance *_text_create_text_instance(const struct format_ty
 							  const char *pvid,
 							  const char *vgname,
 							  const char *vgid,
-							  void *context)
+							  void *context,
+							  int attach_existing_mdas)
 {
 	struct format_instance *fid;
 	int r;

@@ -2065,8 +2071,8 @@ static struct format_instance *_text_create_text_instance(const struct format_ty
 	dm_list_init(&fid->metadata_areas_in_use);
 	dm_list_init(&fid->metadata_areas_ignored);
 
-	r = pvid ? _create_pv_text_instance(fid, pvid) :
-		   _create_vg_text_instance(fid, vgname, vgid, context);
+	r = pvid ? _create_pv_text_instance(fid, pvid, attach_existing_mdas) :
+		   _create_vg_text_instance(fid, vgname, vgid, context, attach_existing_mdas);
 
 	if (r)
 		return fid;

lib/metadata/metadata.c +20 -7

index f6ea0e5..dccc3af 100644
@@ -977,7 +977,7 @@ struct volume_group *vg_create(struct cmd_context *cmd, const char *vg_name)
 	dm_list_init(&vg->removed_pvs);
 
 	if (!(vg->fid = cmd->fmt->ops->create_instance(cmd->fmt, NULL,
-						vg_name, NULL, NULL))) {
+					vg_name, NULL, NULL, 1))) {
 		log_error("Failed to create format instance");
 		goto bad;
 	}

@@ -1642,7 +1642,7 @@ struct physical_volume *pv_create(const struct cmd_context *cmd,
 	}
 
 	if (!(pv->fid = fmt->ops->create_instance(fmt, (const char *) &pv->id,
-						  NULL, NULL, NULL))) {
+						  NULL, NULL, NULL, 0))) {
 		log_error("Couldn't create format instance for PV %s.", pv_dev_name(pv));
 		goto bad;
 	}

@@ -2637,10 +2637,13 @@ static struct volume_group *_vg_read_orphans(struct cmd_context *cmd,
 		goto bad;
 	}
 
-	/* create format instance with appropriate metadata area */
+	/*
+	 * Create an empty format instance. Metadata areas
+	 * will be attached using consequent PV read calls.
+	 */
 	if (!(vg->fid = vginfo->fmt->ops->create_instance(vginfo->fmt, NULL,
 							  orphan_vgname, NULL,
-							  NULL))) {
+							  NULL, 0))) {
 		log_error("Failed to create format instance");
 		goto bad;
 	}

@@ -2818,7 +2821,7 @@ static struct volume_group *_vg_read(struct cmd_context *cmd,
 		use_precommitted = 0;
 
 	/* create format instance with appropriate metadata area */
-	if (!(fid = fmt->ops->create_instance(fmt, NULL, vgname, vgid, NULL))) {
+	if (!(fid = fmt->ops->create_instance(fmt, NULL, vgname, vgid, NULL, 1))) {
 		log_error("Failed to create format instance");
 		return NULL;
 	}

@@ -2973,7 +2976,7 @@ static struct volume_group *_vg_read(struct cmd_context *cmd,
 			use_precommitted = 0;
 
 		/* create format instance with appropriate metadata area */
-		if (!(fid = fmt->ops->create_instance(fmt, NULL, vgname, vgid, NULL))) {
+		if (!(fid = fmt->ops->create_instance(fmt, NULL, vgname, vgid, NULL, 1))) {
 			log_error("Failed to create format instance");
 			return NULL;
 		}

@@ -3416,7 +3419,7 @@ static struct physical_volume *_pv_read(struct cmd_context *cmd,
 	else {
 		if (!(pv->fid = pv->fmt->ops->create_instance(pv->fmt,
 						(const char *) &pv->id,
-						NULL, NULL, NULL))) {
+						NULL, NULL, NULL, 1))) {
 			log_error("_pv_read: Couldn't create format instance "
 				  "for PV %s", pv_name);
 			goto bad;

@@ -3936,6 +3939,16 @@ uint32_t vg_lock_newname(struct cmd_context *cmd, const char *vgname)
 	return FAILED_EXIST;
 }
 
+void change_vg_format_instance(struct volume_group *vg,
+			       struct format_instance *fid)
+{
+	struct pv_list *pvl;
+
+	vg->fid = fid;
+	dm_list_iterate_items(pvl, &vg->pvs)
+		pvl->pv->fid = fid;
+}
+
 static int _convert_key_to_string(const char *key, size_t key_len,
 				  unsigned subkey, char *buf, size_t buf_len)
 {

lib/metadata/metadata.h +3 -1

index 0fbc954..bd638b5 100644
@@ -191,6 +191,7 @@ struct metadata_area *mda_copy(struct dm_pool *mem,
 unsigned mda_is_ignored(struct metadata_area *mda);
 void mda_set_ignored(struct metadata_area *mda, unsigned ignored);
 unsigned mda_locns_match(struct metadata_area *mda1, struct metadata_area *mda2);
+void change_vg_format_instance(struct volume_group *vg, struct format_instance *fid);
 int fid_add_mda(struct format_instance *fid, struct metadata_area *mda,
 		 const char *key, size_t key_len, const unsigned subkey);
 int fid_add_mdas(struct format_instance *fid, struct dm_list *mdas,

@@ -289,7 +290,8 @@ struct format_handler {
 						    const char *pvid,
 						    const char *vgname,
 						    const char *vgid,
-						    void *context);
+						    void *context,
+						    int attach_existing_mdas);
 
 	/*
 	 * Destructor for format instance

Quoted 4 lines · Peter Rajnoha

If attach_existing_mdas parameter is set, we'll attach any existing metadata areas found in the cache to the format_instance. Otherwise, we will only initialise the format_instance and keep the metadata areas list empty (so we can fill that later if that's needed).

As hinted in previous comment, I think it would be better to never attach existing MDAs in create_instance and simply have a single call to do that when it is needed. create_instance parameter list is already a mess...

lib/format_text/format-text.c static struct format_instance *_text_create_text_instance(const struct format_ty · line 46

Quoted 8 lines

lib/format_text/format-text.c +2 -1

@@ -41,7 +41,8 @@ static struct format_instance *_text_create_text_instance(const struct format_ty
 							  const char *pvid,
 							  const char *vgname,
 							  const char *vgid,
-							  void *context);
+							  void *context,
+							  int attach_existing_mdas);
 

...metadata areas are now stored within pv->fid.

Signed-off-by Peter Rajnoha <[email protected]>

---
 lib/format1/format1.c            |    3 +--
 lib/format_pool/format_pool.c    |    1 -
 lib/format_text/format-text.c    |   15 +--------------
 lib/metadata/metadata-exported.h |    4 ++--
 lib/metadata/metadata.c          |   37 +++++++++++++------------------------
 lib/metadata/metadata.h          |    3 +--
 tools/pvremove.c                 |   10 ++++------
 tools/pvresize.c                 |    8 +++-----
 tools/toollib.c                  |   14 ++++++--------
 9 files changed, 31 insertions(+), 64 deletions(-)

lib/format1/format1.c +1 -2

index b79bd56..4f66cdc 100644
@@ -331,8 +331,7 @@ static int _format1_vg_write(struct format_instance *fid, struct volume_group *v
 }
 
 static int _format1_pv_read(const struct format_type *fmt, const char *pv_name,
-		    struct physical_volume *pv, struct dm_list *mdas __attribute__((unused)),
-		    int scan_label_only __attribute__((unused)))
+		    struct physical_volume *pv, int scan_label_only __attribute__((unused)))
 {
 	struct dm_pool *mem = dm_pool_create("lvm1 pv_read", 1024);
 	struct disk_list *dl;

lib/format_pool/format_pool.c +0 -1

index 3ecbfe9..24ccfc2 100644
@@ -206,7 +206,6 @@ static int _pool_pv_setup(const struct format_type *fmt __attribute__((unused)),
 
 static int _pool_pv_read(const struct format_type *fmt, const char *pv_name,
 			 struct physical_volume *pv,
-			 struct dm_list *mdas __attribute__((unused)),
 			 int scan_label_only __attribute__((unused)))
 {
 	struct dm_pool *mem = dm_pool_create("pool pv_read", 1024);

lib/format_text/format-text.c +1 -14

index a2f3d74..5dfe0b8 100644
@@ -1669,10 +1669,8 @@ static uint64_t _metadata_locn_offset_raw(void *metadata_locn)
 }
 
 static int _text_pv_read(const struct format_type *fmt, const char *pv_name,
-		    struct physical_volume *pv, struct dm_list *mdas,
-		    int scan_label_only)
+		    struct physical_volume *pv, int scan_label_only)
 {
-	struct metadata_area *mda, *mda_new;
 	struct label *label;
 	struct device *dev;
 	struct lvmcache_info *info;

@@ -1687,17 +1685,6 @@ static int _text_pv_read(const struct format_type *fmt, const char *pv_name,
 	if (!_populate_pv_fields(info, pv, scan_label_only))
 		return 0;
 
-	if (!mdas)
-		return 1;
-
-	/* Add copy of mdas to supplied list */
-	dm_list_iterate_items(mda, &info->mdas) {
-		mda_new = mda_copy(fmt->cmd->mem, mda);
-		if (!mda_new)
-			return 0;
-		dm_list_add(mdas, &mda_new->list);
-	}
-
 	return 1;
 }
 

lib/metadata/metadata-exported.h +2 -2

index d8247dd..da62764 100644
@@ -340,8 +340,8 @@ int vg_revert(struct volume_group *vg);
 struct volume_group *vg_read_internal(struct cmd_context *cmd, const char *vg_name,
 			     const char *vgid, int warnings, int *consistent);
 struct physical_volume *pv_read(struct cmd_context *cmd, const char *pv_name,
-				struct dm_list *mdas, uint64_t *label_sector,
-				int warnings, int scan_label_only);
+				uint64_t *label_sector, int warnings,
+				int scan_label_only);
 struct dm_list *get_pvs(struct cmd_context *cmd);
 
 /*

lib/metadata/metadata.c +13 -24

index dccc3af..591f7b5 100644
@@ -38,7 +38,6 @@ static struct physical_volume *_pv_read(struct cmd_context *cmd,
 					struct dm_pool *pvmem,
 					const char *pv_name,
 					struct format_instance *fid,
-					struct dm_list *mdas,
 					uint64_t *label_sector,
 					int warnings, int scan_label_only);
 

@@ -1330,14 +1329,11 @@ static int pvcreate_check(struct cmd_context *cmd, const char *name,
 {
 	struct physical_volume *pv;
 	struct device *dev;
-	struct dm_list mdas;
-
-	dm_list_init(&mdas);
 
 	/* FIXME Check partition type is LVM unless --force is given */
 
 	/* Is there a pv here already? */
-	pv = pv_read(cmd, name, &mdas, NULL, 0, 0);
+	pv = pv_read(cmd, name, NULL, 0, 0);
 
 	/*
 	 * If a PV has no MDAs it may appear to be an orphan until the

@@ -1345,10 +1341,10 @@ static int pvcreate_check(struct cmd_context *cmd, const char *name,
 	 * this means checking every VG by scanning every PV on the
 	 * system.
 	 */
-	if (pv && is_orphan(pv) && mdas_empty_or_ignored(&mdas)) {
+	if (pv && is_orphan(pv) && !dm_list_size(&pv->fid->metadata_areas_in_use)) {
 		if (!scan_vgs_for_pvs(cmd, 0))
 			return_0;
-		pv = pv_read(cmd, name, NULL, NULL, 0, 0);
+		pv = pv_read(cmd, name, NULL, 0, 0);
 	}
 
 	/* Allow partial & exported VGs to be destroyed. */

@@ -1811,20 +1807,18 @@ struct physical_volume *find_pv_by_name(struct cmd_context *cmd,
 static struct physical_volume *_find_pv_by_name(struct cmd_context *cmd,
 			 			const char *pv_name)
 {
-	struct dm_list mdas;
 	struct physical_volume *pv;
 
-	dm_list_init(&mdas);
-	if (!(pv = _pv_read(cmd, cmd->mem, pv_name, NULL, &mdas, NULL, 1, 0))) {
+	if (!(pv = _pv_read(cmd, cmd->mem, pv_name, NULL, NULL, 1, 0))) {
 		log_error("Physical volume %s not found", pv_name);
 		return NULL;
 	}
 
-	if (is_orphan_vg(pv->vg_name) && mdas_empty_or_ignored(&mdas)) {
+	if (is_orphan_vg(pv->vg_name) && !dm_list_size(&pv->fid->metadata_areas_in_use)) {
 		/* If a PV has no MDAs - need to search all VGs for it */
 		if (!scan_vgs_for_pvs(cmd, 1))
 			return_NULL;
-		if (!(pv = _pv_read(cmd, cmd->mem, pv_name, NULL, NULL, NULL, 1, 0))) {
+		if (!(pv = _pv_read(cmd, cmd->mem, pv_name, NULL, NULL, 1, 0))) {
 			log_error("Physical volume %s not found", pv_name);
 			return NULL;
 		}

@@ -2649,8 +2643,8 @@ static struct volume_group *_vg_read_orphans(struct cmd_context *cmd,
 	}
 
 	dm_list_iterate_items(info, &vginfo->infos) {
-		if (!(pv = _pv_read(cmd, mem, dev_name(info->dev), vg->fid,
-				    NULL, NULL, warnings, 0))) {
+		if (!(pv = _pv_read(cmd, mem, dev_name(info->dev),
+				    vg->fid, NULL, warnings, 0))) {
 			continue;
 		}
 		if (!(pvl = dm_pool_zalloc(mem, sizeof(*pvl)))) {

@@ -3360,10 +3354,10 @@ const char *find_vgname_from_pvname(struct cmd_context *cmd,
  *   FIXME - liblvm todo - make into function that returns handle
  */
 struct physical_volume *pv_read(struct cmd_context *cmd, const char *pv_name,
-				struct dm_list *mdas, uint64_t *label_sector,
-				int warnings, int scan_label_only)
+				uint64_t *label_sector, int warnings,
+				int scan_label_only)
 {
-	return _pv_read(cmd, cmd->mem, pv_name, NULL, mdas, label_sector, warnings, scan_label_only);
+	return _pv_read(cmd, cmd->mem, pv_name, NULL, label_sector, warnings, scan_label_only);
 }
 
 /* FIXME Use label functions instead of PV functions */

@@ -3371,7 +3365,6 @@ static struct physical_volume *_pv_read(struct cmd_context *cmd,
 					struct dm_pool *pvmem,
 					const char *pv_name,
 					struct format_instance *fid,
-					struct dm_list *mdas,
 					uint64_t *label_sector,
 					int warnings, int scan_label_only)
 {

@@ -3401,8 +3394,7 @@ static struct physical_volume *_pv_read(struct cmd_context *cmd,
 	}
 
 	/* FIXME Move more common code up here */
-	if (!(info->fmt->ops->pv_read(info->fmt, pv_name, pv, mdas,
-	      scan_label_only))) {
+	if (!(info->fmt->ops->pv_read(info->fmt, pv_name, pv, scan_label_only))) {
 		log_error("Failed to read existing physical volume '%s'",
 			  pv_name);
 		goto bad;

@@ -4239,8 +4231,5 @@ char *tags_format_and_copy(struct dm_pool *mem, const struct dm_list *tags)
  */
 struct physical_volume *pv_by_path(struct cmd_context *cmd, const char *pv_name)
 {
-	struct dm_list mdas;
-
-	dm_list_init(&mdas);
-	return _pv_read(cmd, cmd->mem, pv_name, NULL, &mdas, NULL, 1, 0);
+	return _pv_read(cmd, cmd->mem, pv_name, NULL, NULL, 1, 0);
 }

lib/metadata/metadata.h +1 -2

index bd638b5..7ebf85c 100644
@@ -241,8 +241,7 @@ struct format_handler {
 	 * Return PV with given path.
 	 */
 	int (*pv_read) (const struct format_type * fmt, const char *pv_name,
-			struct physical_volume * pv, struct dm_list *mdas,
-			int scan_label_only);
+			struct physical_volume * pv, int scan_label_only);
 
 	/*
 	 * Tweak an already filled out a pv ready for importing into a

tools/pvremove.c +4 -6

index a8717e0..ace3c1d 100644
@@ -25,15 +25,12 @@ const char _really_wipe[] =
 static int pvremove_check(struct cmd_context *cmd, const char *name)
 {
 	struct physical_volume *pv;
-	struct dm_list mdas;
-
-	dm_list_init(&mdas);
 
 	/* FIXME Check partition type is LVM unless --force is given */
 
 	/* Is there a pv here already? */
 	/* If not, this is an error unless you used -f. */
-	if (!(pv = pv_read(cmd, name, &mdas, NULL, 1, 0))) {
+	if (!(pv = pv_read(cmd, name, NULL, 1, 0))) {
 		if (arg_count(cmd, force_ARG))
 			return 1;
 		log_error("Physical Volume %s not found", name);

@@ -47,13 +44,14 @@ static int pvremove_check(struct cmd_context *cmd, const char *name)
 	 * means checking every VG by scanning every
 	 * PV on the system.
 	 */
-	if (is_orphan(pv) && !dm_list_size(&mdas)) {
+	if (is_orphan(pv) && !dm_list_size(&pv->fid->metadata_areas_in_use) &&
+	    !dm_list_size(&pv->fid->metadata_areas_ignored)) {
 		if (!scan_vgs_for_pvs(cmd, 0)) {
 			log_error("Rescan for PVs without metadata areas "
 				  "failed.");
 			return 0;
 		}
-		if (!(pv = pv_read(cmd, name, NULL, NULL, 1, 0))) {
+		if (!(pv = pv_read(cmd, name, NULL, 1, 0))) {
 			log_error("Failed to read physical volume %s", name);
 			return 0;
 		}

tools/pvresize.c +3 -5

index 8582ef4..4f1d610 100644
@@ -32,28 +32,26 @@ static int _pv_resize_single(struct cmd_context *cmd,
 	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, 1, 0))) {
 			unlock_vg(cmd, vg_name);
 			log_error("Unable to read PV \"%s\"", pv_name);
 			return 0;
 		}
 
-		mda_count = dm_list_size(&mdas);
+		mda_count = dm_list_size(&pv->fid->metadata_areas_in_use) +
+			    dm_list_size(&pv->fid->metadata_areas_ignored);
 	} else {
 		vg = vg_read_for_update(cmd, vg_name, NULL, 0);
 

tools/toollib.c +6 -8

index f76aacd..48f8ed5 100644
@@ -640,7 +640,7 @@ static int _process_all_devs(struct cmd_context *cmd, void *handle,
 	}
 
 	while ((dev = dev_iter_get(iter))) {
-		if (!(pv = pv_read(cmd, dev_name(dev), NULL, NULL, 0, 0))) {
+		if (!(pv = pv_read(cmd, dev_name(dev), NULL, 0, 0))) {
 			memset(&pv_dummy, 0, sizeof(pv_dummy));
 			dm_list_init(&pv_dummy.tags);
 			dm_list_init(&pv_dummy.segments);

@@ -682,7 +682,6 @@ int process_each_pv(struct cmd_context *cmd, int argc, char **argv,
 	struct str_list *sll;
 	char *at_sign, *tagname;
 	int scanned = 0;
-	struct dm_list mdas;
 
 	dm_list_init(&tags);
 

@@ -724,10 +723,8 @@ int process_each_pv(struct cmd_context *cmd, int argc, char **argv,
 				}
 				pv = pvl->pv;
 			} else {
-
-				dm_list_init(&mdas);
-				if (!(pv = pv_read(cmd, argv[opt], &mdas,
-						   NULL, 1, scan_label_only))) {
+				if (!(pv = pv_read(cmd, argv[opt], NULL,
+						   1, scan_label_only))) {
 					log_error("Failed to read physical "
 						  "volume \"%s\"", argv[opt]);
 					ret_max = ECMD_FAILED;

@@ -742,7 +739,8 @@ int process_each_pv(struct cmd_context *cmd, int argc, char **argv,
 				 * PV on the system.
 				 */
 				if (!scanned && is_orphan(pv) &&
-				    !dm_list_size(&mdas)) {
+				    !dm_list_size(&pv->fid->metadata_areas_in_use) &&
+				    !dm_list_size(&pv->fid->metadata_areas_ignored)) {
 					if (!scan_label_only &&
 					    !scan_vgs_for_pvs(cmd, 1)) {
 						stack;

@@ -751,7 +749,7 @@ int process_each_pv(struct cmd_context *cmd, int argc, char **argv,
 					}
 					scanned = 1;
 					if (!(pv = pv_read(cmd, argv[opt],
-							   NULL, NULL, 1,
+							   NULL, 1,
 							   scan_label_only))) {
 						log_error("Failed to read "
 							  "physical volume "

My previous version of the patch used the cache and that was not quite a happy solution (but it was easy to use :)) since any revert would be quite complex (or we'd need to destroy the cache item and rescan).

Now, we can use the format_instance to store metadata areas. If anything goes wrong, we can just throw the instance away and create a new one to work with (so we don't need to revert the cache or rescan).

Signed-off-by Peter Rajnoha <[email protected]>

---
 lib/format_text/format-text.c |  245 +++++++++++++++++++++++++++++++++++++++++
 lib/format_text/format-text.h |    5 +
 lib/metadata/metadata.h       |   10 ++
 3 files changed, 260 insertions(+), 0 deletions(-)

lib/format_text/format-text.c +229 -0

index 5dfe0b8..cf18f25 100644
@@ -2031,6 +2031,250 @@ static int _create_vg_text_instance(struct format_instance *fid,
 	return 1;
 }
 
+int add_metadata_area_to_pv(struct physical_volume *pv,
+			    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 *) pv->fmt->private;
+
+	if (mda_index >= FMT_TEXT_MAX_MDAS_PER_PV) {
+		log_error(INTERNAL_ERROR "can't add metadata area with "
+					 "index %u to PV %s. Metadata "
+					 "layout not supported by %s format.",
+					  mda_index, dev_name(pv->dev),
+					  pv->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 = pv->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);
+
+	fid_add_mda(pv->fid ? pv->fid : pv->vg->fid, mda,
+		    (char *) &pv->id, ID_LEN, mda_index);
+
+	return 1;
+}
+
+static int _text_pv_add_metadata_area(const struct format_type *fmt,
+				      struct physical_volume *pv,
+				      int pe_start_locked,
+				      unsigned mda_index,
+				      uint64_t mda_size,
+				      unsigned mda_ignored)
+{
+	struct format_instance *fid = pv->fid;
+	const char *pvid = (char *) &pv->id;
+	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 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 (fid_get_mda_indexed(fid, pvid, ID_LEN, 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 (pe_start_locked)
+			limit = pe_start;
+		else if ((mda = fid_get_mda_indexed(fid, pvid, ID_LEN, 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),
+				  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 (!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 (!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 (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 = fid_get_mda_indexed(fid, pvid, ID_LEN, 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;
+		}
+
+		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;
+		}