[PATCH 0/9 v2] Fix lv_count & max_lv problems in lvm2

[PATCH 0/9 v2] Fix lv_count & max_lv problems in lvm2

[Milan Broz]

(reposted with some fixed thisng after review)

LVM2 allows setting of maximal logical count in metadata.

Logically, the maximal count of LVs should be count of user-visible LVs.

Curently there are several problems:
 - we have several variables and code paths counts visible/diplayable LVs
   not consistently

 - some code paths violates these restrictions

 - there are situations when code allows creating new LV, but then
   during metadata loading the VG is not read (because of violating limits)

   Only possible fix if this happen is to restore metadata from backup
   an manualy edit max_lv (or remove some volume).
   (vgchange -l doesn't work too here)

This patchset
 - rearanges code to be cleaner
 - removes lv_count counter and use function to do that (simplify code)
 - forces max_lv count only for new volume creation
 (per previous discussion we allow temporarily violate max_lv count)

Milan Broz (9):
  Fix snapshot segment import to not use duplicate segments & replace.
  Remove vg->lv_count and use counter function.
  Tidy format1 import LV function.
  Introduce vg_link_lv and vg_unlink_lv functions.
  Fix lv_is_visible to handle virtual origin.
  Introduce lv_set_visible & lv_set_hidden and use lv_is_visible
    always.
  Merge lv_is_displayable and lv_is_visible.
  Remove unneeded import parameter from lv_create_empty.
  Check max_lv on only place and force the check only for new volume.

 lib/activate/activate.c          |    6 +-
 lib/display/display.c            |    4 +-
 lib/format1/disk-rep.h           |    3 +-
 lib/format1/import-export.c      |   30 +++++-----
 lib/format_pool/format_pool.c    |    1 -
 lib/format_pool/import_export.c  |   16 +----
 lib/format_text/export.c         |    4 +-
 lib/format_text/import_vsn1.c    |   22 +------
 lib/metadata/lv_manip.c          |  130 ++++++++++++++++++++++++--------------
 lib/metadata/metadata-exported.h |   23 ++++---
 lib/metadata/metadata.c          |   76 ++++++++++++++--------
 lib/metadata/metadata.h          |    5 --
 lib/metadata/mirror.c            |   16 +++---
 lib/metadata/snapshot_manip.c    |   73 ++++++++++-----------
 lib/report/report.c              |    4 +-
 lib/snapshot/snapshot.c          |    6 +--
 test/t-lvcreate-usage.sh         |   23 +++++++-
 test/test-utils.sh               |    1 +
 tools/lvchange.c                 |    2 +-
 tools/lvconvert.c                |    3 +-
 tools/lvcreate.c                 |   12 +++-
 tools/lvdisplay.c                |    2 +-
 tools/lvscan.c                   |    2 +-
 tools/pvmove.c                   |    2 +-
 tools/reporter.c                 |    4 +-
 tools/vgchange.c                 |    4 +-
 tools/vgmerge.c                  |    1 -
 tools/vgsplit.c                  |    6 +--
 28 files changed, 262 insertions(+), 219 deletions(-)

[PATCH 1/9] Fix snapshot segment import to not use duplicate segments & replace.

[Milan Broz]

The snapshot segment (snapshotX) is created twice
during the text metadata segment processing.

This can cause temporary violation of max_lv count.

Simplify the code, snapshot segment is properly initialized
in init_snapshot_seg function now and do not need to be replaced
by vg_add_snapshot call.

The vg_add_snapshot() is now usefull only for adding new
snapshot and it shares the same initialization function.

The snapshot name is always generated, name paramater can be
removed from function call.

Signed-off-by: Milan Broz <mbroz@redhat.com>
---
 lib/format1/import-export.c      |    2 +-
 lib/format_text/import_vsn1.c    |   10 -------
 lib/metadata/metadata-exported.h |    6 +++-
 lib/metadata/snapshot_manip.c    |   55 ++++++++++++++++++++------------------
 lib/snapshot/snapshot.c          |    6 +---
 tools/lvconvert.c                |    3 +-
 tools/lvcreate.c                 |    2 +-
 7 files changed, 37 insertions(+), 47 deletions(-)

diff --git a/lib/format1/import-export.c b/lib/format1/import-export.c
index a950f9a..5d2e86a 100644
--- a/lib/format1/import-export.c
+++ b/lib/format1/import-export.c
@@ -616,7 +616,7 @@ int import_snapshots(struct dm_pool *mem __attribute((unused)), struct volume_gr
 				continue;
 
 			/* insert the snapshot */
-			if (!vg_add_snapshot(NULL, org, cow, NULL,
+			if (!vg_add_snapshot(org, cow, NULL,
 					     org->le_count,
 					     lvd->lv_chunk_size)) {
 				log_err("Couldn't add snapshot.");
diff --git a/lib/format_text/import_vsn1.c b/lib/format_text/import_vsn1.c
index 0bb9843..a9748aa 100644
--- a/lib/format_text/import_vsn1.c
+++ b/lib/format_text/import_vsn1.c
@@ -603,16 +603,6 @@ static int _read_lvsegs(struct format_instance *fid __attribute((unused)),
 
 	lv->size = (uint64_t) lv->le_count * (uint64_t) vg->extent_size;
 
-	/*
-	 * FIXME We now have 2 LVs for each snapshot. The real one was
-	 * created by vg_add_snapshot from the segment text_import.
-	 */
-	if (lv->status & SNAPSHOT) {
-		vg->lv_count--;
-		dm_list_del(&lvl->list);
-		return 1;
-	}
-
 	lv->minor = -1;
 	if ((lv->status & FIXED_MINOR) &&
 	    !_read_int32(lvn, "minor", &lv->minor)) {
diff --git a/lib/metadata/metadata-exported.h b/lib/metadata/metadata-exported.h
index 9bd3389..6c32ab4 100644
--- a/lib/metadata/metadata-exported.h
+++ b/lib/metadata/metadata-exported.h
@@ -545,8 +545,10 @@ struct lv_segment *find_cow(const struct logical_volume *lv);
 /* Given a cow LV, return its origin */
 struct logical_volume *origin_from_cow(const struct logical_volume *lv);
 
-int vg_add_snapshot(const char *name,
-		    struct logical_volume *origin, struct logical_volume *cow,
+void init_snapshot_seg(struct lv_segment *seg, struct logical_volume *origin,
+		       struct logical_volume *cow, uint32_t chunk_size);
+
+int vg_add_snapshot(struct logical_volume *origin, struct logical_volume *cow,
 		    union lvid *lvid, uint32_t extent_count,
 		    uint32_t chunk_size);
 
diff --git a/lib/metadata/snapshot_manip.c b/lib/metadata/snapshot_manip.c
index 6aa29a0..9364b4f 100644
--- a/lib/metadata/snapshot_manip.c
+++ b/lib/metadata/snapshot_manip.c
@@ -62,7 +62,31 @@ struct logical_volume *origin_from_cow(const struct logical_volume *lv)
 	return lv->snapshot->origin;
 }
 
-int vg_add_snapshot(const char *name, struct logical_volume *origin,
+void init_snapshot_seg(struct lv_segment *seg, struct logical_volume *origin,
+		       struct logical_volume *cow, uint32_t chunk_size)
+{
+	seg->chunk_size = chunk_size;
+	seg->origin = origin;
+	seg->cow = cow;
+
+	// FIXME: direct count manipulation to be removed later
+	cow->status &= ~VISIBLE_LV;
+	cow->vg->lv_count--;
+	cow->snapshot = seg;
+
+	origin->origin_count++;
+	origin->vg->lv_count--;
+
+	/* FIXME Assumes an invisible origin belongs to a sparse device */
+	if (!lv_is_visible(origin))
+		origin->status |= VIRTUAL_ORIGIN;
+
+	seg->lv->status |= (SNAPSHOT | VIRTUAL);
+
+	dm_list_add(&origin->snapshot_segs, &seg->origin_list);
+}
+
+int vg_add_snapshot(struct logical_volume *origin,
 		    struct logical_volume *cow, union lvid *lvid,
 		    uint32_t extent_count, uint32_t chunk_size)
 {
@@ -82,39 +106,18 @@ int vg_add_snapshot(const char *name, struct logical_volume *origin,
 		return 0;
 	}
 
-	/*
-	 * Set origin lv count in advance to prevent fail because
-	 * of temporary violation of LV limits.
-	 */
-	origin->vg->lv_count--;
-
-	if (!(snap = lv_create_empty(name ? name : "snapshot%d",
+	if (!(snap = lv_create_empty("snapshot%d",
 				     lvid, LVM_READ | LVM_WRITE | VISIBLE_LV,
-				     ALLOC_INHERIT, 1, origin->vg))) {
-		origin->vg->lv_count++;
+				     ALLOC_INHERIT, 1, origin->vg)))
 		return_0;
-	}
 
 	snap->le_count = extent_count;
 
 	if (!(seg = alloc_snapshot_seg(snap, 0, 0)))
 		return_0;
 
-	seg->chunk_size = chunk_size;
-	seg->origin = origin;
-	seg->cow = cow;
-	seg->lv->status |= SNAPSHOT;
-
-	origin->origin_count++;
-	cow->snapshot = seg;
-
-	cow->status &= ~VISIBLE_LV;
-
-        /* FIXME Assumes an invisible origin belongs to a sparse device */
-        if (!lv_is_visible(origin))
-                origin->status |= VIRTUAL_ORIGIN;
-
-	dm_list_add(&origin->snapshot_segs, &seg->origin_list);
+	origin->vg->lv_count++;
+	init_snapshot_seg(seg, origin, cow, chunk_size);
 
 	return 1;
 }
diff --git a/lib/snapshot/snapshot.c b/lib/snapshot/snapshot.c
index 1fdf0ab..d704ff9 100644
--- a/lib/snapshot/snapshot.c
+++ b/lib/snapshot/snapshot.c
@@ -39,8 +39,6 @@ static int _snap_text_import(struct lv_segment *seg, const struct config_node *s
 	struct logical_volume *org, *cow;
 	int old_suppress;
 
-	seg->lv->status |= SNAPSHOT;
-
 	if (!get_config_uint32(sn, "chunk_size", &chunk_size)) {
 		log_error("Couldn't read chunk size for snapshot.");
 		return 0;
@@ -74,9 +72,7 @@ static int _snap_text_import(struct lv_segment *seg, const struct config_node *s
 		return 0;
 	}
 
-	if (!vg_add_snapshot(seg->lv->name, org, cow,
-			     &seg->lv->lvid, seg->len, chunk_size))
-		return_0;
+	init_snapshot_seg(seg, org, cow, chunk_size);
 
 	return 1;
 }
diff --git a/tools/lvconvert.c b/tools/lvconvert.c
index c54a2d1..267ff09 100644
--- a/tools/lvconvert.c
+++ b/tools/lvconvert.c
@@ -760,8 +760,7 @@ static int lvconvert_snapshot(struct cmd_context *cmd,
 		return 0;
 	}
 
-	if (!vg_add_snapshot(NULL, org, lv, NULL, org->le_count,
-			     lp->chunk_size)) {
+	if (!vg_add_snapshot(org, lv, NULL, org->le_count, lp->chunk_size)) {
 		log_error("Couldn't create snapshot.");
 		return 0;
 	}
diff --git a/tools/lvcreate.c b/tools/lvcreate.c
index 42608c3..069cc0e 100644
--- a/tools/lvcreate.c
+++ b/tools/lvcreate.c
@@ -918,7 +918,7 @@ static int _lvcreate(struct cmd_context *cmd, struct volume_group *vg,
 
 		/* cow LV remains active and becomes snapshot LV */
 
-		if (!vg_add_snapshot(NULL, org, lv, NULL,
+		if (!vg_add_snapshot(org, lv, NULL,
 				     org->le_count, lp->chunk_size)) {
 			log_error("Couldn't create snapshot.");
 			goto deactivate_and_revert_new_lv;
-- 
1.6.2.4

[PATCH 2/9] Remove vg->lv_count and use counter function.

[Milan Broz]

This should not cause problems but simplifies code a lot.

(the volumes_count is merged with lvs_visible function by following patch.)

Signed-off-by: Milan Broz <mbroz@redhat.com>
---
 lib/format1/import-export.c      |    3 +--
 lib/format_pool/format_pool.c    |    1 -
 lib/format_pool/import_export.c  |    2 --
 lib/format_text/import_vsn1.c    |    1 -
 lib/metadata/lv_manip.c          |    8 +-------
 lib/metadata/metadata-exported.h |    3 ++-
 lib/metadata/metadata.c          |   27 +++++++++++++++++++++------
 lib/metadata/snapshot_manip.c    |    6 ------
 tools/vgchange.c                 |    4 ++--
 tools/vgmerge.c                  |    1 -
 tools/vgsplit.c                  |    6 +-----
 11 files changed, 28 insertions(+), 34 deletions(-)

diff --git a/lib/format1/import-export.c b/lib/format1/import-export.c
index 5d2e86a..56800a2 100644
--- a/lib/format1/import-export.c
+++ b/lib/format1/import-export.c
@@ -282,7 +282,7 @@ int export_vg(struct vg_disk *vgd, struct volume_group *vg)
 		vgd->vg_status |= VG_EXTENDABLE;
 
 	vgd->lv_max = vg->max_lv;
-	vgd->lv_cur = vg->lv_count + snapshot_count(vg);
+	vgd->lv_cur = volumes_count(vg) + snapshot_count(vg);
 
 	vgd->pv_max = vg->max_pv;
 	vgd->pv_cur = vg->pv_count;
@@ -469,7 +469,6 @@ static struct logical_volume *_add_lv(struct dm_pool *mem,
 		return_NULL;
 
 	dm_list_add(&vg->lvs, &ll->list);
-	vg->lv_count++;
 
 	return lv;
 }
diff --git a/lib/format_pool/format_pool.c b/lib/format_pool/format_pool.c
index 114cec1..3f31bba 100644
--- a/lib/format_pool/format_pool.c
+++ b/lib/format_pool/format_pool.c
@@ -119,7 +119,6 @@ static struct volume_group *_build_vg_from_pds(struct format_instance
 	vg->status = 0;
 	vg->extent_count = 0;
 	vg->pv_count = 0;
-	vg->lv_count = 0;
 	vg->seqno = 1;
 	vg->system_id = NULL;
 	dm_list_init(&vg->pvs);
diff --git a/lib/format_pool/import_export.c b/lib/format_pool/import_export.c
index 4d2163d..25d53e7 100644
--- a/lib/format_pool/import_export.c
+++ b/lib/format_pool/import_export.c
@@ -49,7 +49,6 @@ int import_pool_vg(struct volume_group *vg, struct dm_pool *mem, struct dm_list
 		vg->max_lv = 1;
 		vg->max_pv = POOL_MAX_DEVICES;
 		vg->alloc = ALLOC_NORMAL;
-		vg->lv_count = 0;
 	}
 
 	return 1;
@@ -117,7 +116,6 @@ int import_pool_lvs(struct volume_group *vg, struct dm_pool *mem, struct dm_list
 	lv->le_count = lv->size / POOL_PE_SIZE;
 	lvl->lv = lv;
 	dm_list_add(&vg->lvs, &lvl->list);
-	vg->lv_count++;
 
 	return 1;
 }
diff --git a/lib/format_text/import_vsn1.c b/lib/format_text/import_vsn1.c
index a9748aa..8f0d07c 100644
--- a/lib/format_text/import_vsn1.c
+++ b/lib/format_text/import_vsn1.c
@@ -562,7 +562,6 @@ static int _read_lvnames(struct format_instance *fid __attribute((unused)),
 	}
 
 	lv->vg = vg;
-	vg->lv_count++;
 	dm_list_add(&vg->lvs, &lvl->list);
 
 	return 1;
diff --git a/lib/metadata/lv_manip.c b/lib/metadata/lv_manip.c
index 7214b27..64419d1 100644
--- a/lib/metadata/lv_manip.c
+++ b/lib/metadata/lv_manip.c
@@ -438,9 +438,6 @@ static int _lv_reduce(struct logical_volume *lv, uint32_t extents, int delete)
 			return_0;
 
 		dm_list_del(&lvl->list);
-
-		if (!(lv->status & SNAPSHOT))
-			lv->vg->lv_count--;
 	} else if (lv->vg->fid->fmt->ops->lv_setup &&
 		   !lv->vg->fid->fmt->ops->lv_setup(lv->vg->fid, lv))
 		return_0;
@@ -1827,7 +1824,7 @@ struct logical_volume *lv_create_empty(const char *name,
 	struct logical_volume *lv;
 	char dname[NAME_LEN];
 
-	if (vg->max_lv && (vg->max_lv == vg->lv_count)) {
+	if (vg->max_lv && (vg->max_lv == volumes_count(vg))) {
 		log_error("Maximum number of logical volumes (%u) reached "
 			  "in volume group %s", vg->max_lv, vg->name);
 		return NULL;
@@ -1883,9 +1880,6 @@ struct logical_volume *lv_create_empty(const char *name,
 		return_NULL;
 	}
 
-	if (!import)
-		vg->lv_count++;
-
 	dm_list_add(&vg->lvs, &ll->list);
 
 	return lv;
diff --git a/lib/metadata/metadata-exported.h b/lib/metadata/metadata-exported.h
index 6c32ab4..2970b9c 100644
--- a/lib/metadata/metadata-exported.h
+++ b/lib/metadata/metadata-exported.h
@@ -251,7 +251,6 @@ struct volume_group {
 	 * - one for the user-visible mirror LV
 	 * all of the instances are reflected in lv_count.
 	 */
-	uint32_t lv_count;
 	struct dm_list lvs;
 
 	struct dm_list tags;
@@ -556,6 +555,8 @@ int vg_remove_snapshot(struct logical_volume *cow);
 
 int vg_check_status(const struct volume_group *vg, uint32_t status);
 
+unsigned volumes_count(const struct volume_group *vg);
+
 /*
 * Mirroring functions
 */
diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
index 7cd4166..97b9ebb 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -571,7 +571,6 @@ struct volume_group *vg_create(struct cmd_context *cmd, const char *vg_name,
 	vg->pv_count = 0;
 	dm_list_init(&vg->pvs);
 
-	vg->lv_count = 0;
 	dm_list_init(&vg->lvs);
 
 	dm_list_init(&vg->tags);
@@ -1165,6 +1164,22 @@ unsigned snapshot_count(const struct volume_group *vg)
 	return num_snapshots;
 }
 
+unsigned volumes_count(const struct volume_group *vg)
+{
+	struct lv_list *lvl;
+	unsigned lv_count = 0;
+
+	dm_list_iterate_items(lvl, &vg->lvs) {
+		if (lv_is_cow(lvl->lv))
+			continue;
+		if (lvl->lv->status & SNAPSHOT)
+			continue;
+		lv_count++;
+	}
+
+	return lv_count;
+}
+
 /*
  * Determine whether two vgs are compatible for merging.
  */
@@ -1199,7 +1214,7 @@ int vgs_are_compatible(struct cmd_context *cmd __attribute((unused)),
 	}
 
 	if (vg_to->max_lv &&
-	    (vg_to->max_lv < vg_to->lv_count + vg_from->lv_count)) {
+	    (vg_to->max_lv < volumes_count(vg_to) + volumes_count(vg_from))) {
 		log_error("Maximum number of logical volumes (%d) exceeded "
 			  " for \"%s\" and \"%s\"", vg_to->max_lv, vg_to->name,
 			  vg_from->name);
@@ -1455,10 +1470,10 @@ int vg_validate(struct volume_group *vg)
 	}
 
 	if ((lv_count = (uint32_t) dm_list_size(&vg->lvs)) !=
-	    vg->lv_count + 2 * snapshot_count(vg)) {
+	    volumes_count(vg) + 2 * snapshot_count(vg)) {
 		log_error("Internal error: #internal LVs (%u) != #LVs (%"
 			  PRIu32 ") + 2 * #snapshots (%" PRIu32 ") in VG %s",
-			  dm_list_size(&vg->lvs), vg->lv_count,
+			  dm_list_size(&vg->lvs), volumes_count(vg),
 			  snapshot_count(vg), vg->name);
 		r = 0;
 	}
@@ -1502,10 +1517,10 @@ int vg_validate(struct volume_group *vg)
 		r = 0;
 	}
 
-	if (vg->max_lv && (vg->max_lv < vg->lv_count)) {
+	if (vg->max_lv && (vg->max_lv < volumes_count(vg))) {
 		log_error("Internal error: Volume group %s contains %u volumes"
 			  " but the limit is set to %u.",
-			  vg->name, vg->lv_count, vg->max_lv);
+			  vg->name, volumes_count(vg), vg->max_lv);
 		r = 0;
 	}
 
diff --git a/lib/metadata/snapshot_manip.c b/lib/metadata/snapshot_manip.c
index 9364b4f..e186dff 100644
--- a/lib/metadata/snapshot_manip.c
+++ b/lib/metadata/snapshot_manip.c
@@ -69,13 +69,10 @@ void init_snapshot_seg(struct lv_segment *seg, struct logical_volume *origin,
 	seg->origin = origin;
 	seg->cow = cow;
 
-	// FIXME: direct count manipulation to be removed later
 	cow->status &= ~VISIBLE_LV;
-	cow->vg->lv_count--;
 	cow->snapshot = seg;
 
 	origin->origin_count++;
-	origin->vg->lv_count--;
 
 	/* FIXME Assumes an invisible origin belongs to a sparse device */
 	if (!lv_is_visible(origin))
@@ -116,7 +113,6 @@ int vg_add_snapshot(struct logical_volume *origin,
 	if (!(seg = alloc_snapshot_seg(snap, 0, 0)))
 		return_0;
 
-	origin->vg->lv_count++;
 	init_snapshot_seg(seg, origin, cow, chunk_size);
 
 	return 1;
@@ -134,8 +130,6 @@ int vg_remove_snapshot(struct logical_volume *cow)
 	}
 
 	cow->snapshot = NULL;
-
-	cow->vg->lv_count++;
 	cow->status |= VISIBLE_LV;
 
 	return 1;
diff --git a/tools/vgchange.c b/tools/vgchange.c
index eb2b816..d01bc01 100644
--- a/tools/vgchange.c
+++ b/tools/vgchange.c
@@ -308,9 +308,9 @@ static int _vgchange_logicalvolume(struct cmd_context *cmd,
 		}
 	}
 
-	if (max_lv && max_lv < vg->lv_count) {
+	if (max_lv && max_lv < volumes_count(vg)) {
 		log_error("MaxLogicalVolume is less than the current number "
-			  "%d of LVs for \"%s\"", vg->lv_count,
+			  "%d of LVs for %s", volumes_count(vg),
 			  vg->name);
 		return ECMD_FAILED;
 	}
diff --git a/tools/vgmerge.c b/tools/vgmerge.c
index 8b85c7a..57a700e 100644
--- a/tools/vgmerge.c
+++ b/tools/vgmerge.c
@@ -101,7 +101,6 @@ static int _vgmerge_single(struct cmd_context *cmd, const char *vg_name_to,
 		dm_list_move(&vg_to->fid->metadata_areas, mdah);
 	}
 
-	vg_to->lv_count += vg_from->lv_count;
 	vg_to->extent_count += vg_from->extent_count;
 	vg_to->free_count += vg_from->free_count;
 
diff --git a/tools/vgsplit.c b/tools/vgsplit.c
index e4c7620..317aac1 100644
--- a/tools/vgsplit.c
+++ b/tools/vgsplit.c
@@ -106,12 +106,8 @@ static int _move_one_lv(struct volume_group *vg_from,
 		return 0;
 	}
 
-	if (!(lv->status & SNAPSHOT) && !lv_is_cow(lv)) {
-		vg_from->lv_count--;
-		vg_to->lv_count++;
-	}
 	return 1;
-}	
+}
 
 static int _move_lvs(struct volume_group *vg_from, struct volume_group *vg_to)
 {
-- 
1.6.2.4

[PATCH 3/9] Tidy format1 import LV function.

[Milan Broz]

Later patch initializes lv->vg after the LV structure is prepared,
so pass through cmd context and do not use vg->cmd here.
Also move LV id calculation (which uses lv->vg too).

Also properly free memory pool if operation fails.

Signed-off-by: Milan Broz <mbroz@redhat.com>
---
 lib/format1/disk-rep.h      |    3 ++-
 lib/format1/import-export.c |   13 ++++++++-----
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/lib/format1/disk-rep.h b/lib/format1/disk-rep.h
index 4a3b31a..138794e 100644
--- a/lib/format1/disk-rep.h
+++ b/lib/format1/disk-rep.h
@@ -215,7 +215,8 @@ int import_vg(struct dm_pool *mem,
 	      struct volume_group *vg, struct disk_list *dl);
 int export_vg(struct vg_disk *vgd, struct volume_group *vg);
 
-int import_lv(struct dm_pool *mem, struct logical_volume *lv, struct lv_disk *lvd);
+int import_lv(struct cmd_context *cmd, struct dm_pool *mem,
+	      struct logical_volume *lv, struct lv_disk *lvd);
 
 int import_extents(struct cmd_context *cmd, struct volume_group *vg,
 		   struct dm_list *pvds);
diff --git a/lib/format1/import-export.c b/lib/format1/import-export.c
index 56800a2..b32a345 100644
--- a/lib/format1/import-export.c
+++ b/lib/format1/import-export.c
@@ -294,10 +294,9 @@ int export_vg(struct vg_disk *vgd, struct volume_group *vg)
 	return 1;
 }
 
-int import_lv(struct dm_pool *mem, struct logical_volume *lv, struct lv_disk *lvd)
+int import_lv(struct cmd_context *cmd, struct dm_pool *mem,
+	      struct logical_volume *lv, struct lv_disk *lvd)
 {
-	lvid_from_lvnum(&lv->lvid, &lv->vg->id, lvd->lv_number);
-
 	if (!(lv->name = _create_lv_name(mem, (char *)lvd->lv_name)))
 		return_0;
 
@@ -331,7 +330,7 @@ int import_lv(struct dm_pool *mem, struct logical_volume *lv, struct lv_disk *lv
 		lv->alloc = ALLOC_NORMAL;
 
 	if (!lvd->lv_read_ahead)
-		lv->read_ahead = lv->vg->cmd->default_settings.read_ahead;
+		lv->read_ahead = cmd->default_settings.read_ahead;
 	else
 		lv->read_ahead = lvd->lv_read_ahead;
 
@@ -465,8 +464,12 @@ static struct logical_volume *_add_lv(struct dm_pool *mem,
 	lv = ll->lv;
 	lv->vg = vg;
 
-	if (!import_lv(mem, lv, lvd))
+	lvid_from_lvnum(&lv->lvid, &vg->id, lvd->lv_number);
+
+	if (!import_lv(vg->cmd, mem, lv, lvd)) {
+		dm_pool_free(mem, ll);
 		return_NULL;
+	}
 
 	dm_list_add(&vg->lvs, &ll->list);
 
-- 
1.6.2.4

[PATCH 4/9] Introduce vg_link_lv and vg_unlink_lv functions.

[Milan Broz]

vg_link_lv and vg_unlink_lv are the only functions for
adding/removing logical volume into volume group.

Only these function should manipulate with vg->lvs list
and (together with following patches) will update
VG logical volume counter (vg->lv_count).

Signed-off-by: Milan Broz <mbroz@redhat.com>
---
 lib/format1/import-export.c      |   18 ++++-----
 lib/format_pool/import_export.c  |   14 +------
 lib/format_text/import_vsn1.c    |   11 +-----
 lib/metadata/lv_manip.c          |   70 +++++++++++++++++++++----------------
 lib/metadata/metadata-exported.h |    3 ++
 5 files changed, 55 insertions(+), 61 deletions(-)

diff --git a/lib/format1/import-export.c b/lib/format1/import-export.c
index b32a345..773ecad 100644
--- a/lib/format1/import-export.c
+++ b/lib/format1/import-export.c
@@ -455,25 +455,23 @@ static struct logical_volume *_add_lv(struct dm_pool *mem,
 				      struct volume_group *vg,
 				      struct lv_disk *lvd)
 {
-	struct lv_list *ll;
 	struct logical_volume *lv;
 
-	if (!(ll = dm_pool_zalloc(mem, sizeof(*ll))) ||
-	    !(ll->lv = dm_pool_zalloc(mem, sizeof(*ll->lv))))
+	if (!(lv = dm_pool_zalloc(mem, sizeof(*lv))))
 		return_NULL;
-	lv = ll->lv;
-	lv->vg = vg;
 
 	lvid_from_lvnum(&lv->lvid, &vg->id, lvd->lv_number);
 
-	if (!import_lv(vg->cmd, mem, lv, lvd)) {
-		dm_pool_free(mem, ll);
-		return_NULL;
-	}
+	if (!import_lv(vg->cmd, mem, lv, lvd)) 
+		goto_bad;
 
-	dm_list_add(&vg->lvs, &ll->list);
+	if (!vg_link_lv(vg, lv))
+		goto_bad;
 
 	return lv;
+bad:
+	dm_pool_free(mem, lv);
+	return NULL;
 }
 
 int import_lvs(struct dm_pool *mem, struct volume_group *vg, struct dm_list *pvds)
diff --git a/lib/format_pool/import_export.c b/lib/format_pool/import_export.c
index 25d53e7..3277582 100644
--- a/lib/format_pool/import_export.c
+++ b/lib/format_pool/import_export.c
@@ -57,22 +57,14 @@ int import_pool_vg(struct volume_group *vg, struct dm_pool *mem, struct dm_list
 int import_pool_lvs(struct volume_group *vg, struct dm_pool *mem, struct dm_list *pls)
 {
 	struct pool_list *pl;
-	struct lv_list *lvl = dm_pool_zalloc(mem, sizeof(*lvl));
 	struct logical_volume *lv;
 
-	if (!lvl) {
-		log_error("Unable to allocate lv list structure");
-		return 0;
-	}
-
-	if (!(lvl->lv = dm_pool_zalloc(mem, sizeof(*lvl->lv)))) {
+	if (!(lv = dm_pool_zalloc(mem, sizeof(*lv)))) {
 		log_error("Unable to allocate logical volume structure");
 		return 0;
 	}
 
-	lv = lvl->lv;
 	lv->status = 0;
-	lv->vg = vg;
 	lv->alloc = ALLOC_NORMAL;
 	lv->size = 0;
 	lv->name = NULL;
@@ -114,10 +106,8 @@ int import_pool_lvs(struct volume_group *vg, struct dm_pool *mem, struct dm_list
 	}
 
 	lv->le_count = lv->size / POOL_PE_SIZE;
-	lvl->lv = lv;
-	dm_list_add(&vg->lvs, &lvl->list);
 
-	return 1;
+	return vg_link_lv(vg, lv);
 }
 
 int import_pool_pvs(const struct format_type *fmt, struct volume_group *vg,
diff --git a/lib/format_text/import_vsn1.c b/lib/format_text/import_vsn1.c
index 8f0d07c..c2b1020 100644
--- a/lib/format_text/import_vsn1.c
+++ b/lib/format_text/import_vsn1.c
@@ -495,15 +495,11 @@ static int _read_lvnames(struct format_instance *fid __attribute((unused)),
 			 struct dm_hash_table *pv_hash __attribute((unused)))
 {
 	struct logical_volume *lv;
-	struct lv_list *lvl;
 	struct config_node *cn;
 
-	if (!(lvl = dm_pool_zalloc(mem, sizeof(*lvl))) ||
-	    !(lvl->lv = dm_pool_zalloc(mem, sizeof(*lvl->lv))))
+	if (!(lv = dm_pool_zalloc(mem, sizeof(*lv))))
 		return_0;
 
-	lv = lvl->lv;
-
 	if (!(lv->name = dm_pool_strdup(mem, lvn->key)))
 		return_0;
 
@@ -561,10 +557,7 @@ static int _read_lvnames(struct format_instance *fid __attribute((unused)),
 		return 0;
 	}
 
-	lv->vg = vg;
-	dm_list_add(&vg->lvs, &lvl->list);
-
-	return 1;
+	return vg_link_lv(vg, lv);
 }
 
 static int _read_lvsegs(struct format_instance *fid __attribute((unused)),
diff --git a/lib/metadata/lv_manip.c b/lib/metadata/lv_manip.c
index 64419d1..da2f250 100644
--- a/lib/metadata/lv_manip.c
+++ b/lib/metadata/lv_manip.c
@@ -402,7 +402,6 @@ static int _lv_segment_reduce(struct lv_segment *seg, uint32_t reduction)
  */
 static int _lv_reduce(struct logical_volume *lv, uint32_t extents, int delete)
 {
-	struct lv_list *lvl;
 	struct lv_segment *seg;
 	uint32_t count = extents;
 	uint32_t reduction;
@@ -434,10 +433,8 @@ static int _lv_reduce(struct logical_volume *lv, uint32_t extents, int delete)
 
 	/* Remove the LV if it is now empty */
 	if (!lv->le_count) {
-		if (!(lvl = find_lv_in_vg(lv->vg, lv->name)))
+		if(!vg_unlink_lv(lv))
 			return_0;
-
-		dm_list_del(&lvl->list);
 	} else if (lv->vg->fid->fmt->ops->lv_setup &&
 		   !lv->vg->fid->fmt->ops->lv_setup(lv->vg->fid, lv))
 		return_0;
@@ -1819,8 +1816,6 @@ struct logical_volume *lv_create_empty(const char *name,
 				       struct volume_group *vg)
 {
 	struct format_instance *fi = vg->fid;
-	struct cmd_context *cmd = vg->cmd;
-	struct lv_list *ll = NULL;
 	struct logical_volume *lv;
 	char dname[NAME_LEN];
 
@@ -1840,23 +1835,11 @@ struct logical_volume *lv_create_empty(const char *name,
 	if (!import)
 		log_verbose("Creating logical volume %s", name);
 
-	if (!(ll = dm_pool_zalloc(cmd->mem, sizeof(*ll))) ||
-	    !(ll->lv = dm_pool_zalloc(cmd->mem, sizeof(*ll->lv)))) {
-		log_error("lv_list allocation failed");
-		if (ll)
-			dm_pool_free(cmd->mem, ll);
-		return NULL;
-	}
-
-	lv = ll->lv;
-	lv->vg = vg;
+	if (!(lv = dm_pool_zalloc(vg->vgmem, sizeof(*lv))))
+		return_NULL;
 
-	if (!(lv->name = dm_pool_strdup(cmd->mem, name))) {
-		log_error("lv name strdup failed");
-		if (ll)
-			dm_pool_free(cmd->mem, ll);
-		return NULL;
-	}
+	if (!(lv->name = dm_pool_strdup(vg->vgmem, name)))
+		goto_bad;
 
 	lv->status = status;
 	lv->alloc = alloc;
@@ -1874,15 +1857,16 @@ struct logical_volume *lv_create_empty(const char *name,
 	if (lvid)
 		lv->lvid = *lvid;
 
-	if (fi->fmt->ops->lv_setup && !fi->fmt->ops->lv_setup(fi, lv)) {
-		if (ll)
-			dm_pool_free(cmd->mem, ll);
-		return_NULL;
-	}
-
-	dm_list_add(&vg->lvs, &ll->list);
-
+	if (!vg_link_lv(vg, lv))
+		goto_bad;
+ 
+	if (fi->fmt->ops->lv_setup && !fi->fmt->ops->lv_setup(fi, lv))
+		goto_bad;
+ 
 	return lv;
+bad:
+	dm_pool_free(vg->vgmem, lv);
+	return NULL;
 }
 
 static int _add_pvs(struct cmd_context *cmd, struct pv_segment *peg,
@@ -1951,6 +1935,32 @@ struct dm_list *build_parallel_areas_from_lv(struct cmd_context *cmd,
 	return parallel_areas;
 }
 
+int vg_link_lv(struct volume_group *vg, struct logical_volume *lv)
+{
+	struct lv_list *lvl;
+
+	if (!(lvl = dm_pool_zalloc(vg->vgmem, sizeof(*lvl))))
+		return_0;
+
+	lvl->lv = lv;
+	lv->vg = vg;
+	dm_list_add(&vg->lvs, &lvl->list);
+
+	return 1;
+}
+
+int vg_unlink_lv(struct logical_volume *lv)
+{
+	struct lv_list *lvl;
+
+	if (!(lvl = find_lv_in_vg(lv->vg, lv->name)))
+		return_0;
+
+	dm_list_del(&lvl->list);
+
+	return 1;
+}
+
 int lv_remove_single(struct cmd_context *cmd, struct logical_volume *lv,
 		     const force_t force)
 {
diff --git a/lib/metadata/metadata-exported.h b/lib/metadata/metadata-exported.h
index 2970b9c..d534386 100644
--- a/lib/metadata/metadata-exported.h
+++ b/lib/metadata/metadata-exported.h
@@ -368,6 +368,9 @@ struct physical_volume *pv_read(struct cmd_context *cmd, const char *pv_name,
 				int warnings, int scan_label_only);
 struct dm_list *get_pvs(struct cmd_context *cmd);
 
+int vg_link_lv(struct volume_group *vg, struct logical_volume *lv);
+int vg_unlink_lv(struct logical_volume *lv);
+
 /* Set full_scan to 1 to re-read every (filtered) device label */
 struct dm_list *get_vgnames(struct cmd_context *cmd, int full_scan);
 struct dm_list *get_vgids(struct cmd_context *cmd, int full_scan);
-- 
1.6.2.4

[PATCH 5/9] Fix lv_is_visible to handle virtual origin.

[Milan Broz]

Snapshot is visible if its origin is marked visible,
or if the origin is virtual.

Signed-off-by: Milan Broz <mbroz@redhat.com>
---
 lib/metadata/snapshot_manip.c |   11 +++++++++--
 1 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/lib/metadata/snapshot_manip.c b/lib/metadata/snapshot_manip.c
index e186dff..0509fe0 100644
--- a/lib/metadata/snapshot_manip.c
+++ b/lib/metadata/snapshot_manip.c
@@ -30,8 +30,15 @@ int lv_is_cow(const struct logical_volume *lv)
 
 int lv_is_visible(const struct logical_volume *lv)
 {
-	if (lv_is_cow(lv))
-		return lv_is_visible(find_cow(lv)->lv);
+	if (lv->status & SNAPSHOT)
+		return 0;
+
+	if (lv_is_cow(lv)) {
+		if (lv_is_virtual_origin(origin_from_cow(lv)))
+			return 1;
+
+		return lv_is_visible(origin_from_cow(lv));
+	}
 
 	return lv->status & VISIBLE_LV ? 1 : 0;
 }
-- 
1.6.2.4

[PATCH 6/9] Introduce lv_set_visible & lv_set_hidden and use lv_is_visible always.

[Milan Broz]

The vg->lv_count parameter now includes always number of visible
logical volumes.

Note that virtual snapshot volume (snapshotX) is never visible,
but it is stored in metadata with visible flag, so code
uses exception here.

Signed-off-by: Milan Broz <mbroz@redhat.com>
---
 lib/activate/activate.c          |    4 ++--
 lib/metadata/lv_manip.c          |   22 +++++++++++++++++++++-
 lib/metadata/metadata-exported.h |    2 ++
 lib/metadata/mirror.c            |   12 ++++++------
 lib/metadata/snapshot_manip.c    |    5 +++--
 5 files changed, 34 insertions(+), 11 deletions(-)

diff --git a/lib/activate/activate.c b/lib/activate/activate.c
index 7011a09..e38c4d8 100644
--- a/lib/activate/activate.c
+++ b/lib/activate/activate.c
@@ -647,7 +647,7 @@ static int _lvs_in_vg_activated(struct volume_group *vg, unsigned by_uuid_only)
 		return 0;
 
 	dm_list_iterate_items(lvl, &vg->lvs) {
-		if (lvl->lv->status & VISIBLE_LV)
+		if (lv_is_visible(lvl->lv))
 			count += (_lv_active(vg->cmd, lvl->lv, by_uuid_only) == 1);
 	}
 
@@ -996,7 +996,7 @@ int lv_deactivate(struct cmd_context *cmd, const char *lvid_s)
 		goto out;
 	}
 
-	if (info.open_count && (lv->status & VISIBLE_LV)) {
+	if (info.open_count && lv_is_visible(lv)) {
 		log_error("LV %s/%s in use: not deactivating", lv->vg->name,
 			  lv->name);
 		goto out;
diff --git a/lib/metadata/lv_manip.c b/lib/metadata/lv_manip.c
index da2f250..068d745 100644
--- a/lib/metadata/lv_manip.c
+++ b/lib/metadata/lv_manip.c
@@ -1504,7 +1504,7 @@ int lv_add_mirror_lvs(struct logical_volume *lv,
 		if (!set_lv_segment_area_lv(seg, m, sub_lvs[m - old_area_count],
 					    0, status))
 			return_0;
-		sub_lvs[m - old_area_count]->status &= ~VISIBLE_LV;
+		lv_set_hidden(sub_lvs[m - old_area_count]);
 	}
 
 	lv->status |= MIRRORED;
@@ -1961,6 +1961,26 @@ int vg_unlink_lv(struct logical_volume *lv)
 	return 1;
 }
 
+void lv_set_visible(struct logical_volume *lv)
+{
+	if (lv_is_visible(lv))
+		return;
+
+	lv->status |= VISIBLE_LV;
+
+	log_debug("LV %s in VG %s is now visible.",  lv->name, lv->vg->name);
+}
+
+void lv_set_hidden(struct logical_volume *lv)
+{
+	if (!lv_is_visible(lv))
+		return;
+
+	lv->status &= ~VISIBLE_LV;
+
+	log_debug("LV %s in VG %s is now hidden.",  lv->name, lv->vg->name);
+}
+
 int lv_remove_single(struct cmd_context *cmd, struct logical_volume *lv,
 		     const force_t force)
 {
diff --git a/lib/metadata/metadata-exported.h b/lib/metadata/metadata-exported.h
index d534386..64b30f7 100644
--- a/lib/metadata/metadata-exported.h
+++ b/lib/metadata/metadata-exported.h
@@ -370,6 +370,8 @@ struct dm_list *get_pvs(struct cmd_context *cmd);
 
 int vg_link_lv(struct volume_group *vg, struct logical_volume *lv);
 int vg_unlink_lv(struct logical_volume *lv);
+void lv_set_visible(struct logical_volume *lv);
+void lv_set_hidden(struct logical_volume *lv);
 
 /* Set full_scan to 1 to re-read every (filtered) device label */
 struct dm_list *get_vgnames(struct cmd_context *cmd, int full_scan);
diff --git a/lib/metadata/mirror.c b/lib/metadata/mirror.c
index 5eb53f4..087ce73 100644
--- a/lib/metadata/mirror.c
+++ b/lib/metadata/mirror.c
@@ -260,7 +260,7 @@ static int _init_mirror_log(struct cmd_context *cmd,
 	}
 
 	/* Temporary make it visible for set_lv() */
-	log_lv->status |= VISIBLE_LV;
+	lv_set_visible(log_lv);
 
 	/* Temporary tag mirror log for activation */
 	dm_list_iterate_items(sl, tags)
@@ -303,7 +303,7 @@ static int _init_mirror_log(struct cmd_context *cmd,
 		return 0;
 	}
 
-	log_lv->status &= ~VISIBLE_LV;
+	lv_set_hidden(log_lv);
 
 	if (was_active && !activate_lv(cmd, log_lv))
 		return_0;
@@ -410,7 +410,7 @@ struct logical_volume *detach_mirror_log(struct lv_segment *mirrored_seg)
 
 	log_lv = mirrored_seg->log_lv;
 	mirrored_seg->log_lv = NULL;
-	log_lv->status |= VISIBLE_LV;
+	lv_set_visible(log_lv);
 	log_lv->status &= ~MIRROR_LOG;
 	remove_seg_from_segs_using_this_lv(log_lv, mirrored_seg);
 
@@ -536,7 +536,7 @@ static int _remove_mirror_images(struct logical_volume *lv,
 	dm_list_init(&tmp_orphan_lvs);
 	for (m = new_area_count; m < mirrored_seg->area_count; m++) {
 		seg_lv(mirrored_seg, m)->status &= ~MIRROR_IMAGE;
-		seg_lv(mirrored_seg, m)->status |= VISIBLE_LV;
+		lv_set_visible(seg_lv(mirrored_seg, m));
 		if (!(lvl = dm_pool_alloc(lv->vg->cmd->mem, sizeof(*lvl)))) {
 			log_error("lv_list alloc failed");
 			return 0;
@@ -554,7 +554,7 @@ static int _remove_mirror_images(struct logical_volume *lv,
 	if (new_area_count == 1 && !is_temporary_mirror_layer(lv)) {
 		lv1 = seg_lv(mirrored_seg, 0);
 		lv1->status &= ~MIRROR_IMAGE;
-		lv1->status |= VISIBLE_LV;
+		lv_set_visible(lv1);
 		detached_log_lv = detach_mirror_log(mirrored_seg);
 		if (!remove_layer_from_lv(lv, lv1))
 			return_0;
@@ -1340,7 +1340,7 @@ int attach_mirror_log(struct lv_segment *seg, struct logical_volume *log_lv)
 {
 	seg->log_lv = log_lv;
 	log_lv->status |= MIRROR_LOG;
-	log_lv->status &= ~VISIBLE_LV;
+	lv_set_hidden(log_lv);
 	return add_seg_to_segs_using_this_lv(log_lv, seg);
 }
 
diff --git a/lib/metadata/snapshot_manip.c b/lib/metadata/snapshot_manip.c
index 0509fe0..ff608c3 100644
--- a/lib/metadata/snapshot_manip.c
+++ b/lib/metadata/snapshot_manip.c
@@ -76,7 +76,8 @@ void init_snapshot_seg(struct lv_segment *seg, struct logical_volume *origin,
 	seg->origin = origin;
 	seg->cow = cow;
 
-	cow->status &= ~VISIBLE_LV;
+	lv_set_hidden(cow);
+
 	cow->snapshot = seg;
 
 	origin->origin_count++;
@@ -137,7 +138,7 @@ int vg_remove_snapshot(struct logical_volume *cow)
 	}
 
 	cow->snapshot = NULL;
-	cow->status |= VISIBLE_LV;
+	lv_set_visible(cow);
 
 	return 1;
 }
-- 
1.6.2.4

[PATCH 7/9] Merge lv_is_displayable and lv_is_visible.

[Milan Broz]

Displayable and visible is the same thing.

volumes_count(vg) now always returns number of LVs from
user perspective.

Signed-off-by: Milan Broz <mbroz@redhat.com>
---
 lib/activate/activate.c          |    2 +-
 lib/display/display.c            |    4 +-
 lib/format_text/export.c         |    4 +-
 lib/metadata/lv_manip.c          |    2 +-
 lib/metadata/metadata-exported.h |    7 +---
 lib/metadata/metadata.c          |   59 ++++++++++++++++++++++---------------
 lib/metadata/metadata.h          |    5 ---
 lib/metadata/snapshot_manip.c    |    8 -----
 lib/report/report.c              |    4 +-
 tools/lvchange.c                 |    2 +-
 tools/lvdisplay.c                |    2 +-
 tools/lvscan.c                   |    2 +-
 tools/reporter.c                 |    4 +-
 13 files changed, 50 insertions(+), 55 deletions(-)

diff --git a/lib/activate/activate.c b/lib/activate/activate.c
index e38c4d8..34f979a 100644
--- a/lib/activate/activate.c
+++ b/lib/activate/activate.c
@@ -673,7 +673,7 @@ int lvs_in_vg_opened(const struct volume_group *vg)
 		return 0;
 
 	dm_list_iterate_items(lvl, &vg->lvs) {
-		if (lv_is_displayable(lvl->lv))
+		if (lv_is_visible(lvl->lv))
 			count += (_lv_open_count(vg->cmd, lvl->lv) > 0);
 	}
 
diff --git a/lib/display/display.c b/lib/display/display.c
index c1ae8db..e0e1369 100644
--- a/lib/display/display.c
+++ b/lib/display/display.c
@@ -606,7 +606,7 @@ void vgdisplay_full(const struct volume_group *vg)
 	}
 
 	log_print("MAX LV                %u", vg->max_lv);
-	log_print("Cur LV                %u", displayable_lvs_in_vg(vg));
+	log_print("Cur LV                %u", volumes_count(vg));
 	log_print("Open LV               %u", lvs_in_vg_opened(vg));
 /****** FIXME Max LV Size
       log_print ( "MAX LV Size           %s",
@@ -681,7 +681,7 @@ void vgdisplay_colons(const struct volume_group *vg)
 		vg->status,
 		/* internal volume group number; obsolete */
 		vg->max_lv,
-		displayable_lvs_in_vg(vg),
+		volumes_count(vg),
 		lvs_in_vg_opened(vg),
 		/* FIXME: maximum logical volume size */
 		vg->max_pv,
diff --git a/lib/format_text/export.c b/lib/format_text/export.c
index 604138c..ce6676e 100644
--- a/lib/format_text/export.c
+++ b/lib/format_text/export.c
@@ -592,14 +592,14 @@ static int _print_lvs(struct formatter *f, struct volume_group *vg)
 	 * Write visible LVs first
 	 */
 	dm_list_iterate_items(lvl, &vg->lvs) {
-		if (!(lv_is_displayable(lvl->lv)))
+		if (!(lv_is_visible(lvl->lv)))
 			continue;
 		if (!_print_lv(f, lvl->lv))
 			return_0;
 	}
 
 	dm_list_iterate_items(lvl, &vg->lvs) {
-		if ((lv_is_displayable(lvl->lv)))
+		if ((lv_is_visible(lvl->lv)))
 			continue;
 		if (!_print_lv(f, lvl->lv))
 			return_0;
diff --git a/lib/metadata/lv_manip.c b/lib/metadata/lv_manip.c
index 068d745..80c1197 100644
--- a/lib/metadata/lv_manip.c
+++ b/lib/metadata/lv_manip.c
@@ -1733,7 +1733,7 @@ int lv_rename(struct cmd_context *cmd, struct logical_volume *lv,
 	int r = 0;
 
 	/* rename is not allowed on sub LVs */
-	if (!lv_is_displayable(lv)) {
+	if (!lv_is_visible(lv)) {
 		log_error("Cannot rename internal LV \"%s\".", lv->name);
 		return 0;
 	}
diff --git a/lib/metadata/metadata-exported.h b/lib/metadata/metadata-exported.h
index 64b30f7..680be10 100644
--- a/lib/metadata/metadata-exported.h
+++ b/lib/metadata/metadata-exported.h
@@ -238,18 +238,16 @@ struct volume_group {
 	/*
 	 * logical volumes
 	 * The following relationship should always hold:
-	 * dm_list_size(lvs) = lv_count + 2 * snapshot_count
+	 * dm_list_size(lvs) = user visible lv_count + snapshot_count + other unvisible LVs
 	 *
 	 * Snapshots consist of 2 instances of "struct logical_volume":
 	 * - cow (lv_name is visible to the user)
 	 * - snapshot (lv_name is 'snapshotN')
-	 * Neither of these instances is reflected in lv_count.
 	 *
 	 * Mirrors consist of multiple instances of "struct logical_volume":
 	 * - one for the mirror log
 	 * - one for each mirror leg
 	 * - one for the user-visible mirror LV
-	 * all of the instances are reflected in lv_count.
 	 */
 	struct dm_list lvs;
 
@@ -536,10 +534,9 @@ struct lv_segment *first_seg(const struct logical_volume *lv);
 int lv_is_origin(const struct logical_volume *lv);
 int lv_is_virtual_origin(const struct logical_volume *lv);
 int lv_is_cow(const struct logical_volume *lv);
-int lv_is_visible(const struct logical_volume *lv);
 
 /* Test if given LV is visible from user's perspective */
-int lv_is_displayable(const struct logical_volume *lv);
+int lv_is_visible(const struct logical_volume *lv);
 
 int pv_is_in_vg(struct volume_group *vg, struct physical_volume *pv);
 
diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
index 97b9ebb..82ee6cb 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -376,7 +376,7 @@ int vg_remove_single(struct cmd_context *cmd, const char *vg_name,
 	if (!vg_check_status(vg, EXPORTED_VG))
 		return 0;
 
-	lv_count = displayable_lvs_in_vg(vg);
+	lv_count = volumes_count(vg);
 
 	if (lv_count) {
 		if ((force == PROMPT) &&
@@ -391,8 +391,8 @@ int vg_remove_single(struct cmd_context *cmd, const char *vg_name,
 			return 0;
 	}
 
-	lv_count = displayable_lvs_in_vg(vg);
-	
+	lv_count = volumes_count(vg);
+
 	if (lv_count) {
 		log_error("Volume group \"%s\" still contains %u "
 			  "logical volume(s)", vg_name, lv_count);
@@ -1140,18 +1140,6 @@ int vg_remove(struct volume_group *vg)
 	return 1;
 }
 
-unsigned displayable_lvs_in_vg(const struct volume_group *vg)
-{
-	struct lv_list *lvl;
-	unsigned lv_count = 0;
-
-	dm_list_iterate_items(lvl, &vg->lvs)
-		if (lv_is_displayable(lvl->lv))
-			lv_count++;
-
-	return lv_count;
-}
-
 unsigned snapshot_count(const struct volume_group *vg)
 {
 	struct lv_list *lvl;
@@ -1170,11 +1158,8 @@ unsigned volumes_count(const struct volume_group *vg)
 	unsigned lv_count = 0;
 
 	dm_list_iterate_items(lvl, &vg->lvs) {
-		if (lv_is_cow(lvl->lv))
-			continue;
-		if (lvl->lv->status & SNAPSHOT)
-			continue;
-		lv_count++;
+		if (lv_is_visible(lvl->lv))
+			lv_count++;
 	}
 
 	return lv_count;
@@ -1469,12 +1454,38 @@ int vg_validate(struct volume_group *vg)
 		r = 0;
 	}
 
-	if ((lv_count = (uint32_t) dm_list_size(&vg->lvs)) !=
-	    volumes_count(vg) + 2 * snapshot_count(vg)) {
+	/*
+	 * Count all non-snapshot unvisible LVs
+	 */
+	lv_count = 0;
+	dm_list_iterate_items(lvl, &vg->lvs) {
+		if (lvl->lv->status & VISIBLE_LV)
+			continue;
+
+		/* snapshots */
+		if (lv_is_cow(lvl->lv) || lv_is_origin(lvl->lv))
+			continue;
+
+		/* count other non-snapshot invisible volumes */
+		lv_count++;
+
+		/*
+		 *  FIXME: add check for unreferenced unvisible LVs
+		 *   - snapshot cow & origin
+		 *   - mirror log & images
+		 *   - mirror conversion volumes (_mimagetmp*)
+		 */
+	}
+
+	/*
+	 * all volumes = visible LVs + snapshot_cows + unvisible LVs
+	 */
+	if (((uint32_t) dm_list_size(&vg->lvs)) !=
+	    volumes_count(vg) + snapshot_count(vg) + lv_count) {
 		log_error("Internal error: #internal LVs (%u) != #LVs (%"
-			  PRIu32 ") + 2 * #snapshots (%" PRIu32 ") in VG %s",
+			  PRIu32 ") + #snapshots (%" PRIu32 ") + #invisible LVs %u in VG %s",
 			  dm_list_size(&vg->lvs), volumes_count(vg),
-			  snapshot_count(vg), vg->name);
+			  snapshot_count(vg), lv_count, vg->name);
 		r = 0;
 	}
 
diff --git a/lib/metadata/metadata.h b/lib/metadata/metadata.h
index aca57f4..b2294bb 100644
--- a/lib/metadata/metadata.h
+++ b/lib/metadata/metadata.h
@@ -340,11 +340,6 @@ int remove_seg_from_segs_using_this_lv(struct logical_volume *lv, struct lv_segm
 struct lv_segment *get_only_segment_using_this_lv(struct logical_volume *lv);
 
 /*
- * Count LVs that are visible from user's perspective.
- */
-unsigned displayable_lvs_in_vg(const struct volume_group *vg);
-
-/*
  * Count snapshot LVs.
  */
 unsigned snapshot_count(const struct volume_group *vg);
diff --git a/lib/metadata/snapshot_manip.c b/lib/metadata/snapshot_manip.c
index ff608c3..bf694f7 100644
--- a/lib/metadata/snapshot_manip.c
+++ b/lib/metadata/snapshot_manip.c
@@ -43,14 +43,6 @@ int lv_is_visible(const struct logical_volume *lv)
 	return lv->status & VISIBLE_LV ? 1 : 0;
 }
 
-int lv_is_displayable(const struct logical_volume *lv)
-{
-	if (lv->status & SNAPSHOT)
-		return 0;
-
-	return (lv->status & VISIBLE_LV) || lv_is_cow(lv) ? 1 : 0;
-}
-
 int lv_is_virtual_origin(const struct logical_volume *lv)
 {
 	return (lv->status & VIRTUAL_ORIGIN) ? 1 : 0;
diff --git a/lib/report/report.c b/lib/report/report.c
index d8f7961..86c6743 100644
--- a/lib/report/report.c
+++ b/lib/report/report.c
@@ -497,7 +497,7 @@ static int _lvname_disp(struct dm_report *rh, struct dm_pool *mem,
 	char *repstr, *lvname;
 	size_t len;
 
-	if (lv_is_displayable(lv)) {
+	if (lv_is_visible(lv)) {
 		repstr = lv->name;
 		return dm_report_field_string(rh, field, (const char **) &repstr);
 	}
@@ -974,7 +974,7 @@ static int _lvcount_disp(struct dm_report *rh, struct dm_pool *mem,
 	const struct volume_group *vg = (const struct volume_group *) data;
 	uint32_t count;
 
-	count = displayable_lvs_in_vg(vg);	
+	count = volumes_count(vg);
 
 	return _uint32_disp(rh, mem, field, &count, private);
 }
diff --git a/tools/lvchange.c b/tools/lvchange.c
index 2696a95..466fe29 100644
--- a/tools/lvchange.c
+++ b/tools/lvchange.c
@@ -584,7 +584,7 @@ static int lvchange_single(struct cmd_context *cmd, struct logical_volume *lv,
 		return ECMD_FAILED;
 	}
 
-	if (!(lv_is_displayable(lv))) {
+	if (!(lv_is_visible(lv))) {
 		log_error("Unable to change internal LV %s directly",
 			  lv->name);
 		return ECMD_FAILED;
diff --git a/tools/lvdisplay.c b/tools/lvdisplay.c
index 5263e0d..16e7052 100644
--- a/tools/lvdisplay.c
+++ b/tools/lvdisplay.c
@@ -18,7 +18,7 @@
 static int _lvdisplay_single(struct cmd_context *cmd, struct logical_volume *lv,
 			     void *handle)
 {
-	if (!arg_count(cmd, all_ARG) && !lv_is_displayable(lv))
+	if (!arg_count(cmd, all_ARG) && !lv_is_visible(lv))
 		return ECMD_PROCESSED;
 
 	if (arg_count(cmd, colon_ARG))
diff --git a/tools/lvscan.c b/tools/lvscan.c
index 1186b3b..a286fe1 100644
--- a/tools/lvscan.c
+++ b/tools/lvscan.c
@@ -27,7 +27,7 @@ static int lvscan_single(struct cmd_context *cmd, struct logical_volume *lv,
 
 	const char *active_str, *snapshot_str;
 
-	if (!arg_count(cmd, all_ARG) && !lv_is_displayable(lv))
+	if (!arg_count(cmd, all_ARG) && !lv_is_visible(lv))
 		return ECMD_PROCESSED;
 
 	inkernel = lv_info(cmd, lv, &info, 1, 0) && info.exists;
diff --git a/tools/reporter.c b/tools/reporter.c
index 0a9de09..28393f8 100644
--- a/tools/reporter.c
+++ b/tools/reporter.c
@@ -36,7 +36,7 @@ static int _vgs_single(struct cmd_context *cmd __attribute((unused)),
 static int _lvs_single(struct cmd_context *cmd, struct logical_volume *lv,
 		       void *handle)
 {
-	if (!arg_count(cmd, all_ARG) && !lv_is_displayable(lv))
+	if (!arg_count(cmd, all_ARG) && !lv_is_visible(lv))
 		return ECMD_PROCESSED;
 
 	if (!report_object(handle, lv->vg, lv, NULL, NULL, NULL))
@@ -113,7 +113,7 @@ static int _pvsegs_sub_single(struct cmd_context *cmd,
 static int _lvsegs_single(struct cmd_context *cmd, struct logical_volume *lv,
 			  void *handle)
 {
-	if (!arg_count(cmd, all_ARG) && !lv_is_displayable(lv))
+	if (!arg_count(cmd, all_ARG) && !lv_is_visible(lv))
 		return ECMD_PROCESSED;
 
 	return process_each_segment_in_lv(cmd, lv, handle, _segs_single);
-- 
1.6.2.4

[PATCH 8/9] Remove unneeded import parameter from lv_create_empty.

[Milan Broz]

Signed-off-by: Milan Broz <mbroz@redhat.com>
---
 lib/metadata/lv_manip.c          |    6 ++----
 lib/metadata/metadata-exported.h |    1 -
 lib/metadata/mirror.c            |    4 ++--
 lib/metadata/snapshot_manip.c    |    2 +-
 tools/lvcreate.c                 |    4 ++--
 tools/pvmove.c                   |    2 +-
 6 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/lib/metadata/lv_manip.c b/lib/metadata/lv_manip.c
index 80c1197..1c758ea 100644
--- a/lib/metadata/lv_manip.c
+++ b/lib/metadata/lv_manip.c
@@ -1812,7 +1812,6 @@ struct logical_volume *lv_create_empty(const char *name,
 				       union lvid *lvid,
 				       uint32_t status,
 				       alloc_policy_t alloc,
-				       int import,
 				       struct volume_group *vg)
 {
 	struct format_instance *fi = vg->fid;
@@ -1832,8 +1831,7 @@ struct logical_volume *lv_create_empty(const char *name,
 		return NULL;
 	}
 
-	if (!import)
-		log_verbose("Creating logical volume %s", name);
+	log_verbose("Creating logical volume %s", name);
 
 	if (!(lv = dm_pool_zalloc(vg->vgmem, sizeof(*lv))))
 		return_NULL;
@@ -2418,7 +2416,7 @@ struct logical_volume *insert_layer_for_lv(struct cmd_context *cmd,
 	}
 
 	if (!(layer_lv = lv_create_empty(name, NULL, LVM_READ | LVM_WRITE,
-					 ALLOC_INHERIT, 0, lv_where->vg))) {
+					 ALLOC_INHERIT, lv_where->vg))) {
 		log_error("Creation of layer LV failed");
 		return NULL;
 	}
diff --git a/lib/metadata/metadata-exported.h b/lib/metadata/metadata-exported.h
index 680be10..1477a0b 100644
--- a/lib/metadata/metadata-exported.h
+++ b/lib/metadata/metadata-exported.h
@@ -445,7 +445,6 @@ struct logical_volume *lv_create_empty(const char *name,
 				       union lvid *lvid,
 				       uint32_t status,
 				       alloc_policy_t alloc,
-				       int import,
 				       struct volume_group *vg);
 
 /* Write out LV contents */
diff --git a/lib/metadata/mirror.c b/lib/metadata/mirror.c
index 087ce73..ed6f779 100644
--- a/lib/metadata/mirror.c
+++ b/lib/metadata/mirror.c
@@ -932,7 +932,7 @@ static int _create_mimage_lvs(struct alloc_handle *ah,
 	for (m = 0; m < num_mirrors; m++) {
 		if (!(img_lvs[m] = lv_create_empty(img_name,
 					     NULL, LVM_READ | LVM_WRITE,
-					     ALLOC_INHERIT, 0, lv->vg))) {
+					     ALLOC_INHERIT, lv->vg))) {
 			log_error("Aborting. Failed to create mirror image LV. "
 				  "Remove new LV and retry.");
 			return 0;
@@ -1266,7 +1266,7 @@ static struct logical_volume *_create_mirror_log(struct logical_volume *lv,
 
 	if (!(log_lv = lv_create_empty(log_name, NULL,
 				       VISIBLE_LV | LVM_READ | LVM_WRITE,
-				       alloc, 0, lv->vg)))
+				       alloc, lv->vg)))
 		return_NULL;
 
 	if (!lv_add_log_segment(ah, log_lv))
diff --git a/lib/metadata/snapshot_manip.c b/lib/metadata/snapshot_manip.c
index bf694f7..83bc385 100644
--- a/lib/metadata/snapshot_manip.c
+++ b/lib/metadata/snapshot_manip.c
@@ -105,7 +105,7 @@ int vg_add_snapshot(struct logical_volume *origin,
 
 	if (!(snap = lv_create_empty("snapshot%d",
 				     lvid, LVM_READ | LVM_WRITE | VISIBLE_LV,
-				     ALLOC_INHERIT, 1, origin->vg)))
+				     ALLOC_INHERIT, origin->vg)))
 		return_0;
 
 	snap->le_count = extent_count;
diff --git a/tools/lvcreate.c b/tools/lvcreate.c
index 069cc0e..1d06c4b 100644
--- a/tools/lvcreate.c
+++ b/tools/lvcreate.c
@@ -577,7 +577,7 @@ static struct logical_volume *_create_virtual_origin(struct cmd_context *cmd,
 	}
 
 	if (!(lv = lv_create_empty(vorigin_name, NULL, permission,
-				   ALLOC_INHERIT, 0, vg)))
+				   ALLOC_INHERIT, vg)))
 		return_0;
 
 	if (!lv_extend(lv, segtype, 1, 0, 1, voriginextents, NULL, 0u, 0u,
@@ -824,7 +824,7 @@ static int _lvcreate(struct cmd_context *cmd, struct volume_group *vg,
 	}
 
 	if (!(lv = lv_create_empty(lv_name ? lv_name : "lvol%d", NULL,
-				   status, lp->alloc, 0, vg)))
+				   status, lp->alloc, vg)))
 		return_0;
 
 	if (lp->read_ahead) {
diff --git a/tools/pvmove.c b/tools/pvmove.c
index 55285df..d760802 100644
--- a/tools/pvmove.c
+++ b/tools/pvmove.c
@@ -191,7 +191,7 @@ static struct logical_volume *_set_up_pvmove_lv(struct cmd_context *cmd,
 	/* FIXME Cope with non-contiguous => splitting existing segments */
 	if (!(lv_mirr = lv_create_empty("pvmove%d", NULL,
 					LVM_READ | LVM_WRITE,
-					ALLOC_CONTIGUOUS, 0, vg))) {
+					ALLOC_CONTIGUOUS, vg))) {
 		log_error("Creation of temporary pvmove LV failed");
 		return NULL;
 	}
-- 
1.6.2.4

[PATCH 9/9] Check max_lv on only place and force the check only for new volume.

[Milan Broz]

We can temporarily violate max_lv during mirror conversion etc.

(If the operation fails, orphan mirror images are visible to administrator
for manual remove for example. Not that this should ever happen:-)

Force limit only for lvcreate (and vg merge) command.

Patch also adds simple max_lv tests into testsuite

Signed-off-by: Milan Broz <mbroz@redhat.com>
---
 lib/metadata/lv_manip.c          |   24 +++++++++++++++++++-----
 lib/metadata/metadata-exported.h |    1 +
 lib/metadata/metadata.c          |    8 ++------
 test/t-lvcreate-usage.sh         |   23 ++++++++++++++++++++++-
 test/test-utils.sh               |    1 +
 tools/lvcreate.c                 |    6 ++++++
 6 files changed, 51 insertions(+), 12 deletions(-)

diff --git a/lib/metadata/lv_manip.c b/lib/metadata/lv_manip.c
index 1c758ea..b1ceef4 100644
--- a/lib/metadata/lv_manip.c
+++ b/lib/metadata/lv_manip.c
@@ -1805,6 +1805,20 @@ char *generate_lv_name(struct volume_group *vg, const char *format,
 	return buffer;
 }
 
+int vg_max_lv_reached(struct volume_group *vg)
+{
+	if (!vg->max_lv)
+		return 0;
+
+	if (vg->max_lv > volumes_count(vg))
+		return 0;
+
+	log_verbose("Maximum number of logical volumes (%u) reached "
+		    "in volume group %s", vg->max_lv, vg->name);
+
+	return 1;
+}
+
 /*
  * Create a new empty LV.
  */
@@ -1818,11 +1832,8 @@ struct logical_volume *lv_create_empty(const char *name,
 	struct logical_volume *lv;
 	char dname[NAME_LEN];
 
-	if (vg->max_lv && (vg->max_lv == volumes_count(vg))) {
-		log_error("Maximum number of logical volumes (%u) reached "
-			  "in volume group %s", vg->max_lv, vg->name);
-		return NULL;
-	}
+	if (vg_max_lv_reached(vg))
+		stack;
 
 	if (strstr(name, "%d") &&
 	    !(name = generate_lv_name(vg, name, dname, sizeof(dname)))) {
@@ -1937,6 +1948,9 @@ int vg_link_lv(struct volume_group *vg, struct logical_volume *lv)
 {
 	struct lv_list *lvl;
 
+	if (vg_max_lv_reached(vg))
+		stack;
+
 	if (!(lvl = dm_pool_zalloc(vg->vgmem, sizeof(*lvl))))
 		return_0;
 
diff --git a/lib/metadata/metadata-exported.h b/lib/metadata/metadata-exported.h
index 1477a0b..ca6a0c2 100644
--- a/lib/metadata/metadata-exported.h
+++ b/lib/metadata/metadata-exported.h
@@ -557,6 +557,7 @@ int vg_remove_snapshot(struct logical_volume *cow);
 int vg_check_status(const struct volume_group *vg, uint32_t status);
 
 unsigned volumes_count(const struct volume_group *vg);
+int vg_max_lv_reached(struct volume_group *vg);
 
 /*
 * Mirroring functions
diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
index 82ee6cb..0894320 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -1528,12 +1528,8 @@ int vg_validate(struct volume_group *vg)
 		r = 0;
 	}
 
-	if (vg->max_lv && (vg->max_lv < volumes_count(vg))) {
-		log_error("Internal error: Volume group %s contains %u volumes"
-			  " but the limit is set to %u.",
-			  vg->name, volumes_count(vg), vg->max_lv);
-		r = 0;
-	}
+	if (vg_max_lv_reached(vg))
+		stack;
 
 	return r;
 }
diff --git a/test/t-lvcreate-usage.sh b/test/t-lvcreate-usage.sh
index 6c60b15..43cf716 100755
--- a/test/t-lvcreate-usage.sh
+++ b/test/t-lvcreate-usage.sh
@@ -55,12 +55,33 @@ grep "^  Invalid stripe size 3\.00 KB\$" err
 case $(lvdisplay $vg) in "") true ;; *) false ;; esac
 
 # Setting max_lv works. (bz490298)
-vgchange -l 4 $vg
+lvremove -ff $vg
+vgchange -l 3 $vg
 lvcreate -l1 -n $lv1 $vg
 lvcreate -l1 -s -n $lv2 $vg/$lv1
 lvcreate -l1 -n $lv3 $vg
 not lvcreate -l1 -n $lv4 $vg
+
+lvremove -ff $vg/$lv3
+lvcreate -l1 -s -n $lv3 $vg/$lv1
+not lvcreate -l1 -n $lv4 $vg
+not lvcreate -l1 -m1 -n $lv4 $vg
+
+lvremove -ff $vg/$lv3
+lvcreate -l1 -m1 -n $lv3 $vg
+lvs
+vgs -o +max_lv
+not lvcreate -l1 -n $lv4 $vg
+not lvcreate -l1 -m1 -n $lv4 $vg
+
+lvconvert -m0 $vg/$lv3
+lvconvert -m2 -i 1 $vg/$lv3
+lvconvert -m1 $vg/$lv3
+
+not vgchange -l 2
+vgchange -l 4
 vgs $vg
+
 lvremove -ff $vg
 vgchange -l 0 $vg
 
diff --git a/test/test-utils.sh b/test/test-utils.sh
index e078a33..7c57972 100644
--- a/test/test-utils.sh
+++ b/test/test-utils.sh
@@ -128,6 +128,7 @@ prepare_devs() {
 	lv1=LV1
 	lv2=LV2
 	lv3=LV3
+	lv4=LV4
 }
 
 disable_dev() {
diff --git a/tools/lvcreate.c b/tools/lvcreate.c
index 1d06c4b..ed29855 100644
--- a/tools/lvcreate.c
+++ b/tools/lvcreate.c
@@ -613,6 +613,12 @@ static int _lvcreate(struct cmd_context *cmd, struct volume_group *vg,
 		return 0;
 	}
 
+	if (vg_max_lv_reached(vg)) {
+		log_error("Maximum number of logical volumes (%u) reached "
+			  "in volume group %s", vg->max_lv, vg->name);
+		return 0;
+	}
+
 	if (lp->mirrors > 1 && !(vg->fid->fmt->features & FMT_SEGMENTS)) {
 		log_error("Metadata does not support mirroring.");
 		return 0;
-- 
1.6.2.4