* [PATCH 0/7] Fix lv_count & max_lv problems in lvm2
@ 2009-05-06 14:42 Milan Broz
2009-05-06 14:42 ` [PATCH 1/7] Fix snapshot segment import to not use duplicate segments & replace Milan Broz
` (6 more replies)
0 siblings, 7 replies; 26+ messages in thread
From: Milan Broz @ 2009-05-06 14:42 UTC (permalink / raw)
To: lvm-devel
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 tries to consistenly use visible LV count and fixes
all code paths which are temporarily violating the restrictions.
See also bug https://bugzilla.redhat.com/show_bug.cgi?id=490298
(with reproducer)
Milan Broz (7):
Fix snapshot segment import to not use duplicate segments & replace.
Never set mirror log and images directly visible in metadata.
Remove snapshot_count from VG and use function instead.
Introduce vg_add_lc and vg_remove_lv functions.
Introduce lv_set_visible & lv_set_hidden and use lv_is_visible
always.
Merge lv_is_displayable and lv_is_visible.
Remove import paramater from lv_create_empty.
lib/activate/activate.c | 6 +-
lib/display/display.c | 4 +-
lib/format1/disk-rep.h | 3 +-
lib/format1/import-export.c | 28 ++++-----
lib/format_pool/format_pool.c | 1 -
lib/format_pool/import_export.c | 15 +----
lib/format_text/export.c | 4 +-
lib/format_text/import_vsn1.c | 22 +------
lib/metadata/lv_manip.c | 127 +++++++++++++++++++++++--------------
lib/metadata/metadata-exported.h | 20 +++---
lib/metadata/metadata.c | 35 +++++------
lib/metadata/metadata.h | 4 +-
lib/metadata/mirror.c | 20 +++----
lib/metadata/snapshot_manip.c | 44 ++++++-------
lib/report/columns.h | 4 +-
lib/report/report.c | 22 +++---
lib/snapshot/snapshot.c | 19 +++++-
test/t-lvcreate-usage.sh | 22 ++++++-
test/test-utils.sh | 1 +
tools/lvchange.c | 2 +-
tools/lvconvert.c | 3 +-
tools/lvcreate.c | 6 +-
tools/lvdisplay.c | 2 +-
tools/lvscan.c | 2 +-
tools/pvmove.c | 2 +-
tools/reporter.c | 4 +-
tools/vgmerge.c | 2 -
tools/vgsplit.c | 5 +-
28 files changed, 221 insertions(+), 208 deletions(-)
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 1/7] Fix snapshot segment import to not use duplicate segments & replace.
2009-05-06 14:42 [PATCH 0/7] Fix lv_count & max_lv problems in lvm2 Milan Broz
@ 2009-05-06 14:42 ` Milan Broz
2009-05-11 6:27 ` Petr Rockai
2009-05-06 14:42 ` [PATCH 2/7] Never set mirror log and images directly visible in metadata Milan Broz
` (5 subsequent siblings)
6 siblings, 1 reply; 26+ messages in thread
From: Milan Broz @ 2009-05-06 14:42 UTC (permalink / raw)
To: lvm-devel
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 text_import 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. The name is always gewerated, remove it 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 | 3 +--
lib/metadata/snapshot_manip.c | 4 ++--
lib/snapshot/snapshot.c | 23 ++++++++++++++++++++---
tools/lvconvert.c | 3 +--
tools/lvcreate.c | 2 +-
7 files changed, 26 insertions(+), 21 deletions(-)
diff --git a/lib/format1/import-export.c b/lib/format1/import-export.c
index 66e2390..4321965 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 3b13e37..d7099b7 100644
--- a/lib/metadata/metadata-exported.h
+++ b/lib/metadata/metadata-exported.h
@@ -547,8 +547,7 @@ 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,
+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 1189dbd..2297c54 100644
--- a/lib/metadata/snapshot_manip.c
+++ b/lib/metadata/snapshot_manip.c
@@ -62,7 +62,7 @@ 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,
+int vg_add_snapshot(struct logical_volume *origin,
struct logical_volume *cow, union lvid *lvid,
uint32_t extent_count, uint32_t chunk_size)
{
@@ -88,7 +88,7 @@ int vg_add_snapshot(const char *name, struct logical_volume *origin,
*/
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++;
diff --git a/lib/snapshot/snapshot.c b/lib/snapshot/snapshot.c
index 1fdf0ab..a9eea45 100644
--- a/lib/snapshot/snapshot.c
+++ b/lib/snapshot/snapshot.c
@@ -74,9 +74,26 @@ 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;
+ seg->chunk_size = chunk_size;
+ seg->origin = org;
+ seg->cow = cow;
+
+ // FIXME: direct count manipulation to be removed later
+ cow->status &= ~VISIBLE_LV;
+ cow->vg->lv_count--;
+ cow->snapshot = seg;
+
+ org->origin_count++;
+ org->vg->lv_count--;
+ org->vg->snapshot_count++;
+
+ /* FIXME Assumes an invisible origin belongs to a sparse device */
+ if (!lv_is_visible(org))
+ org->status |= VIRTUAL_ORIGIN;
+
+ seg->lv->status |= VIRTUAL;
+
+ dm_list_add(&org->snapshot_segs, &seg->origin_list);
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
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 2/7] Never set mirror log and images directly visible in metadata.
2009-05-06 14:42 [PATCH 0/7] Fix lv_count & max_lv problems in lvm2 Milan Broz
2009-05-06 14:42 ` [PATCH 1/7] Fix snapshot segment import to not use duplicate segments & replace Milan Broz
@ 2009-05-06 14:42 ` Milan Broz
2009-05-10 18:33 ` Petr Rockai
2009-05-11 1:54 ` Alasdair G Kergon
2009-05-06 14:43 ` [PATCH 3/7] Remove snapshot_count from VG and use function instead Milan Broz
` (4 subsequent siblings)
6 siblings, 2 replies; 26+ messages in thread
From: Milan Broz @ 2009-05-06 14:42 UTC (permalink / raw)
To: lvm-devel
Mirror creation and conversion force set mirror log and images
to be visible. This can violate VG max_lv restriction on LV.
There is no need to set these volumes visible, even during
mirror images conversion.
The log must be cleared, but the requested symlink is created
if the volume is manipulated directly.
[Is it true for udev?]
[Is there some side effect on cluster with this patch?]
Signed-off-by: Milan Broz <mbroz@redhat.com>
---
lib/metadata/mirror.c | 14 +++++---------
1 files changed, 5 insertions(+), 9 deletions(-)
diff --git a/lib/metadata/mirror.c b/lib/metadata/mirror.c
index 5eb53f4..678e54f 100644
--- a/lib/metadata/mirror.c
+++ b/lib/metadata/mirror.c
@@ -259,9 +259,6 @@ static int _init_mirror_log(struct cmd_context *cmd,
was_active = 1;
}
- /* Temporary make it visible for set_lv() */
- log_lv->status |= VISIBLE_LV;
-
/* Temporary tag mirror log for activation */
dm_list_iterate_items(sl, tags)
if (!str_list_add(cmd->mem, &log_lv->tags, sl->str)) {
@@ -275,6 +272,10 @@ static int _init_mirror_log(struct cmd_context *cmd,
backup(log_lv->vg);
+ /*
+ * Directly activating log_lv will create symlink for set_lv,
+ * even then log_lv is never directly visible
+ */
if (!activate_lv(cmd, log_lv)) {
log_error("Aborting. Failed to activate mirror log.");
goto revert_new_lv;
@@ -303,8 +304,6 @@ static int _init_mirror_log(struct cmd_context *cmd,
return 0;
}
- log_lv->status &= ~VISIBLE_LV;
-
if (was_active && !activate_lv(cmd, log_lv))
return_0;
@@ -410,7 +409,6 @@ 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;
log_lv->status &= ~MIRROR_LOG;
remove_seg_from_segs_using_this_lv(log_lv, mirrored_seg);
@@ -536,7 +534,6 @@ 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;
if (!(lvl = dm_pool_alloc(lv->vg->cmd->mem, sizeof(*lvl)))) {
log_error("lv_list alloc failed");
return 0;
@@ -554,7 +551,6 @@ 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;
detached_log_lv = detach_mirror_log(mirrored_seg);
if (!remove_layer_from_lv(lv, lv1))
return_0;
@@ -1265,7 +1261,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,
+ LVM_READ | LVM_WRITE,
alloc, 0, lv->vg)))
return_NULL;
--
1.6.2.4
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 3/7] Remove snapshot_count from VG and use function instead.
2009-05-06 14:42 [PATCH 0/7] Fix lv_count & max_lv problems in lvm2 Milan Broz
2009-05-06 14:42 ` [PATCH 1/7] Fix snapshot segment import to not use duplicate segments & replace Milan Broz
2009-05-06 14:42 ` [PATCH 2/7] Never set mirror log and images directly visible in metadata Milan Broz
@ 2009-05-06 14:43 ` Milan Broz
2009-05-10 18:41 ` Petr Rockai
2009-05-11 11:42 ` Alasdair G Kergon
2009-05-06 14:43 ` [PATCH 4/7] Introduce vg_add_lc and vg_remove_lv functions Milan Broz
` (3 subsequent siblings)
6 siblings, 2 replies; 26+ messages in thread
From: Milan Broz @ 2009-05-06 14:43 UTC (permalink / raw)
To: lvm-devel
Signed-off-by: Milan Broz <mbroz@redhat.com>
---
lib/format1/import-export.c | 2 +-
lib/format_pool/format_pool.c | 1 -
lib/metadata/metadata-exported.h | 8 ++------
lib/metadata/metadata.c | 18 ++++++++++++++----
lib/metadata/metadata.h | 5 +++++
lib/metadata/snapshot_manip.c | 2 --
lib/report/columns.h | 2 +-
lib/report/report.c | 12 ++++++++++++
lib/snapshot/snapshot.c | 1 -
tools/vgmerge.c | 2 --
tools/vgsplit.c | 5 +----
11 files changed, 36 insertions(+), 22 deletions(-)
diff --git a/lib/format1/import-export.c b/lib/format1/import-export.c
index 4321965..627ee05 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 + vg->snapshot_count;
+ vgd->lv_cur = vg->lv_count + snapshot_lvs_in_vg(vg);
vgd->pv_max = vg->max_pv;
vgd->pv_cur = vg->pv_count;
diff --git a/lib/format_pool/format_pool.c b/lib/format_pool/format_pool.c
index 9a34f94..114cec1 100644
--- a/lib/format_pool/format_pool.c
+++ b/lib/format_pool/format_pool.c
@@ -120,7 +120,6 @@ static struct volume_group *_build_vg_from_pds(struct format_instance
vg->extent_count = 0;
vg->pv_count = 0;
vg->lv_count = 0;
- vg->snapshot_count = 0;
vg->seqno = 1;
vg->system_id = NULL;
dm_list_init(&vg->pvs);
diff --git a/lib/metadata/metadata-exported.h b/lib/metadata/metadata-exported.h
index d7099b7..92728ff 100644
--- a/lib/metadata/metadata-exported.h
+++ b/lib/metadata/metadata-exported.h
@@ -236,15 +236,12 @@ struct volume_group {
struct dm_list pvs;
/*
- * logical volumes
- * The following relationship should always hold:
- * dm_list_size(lvs) = lv_count + 2 * snapshot_count
+ * logical volumes count
*
* 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, but we
- * multiply the snapshot_count by 2.
+ * Neither of these instances is reflected in lv_count.
*
* Mirrors consist of multiple instances of "struct logical_volume":
* - one for the mirror log
@@ -253,7 +250,6 @@ struct volume_group {
* all of the instances are reflected in lv_count.
*/
uint32_t lv_count;
- uint32_t snapshot_count;
struct dm_list lvs;
struct dm_list tags;
diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
index 13ab8f3..50762f6 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -574,8 +574,6 @@ struct volume_group *vg_create(struct cmd_context *cmd, const char *vg_name,
vg->lv_count = 0;
dm_list_init(&vg->lvs);
- vg->snapshot_count = 0;
-
dm_list_init(&vg->tags);
if (!(vg->fid = cmd->fmt->ops->create_instance(cmd->fmt, vg_name,
@@ -1155,6 +1153,18 @@ unsigned displayable_lvs_in_vg(const struct volume_group *vg)
return lv_count;
}
+unsigned snapshot_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_cow(lvl->lv))
+ lv_count++;
+
+ return lv_count;
+}
+
/*
* Determine whether two vgs are compatible for merging.
*/
@@ -1445,11 +1455,11 @@ int vg_validate(struct volume_group *vg)
}
if ((lv_count = (uint32_t) dm_list_size(&vg->lvs)) !=
- vg->lv_count + 2 * vg->snapshot_count) {
+ vg->lv_count + 2 * snapshot_lvs_in_vg(vg)) {
log_error("Internal error: #internal LVs (%u) != #LVs (%"
PRIu32 ") + 2 * #snapshots (%" PRIu32 ") in VG %s",
dm_list_size(&vg->lvs), vg->lv_count,
- vg->snapshot_count, vg->name);
+ snapshot_lvs_in_vg(vg), vg->name);
r = 0;
}
diff --git a/lib/metadata/metadata.h b/lib/metadata/metadata.h
index eff2380..410a9f9 100644
--- a/lib/metadata/metadata.h
+++ b/lib/metadata/metadata.h
@@ -345,6 +345,11 @@ struct lv_segment *get_only_segment_using_this_lv(struct logical_volume *lv);
unsigned displayable_lvs_in_vg(const struct volume_group *vg);
/*
+ * Count snapshot LVs.
+ */
+unsigned snapshot_lvs_in_vg(const struct volume_group *vg);
+
+/*
* For internal metadata caching.
*/
int export_vg_to_buffer(struct volume_group *vg, char **buf);
diff --git a/lib/metadata/snapshot_manip.c b/lib/metadata/snapshot_manip.c
index 2297c54..cc57aeb 100644
--- a/lib/metadata/snapshot_manip.c
+++ b/lib/metadata/snapshot_manip.c
@@ -106,7 +106,6 @@ int vg_add_snapshot(struct logical_volume *origin,
seg->lv->status |= SNAPSHOT;
origin->origin_count++;
- origin->vg->snapshot_count++;
cow->snapshot = seg;
cow->status &= ~VISIBLE_LV;
@@ -133,7 +132,6 @@ int vg_remove_snapshot(struct logical_volume *cow)
cow->snapshot = NULL;
- cow->vg->snapshot_count--;
cow->vg->lv_count++;
cow->status |= VISIBLE_LV;
diff --git a/lib/report/columns.h b/lib/report/columns.h
index 707347b..6ca24e5 100644
--- a/lib/report/columns.h
+++ b/lib/report/columns.h
@@ -105,7 +105,7 @@ FIELD(VGS, vg, NUM, "MaxLV", max_lv, 5, uint32, "max_lv", "Maximum number of LVs
FIELD(VGS, vg, NUM, "MaxPV", max_pv, 5, uint32, "max_pv", "Maximum number of PVs allowed in VG or 0 if unlimited.")
FIELD(VGS, vg, NUM, "#PV", pv_count, 3, uint32, "pv_count", "Number of PVs.")
FIELD(VGS, vg, NUM, "#LV", cmd, 3, lvcount, "lv_count", "Number of LVs.")
-FIELD(VGS, vg, NUM, "#SN", snapshot_count, 3, uint32, "snap_count", "Number of snapshots.")
+FIELD(VGS, vg, NUM, "#SN", cmd, 3, snapcount, "snap_count", "Number of snapshots.")
FIELD(VGS, vg, NUM, "Seq", seqno, 3, uint32, "vg_seqno", "Revision number of internal metadata. Incremented whenever it changes.")
FIELD(VGS, vg, STR, "VG Tags", tags, 7, tags, "vg_tags", "Tags, if any.")
FIELD(VGS, vg, NUM, "#VMda", cmd, 5, vgmdas, "vg_mda_count", "Number of metadata areas in use by this VG.")
diff --git a/lib/report/report.c b/lib/report/report.c
index 91e2b98..b02fdd8 100644
--- a/lib/report/report.c
+++ b/lib/report/report.c
@@ -991,6 +991,18 @@ static int _lvsegcount_disp(struct dm_report *rh, struct dm_pool *mem,
return _uint32_disp(rh, mem, field, &count, private);
}
+static int _snapcount_disp(struct dm_report *rh, struct dm_pool *mem,
+ struct dm_report_field *field,
+ const void *data, void *private)
+{
+ const struct volume_group *vg = (const struct volume_group *) data;
+ uint32_t count;
+
+ count = snapshot_lvs_in_vg(vg);
+
+ return _uint32_disp(rh, mem, field, &count, private);
+}
+
static int _snpercent_disp(struct dm_report *rh __attribute((unused)), struct dm_pool *mem,
struct dm_report_field *field,
const void *data, void *private __attribute((unused)))
diff --git a/lib/snapshot/snapshot.c b/lib/snapshot/snapshot.c
index a9eea45..ad5d158 100644
--- a/lib/snapshot/snapshot.c
+++ b/lib/snapshot/snapshot.c
@@ -85,7 +85,6 @@ static int _snap_text_import(struct lv_segment *seg, const struct config_node *s
org->origin_count++;
org->vg->lv_count--;
- org->vg->snapshot_count++;
/* FIXME Assumes an invisible origin belongs to a sparse device */
if (!lv_is_visible(org))
diff --git a/tools/vgmerge.c b/tools/vgmerge.c
index c847c7e..82c3456 100644
--- a/tools/vgmerge.c
+++ b/tools/vgmerge.c
@@ -102,8 +102,6 @@ static int _vgmerge_single(struct cmd_context *cmd, const char *vg_name_to,
}
vg_to->lv_count += vg_from->lv_count;
- vg_to->snapshot_count += vg_from->snapshot_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 87e74a4..c4b583a 100644
--- a/tools/vgsplit.c
+++ b/tools/vgsplit.c
@@ -106,10 +106,7 @@ static int _move_one_lv(struct volume_group *vg_from,
return 0;
}
- if (lv->status & SNAPSHOT) {
- vg_from->snapshot_count--;
- vg_to->snapshot_count++;
- } else if (!lv_is_cow(lv)) {
+ if (!(lv->status & SNAPSHOT) && !lv_is_cow(lv)) {
vg_from->lv_count--;
vg_to->lv_count++;
}
--
1.6.2.4
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 4/7] Introduce vg_add_lc and vg_remove_lv functions.
2009-05-06 14:42 [PATCH 0/7] Fix lv_count & max_lv problems in lvm2 Milan Broz
` (2 preceding siblings ...)
2009-05-06 14:43 ` [PATCH 3/7] Remove snapshot_count from VG and use function instead Milan Broz
@ 2009-05-06 14:43 ` Milan Broz
2009-05-10 19:06 ` Petr Rockai
2009-05-06 14:43 ` [PATCH 5/7] Introduce lv_set_visible & lv_set_hidden and use lv_is_visible always Milan Broz
` (2 subsequent siblings)
6 siblings, 1 reply; 26+ messages in thread
From: Milan Broz @ 2009-05-06 14:43 UTC (permalink / raw)
To: lvm-devel
vg_add_lv and vg_remove_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/disk-rep.h | 3 +-
lib/format1/import-export.c | 24 +++++-------
lib/format_pool/import_export.c | 15 +------
lib/format_text/import_vsn1.c | 12 +-----
lib/metadata/lv_manip.c | 81 ++++++++++++++++++++++---------------
lib/metadata/metadata-exported.h | 3 +
6 files changed, 67 insertions(+), 71 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 627ee05..573836d 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;
@@ -456,22 +455,19 @@ 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;
- if (!import_lv(mem, lv, lvd))
- return_NULL;
+ lvid_from_lvnum(&lv->lvid, &vg->id, lvd->lv_number);
- dm_list_add(&vg->lvs, &ll->list);
- vg->lv_count++;
+ if (!import_lv(vg->cmd, mem, lv, lvd)) {
+ dm_pool_free(mem, lv);
+ return_NULL;
+ }
- return lv;
+ return vg_add_lv(vg, lv) ? lv : 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 4d2163d..91e3bcd 100644
--- a/lib/format_pool/import_export.c
+++ b/lib/format_pool/import_export.c
@@ -58,22 +58,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;
@@ -115,11 +107,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);
- vg->lv_count++;
- return 1;
+ return vg_add_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 a9748aa..52ef075 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,11 +557,7 @@ static int _read_lvnames(struct format_instance *fid __attribute((unused)),
return 0;
}
- lv->vg = vg;
- vg->lv_count++;
- dm_list_add(&vg->lvs, &lvl->list);
-
- return 1;
+ return vg_add_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 7214b27..1903320 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,13 +433,11 @@ 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_remove_lv(lv))
return_0;
-
- dm_list_del(&lvl->list);
-
- if (!(lv->status & SNAPSHOT))
- lv->vg->lv_count--;
+ //FIXME: cow still in VG, fix count, remove this later
+ 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;
@@ -1822,8 +1819,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];
@@ -1843,23 +1838,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;
- }
+ if (!(lv = dm_pool_zalloc(vg->vgmem, sizeof(*lv))))
+ return_NULL;
- lv = ll->lv;
- lv->vg = vg;
-
- 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;
@@ -1877,18 +1860,20 @@ 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;
- }
+ if (!vg_add_lv(vg, lv))
+ goto_bad;
- if (!import)
- vg->lv_count++;
+ if (fi->fmt->ops->lv_setup && !fi->fmt->ops->lv_setup(fi, lv))
+ goto_bad;
- dm_list_add(&vg->lvs, &ll->list);
+ //FIXME: remove that
+ if (import)
+ vg->lv_count--;
return lv;
+bad:
+ dm_pool_free(vg->vgmem, lv);
+ return_NULL;
}
static int _add_pvs(struct cmd_context *cmd, struct pv_segment *peg,
@@ -1957,6 +1942,36 @@ struct dm_list *build_parallel_areas_from_lv(struct cmd_context *cmd,
return parallel_areas;
}
+int vg_add_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);
+
+ vg->lv_count++;
+
+ return 1;
+}
+
+int vg_remove_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);
+
+ lvl->lv->vg->lv_count--;
+
+ 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 92728ff..92942e1 100644
--- a/lib/metadata/metadata-exported.h
+++ b/lib/metadata/metadata-exported.h
@@ -367,6 +367,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_add_lv(struct volume_group *vg, struct logical_volume *lv);
+int vg_remove_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
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 5/7] Introduce lv_set_visible & lv_set_hidden and use lv_is_visible always.
2009-05-06 14:42 [PATCH 0/7] Fix lv_count & max_lv problems in lvm2 Milan Broz
` (3 preceding siblings ...)
2009-05-06 14:43 ` [PATCH 4/7] Introduce vg_add_lc and vg_remove_lv functions Milan Broz
@ 2009-05-06 14:43 ` Milan Broz
2009-05-10 19:23 ` Petr Rockai
2009-05-06 14:43 ` [PATCH 6/7] Merge lv_is_displayable and lv_is_visible Milan Broz
2009-05-06 14:43 ` [PATCH 7/7] Remove import paramater from lv_create_empty Milan Broz
6 siblings, 1 reply; 26+ messages in thread
From: Milan Broz @ 2009-05-06 14:43 UTC (permalink / raw)
To: lvm-devel
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
use exception here.
Signed-off-by: Milan Broz <mbroz@redhat.com>
---
lib/activate/activate.c | 4 +-
lib/metadata/lv_manip.c | 45 ++++++++++++++++++++++++++-----------
lib/metadata/metadata-exported.h | 2 +
lib/metadata/metadata.c | 16 ++++++++-----
lib/metadata/mirror.c | 2 +-
lib/metadata/snapshot_manip.c | 28 +++++++++++++++--------
lib/snapshot/snapshot.c | 9 ++-----
tools/vgsplit.c | 2 +-
8 files changed, 68 insertions(+), 40 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 1903320..705fa2b 100644
--- a/lib/metadata/lv_manip.c
+++ b/lib/metadata/lv_manip.c
@@ -432,13 +432,9 @@ static int _lv_reduce(struct logical_volume *lv, uint32_t extents, int delete)
return 1;
/* Remove the LV if it is now empty */
- if (!lv->le_count) {
- if(!vg_remove_lv(lv))
- return_0;
- //FIXME: cow still in VG, fix count, remove this later
- if (lv->status & SNAPSHOT)
- lv->vg->lv_count++;
- } else if (lv->vg->fid->fmt->ops->lv_setup &&
+ if (!lv->le_count && !vg_remove_lv(lv))
+ return_0;
+ else if (lv->vg->fid->fmt->ops->lv_setup &&
!lv->vg->fid->fmt->ops->lv_setup(lv->vg->fid, lv))
return_0;
@@ -1507,7 +1503,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;
@@ -1866,10 +1862,6 @@ struct logical_volume *lv_create_empty(const char *name,
if (fi->fmt->ops->lv_setup && !fi->fmt->ops->lv_setup(fi, lv))
goto_bad;
- //FIXME: remove that
- if (import)
- vg->lv_count--;
-
return lv;
bad:
dm_pool_free(vg->vgmem, lv);
@@ -1953,7 +1945,8 @@ int vg_add_lv(struct volume_group *vg, struct logical_volume *lv)
lv->vg = vg;
dm_list_add(&vg->lvs, &lvl->list);
- vg->lv_count++;
+ if (lv_is_visible(lv))
+ vg->lv_count++;
return 1;
}
@@ -1967,11 +1960,35 @@ int vg_remove_lv(struct logical_volume *lv)
dm_list_del(&lvl->list);
- lvl->lv->vg->lv_count--;
+ if (lv_is_visible(lvl->lv))
+ lvl->lv->vg->lv_count--;
return 1;
}
+void lv_set_visible(struct logical_volume *lv)
+{
+ if (lv_is_visible(lv))
+ return;
+
+ if (lv->status & SNAPSHOT)
+ return;
+
+ log_debug("LV %s in VG %s is now visible.", lv->name, lv->vg->name);
+ lv->status |= VISIBLE_LV;
+ lv->vg->lv_count++;
+}
+
+void lv_set_hidden(struct logical_volume *lv)
+{
+ if (!lv_is_visible(lv))
+ return;
+
+ log_debug("LV %s in VG %s is now hidden.", lv->name, lv->vg->name);
+ lv->status &= ~VISIBLE_LV;
+ lv->vg->lv_count--;
+}
+
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 92942e1..ac5de87 100644
--- a/lib/metadata/metadata-exported.h
+++ b/lib/metadata/metadata-exported.h
@@ -369,6 +369,8 @@ struct dm_list *get_pvs(struct cmd_context *cmd);
int vg_add_lv(struct volume_group *vg, struct logical_volume *lv);
int vg_remove_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/metadata.c b/lib/metadata/metadata.c
index 50762f6..745083b 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -1454,12 +1454,16 @@ int vg_validate(struct volume_group *vg)
r = 0;
}
- if ((lv_count = (uint32_t) dm_list_size(&vg->lvs)) !=
- vg->lv_count + 2 * snapshot_lvs_in_vg(vg)) {
- log_error("Internal error: #internal LVs (%u) != #LVs (%"
- PRIu32 ") + 2 * #snapshots (%" PRIu32 ") in VG %s",
- dm_list_size(&vg->lvs), vg->lv_count,
- snapshot_lvs_in_vg(vg), vg->name);
+ lv_count = 0;
+ dm_list_iterate_items(lvl, &vg->lvs)
+ if (lv_is_visible(lvl->lv))
+ lv_count++;
+
+ if (lv_count != vg->lv_count) {
+ log_error("Internal error: #internal visible LVs (%u) != "
+ "visible #LVs (%u of %" PRIu32 ") in VG %s",
+ lv_count, vg->lv_count, dm_list_size(&vg->lvs),
+ vg->name);
r = 0;
}
diff --git a/lib/metadata/mirror.c b/lib/metadata/mirror.c
index 678e54f..f46381e 100644
--- a/lib/metadata/mirror.c
+++ b/lib/metadata/mirror.c
@@ -1336,7 +1336,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 cc57aeb..852ad76 100644
--- a/lib/metadata/snapshot_manip.c
+++ b/lib/metadata/snapshot_manip.c
@@ -30,8 +30,11 @@ int lv_is_cow(const struct logical_volume *lv)
int lv_is_visible(const struct logical_volume *lv)
{
+ if (lv->status & SNAPSHOT)
+ return 0;
+
if (lv_is_cow(lv))
- return lv_is_visible(find_cow(lv)->lv);
+ return lv_is_visible(origin_from_cow(lv));
return lv->status & VISIBLE_LV ? 1 : 0;
}
@@ -83,15 +86,15 @@ int vg_add_snapshot(struct logical_volume *origin,
}
/*
- * Set origin lv count in advance to prevent fail because
+ * Remove COW from visible volumes in advance to prevent fail because
* of temporary violation of LV limits.
*/
- origin->vg->lv_count--;
+ lv_set_hidden(cow);
if (!(snap = lv_create_empty("snapshot%d",
lvid, LVM_READ | LVM_WRITE | VISIBLE_LV,
ALLOC_INHERIT, 1, origin->vg))) {
- origin->vg->lv_count++;
+ lv_set_visible(cow);
return_0;
}
@@ -108,11 +111,11 @@ int vg_add_snapshot(struct logical_volume *origin,
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))
+ if (!lv_is_visible(origin)) {
+ origin->vg->lv_count--;
origin->status |= VIRTUAL_ORIGIN;
+ }
dm_list_add(&origin->snapshot_segs, &seg->origin_list);
@@ -130,10 +133,15 @@ int vg_remove_snapshot(struct logical_volume *cow)
return 0;
}
- cow->snapshot = NULL;
+ /*
+ * Virtual snapshot volume was removed
+ * LV count must be fixed...
+ */
+ if (!lv_is_virtual_origin(cow->snapshot->origin))
+ cow->vg->lv_count--;
- cow->vg->lv_count++;
- cow->status |= VISIBLE_LV;
+ lv_set_visible(cow);
+ cow->snapshot = NULL;
return 1;
}
diff --git a/lib/snapshot/snapshot.c b/lib/snapshot/snapshot.c
index ad5d158..4dc7111 100644
--- a/lib/snapshot/snapshot.c
+++ b/lib/snapshot/snapshot.c
@@ -78,17 +78,14 @@ static int _snap_text_import(struct lv_segment *seg, const struct config_node *s
seg->origin = org;
seg->cow = cow;
- // FIXME: direct count manipulation to be removed later
- cow->status &= ~VISIBLE_LV;
- cow->vg->lv_count--;
cow->snapshot = seg;
-
org->origin_count++;
- org->vg->lv_count--;
/* FIXME Assumes an invisible origin belongs to a sparse device */
- if (!lv_is_visible(org))
+ if (!lv_is_visible(org)) {
+ org->vg->lv_count--;
org->status |= VIRTUAL_ORIGIN;
+ }
seg->lv->status |= VIRTUAL;
diff --git a/tools/vgsplit.c b/tools/vgsplit.c
index c4b583a..a960302 100644
--- a/tools/vgsplit.c
+++ b/tools/vgsplit.c
@@ -106,7 +106,7 @@ static int _move_one_lv(struct volume_group *vg_from,
return 0;
}
- if (!(lv->status & SNAPSHOT) && !lv_is_cow(lv)) {
+ if (lv_is_visible(lv)) {
vg_from->lv_count--;
vg_to->lv_count++;
}
--
1.6.2.4
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 6/7] Merge lv_is_displayable and lv_is_visible.
2009-05-06 14:42 [PATCH 0/7] Fix lv_count & max_lv problems in lvm2 Milan Broz
` (4 preceding siblings ...)
2009-05-06 14:43 ` [PATCH 5/7] Introduce lv_set_visible & lv_set_hidden and use lv_is_visible always Milan Broz
@ 2009-05-06 14:43 ` Milan Broz
2009-05-11 6:09 ` Petr Rockai
2009-05-06 14:43 ` [PATCH 7/7] Remove import paramater from lv_create_empty Milan Broz
6 siblings, 1 reply; 26+ messages in thread
From: Milan Broz @ 2009-05-06 14:43 UTC (permalink / raw)
To: lvm-devel
Displayable and visible is the same thing, stored
in lv_count volume group attribute.
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 | 3 +--
lib/metadata/metadata.c | 25 ++++---------------------
lib/metadata/metadata.h | 5 -----
lib/metadata/snapshot_manip.c | 8 --------
lib/report/columns.h | 2 +-
lib/report/report.c | 14 +-------------
tools/lvchange.c | 2 +-
tools/lvdisplay.c | 2 +-
tools/lvscan.c | 2 +-
tools/reporter.c | 4 ++--
14 files changed, 18 insertions(+), 61 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..042d760 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", vg->lv_count);
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),
+ vg->lv_count,
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 705fa2b..fa83614 100644
--- a/lib/metadata/lv_manip.c
+++ b/lib/metadata/lv_manip.c
@@ -1732,7 +1732,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 ac5de87..ffb1510 100644
--- a/lib/metadata/metadata-exported.h
+++ b/lib/metadata/metadata-exported.h
@@ -535,10 +535,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 745083b..ceea447 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -362,7 +362,6 @@ int vg_remove_single(struct cmd_context *cmd, const char *vg_name,
{
struct physical_volume *pv;
struct pv_list *pvl;
- unsigned lv_count;
int ret = 1;
if (!vg || !consistent || vg_missing_pv_count(vg)) {
@@ -376,14 +375,12 @@ 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);
-
- if (lv_count) {
+ if (vg->lv_count) {
if ((force == PROMPT) &&
(yes_no_prompt("Do you really want to remove volume "
"group \"%s\" containing %u "
"logical volumes? [y/n]: ",
- vg_name, lv_count) == 'n')) {
+ vg_name, vg->lv_count) == 'n')) {
log_print("Volume group \"%s\" not removed", vg_name);
return 0;
}
@@ -391,11 +388,9 @@ int vg_remove_single(struct cmd_context *cmd, const char *vg_name,
return 0;
}
- lv_count = displayable_lvs_in_vg(vg);
-
- if (lv_count) {
+ if (vg->lv_count) {
log_error("Volume group \"%s\" still contains %u "
- "logical volume(s)", vg_name, lv_count);
+ "logical volume(s)", vg_name, vg->lv_count);
return 0;
}
@@ -1141,18 +1136,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_lvs_in_vg(const struct volume_group *vg)
{
struct lv_list *lvl;
diff --git a/lib/metadata/metadata.h b/lib/metadata/metadata.h
index 410a9f9..b1ea5cb 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_lvs_in_vg(const struct volume_group *vg);
diff --git a/lib/metadata/snapshot_manip.c b/lib/metadata/snapshot_manip.c
index 852ad76..154c404 100644
--- a/lib/metadata/snapshot_manip.c
+++ b/lib/metadata/snapshot_manip.c
@@ -39,14 +39,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/columns.h b/lib/report/columns.h
index 6ca24e5..8366a40 100644
--- a/lib/report/columns.h
+++ b/lib/report/columns.h
@@ -104,7 +104,7 @@ FIELD(VGS, vg, NUM, "Free", free_count, 4, uint32, "vg_free_count", "Total numbe
FIELD(VGS, vg, NUM, "MaxLV", max_lv, 5, uint32, "max_lv", "Maximum number of LVs allowed in VG or 0 if unlimited.")
FIELD(VGS, vg, NUM, "MaxPV", max_pv, 5, uint32, "max_pv", "Maximum number of PVs allowed in VG or 0 if unlimited.")
FIELD(VGS, vg, NUM, "#PV", pv_count, 3, uint32, "pv_count", "Number of PVs.")
-FIELD(VGS, vg, NUM, "#LV", cmd, 3, lvcount, "lv_count", "Number of LVs.")
+FIELD(VGS, vg, NUM, "#LV", lv_count, 3, uint32, "lv_count", "Number of LVs.")
FIELD(VGS, vg, NUM, "#SN", cmd, 3, snapcount, "snap_count", "Number of snapshots.")
FIELD(VGS, vg, NUM, "Seq", seqno, 3, uint32, "vg_seqno", "Revision number of internal metadata. Incremented whenever it changes.")
FIELD(VGS, vg, STR, "VG Tags", tags, 7, tags, "vg_tags", "Tags, if any.")
diff --git a/lib/report/report.c b/lib/report/report.c
index b02fdd8..0a1eeb1 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);
}
@@ -967,18 +967,6 @@ static int _vgmdafree_disp(struct dm_report *rh, struct dm_pool *mem,
return _size64_disp(rh, mem, field, &freespace, private);
}
-static int _lvcount_disp(struct dm_report *rh, struct dm_pool *mem,
- struct dm_report_field *field,
- const void *data, void *private)
-{
- const struct volume_group *vg = (const struct volume_group *) data;
- uint32_t count;
-
- count = displayable_lvs_in_vg(vg);
-
- return _uint32_disp(rh, mem, field, &count, private);
-}
-
static int _lvsegcount_disp(struct dm_report *rh, struct dm_pool *mem,
struct dm_report_field *field,
const void *data, void *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
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 7/7] Remove import paramater from lv_create_empty.
2009-05-06 14:42 [PATCH 0/7] Fix lv_count & max_lv problems in lvm2 Milan Broz
` (5 preceding siblings ...)
2009-05-06 14:43 ` [PATCH 6/7] Merge lv_is_displayable and lv_is_visible Milan Broz
@ 2009-05-06 14:43 ` Milan Broz
2009-05-11 6:15 ` Petr Rockai
6 siblings, 1 reply; 26+ messages in thread
From: Milan Broz @ 2009-05-06 14:43 UTC (permalink / raw)
To: lvm-devel
Now the max_lv check is performed in vg_add_lv and import
parameter in lv_create_empty has no use.
Patch also adds simple max_lv tests into testsuite
Signed-off-by: Milan Broz <mbroz@redhat.com>
---
lib/metadata/lv_manip.c | 17 ++++++++---------
lib/metadata/metadata-exported.h | 1 -
lib/metadata/mirror.c | 4 ++--
lib/metadata/snapshot_manip.c | 2 +-
test/t-lvcreate-usage.sh | 22 ++++++++++++++++++++--
test/test-utils.sh | 1 +
tools/lvcreate.c | 4 ++--
tools/pvmove.c | 2 +-
8 files changed, 35 insertions(+), 18 deletions(-)
diff --git a/lib/metadata/lv_manip.c b/lib/metadata/lv_manip.c
index fa83614..0ddf173 100644
--- a/lib/metadata/lv_manip.c
+++ b/lib/metadata/lv_manip.c
@@ -1811,19 +1811,12 @@ 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;
struct logical_volume *lv;
char dname[NAME_LEN];
- if (vg->max_lv && (vg->max_lv == vg->lv_count)) {
- log_error("Maximum number of logical volumes (%u) reached "
- "in volume group %s", vg->max_lv, vg->name);
- return NULL;
- }
-
if (strstr(name, "%d") &&
!(name = generate_lv_name(vg, name, dname, sizeof(dname)))) {
log_error("Failed to generate unique name for the new "
@@ -1831,7 +1824,7 @@ struct logical_volume *lv_create_empty(const char *name,
return NULL;
}
- if (!import)
+ if (status & VISIBLE_LV)
log_verbose("Creating logical volume %s", name);
if (!(lv = dm_pool_zalloc(vg->vgmem, sizeof(*lv))))
@@ -1938,6 +1931,12 @@ int vg_add_lv(struct volume_group *vg, struct logical_volume *lv)
{
struct lv_list *lvl;
+ if (lv_is_visible(lv) && vg->max_lv && vg->max_lv <= vg->lv_count) {
+ log_error("Maximum number of logical volumes (%u) reached "
+ "in volume group %s", vg->max_lv, vg->name);
+ return 0;
+ }
+
if (!(lvl = dm_pool_zalloc(vg->vgmem, sizeof(*lvl))))
return_0;
@@ -2426,7 +2425,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 ffb1510..85000a5 100644
--- a/lib/metadata/metadata-exported.h
+++ b/lib/metadata/metadata-exported.h
@@ -446,7 +446,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 f46381e..6c2cc10 100644
--- a/lib/metadata/mirror.c
+++ b/lib/metadata/mirror.c
@@ -928,7 +928,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;
@@ -1262,7 +1262,7 @@ static struct logical_volume *_create_mirror_log(struct logical_volume *lv,
if (!(log_lv = lv_create_empty(log_name, NULL,
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 154c404..379587f 100644
--- a/lib/metadata/snapshot_manip.c
+++ b/lib/metadata/snapshot_manip.c
@@ -85,7 +85,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))) {
lv_set_visible(cow);
return_0;
}
diff --git a/test/t-lvcreate-usage.sh b/test/t-lvcreate-usage.sh
index 13abbdf..48a4f4e 100755
--- a/test/t-lvcreate-usage.sh
+++ b/test/t-lvcreate-usage.sh
@@ -13,7 +13,7 @@
. ./test-utils.sh
-aux prepare_pvs 2
+aux prepare_pvs 4
aux pvcreate --metadatacopies 0 $dev1
vgcreate -cn $vg $devs
@@ -59,11 +59,29 @@ grep "^ Invalid stripe size 3\.00 KB\$" err
case $(lvdisplay $vg) in "") true ;; *) false ;; esac
# Setting max_lv works. (bz490298)
-vgchange -l 4 $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
+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 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
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 2/7] Never set mirror log and images directly visible in metadata.
2009-05-06 14:42 ` [PATCH 2/7] Never set mirror log and images directly visible in metadata Milan Broz
@ 2009-05-10 18:33 ` Petr Rockai
2009-05-10 18:42 ` Milan Broz
2009-05-11 1:39 ` Alasdair G Kergon
2009-05-11 1:54 ` Alasdair G Kergon
1 sibling, 2 replies; 26+ messages in thread
From: Petr Rockai @ 2009-05-10 18:33 UTC (permalink / raw)
To: lvm-devel
Milan Broz <mbroz@redhat.com> writes:
> Mirror creation and conversion force set mirror log and images
> to be visible. This can violate VG max_lv restriction on LV.
>
> There is no need to set these volumes visible, even during
> mirror images conversion.
Well, I believe the use-case is that when the operation fails halfway (which
happens from time to time -- more often than would be comfortable, even), you
can clean up by hand, since those offending LVs became visible. If there is a
reasonable procedure that the admins can follow to get rid of the junk LVs in
those scenarios, I'm fine.
(The whole non-atomicity of those mirror operation is very unfortunate for
other reasons, but I didn't yet have time to investigate how hard it would be
to make those really atomic.)
Other than that, I have reviewed the patch, and it does what the description
advertises, so (bar the above issue) it is good to go, IMHO.
--
Peter Rockai | me()mornfall!net | prockai()redhat!com
http://blog.mornfall.net | http://web.mornfall.net
"In My Egotistical Opinion, most people's C programs should be
indented six feet downward and covered with dirt."
-- Blair P. Houghton on the subject of C program indentation
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 3/7] Remove snapshot_count from VG and use function instead.
2009-05-06 14:43 ` [PATCH 3/7] Remove snapshot_count from VG and use function instead Milan Broz
@ 2009-05-10 18:41 ` Petr Rockai
2009-05-11 11:48 ` Alasdair G Kergon
2009-05-11 11:42 ` Alasdair G Kergon
1 sibling, 1 reply; 26+ messages in thread
From: Petr Rockai @ 2009-05-10 18:41 UTC (permalink / raw)
To: lvm-devel
Patch looks good. Good to get rid of redundant counter, which is prone to
getting out of sync. I would, however, propose to rename "snapshot_lvs_in_vg"
to "vg_snapshot_lv_count" -- we already have "vg_missing_pv_count" as a
precedent for that naming scheme.
Yours,
Petr.
PS: I would welcome a similar approach to removing lv_count as well (and other
redundant counters), since the counting function's implementation clearly
defines what gets counted in the given counter (vg_lv_count, say), *and* it
never gets out of sync. In other words, I believe this approach is much more
transparent, albeit a little less efficient -- however, I don't believe such
counting function is going to show up on any profile of any LVM2 command
whatsoever.
--
Peter Rockai | me()mornfall!net | prockai()redhat!com
http://blog.mornfall.net | http://web.mornfall.net
"In My Egotistical Opinion, most people's C programs should be
indented six feet downward and covered with dirt."
-- Blair P. Houghton on the subject of C program indentation
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 2/7] Never set mirror log and images directly visible in metadata.
2009-05-10 18:33 ` Petr Rockai
@ 2009-05-10 18:42 ` Milan Broz
2009-05-10 19:24 ` Petr Rockai
2009-05-11 1:39 ` Alasdair G Kergon
1 sibling, 1 reply; 26+ messages in thread
From: Milan Broz @ 2009-05-10 18:42 UTC (permalink / raw)
To: lvm-devel
Petr Rockai wrote:
> Milan Broz <mbroz@redhat.com> writes:
>> Mirror creation and conversion force set mirror log and images
>> to be visible. This can violate VG max_lv restriction on LV.
>>
>> There is no need to set these volumes visible, even during
>> mirror images conversion.
> Well, I believe the use-case is that when the operation fails halfway (which
> happens from time to time -- more often than would be comfortable, even), you
> can clean up by hand, since those offending LVs became visible. If there is a
> reasonable procedure that the admins can follow to get rid of the junk LVs in
> those scenarios, I'm fine.
Maybe we should display these "invisible" LVs to user (in reporting commands)
if no visible LV references them (I mean _mimage, _mlog & Co. devices)
Admin can remove these insvible LV even now (lvremove works), but must
run "lvs -a" to found them first...
Milan
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 4/7] Introduce vg_add_lc and vg_remove_lv functions.
2009-05-06 14:43 ` [PATCH 4/7] Introduce vg_add_lc and vg_remove_lv functions Milan Broz
@ 2009-05-10 19:06 ` Petr Rockai
2009-05-12 13:37 ` Milan Broz
0 siblings, 1 reply; 26+ messages in thread
From: Petr Rockai @ 2009-05-10 19:06 UTC (permalink / raw)
To: lvm-devel
Milan Broz <mbroz@redhat.com> writes:
> vg_add_lv and vg_remove_lv are the only functions for
> adding/removing logical volume into volume group.
Another good refactor.
> Only these function should manipulate with vg->lvs list and (together with
> following patches) will update VG logical volume counter (vg->lv_count).
I guess there's no easy way in C to enforce this? (vg->lvs is only ever touched
by vg_add_lv, vg_remove_lv). Not necessary, but would be nice to have.
Btw., would vg_link_lv and vg_unlink_lv describe better what these two
functions do? It seems a little easy to confuse _add_lv and vg_add_lv, while
the two do quite a different thing.
Acked-By: Petr Rockai <prockai@redhat.com>
(Detailed review comments inline.)
LVM1 format
-----------
> 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 627ee05..573836d 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;
>
I'm not sure why is this change needed? (Adding cmd_context parameter to
import_lv.)
> @@ -456,22 +455,19 @@ 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;
>
> - if (!import_lv(mem, lv, lvd))
> - return_NULL;
> + lvid_from_lvnum(&lv->lvid, &vg->id, lvd->lv_number);
>
> - dm_list_add(&vg->lvs, &ll->list);
> - vg->lv_count++;
> + if (!import_lv(vg->cmd, mem, lv, lvd)) {
> + dm_pool_free(mem, lv);
> + return_NULL;
> + }
>
> - return lv;
> + return vg_add_lv(vg, lv) ? lv : NULL;
> }
Does not change what the code does, just shuffles the lines somewhat. It does
change semantics of import_lv, but that function is never used outside _add_lv
anyway (would it make sense to make it static _import_lv, in a separate
patch?).
Pool Format
-----------
> diff --git a/lib/format_pool/import_export.c b/lib/format_pool/import_export.c
> index 4d2163d..91e3bcd 100644
> --- a/lib/format_pool/import_export.c
> +++ b/lib/format_pool/import_export.c
> @@ -58,22 +58,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;
> @@ -115,11 +107,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);
> - vg->lv_count++;
>
> - return 1;
> + return vg_add_lv(vg, lv);
> }
Again, harmless reordering of code. As a side-effect, unifies the error
handling code with other instances of similar code.
Text (LVM2) format
------------------
> diff --git a/lib/format_text/import_vsn1.c b/lib/format_text/import_vsn1.c
> index a9748aa..52ef075 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,11 +557,7 @@ static int _read_lvnames(struct format_instance *fid __attribute((unused)),
> return 0;
> }
>
> - lv->vg = vg;
> - vg->lv_count++;
> - dm_list_add(&vg->lvs, &lvl->list);
> -
> - return 1;
> + return vg_add_lv(vg, lv);
> }
The new code is equivalent to the older version, just the ordering is
(harmlessly) different.
General metadata code
---------------------
> diff --git a/lib/metadata/lv_manip.c b/lib/metadata/lv_manip.c
> index 7214b27..1903320 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,13 +433,11 @@ 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_remove_lv(lv))
> return_0;
> -
> - dm_list_del(&lvl->list);
> -
> - if (!(lv->status & SNAPSHOT))
> - lv->vg->lv_count--;
> + //FIXME: cow still in VG, fix count, remove this later
> + if (lv->status & SNAPSHOT)
> + lv->vg->lv_count++;
^^ This is indeed ugly (see also my previous mail: it might be worth getting
rid of lv_count altogether) ... although this patch is a definite improvement,
it is still suboptimal in this respect.
> } else if (lv->vg->fid->fmt->ops->lv_setup &&
> !lv->vg->fid->fmt->ops->lv_setup(lv->vg->fid, lv))
> return_0;
> @@ -1822,8 +1819,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];
>
> @@ -1843,23 +1838,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;
> - }
> + if (!(lv = dm_pool_zalloc(vg->vgmem, sizeof(*lv))))
> + return_NULL;
.
>
> - lv = ll->lv;
> - lv->vg = vg;
> -
> - 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;
> @@ -1877,18 +1860,20 @@ 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;
> - }
> + if (!vg_add_lv(vg, lv))
> + goto_bad;
.
>
> - if (!import)
> - vg->lv_count++;
> + if (fi->fmt->ops->lv_setup && !fi->fmt->ops->lv_setup(fi, lv))
> + goto_bad;
>
> - dm_list_add(&vg->lvs, &ll->list);
> + //FIXME: remove that
> + if (import)
> + vg->lv_count--;
The same about the FIXME, see above.
>
> return lv;
> +bad:
> + dm_pool_free(vg->vgmem, lv);
> + return_NULL;
> }
Other than that, the code looks equivalent to me, although there's a fair
amount of logic shuffling. It goes to a great length to make the function less
tangled though, so I guess it's a good thing (for example it gets rid of the
repeated error handler by using goto_bad instead).
> static int _add_pvs(struct cmd_context *cmd, struct pv_segment *peg,
> @@ -1957,6 +1942,36 @@ struct dm_list *build_parallel_areas_from_lv(struct cmd_context *cmd,
> return parallel_areas;
> }
>
> +int vg_add_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);
> +
> + vg->lv_count++;
> +
> + return 1;
> +}
> +
> +int vg_remove_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);
> +
> + lvl->lv->vg->lv_count--;
> +
> + return 1;
> +}
These two look good, and are implemented in accord with how they are used in
the code above.
Yours,
Petr.
--
Peter Rockai | me()mornfall!net | prockai()redhat!com
http://blog.mornfall.net | http://web.mornfall.net
"In My Egotistical Opinion, most people's C programs should be
indented six feet downward and covered with dirt."
-- Blair P. Houghton on the subject of C program indentation
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 5/7] Introduce lv_set_visible & lv_set_hidden and use lv_is_visible always.
2009-05-06 14:43 ` [PATCH 5/7] Introduce lv_set_visible & lv_set_hidden and use lv_is_visible always Milan Broz
@ 2009-05-10 19:23 ` Petr Rockai
2009-05-12 15:12 ` Milan Broz
0 siblings, 1 reply; 26+ messages in thread
From: Petr Rockai @ 2009-05-10 19:23 UTC (permalink / raw)
To: lvm-devel
This patch does improve upon the previous one with regards to the consistency
of lv_count handling (to the point of addressing most of my concerns with
previous).
Milan Broz <mbroz@redhat.com> writes:
> The vg->lv_count parameter now includes always number of visible logical
> volumes.
The patch does as advertised.
> Note that virtual snapshot volume (snapshotX) is never visible,
> but it is stored in metadata with visible flag, so code
> use exception here.
The & SNAPSHOT logic is a little dubious, but seems to be a sufficient solution
to the issue at hand.
Overall, seems good enough to me,
Acked-By: Petr Rockai <prockai@redhat.com>
> diff --git a/lib/metadata/snapshot_manip.c b/lib/metadata/snapshot_manip.c
> index cc57aeb..852ad76 100644
> --- a/lib/metadata/snapshot_manip.c
> +++ b/lib/metadata/snapshot_manip.c
> @@ -30,8 +30,11 @@ int lv_is_cow(const struct logical_volume *lv)
>
> int lv_is_visible(const struct logical_volume *lv)
> {
> + if (lv->status & SNAPSHOT)
> + return 0;
> +
> if (lv_is_cow(lv))
> - return lv_is_visible(find_cow(lv)->lv);
> + return lv_is_visible(origin_from_cow(lv));
>
> return lv->status & VISIBLE_LV ? 1 : 0;
> }
Why is the latter change included here? Doesn't seem to be directly related to
the rest of the patch (although I might just be missing something).
> /* FIXME Assumes an invisible origin belongs to a sparse device */
> - if (!lv_is_visible(origin))
> + if (!lv_is_visible(origin)) {
> + origin->vg->lv_count--;
> origin->status |= VIRTUAL_ORIGIN;
> + }
>
> dm_list_add(&origin->snapshot_segs, &seg->origin_list);
>
> @@ -130,10 +133,15 @@ int vg_remove_snapshot(struct logical_volume *cow)
> return 0;
> }
>
> - cow->snapshot = NULL;
> + /*
> + * Virtual snapshot volume was removed
> + * LV count must be fixed...
> + */
> + if (!lv_is_virtual_origin(cow->snapshot->origin))
> + cow->vg->lv_count--;
These two are ugly hacks again, and spill the lv_count manipulation (and
knowledge) out of the supposed keeper functions (set visible/hidden, vg
add/remove). Nevertheless, it's hard to tell how to fix this more cleanly.
> diff --git a/lib/snapshot/snapshot.c b/lib/snapshot/snapshot.c
> index ad5d158..4dc7111 100644
> --- a/lib/snapshot/snapshot.c
> +++ b/lib/snapshot/snapshot.c
> @@ -78,17 +78,14 @@ static int _snap_text_import(struct lv_segment *seg, const struct config_node *s
> seg->origin = org;
> seg->cow = cow;
>
> - // FIXME: direct count manipulation to be removed later
> - cow->status &= ~VISIBLE_LV;
> - cow->vg->lv_count--;
> cow->snapshot = seg;
> -
> org->origin_count++;
> - org->vg->lv_count--;
>
> /* FIXME Assumes an invisible origin belongs to a sparse device */
> - if (!lv_is_visible(org))
> + if (!lv_is_visible(org)) {
> + org->vg->lv_count--;
Same as above.
> org->status |= VIRTUAL_ORIGIN;
> + }
Yours,
Petr.
--
Peter Rockai | me()mornfall!net | prockai()redhat!com
http://blog.mornfall.net | http://web.mornfall.net
"In My Egotistical Opinion, most people's C programs should be
indented six feet downward and covered with dirt."
-- Blair P. Houghton on the subject of C program indentation
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 2/7] Never set mirror log and images directly visible in metadata.
2009-05-10 18:42 ` Milan Broz
@ 2009-05-10 19:24 ` Petr Rockai
2009-05-11 1:30 ` Alasdair G Kergon
0 siblings, 1 reply; 26+ messages in thread
From: Petr Rockai @ 2009-05-10 19:24 UTC (permalink / raw)
To: lvm-devel
Milan Broz <mbroz@redhat.com> writes:
> Petr Rockai wrote:
>> Well, I believe the use-case is that when the operation fails halfway (which
>> happens from time to time -- more often than would be comfortable, even), you
>> can clean up by hand, since those offending LVs became visible. If there is a
>> reasonable procedure that the admins can follow to get rid of the junk LVs in
>> those scenarios, I'm fine.
>
> Maybe we should display these "invisible" LVs to user (in reporting commands)
> if no visible LV references them (I mean _mimage, _mlog & Co. devices)
Sounds like a decent solution to me.
> Admin can remove these insvible LV even now (lvremove works), but must
> run "lvs -a" to found them first...
Ok.
Yours,
Petr.
--
Peter Rockai | me()mornfall!net | prockai()redhat!com
http://blog.mornfall.net | http://web.mornfall.net
"In My Egotistical Opinion, most people's C programs should be
indented six feet downward and covered with dirt."
-- Blair P. Houghton on the subject of C program indentation
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 2/7] Never set mirror log and images directly visible in metadata.
2009-05-10 19:24 ` Petr Rockai
@ 2009-05-11 1:30 ` Alasdair G Kergon
0 siblings, 0 replies; 26+ messages in thread
From: Alasdair G Kergon @ 2009-05-11 1:30 UTC (permalink / raw)
To: lvm-devel
On Sun, May 10, 2009 at 09:24:52PM +0200, Peter Rockai wrote:
> Milan Broz <mbroz@redhat.com> writes:
> > Maybe we should display these "invisible" LVs to user (in reporting commands)
> > if no visible LV references them (I mean _mimage, _mlog & Co. devices)
> Sounds like a decent solution to me.
But then - what's our definition of visible? - and we're back to the original
counting problem with multiple confusing definitions.
Can we really not consolidate this into a single definition?
(BTW max_lv etc. is not a factor that skew other policy decisions - it's an
unimportant feature.)
Alasdair
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 2/7] Never set mirror log and images directly visible in metadata.
2009-05-10 18:33 ` Petr Rockai
2009-05-10 18:42 ` Milan Broz
@ 2009-05-11 1:39 ` Alasdair G Kergon
1 sibling, 0 replies; 26+ messages in thread
From: Alasdair G Kergon @ 2009-05-11 1:39 UTC (permalink / raw)
To: lvm-devel
On Sun, May 10, 2009 at 08:33:22PM +0200, Peter Rockai wrote:
> (The whole non-atomicity of those mirror operation is very unfortunate for
> other reasons, but I didn't yet have time to investigate how hard it would be
> to make those really atomic.)
We've two factors here:
All single-VG command-line-level changes must have a single commit
point with no chance of data/metadata corruption. (See Rik's lvconvert
bug report.) pvmove offers a model.
Not all lvm2 code does everything possible to clean up after an
operation fails. More code could take the trouble to undo operations
rather than logging error messages telling the admin to remove something
by hand. More code could detect things left behind by an earlier failed
operation and clean them up transparently.
Alasdair
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 2/7] Never set mirror log and images directly visible in metadata.
2009-05-06 14:42 ` [PATCH 2/7] Never set mirror log and images directly visible in metadata Milan Broz
2009-05-10 18:33 ` Petr Rockai
@ 2009-05-11 1:54 ` Alasdair G Kergon
2009-05-12 12:34 ` Milan Broz
1 sibling, 1 reply; 26+ messages in thread
From: Alasdair G Kergon @ 2009-05-11 1:54 UTC (permalink / raw)
To: lvm-devel
On Wed, May 06, 2009 at 04:42:59PM +0200, Milan Broz wrote:
> This can violate VG max_lv restriction on LV.
I'm not bothered if the code exceeds max_lv temporarily.
I mean, does anyone have a real-world use case for that parameter?
(Perhaps make it so VGs exceeding that limit can still be manipulated
internally, but users cannot add new LVs until it drops below the
limit i.e. a soft limit enforced on the user.)
This comes back to whether we actually need the distinction between
TOPLEVEL (nothing in lvm uses the LV) and VISIBLE (it's for users/tools
to open) or not, a distinction we've been eliminating I think.
(If we got automatic clean-up of incomplete multi-step operations
wouldn't the problem go away? Might be as simple as flagging LVs as
temporary/uncommitted in the metadata and having them deleted next time
the VG lock is taken in the same way as we deal with multiple metadata
sequence numbers in different VG mdas.)
Alasdair
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 6/7] Merge lv_is_displayable and lv_is_visible.
2009-05-06 14:43 ` [PATCH 6/7] Merge lv_is_displayable and lv_is_visible Milan Broz
@ 2009-05-11 6:09 ` Petr Rockai
0 siblings, 0 replies; 26+ messages in thread
From: Petr Rockai @ 2009-05-11 6:09 UTC (permalink / raw)
To: lvm-devel
Looks good, no obvious regressions, simplifies code.
Milan Broz <mbroz@redhat.com> writes:
> Signed-off-by: Milan Broz <mbroz@redhat.com>
Acked-By: Petr Rockai <prockai@redhat.com>
--
Peter Rockai | me()mornfall!net | prockai()redhat!com
http://blog.mornfall.net | http://web.mornfall.net
"In My Egotistical Opinion, most people's C programs should be
indented six feet downward and covered with dirt."
-- Blair P. Houghton on the subject of C program indentation
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 7/7] Remove import paramater from lv_create_empty.
2009-05-06 14:43 ` [PATCH 7/7] Remove import paramater from lv_create_empty Milan Broz
@ 2009-05-11 6:15 ` Petr Rockai
0 siblings, 0 replies; 26+ messages in thread
From: Petr Rockai @ 2009-05-11 6:15 UTC (permalink / raw)
To: lvm-devel
Milan Broz <mbroz@redhat.com> writes:
> Now the max_lv check is performed in vg_add_lv and import parameter in
> lv_create_empty has no use.
Indeed -- and good riddance, too.
> Patch also adds simple max_lv tests into testsuite
Good.
> Signed-off-by: Milan Broz <mbroz@redhat.com>
Acked-By: Petr Rockai <prockai@redhat.com>
> - if (!import)
> + if (status & VISIBLE_LV)
> log_verbose("Creating logical volume %s", name);
Would it make sense to use lv_is_visible there? It's just a verbose log
message, so I guess it wouldn't matter much?
Yours,
Petr.
--
Peter Rockai | me()mornfall!net | prockai()redhat!com
http://blog.mornfall.net | http://web.mornfall.net
"In My Egotistical Opinion, most people's C programs should be
indented six feet downward and covered with dirt."
-- Blair P. Houghton on the subject of C program indentation
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 1/7] Fix snapshot segment import to not use duplicate segments & replace.
2009-05-06 14:42 ` [PATCH 1/7] Fix snapshot segment import to not use duplicate segments & replace Milan Broz
@ 2009-05-11 6:27 ` Petr Rockai
2009-05-11 11:29 ` Alasdair G Kergon
0 siblings, 1 reply; 26+ messages in thread
From: Petr Rockai @ 2009-05-11 6:27 UTC (permalink / raw)
To: lvm-devel
Milan Broz <mbroz@redhat.com> writes:
> Simplify the code, snapshot segment is properly initialized
> in text_import function now and do not need to be replaced
> by vg_add_snapshot call.
Looks OK, although it does duplicate part of the code. Nevertheless, seems like
a better compromise than the previous one. Also, the ugly part of the code
where lv_count is fiddled manually is fixed by a later patch.
> The vg_add_snapshot() is now usefull only for adding new snapshot. The name
> is always gewerated, remove it from function call.
Ack.
> Signed-off-by: Milan Broz <mbroz@redhat.com>
Acked-By: Petr Rockai <prockai@redhat.com>
--
Peter Rockai | me()mornfall!net | prockai()redhat!com
http://blog.mornfall.net | http://web.mornfall.net
"In My Egotistical Opinion, most people's C programs should be
indented six feet downward and covered with dirt."
-- Blair P. Houghton on the subject of C program indentation
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 1/7] Fix snapshot segment import to not use duplicate segments & replace.
2009-05-11 6:27 ` Petr Rockai
@ 2009-05-11 11:29 ` Alasdair G Kergon
0 siblings, 0 replies; 26+ messages in thread
From: Alasdair G Kergon @ 2009-05-11 11:29 UTC (permalink / raw)
To: lvm-devel
On Mon, May 11, 2009 at 08:27:13AM +0200, Peter Rockai wrote:
> Milan Broz <mbroz@redhat.com> writes:
> > Simplify the code, snapshot segment is properly initialized
> > in text_import function now and do not need to be replaced
> > by vg_add_snapshot call.
> Looks OK, although it does duplicate part of the code.
Indeed - please avoid duplicating that code.
Alasdair
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 3/7] Remove snapshot_count from VG and use function instead.
2009-05-06 14:43 ` [PATCH 3/7] Remove snapshot_count from VG and use function instead Milan Broz
2009-05-10 18:41 ` Petr Rockai
@ 2009-05-11 11:42 ` Alasdair G Kergon
1 sibling, 0 replies; 26+ messages in thread
From: Alasdair G Kergon @ 2009-05-11 11:42 UTC (permalink / raw)
To: lvm-devel
On Wed, May 06, 2009 at 04:43:00PM +0200, Milan Broz wrote:
> - * The following relationship should always hold:
> - * dm_list_size(lvs) = lv_count + 2 * snapshot_count
> + * logical volumes count
Please leave the description of dm_list_size(lvs).
"Number of snapshots" instead of snapshot_count?
(Or the new function?)
> +unsigned snapshot_lvs_in_vg(const struct volume_group *vg)
> +{
> + struct lv_list *lvl;
> + unsigned lv_count = 0;
A confusing alternative use for a variable called lv_count?
'unsigned snapshot_count'?
Ack.
Alasdair.
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 3/7] Remove snapshot_count from VG and use function instead.
2009-05-10 18:41 ` Petr Rockai
@ 2009-05-11 11:48 ` Alasdair G Kergon
0 siblings, 0 replies; 26+ messages in thread
From: Alasdair G Kergon @ 2009-05-11 11:48 UTC (permalink / raw)
To: lvm-devel
On Sun, May 10, 2009 at 08:41:32PM +0200, Peter Rockai wrote:
> Patch looks good. Good to get rid of redundant counter, which is prone to
> getting out of sync. I would, however, propose to rename "snapshot_lvs_in_vg"
> to "vg_snapshot_lv_count" -- we already have "vg_missing_pv_count" as a
> precedent for that naming scheme.
Ack.
> PS: I would welcome a similar approach to removing lv_count as well (and other
> redundant counters), since the counting function's implementation clearly
> defines what gets counted in the given counter (vg_lv_count, say), *and* it
> never gets out of sync. In other words, I believe this approach is much more
> transparent, albeit a little less efficient -- however, I don't believe such
> counting function is going to show up on any profile of any LVM2 command
> whatsoever.
If it ever did, we could just optimise things by putting it back in:-)
It's only needed for display tools (and there are caching tricks we could use
there but don't) and the tiny subset of people these days who found a use for
max_lv (or don't need it but are running systems that inherited it from LVM1).
Alasdair
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 2/7] Never set mirror log and images directly visible in metadata.
2009-05-11 1:54 ` Alasdair G Kergon
@ 2009-05-12 12:34 ` Milan Broz
0 siblings, 0 replies; 26+ messages in thread
From: Milan Broz @ 2009-05-12 12:34 UTC (permalink / raw)
To: lvm-devel
Alasdair G Kergon wrote:
> On Wed, May 06, 2009 at 04:42:59PM +0200, Milan Broz wrote:
>> This can violate VG max_lv restriction on LV.
>
> I'm not bothered if the code exceeds max_lv temporarily.
It is not temporarily, it is stored in metadata on disk in this case.
That's why I do not want to store VISIBLE flag there.
Even without mentioning max_lv, the proper solution should be properly
revert these invisible volumes if operation fails in the middle.
> I mean, does anyone have a real-world use case for that parameter?
That parameter is mistake for text metadata format IMHO.
The only real use is to downconvert to lvm1 format (which has limited LVs)
But we have it documented, "supported" so removing it is probably not good way.
> (Perhaps make it so VGs exceeding that limit can still be manipulated
> internally, but users cannot add new LVs until it drops below the
> limit i.e. a soft limit enforced on the user.)
What the internally means here? Not stored in persistent metadata?
Or stored with an internal/temporary flag?
> This comes back to whether we actually need the distinction between
> TOPLEVEL (nothing in lvm uses the LV) and VISIBLE (it's for users/tools
> to open) or not, a distinction we've been eliminating I think.
Which will probably re-complicate again when replicator & crypto segments/LVs
are introduced...
> (If we got automatic clean-up of incomplete multi-step operations
> wouldn't the problem go away? Might be as simple as flagging LVs as
> temporary/uncommitted in the metadata and having them deleted next time
> the VG lock is taken in the same way as we deal with multiple metadata
> sequence numbers in different VG mdas.)
Is this possible without breaking compatibility with old versions?
(new LV flag?)
Milan
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 4/7] Introduce vg_add_lc and vg_remove_lv functions.
2009-05-10 19:06 ` Petr Rockai
@ 2009-05-12 13:37 ` Milan Broz
0 siblings, 0 replies; 26+ messages in thread
From: Milan Broz @ 2009-05-12 13:37 UTC (permalink / raw)
To: lvm-devel
Petr Rockai wrote:
> Btw., would vg_link_lv and vg_unlink_lv describe better what these two
> functions do? It seems a little easy to confuse _add_lv and vg_add_lv, while
> the two do quite a different thing.
yes, renamed
>> @@ -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;
>>
> I'm not sure why is this change needed? (Adding cmd_context parameter to
> import_lv.)
just code shuffle, because lv->vg is initialized later in vg_add_lv and we
need cmd for default settings.
(probably explains some code shuffle later too)
> Does not change what the code does, just shuffles the lines somewhat. It does
> change semantics of import_lv, but that function is never used outside _add_lv
> anyway (would it make sense to make it static _import_lv, in a separate
> patch?).
ok, I'll move it to separate patch
>> + //FIXME: cow still in VG, fix count, remove this later
>> + if (lv->status & SNAPSHOT)
>> + lv->vg->lv_count++;
> ^^ This is indeed ugly (see also my previous mail: it might be worth getting
> rid of lv_count altogether) ... although this patch is a definite improvement,
> it is still suboptimal in this respect.
later patch removes that, I keep it here just that every separate patch can pass
full testing.
Milan
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 5/7] Introduce lv_set_visible & lv_set_hidden and use lv_is_visible always.
2009-05-10 19:23 ` Petr Rockai
@ 2009-05-12 15:12 ` Milan Broz
0 siblings, 0 replies; 26+ messages in thread
From: Milan Broz @ 2009-05-12 15:12 UTC (permalink / raw)
To: lvm-devel
Petr Rockai wrote:
>> diff --git a/lib/metadata/snapshot_manip.c b/lib/metadata/snapshot_manip.c
>> index cc57aeb..852ad76 100644
>> --- a/lib/metadata/snapshot_manip.c
>> +++ b/lib/metadata/snapshot_manip.c
>> @@ -30,8 +30,11 @@ int lv_is_cow(const struct logical_volume *lv)
>>
>> int lv_is_visible(const struct logical_volume *lv)
>> {
>> + if (lv->status & SNAPSHOT)
>> + return 0;
>> +
>> if (lv_is_cow(lv))
>> - return lv_is_visible(find_cow(lv)->lv);
>> + return lv_is_visible(origin_from_cow(lv));
>>
>> return lv->status & VISIBLE_LV ? 1 : 0;
>> }
> Why is the latter change included here? Doesn't seem to be directly related to
> the rest of the patch (although I might just be missing something).
This is kind of bugfix, I'll move it to separate patch.
If the query LV is COW, ut returns visibility status of its origin because
the virtual LV (which find_cow(lv)->lv points to) is never set visible in metadata.
But with virtual origin is this assumption wrong, I'll fix it somehow.
Milan
^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2009-05-12 15:12 UTC | newest]
Thread overview: 26+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-05-06 14:42 [PATCH 0/7] Fix lv_count & max_lv problems in lvm2 Milan Broz
2009-05-06 14:42 ` [PATCH 1/7] Fix snapshot segment import to not use duplicate segments & replace Milan Broz
2009-05-11 6:27 ` Petr Rockai
2009-05-11 11:29 ` Alasdair G Kergon
2009-05-06 14:42 ` [PATCH 2/7] Never set mirror log and images directly visible in metadata Milan Broz
2009-05-10 18:33 ` Petr Rockai
2009-05-10 18:42 ` Milan Broz
2009-05-10 19:24 ` Petr Rockai
2009-05-11 1:30 ` Alasdair G Kergon
2009-05-11 1:39 ` Alasdair G Kergon
2009-05-11 1:54 ` Alasdair G Kergon
2009-05-12 12:34 ` Milan Broz
2009-05-06 14:43 ` [PATCH 3/7] Remove snapshot_count from VG and use function instead Milan Broz
2009-05-10 18:41 ` Petr Rockai
2009-05-11 11:48 ` Alasdair G Kergon
2009-05-11 11:42 ` Alasdair G Kergon
2009-05-06 14:43 ` [PATCH 4/7] Introduce vg_add_lc and vg_remove_lv functions Milan Broz
2009-05-10 19:06 ` Petr Rockai
2009-05-12 13:37 ` Milan Broz
2009-05-06 14:43 ` [PATCH 5/7] Introduce lv_set_visible & lv_set_hidden and use lv_is_visible always Milan Broz
2009-05-10 19:23 ` Petr Rockai
2009-05-12 15:12 ` Milan Broz
2009-05-06 14:43 ` [PATCH 6/7] Merge lv_is_displayable and lv_is_visible Milan Broz
2009-05-11 6:09 ` Petr Rockai
2009-05-06 14:43 ` [PATCH 7/7] Remove import paramater from lv_create_empty Milan Broz
2009-05-11 6:15 ` Petr Rockai
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.