On Saturday 08 December 2007, Frans Pop wrote: > Here are some of my thoughts: > - a better display of what exactly will be erased for LVM would be very > useful, maybe this should be the first step in the reimplementation The attached patch reimplements this. I have chosen to implement the removal function a bit differently from David while still using his changes as my base: - I feel we should continue to do the removal of LVM from the partman $dev and not from the $realdev - when we initialize a disk we should allow for the fact that the disk has more than one VG (on different partitions); this is an improvement over the old code as well - the separate function remove_lvm_find_vgs can be used to check if there is LVM date on a device and if that can be removed automatically; should be useful when implementing crypto removal I have also added support for setting and removing locks. Setting locks because I noticed that we did set locks when setting up LVM using manual partitioning, but not using guided partitioning. Removing locks because during testing I found myself blocked by stale old locks a few times [1]. Currently partman also checks for locks when a whole device is selected, which prevents initializing a disk by creating a new disk label. That check should probably be removed now that creating a new disk label supports automatic removal of LVM. > - should we also support targeted removal of a specific encrypted > volume, e.g. when the parent device is selected? seems worth doing > and it should be possible to use largely the same code for that, but > it is only useful if we can unlock the parent device after doing so Possible that was already supported by the code David had added to partman-base/choose_partition/partition_tree/do_option, but I'm not completely sure. In line with this it should in theory also be possible to extend device_remove_lvm() to remove LVM data from a partition when that partition is deleted, but that would require quite a bit of work (we'd need to pass the partman partition ID in and translate that to the real device). Cheers, FJP [1] Now that I write this I see that David had also implemented that the same way in dm_wipe_lvm() :-P
commit 1e45c4838f269003a4a9f917142dc4f271ff0c02
Author: Frans Pop <fjp@debian.org>
Date: Mon Dec 10 13:25:25 2007 +0100
Improve removal of LVM volumes from a device
List LVM volumes to be removed; based on earlier work by David Härdeman.
A disk could contain multiple volume groups; make sure we remove them all.
When a physical volume is deleted, also unlock the device it was on.
diff --git a/packages/partman/partman-lvm/debian/changelog b/packages/partman/partman-lvm/debian/changelog
index c31fe79..5cb7cfb 100644
--- a/packages/partman/partman-lvm/debian/changelog
+++ b/packages/partman/partman-lvm/debian/changelog
@@ -1,3 +1,12 @@
+partman-lvm (58) UNRELEASED; urgency=low
+
+ * lvm-remove.sh: improve automatic removal of LVM data
+ - list LVM volumes to be removed; based on earlier work by David Härdeman
+ - a disk can contain multiple volume groups; make sure we remove them all
+ - when a physical volume is deleted, also unlock the device it was on
+
+ -- Frans Pop <fjp@debian.org> Mon, 10 Dec 2007 18:13:40 +0100
+
partman-lvm (57) UNRELEASED; urgency=low
* Moved definitions.sh to ./lib/base.sh. Requires partman-base (>= 114).
diff --git a/packages/partman/partman-lvm/debian/partman-lvm.templates b/packages/partman/partman-lvm/debian/partman-lvm.templates
index 97a89a7..1e02c40 100644
--- a/packages/partman/partman-lvm/debian/partman-lvm.templates
+++ b/packages/partman/partman-lvm/debian/partman-lvm.templates
@@ -397,9 +397,14 @@ Template: partman-lvm/device_remove_lvm
Type: boolean
Default: false
_Description: Remove existing logical volume data?
- The selected device already contains logical volumes and/or
- volume groups from a previous LVM installation, which must be removed
- from the disk before creating any partitions.
+ The selected device already contains the following LVM logical volumes,
+ volume groups and physical volumes which are about to be removed:
+ .
+ Logical volume(s) to be removed: ${LVTARGETS}
+ .
+ Volume group(s) to be removed: ${VGTARGETS}
+ .
+ Physical volume(s) to be removed: ${PVTARGETS}
.
Note that this will also permanently erase any data currently on the
logical volumes.
diff --git a/packages/partman/partman-lvm/lib/lvm-base.sh b/packages/partman/partman-lvm/lib/lvm-base.sh
index 79500ab..8ccfd84 100644
--- a/packages/partman/partman-lvm/lib/lvm-base.sh
+++ b/packages/partman/partman-lvm/lib/lvm-base.sh
@@ -181,6 +181,8 @@ lvm_name_ok() {
###############################################################################
# Check if a device contains PVs
+# If called for a disk, this will also check all partitions;
+# if called for anything other, it can return false positives!
pv_on_device() {
local device
device="$1"
diff --git a/packages/partman/partman-lvm/lib/lvm-remove.sh b/packages/partman/partman-lvm/lib/lvm-remove.sh
index 19e2ef9..7edd145 100644
--- a/packages/partman/partman-lvm/lib/lvm-remove.sh
+++ b/packages/partman/partman-lvm/lib/lvm-remove.sh
@@ -1,9 +1,38 @@
. /lib/partman/lib/lvm-base.sh
+# List PVs to be removed to initialize a device
+remove_lvm_find_vgs() {
+ local realdev vg pvs
+ realdev="$1"
+
+ # Check all VGs to see which PV needs removing
+ # BUGME: the greps in this loop should be properly bounded so they
+ # do not match on partial matches!
+ # Except that we want partial matches for disks...
+ for vg in $(vg_list); do
+ pvs="$(vg_list_pvs $vg)"
+
+ if ! $(echo "$pvs" | grep -q "$realdev"); then
+ continue
+ fi
+
+ # Make sure the VG doesn't span any other disks
+ if $(echo -n "$pvs" | grep -q -v "$realdev"); then
+ log-output -t partman-lvm vgs
+ db_input critical partman-lvm/device_remove_lvm_span || true
+ db_go || true
+ return 1
+ fi
+ echo "$vg"
+ done
+}
+
# Wipes any traces of LVM from a disk
# Normally called from a function that initializes a device
+# Note: if the device contains an empty PV, it will not be removed
device_remove_lvm() {
- local dev realdev vg pvs pv lv tmpdev restart
+ local dev realdev tmpdev restart
+ local pvs pv vgs vg lvs lv pvtext vgtext lvtext
dev="$1"
cd $dev
@@ -13,7 +42,33 @@ device_remove_lvm() {
return 0
fi
+ vgs="$(remove_lvm_find_vgs $realdev)" || return 1
+ [ "$vgs" ] || return 0
+
+ pvs=""
+ lvs=""
+ for vg in $vgs; do
+ pvs="${pvs:+$pvs$NL}$(vg_list_pvs $vg)"
+ lvs="${lvs:+$lvs$NL}$(vg_list_lvs $vg)"
+ done
+
# Ask for permission to erase LVM volumes
+ lvtext=""
+ for lv in $lvs; do
+ lvtext="${lvtext:+$lvtext, }$lv"
+ done
+ vgtext=""
+ for vg in $vgs; do
+ vgtext="${vgtext:+$vgtext, }$vg"
+ done
+ pvtext=""
+ for pv in $pvs; do
+ pvtext="${pvtext:+$pvtext, }$pv"
+ done
+
+ db_subst partman-lvm/device_remove_lvm LVTARGETS "$lvtext"
+ db_subst partman-lvm/device_remove_lvm VGTARGETS "$vgtext"
+ db_subst partman-lvm/device_remove_lvm PVTARGETS "$pvtext"
db_input critical partman-lvm/device_remove_lvm
db_go || return 1
db_get partman-lvm/device_remove_lvm
@@ -21,38 +76,22 @@ device_remove_lvm() {
return 1
fi
- # We need devicemapper support
+ # We need devicemapper support here
modprobe dm-mod >/dev/null 2>&1
- # Check all VG's
- # BUGME: the greps in this loop should be properly bounded so they
- # do not match on partial matches!
- for vg in $(vg_list); do
- pvs=$(vg_list_pvs $vg)
-
- # Only deal with VG's on the selected disk
- if ! $(echo "$pvs" | grep -q "$realdev"); then
- continue
- fi
-
- # Make sure the VG don't span any other disks
- if $(echo -n "$pvs" | grep -q -v "$realdev"); then
- log-output -t partman-lvm vgs
- db_input critical partman-lvm/device_remove_lvm_span || true
- db_go || true
- return 1
- fi
-
- # Remove LV's from the VG
+ for vg in $vgs; do
+ # Remove LVs from the VG
for lv in $(vg_list_lvs $vg); do
lv_delete $vg $lv
done
- # Remove the VG and its PV's
+ # Remove the VG
vg_delete $vg
- for pv in $pvs; do
- pv_delete $pv
- done
+ done
+ # Remove the PVs and unlock the devices
+ for pv in $pvs; do
+ pv_delete $pv
+ partman_unlock_unit $pv
done
# Make sure that parted has no stale LVM info
commit 5a98c7700fd6775d7a0227cf4d79da335ffe5c9a
Author: Frans Pop <fjp@debian.org>
Date: Mon Dec 10 18:47:36 2007 +0100
Lock devices used for physical volumes
This brings guided partitioning using LVM in line with setting up
LVM using manual partitioning.
diff --git a/packages/partman/partman-auto-lvm/debian/changelog b/packages/partman/partman-auto-lvm/debian/changelog
index 85fee18..b12d757 100644
--- a/packages/partman/partman-auto-lvm/debian/changelog
+++ b/packages/partman/partman-auto-lvm/debian/changelog
@@ -1,3 +1,10 @@
+partman-auto-lvm (25) UNRELEASED; urgency=low
+
+ * Lock devices that are used for physical volumes, as is done when setting
+ up LVM using manual partitioning.
+
+ -- Frans Pop <fjp@debian.org> Mon, 10 Dec 2007 18:44:03 +0100
+
partman-auto-lvm (24) UNRELEASED; urgency=low
* Move deletion of SVN directories to install-rc script.
diff --git a/packages/partman/partman-auto-lvm/lib/auto-lvm.sh b/packages/partman/partman-auto-lvm/lib/auto-lvm.sh
index 3796e4c..e8a7782 100644
--- a/packages/partman/partman-auto-lvm/lib/auto-lvm.sh
+++ b/packages/partman/partman-auto-lvm/lib/auto-lvm.sh
@@ -117,7 +117,7 @@ auto_lvm_prepare() {
auto_lvm_perform() {
# Use hostname as default vg name (if available)
- local defvgname
+ local defvgname pv
db_get partman-auto-lvm/new_vg_name
if [ -z "$RET" ]; then
if [ -s /etc/hostname ]; then
@@ -151,6 +151,11 @@ auto_lvm_perform() {
else
bail_out vg_create_error
fi
+ db_subst partman-lvm/text/in_use VG "$VG_name"
+ db_metaget partman-lvm/text/in_use description
+ for pv in $pv_devices; do
+ partman_lock_unit "$pv" "$RET"
+ done
# Default to accepting the autopartitioning
menudir_default_choice /lib/partman/choose_partition finish finish || true
Attachment:
signature.asc
Description: This is a digitally signed message part.