On Friday 30 May 2008, Jérémy Bobbio wrote: > After loosing 2 hours yesterday fighting against the RAIDvsCrypto > issue [1], I decided that I should not let this happen to anyone else. s/loosing/losing/ :-) > Attached you will find an attempt to do so. The changeset is only > broke down in two patches: In general it looks OK, although I'd like to see mdcfg properly integrated into partman fairly soon, so it might make sense to just duplicate the md-init.sh code already instead of splitting it out into a separate script and including it in mdcfg-utils (or rather: to keep it duplicated). AFAICT you are effectively also closing #475479 (which is good!). See my analysis in http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=475479#17. Has that been explicitly tested? Probably has as I think it's the same as #391479. AFAICT #391483 already was solved by my recent changes (if not earlier). What should also be explicitly tested with these changes is: - LVM on RAID - delete a RAID device -> create new RAID device - delete a RAID device -> finish (is partman info updated> -> create new RAID device I'll do some testing myself too after the changes have been committed. > +++ b/packages/mdcfg/md-init.sh > +modprobe md >/dev/null 2>&1 || modprobe md-mod >/dev/null 2>&1 Isn't the md module so ancient by now we could just drop it? > +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 From what I have seen there is no need to load the raid* modules only to detect existing RAID partitions: they will be autoloaded as needed. Only md-mod is needed. Loading the raid* modules when the actual RAID configuration options are started does make sense, especially if we check which RAID variants are supported before offering them. If not, they are probably not needed there either. > +++ b/packages/partman/partman-base/init.d/parted > - grep -v '^/dev/md' | > - sed 's,^/dev/\(ide\|scsi\|[hs]d\),!/dev/\1,' | > + sed 's,^/dev/\(ide\|scsi\|[hsm]d\),!/dev/\1,' | This will not catch /dev/md/X devices. Also, I don't think it's really a good idea to combine them with hda/sda as it is a completely different class of device. > +++ b/packages/partman/partman-md/init.d/_numbers > -31 md-devices > +25 md-devices > 51 md I think it would be clearer to have the check that /proc/mdstat already exists in md-devices, so it is clear that it is effectively only run the first time. The comment should be clearer about this too. Did you make any real changes in the code moved from md-devices to md? > +++ b/packages/partman/partman-md/init.d/md > + # replace "Unknown" model by a more appropriate string I would change that to: "Set an appropriate value for device model". You're not actually replacing anything I think. Cheers, FJP
Attachment:
signature.asc
Description: This is a digitally signed message part.