A couple of comments, inline...

On Thu, 2010-04-22 at 19:27 +0200, Hans de Goede wrote:
> ---
> storage/devices.py | 39 ++++++++++++++++++++++++++++-----------
> storage/devicetree.py | 5 +++--
> 2 files changed, 31 insertions(+), 13 deletions(-)
>
> diff --git a/storage/devices.py b/storage/devices.py
> index a990708..95b0b50 100644
> --- a/storage/devices.py
> +++ b/storage/devices.py
> @@ -2455,6 +2455,10 @@ class MDRaidArrayDevice(StorageDevice):
> self.chunkSize = 64.0 / 1024.0 # chunk size in MB
> self.superBlockSize = 128.0 / 1024.0 # superblock size in MB
>
> + self.createMetadataVer = "1.1"
> + # bitmaps are not meaningful on raid0 according to mdadm-3.0.3
> + self.createBitmap = self.level != 0
> +

There is already an unused "bitmap" attribute that could serve this
purpose or else be removed.

> # For container members probe size now, as we cannot determine it
> # when teared down.
> if self.parents and self.parents[0].type == "mdcontainer":
> @@ -2814,6 +2818,28 @@ class MDRaidArrayDevice(StorageDevice):
> if recursive:
> self.teardownParents(recursive=recursive)
>
> + def preCommitFixup(self, *args, **kwargs):
> + """ Determine create parameters for this set """
> + mountpoints = kwargs.pop("mountpoints")
> + log_method_call(self, self.name, mountpoints)
> +
> + if "/boot" in mountpoints:
> + bootmountpoint = "/boot"
> + else:
> + bootmountpoint = "/"
> +
> + # If we are used to boot from we cannot use 1.1 metadata
> + if getattr(self.format, "mountpoint", None) == bootmountpoint or
> + getattr(self.format, "mountpoint", None) == "/boot/efi" or
> + self.format.type == "prepboot":
> + self.createMetadataVer = "1.0"
> +
> + # Bitmaps are not useful for swap and small partitions
> + if getattr(self.format, "mountpoint", None) == "/boot" or
> + getattr(self.format, "mountpoint", None) == "/boot/efi" or
> + self.format.type == "swap":
> + self.createBitmap = False

If bitmaps aren't useful for "small" devices we should define "small"
and do some math instead of checking the mountpoint. Otherwise, this
looks good to me.

Dave

> +
> def create(self, intf=None):
> """ Create the device. """
> log_method_call(self, self.name, status=self.status)
> @@ -2832,21 +2858,12 @@ class MDRaidArrayDevice(StorageDevice):
>
> disks = [disk.path for disk in self.devices]
> spares = len(self.devices) - self.memberDevices
> - # Figure out format specific options
> - metadata="1.1"
> - # bitmaps are not meaningful on raid0 according to mdadm-3.0.3
> - bitmap = self.level != 0
> - if getattr(self.format, "mountpoint", None) == "/boot":
> - metadata="1.0"
> - bitmap=False
> - elif self.format.type == "swap":
> - bitmap=False
> mdraid.mdcreate(self.path,
> self.level,
> disks,
> spares,
> - metadataVer=metadata,
> - bitmap=bitmap,
> + metadataVer=self.createMetadataVer,
> + bitmap=self.createBitmap,
> progress=w)
> except Exception:
> raise
> diff --git a/storage/devicetree.py b/storage/devicetree.py
> index 0bc07e5..a128f55 100644
> --- a/storage/devicetree.py
> +++ b/storage/devicetree.py
> @@ -632,14 +632,15 @@ class DeviceTree(object):
> device.originalFormat.resetPartedDisk()
>
> # Call preCommitFixup on all devices
> + mpoints = [getattr(d.format, 'mountpoint', "") for d in self.devices]
> for device in self.devices:
> - device.preCommitFixup()
> + device.preCommitFixup(mountpoints=mpoints)
>
> # Also call preCommitFixup on any devices we're going to
> # destroy (these are already removed from the tree)
> for action in self._actions:
> if isinstance(action, ActionDestroyDevice):
> - action.device.preCommitFixup()
> + action.device.preCommitFixup(mountpoints=mpoints)
>
> # setup actions to create any extended partitions we added
> #
> --
> 1.7.0.1
>
> _______________________________________________
> Anaconda-devel-list mailing list
> Anaconda-devel-list@redhat.com
> https://www.redhat.com/mailman/listinfo/anaconda-devel-list


_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list

Hi,

On 04/22/2010 08:01 PM, David Lehman wrote:

A couple of comments, inline...



Thanks for the review.


On Thu, 2010-04-22 at 19:27 +0200, Hans de Goede wrote:

---
storage/devices.py | 39 ++++++++++++++++++++++++++++-----------
storage/devicetree.py | 5 +++--
2 files changed, 31 insertions(+), 13 deletions(-)

diff --git a/storage/devices.py b/storage/devices.py
index a990708..95b0b50 100644
--- a/storage/devices.py
+++ b/storage/devices.py
@@ -2455,6 +2455,10 @@ class MDRaidArrayDevice(StorageDevice):
self.chunkSize = 64.0 / 1024.0 # chunk size in MB
self.superBlockSize = 128.0 / 1024.0 # superblock size in MB

+ self.createMetadataVer = "1.1"
+ # bitmaps are not meaningful on raid0 according to mdadm-3.0.3
+ self.createBitmap = self.level != 0
+


There is already an unused "bitmap" attribute that could serve this
purpose or else be removed.



I opted for removing it.


# For container members probe size now, as we cannot determine it
# when teared down.
if self.parents and self.parents[0].type == "mdcontainer":
@@ -2814,6 +2818,28 @@ class MDRaidArrayDevice(StorageDevice):
if recursive:
self.teardownParents(recursive=recursive)

+ def preCommitFixup(self, *args, **kwargs):
+ """ Determine create parameters for this set """
+ mountpoints = kwargs.pop("mountpoints")
+ log_method_call(self, self.name, mountpoints)
+
+ if "/boot" in mountpoints:
+ bootmountpoint = "/boot"
+ else:
+ bootmountpoint = "/"
+
+ # If we are used to boot from we cannot use 1.1 metadata
+ if getattr(self.format, "mountpoint", None) == bootmountpoint or
+ getattr(self.format, "mountpoint", None) == "/boot/efi" or
+ self.format.type == "prepboot":
+ self.createMetadataVer = "1.0"
+
+ # Bitmaps are not useful for swap and small partitions
+ if getattr(self.format, "mountpoint", None) == "/boot" or
+ getattr(self.format, "mountpoint", None) == "/boot/efi" or
+ self.format.type == "swap":
+ self.createBitmap = False


If bitmaps aren't useful for "small" devices we should define "small"
and do some math instead of checking the mountpoint.


Oh good one, now why didn't I think of that myself ? New version coming up.

What do you think about this patch and F-13 branch I'm a bit undecided myself
I know fedora users do mdraid 1 installs, and if they choose to not have
a separate /boot this will bite them, otoh we could just do a release note
telling them to create a separate /boot.

Regards,

Hans

_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list

On Thu, 2010-04-22 at 21:39 +0200, Hans de Goede wrote:
> What do you think about this patch and F-13 branch I'm a bit undecided myself
> I know fedora users do mdraid 1 installs, and if they choose to not have
> a separate /boot this will bite them, otoh we could just do a release note
> telling them to create a separate /boot.

I would be satisfied with either approach for F13. I'd like to get
feedback from rel-eng and QA and at least one other member of our team
before making a decision if that suits you.

Dave

_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list

Hi,

On 04/22/2010 10:42 PM, David Lehman wrote:

On Thu, 2010-04-22 at 21:39 +0200, Hans de Goede wrote:

What do you think about this patch and F-13 branch I'm a bit undecided myself
I know fedora users do mdraid 1 installs, and if they choose to not have
a separate /boot this will bite them, otoh we could just do a release note
telling them to create a separate /boot.


I would be satisfied with either approach for F13. I'd like to get
feedback from rel-eng and QA and at least one other member of our team
before making a decision if that suits you.



I tried to back port the patches, but turns out to not be 100% trivial,
so lets just release note it.

So how do we make sure we get this included in the release notes ?

Regards,

Hans

_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list

On Fri, 2010-04-23 at 09:41 +0200, Hans de Goede wrote:
> Hi,
>
> On 04/22/2010 10:42 PM, David Lehman wrote:
> > On Thu, 2010-04-22 at 21:39 +0200, Hans de Goede wrote:
> >> What do you think about this patch and F-13 branch I'm a bit undecided myself
> >> I know fedora users do mdraid 1 installs, and if they choose to not have
> >> a separate /boot this will bite them, otoh we could just do a release note
> >> telling them to create a separate /boot.
> >
> > I would be satisfied with either approach for F13. I'd like to get
> > feedback from rel-eng and QA and at least one other member of our team
> > before making a decision if that suits you.
> >
>
> I tried to back port the patches, but turns out to not be 100% trivial,
> so lets just release note it.
>
> So how do we make sure we get this included in the release notes ?

If this is part of a feature or intended behavior change, release notes
is the right place for this. The many different ways to contribute
release notes content are described at
https://fedoraproject.org/wiki/DocsProject/ReleaseNotes/Process#Six_Ways_to_Submit_a_Release_Note

Otherwise, if this is from an unintentional change, and we anticipate a
fair amount of questions/concerns on the issue, the Common bugs wiki
might be a better fit.

http://fedoraproject.org/wiki/Common_F13_bugs

Thanks,
James
_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list

Hi,

On 04/23/2010 01:21 PM, James Laska wrote:

On Fri, 2010-04-23 at 09:41 +0200, Hans de Goede wrote:

Hi,

On 04/22/2010 10:42 PM, David Lehman wrote:

On Thu, 2010-04-22 at 21:39 +0200, Hans de Goede wrote:

What do you think about this patch and F-13 branch I'm a bit undecided myself
I know fedora users do mdraid 1 installs, and if they choose to not have
a separate /boot this will bite them, otoh we could just do a release note
telling them to create a separate /boot.


I would be satisfied with either approach for F13. I'd like to get
feedback from rel-eng and QA and at least one other member of our team
before making a decision if that suits you.



I tried to back port the patches, but turns out to not be 100% trivial,
so lets just release note it.

So how do we make sure we get this included in the release notes ?


If this is part of a feature or intended behavior change, release notes
is the right place for this. The many different ways to contribute
release notes content are described at
https://fedoraproject.org/wiki/DocsProject/ReleaseNotes/Process#Six_Ways_to_Submit_a_Release_Note

Otherwise, if this is from an unintentional change, and we anticipate a
fair amount of questions/concerns on the issue, the Common bugs wiki
might be a better fit.

http://fedoraproject.org/wiki/Common_F13_bugs



Thanks for the input, I've added an entry to the Common_F13_bugs wiki page.

Regards,

Hans

_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list