Re: [RFC] Some love for partman-md
Hi Jérémy,
> Attached you will find an attempt to do so. The changeset is only broke
> down in two patches:
> * The first is a bit invasive (partman-md, partman-base, mdcfg) and
> improve globally the way MD devices are initialized and from my tests
> fix 10 bugs at ounce (counting merged ones).
Impressive. Thanks for doing this work :-)
Apart from one change which I'm not sure is correct, I
couldn't spot any significant problems. More comments
below inline with the patches.
Note that I haven't actually tested the changes so far
but I'm planning to do a few test installs later today.
> * The second is in partman-partitioning and prevent deletion and
> resizing for MD, LVM and crypto devices as they do not work correctly
> for any of these devices.
This one is obviously correct.
I would actually suggest to commit it straight away,
independent of the more invasive changes above.
...
> diff --git a/packages/mdcfg/md-init.sh b/packages/mdcfg/md-init.sh
> new file mode 100755
> index 0000000..53645f8
> --- /dev/null
> +++ b/packages/mdcfg/md-init.sh
> @@ -0,0 +1,24 @@
> +#!/bin/sh
> +
> +if [ -e /proc/mdstat ]; then
> + exit 0
> +fi
> +
> +# Try to load the necesarry modules.
(Typo - necessary)
> +# Supported schemes: RAID 0, RAID 1, RAID 5
> +depmod -a >/dev/null 2>&1
> +modprobe md >/dev/null 2>&1 || modprobe md-mod >/dev/null 2>&1
> +modprobe raid0 >/dev/null 2>&1
> +modprobe raid1 >/dev/null 2>&1
> +# kernels >=2.6.18 have raid456
> +modprobe raid456 >/dev/null 2>&1 || modprobe raid5 >/dev/null 2>&1
A thought unrelated to your changes since this code is
just moved from mdcfg.sh:
Should we use something like
load_module() {
log-output modprobe $1
}
load_module md || load_module md-mod
.. etc
To know when there are problems loading the modules?
> diff --git a/packages/partman/partman-base/debian/changelog b/packages/partman/partman-base/debian/changelog
> index 2d63730..31b93af 100644
> --- a/packages/partman/partman-base/debian/changelog
> +++ b/packages/partman/partman-base/debian/changelog
> @@ -1,3 +1,11 @@
> +partman-base (121) UNRELEASED; urgency=low
> +
> + [ Jérémy Bobbio ]
> + * Instead of skipping every MD devices during partman initialization, we
> + know only skip the ones that are deactivated.
Typo s/know/now/ ?
> +
> + -- Jérémy Bobbio <lunar@debian.org> Thu, 29 May 2008 16:07:37 +0000
> +
> partman-base (120) unstable; urgency=high
>
> [ Jérémy Bobbio ]
> diff --git a/packages/partman/partman-base/init.d/parted b/packages/partman/partman-base/init.d/parted
> index 346170a..f7cfe30 100755
> --- a/packages/partman/partman-base/init.d/parted
> +++ b/packages/partman/partman-base/init.d/parted
...
> part_of_sataraid () {
> local raiddev
> for raiddev in $(dmraid -r -c); do
> @@ -52,8 +61,7 @@ if [ ! -f /var/run/parted_server.pid ]; then
>
> IFS="$NL"
> for partdev in $(parted_devices |
> - grep -v '^/dev/md' |
> - sed 's,^/dev/\(ide\|scsi\|[hs]d\),!/dev/\1,' |
> + sed 's,^/dev/\(ide\|scsi\|[hsm]d\),!/dev/\1,' |
Could be renamed to part_of_raid() or something after the change?
> diff --git a/packages/partman/partman-md/debian/changelog b/packages/partman/partman-md/debian/changelog
> index e93fc37..384d64a 100644
> --- a/packages/partman/partman-md/debian/changelog
> +++ b/packages/partman/partman-md/debian/changelog
> @@ -1,3 +1,16 @@
> +partman-md (42) UNRELEASED; urgency=low
> +
> + [ Jérémy Bobbio ]
> + * Clean up the initialization of MD devices. Together with the changes
> + introduced in partman-base (>> 120), setup of RAID devices won't be lost
> + accross partman restarts anymore.
> + (Closes: #391479, #391483, #393728, #398668)
s/accross/across/
> + * Load the necassary modules and scan RAID arrays during partman
> + initialization. (Closes: #391474)
s/necassary/necessary/
> + Requires mdcfg-utils (>> 1.26).
> +
> + -- Jérémy Bobbio <lunar@debian.org> Thu, 29 May 2008 16:02:39 +0000
> +
> partman-md (41) unstable; urgency=low
>
> [ Frans Pop ]
> diff --git a/packages/partman/partman-md/init.d/md b/packages/partman/partman-md/init.d/md
> index e5024f2..96b0b52 100755
> --- a/packages/partman/partman-md/init.d/md
> +++ b/packages/partman/partman-md/init.d/md
> @@ -2,16 +2,25 @@
>
> . /lib/partman/lib/base.sh
>
> -# Load modules
> -#depmod -a 1>/dev/null 2>&1
> -#modprobe md 1>/dev/null 2>&1 || modprobe md-mod 1>/dev/null 2>&1
> -#
> -## Load supported personalities
> -#modprobe raid1 1>/dev/null 2>&1
> -#
> -## Detect and start MD devices
> -#/sbin/mdadm --examine --scan --config=partitions >/tmp/mdadm.conf
> -#/sbin/mdadm --assemble --scan --run --config=/tmp/mdadm.conf --auto=yes
> +# Check if we have RAID
> +if [ ! -f /proc/mdstat ]; then
> + exit 0
> +fi
> +
> +# Obtain the size of an MD device
> +get_size () {
> + while [ -z "$(echo "$line" | grep "^md$NUMBER :")" ]; do
> + read line
> + [ $? -eq 1 ] && break # EOF
> + done
> + read line
> + size=$(echo "$line" | sed -e 's/ blocks.*//')
> + # If the sed failed, the line didn't contain the size; just
> + # return 0 in that case.
> + if [ "$size" = "$line" ]; then
> + size=0
> + fi
> +}
This function (just moved, I know) is really confusing.
After your changes I'm tempted to clean this up to take
an explicit parameter and explicitly return size.
> # Mark all RAID partitions as being MD
> for dev in $DEVICES/*; do
> @@ -28,6 +37,7 @@ for dev in $DEVICES/*; do
> done
> close_dialog
>
> + # Check if device/partitions are used for software RAID
> for id in $partitions; do
> md=no
> open_dialog GET_FLAGS $id
> @@ -43,38 +53,57 @@ for dev in $DEVICES/*; do
> fi
> done
>
> - # Next, check if the device is a part of an MD setup
> -# if [ -f device ]; then
> -# DEVICE=`sed -e "s/\/dev\///" device`
> -# grep -q "${DEVICE}" /proc/mdstat
> -# if [ $? -eq 0 ]; then
> -# open_dialog NEW_LABEL loop
> -# close_dialog
> -# fi
> -# fi
> -done
> + # Check if the device is a part of an MD setup
> + if [ -f device ]; then
> + NUMBER=$(sed -e 's,/dev/md/\?,,' device)
> + if ! grep -q "^md$NUMBER :" /proc/mdstat; then
> + continue
> + fi
This checks if the device is a RAID device, but the
comment suggests that we want to check for it being
part of a RAID setup. Is this intended?
...
> + # create a label
> + open_dialog NEW_LABEL loop
> + close_dialog
> + # find the free space
> + open_dialog PARTITIONS
> + free_space=''
> + while { read_line num id size type fs path name; [ "$id" ]; }; do
> + if [ "$fs" = free ]; then
> + free_space=$id
> + free_size=$size
> + fi
> + done
> + close_dialog
> + # create partition in the free space
> + if [ "$free_space" ]; then
> + open_dialog NEW_PARTITION primary ext2 $free_space full $free_size
> + read_line num id size type fs path name
> + close_dialog
> + if [ "$id" ]; then
> + open_dialog GET_FILE_SYSTEM $id
> + read_line filesystem
> + close_dialog
> + if [ "$filesystem" != none ]; then
> + open_dialog CHANGE_FILE_SYSTEM $id $filesystem
> + close_dialog
> + fi
> + fi
> + fi
> + open_dialog DISK_UNCHANGED
> + close_dialog
Another thought unrelated to your changes:
This code exists in -crypto, -md, -lvm and with a few
differences also in partman-auto-lvm. Perhaps we should
turn this into a function in -base/lib/.
.. <snip>
Everything else looks good to me. No problems spotted.
------------
> commit 922e996d3dfa9f545b2e4426d79a5bdcb588fd64
> Author: Jérémy Bobbio <lunar@debian.org>
> Date: Thu May 29 23:08:16 2008 +0000
>
> Disable "resize" and "delete" for partitions on "loop"
>
> For RAID, crypto and LVM devices only one partition can be handled at a
> time. As it makes no sense to resize or delete these kind of partitions,
> let's hide the options instead of tricking the users.
>
> As all these devices are using "loop" partition tables, that's our
> filtering criteria here.
>
> (Closes: #318228, #417973)
>
> diff --git a/packages/partman/partman-partitioning/active_partition/delete/choices b/packages/partman/partman-partitioning/active_partition/delete/choices
> index 6587145..5935d7c 100755
> --- a/packages/partman/partman-partitioning/active_partition/delete/choices
> +++ b/packages/partman/partman-partitioning/active_partition/delete/choices
> @@ -1,7 +1,19 @@
> #!/bin/sh
>
> -. /usr/share/debconf/confmodule
> +. /lib/partman/lib/base.sh
> +
> +dev=$1
> +id=$2
> +cd $dev
> +
> +open_dialog GET_LABEL_TYPE
> +read_line label
> +close_dialog
> +
> +# Disable on devices where there is no "real" partitioning
> +if [ "$label" = loop ]; then
> + exit 0
> +fi
>
> db_metaget partman-partitioning/text/delete description
> printf "delete\t${RET}\n"
> -
> diff --git a/packages/partman/partman-partitioning/active_partition/resize/choices b/packages/partman/partman-partitioning/active_partition/resize/choices
> index 3b25d94..ef53072 100755
> --- a/packages/partman/partman-partitioning/active_partition/resize/choices
> +++ b/packages/partman/partman-partitioning/active_partition/resize/choices
> @@ -6,6 +6,15 @@ dev=$1
> id=$2
> cd $dev
>
> +open_dialog GET_LABEL_TYPE
> +read_line label
> +close_dialog
> +
> +# Disable on devices where there is no "real" partitioning
> +if [ "$label" = loop ]; then
> + exit 0
> +fi
> +
> if [ -f $id/detected_filesystem ]; then
> fs=$(cat $id/detected_filesystem)
> case "$fs" in
As said above, the idea is obviously correct and the
implementation also seems fine to me. :-)
Max
Reply to: