[PATCH 6/6] Require resize target sizes to yield aligned partitions. (#1120964)

Anne Mulhern amulhern at redhat.com
Wed Oct 29 12:58:18 UTC 2014



----- Original Message -----
From: "David Lehman" <dlehman at redhat.com>
To: anaconda-patches at lists.fedorahosted.org
Sent: Tuesday, October 28, 2014 3:20:03 PM
Subject: [PATCH 6/6] Require resize target sizes to yield aligned partitions.	(#1120964)

New method StorageDevice.alignTargetSize can be used to modify target
sizes as needed to account for alignment.

Modify minSize and maxSize to return values that will yield an aligned
partition.
---
 blivet/devices/partition.py |  47 ++++++++++-
 blivet/devices/storage.py   |  17 +++-
 tests/devices_test.py       | 193 ++++++++++++++++++++++++++++++++++++++++++++
 tests/storagetestcase.py    |   2 +
 4 files changed, 255 insertions(+), 4 deletions(-)

diff --git a/blivet/devices/partition.py b/blivet/devices/partition.py
index d8e5de8..0c130f8 100644
--- a/blivet/devices/partition.py
+++ b/blivet/devices/partition.py
@@ -224,11 +224,36 @@ class PartitionDevice(StorageDevice):
                       "flags": self.partedPartition.getFlagsAsString()})
         return d
 
+    def alignTargetSize(self, newsize):
+        """ Return newsize adjusted to allow for an end-aligned partition.
+
+            :param :class:`~.Size` newsize: proposed/unaligned target size
+            :raises _ped.CreateException: if the size extends beyond the end of
+                                          the disk
+            :returns: newsize modified to yield an end-aligned partition
+            :rtype: :class:`~.Size`
+        """
+        if newsize == 0:

!!! More proper to do comparison with Size(0) here.

+            return newsize
+
+        (_constraint, geometry) = self._computeResize(self.partedPartition,
+                                                      newsize=newsize)
+        return Size(geometry.getLength(unit="B"))
+
     def _setTargetSize(self, newsize):
         if not isinstance(newsize, Size):
             raise ValueError("new size must of type Size")
 
         if newsize != self.size:
+            try:
+                aligned = self.alignTargetSize(newsize)
+            except _ped.CreateException:
+                # this gets handled in superclass setter, below
+                aligned = newsize
+
+            if aligned != newsize:
+                raise ValueError("new size will not yield an aligned partition")
+
             # change this partition's geometry in-memory so that other
             # partitioning operations can complete (e.g., autopart)
             super(PartitionDevice, self)._setTargetSize(newsize)
@@ -748,16 +773,34 @@ class PartitionDevice(StorageDevice):
             pass
         else:
             if partition.type == parted.PARTITION_FREESPACE:
-                maxPartSize = self.size + Size(partition.getLength(unit="B"))
+                maxPartSize += Size(partition.getLength(unit="B"))
 
         return maxPartSize
 
     @property
+    def minSize(self):
+        min_size = super(PartitionDevice, self).minSize
+        if self.resizable and min_size:
+            # Adjust the min size as needed so that aligning the end sector
+            # won't drive the actual size below the formatting's minimum.
+            # align the end sector (up, if possible)
+            aligned = self.alignTargetSize(min_size)
+            if aligned < min_size:
+                # If it aligned down, that must mean it cannot align up. Just
+                # return our current size.
+                current = self.currentSize

!!! The line above and the variable current are unnecessary, can just set min_size to
self.currentSize below.

!!! OR use current in debug message (probably original idea).

+                log.debug("failed to align min size up; returning current size")
+                min_size = current
+
+        return min_size
+
+    @property
     def maxSize(self):
         """ The maximum size this partition can be. """
         maxPartSize = self._unalignedMaxPartSize
         maxFormatSize = self.format.maxSize
-        return min(maxFormatSize, maxPartSize) if maxFormatSize else maxPartSize
+        unalignedMax = min(maxFormatSize, maxPartSize) if maxFormatSize else maxPartSize
+        return self.alignTargetSize(unalignedMax)

!!! Should both candidates be aligned and THEN the values compared?
 
     @property
     def currentSize(self):
diff --git a/blivet/devices/storage.py b/blivet/devices/storage.py
index 531ab8c..991b3d4 100644
--- a/blivet/devices/storage.py
+++ b/blivet/devices/storage.py
@@ -238,6 +238,16 @@ class StorageDevice(Device):
 
         super(StorageDevice, self)._setName(value)
 
+    def alignTargetSize(self, newsize):
+        """ Return a proposed target size adjusted for device specifics.
+
+            :param :class:`~.Size` newsize: the proposed/unaligned target size
+            :returns: newsize modified to yield an aligned device
+            :rtype: :class:`~.Size`
+        """
+
+        return newsize
+
     def _getTargetSize(self):
         return self._targetSize
 
@@ -254,6 +264,9 @@ class StorageDevice(Device):
                       newsize, self.minSize)
             raise ValueError("size is smaller than the minimum for this device")
 
+        if self.alignTargetSize(newsize) != newsize:
+            raise ValueError("new size would violate alignment requirements")
+
         self._targetSize = newsize
 
     targetSize = property(lambda s: s._getTargetSize(),
@@ -586,12 +599,12 @@ class StorageDevice(Device):
     @property
     def minSize(self):
         """ The minimum size this device can be. """
-        return self.format.minSize if self.resizable else self.currentSize
+        return self.alignTargetSize(self.format.minSize) if self.resizable else self.currentSize
 
     @property
     def maxSize(self):
         """ The maximum size this device can be. """
-        return self.format.maxSize if self.resizable else self.currentSize
+        return self.alignTargetSize(self.format.maxSize) if self.resizable else self.currentSize
 
     @property
     def status(self):
diff --git a/tests/devices_test.py b/tests/devices_test.py
index 7221b62..b039774 100644
--- a/tests/devices_test.py
+++ b/tests/devices_test.py
@@ -1,6 +1,7 @@
 #!/usr/bin/python
 # vim:set fileencoding=utf-8
 
+import os
 import unittest
 
 from mock import Mock
@@ -14,6 +15,7 @@ from blivet.devices import BTRFSSnapShotDevice
 from blivet.devices import BTRFSSubVolumeDevice
 from blivet.devices import BTRFSVolumeDevice
 from blivet.devices import DiskDevice
+from blivet.devices import DiskFile
 from blivet.devices import LVMLogicalVolumeDevice
 from blivet.devices import LVMSnapShotDevice
 from blivet.devices import LVMThinPoolDevice
@@ -22,11 +24,13 @@ from blivet.devices import LVMThinSnapShotDevice
 from blivet.devices import LVMVolumeGroupDevice
 from blivet.devices import MDRaidArrayDevice
 from blivet.devices import OpticalDevice
+from blivet.devices import PartitionDevice
 from blivet.devices import StorageDevice
 from blivet.devices import ParentList
 from blivet.devicelibs import btrfs
 from blivet.devicelibs import mdraid
 from blivet.size import Size
+from blivet.util import sparsetmpfile
 
 from blivet.formats import getFormat
 
@@ -770,6 +774,195 @@ class LVMDeviceTest(unittest.TestCase):
         snap1.exists = True
         self.assertEqual(snap1.dependsOn(thinlv), False)
 
+    def testTargetSize(self):
+        pv = StorageDevice("pv1", fmt=blivet.formats.getFormat("lvmpv"),
+                           size=Size("1 GiB"))
+        vg = LVMVolumeGroupDevice("testvg", parents=[pv])
+        orig_size = Size("800 MiB")
+        lv = LVMLogicalVolumeDevice("testlv", parents=[vg], size=orig_size,
+                                    fmt=blivet.formats.getFormat("ext4"),
+                                    exists=True)
+
+        min_size = Size("200 MiB")
+        lv.format.exists = True
+        lv.format._minInstanceSize = min_size
+
+        # Make sure things are as expected to begin with.
+        self.assertEqual(lv.minSize, min_size)
+        self.assertEqual(lv.maxSize, Size("1020 MiB"))
+        self.assertEqual(lv.size, orig_size)
+
+        # ValueError if size smaller than minSize
+        with self.assertRaisesRegexp(ValueError,
+                                     "size.*smaller than the minimum"):
+            lv.targetSize = Size("1 MiB")
+
+        # target size should be unchanged
+        self.assertEqual(lv.targetSize, orig_size)
+
+        # ValueError if size larger than maxSize
+        with self.assertRaisesRegexp(ValueError,
+                                     "size.*larger than the maximum"):
+            lv.targetSize = Size("1 GiB")
+
+        # target size should be unchanged
+        self.assertEqual(lv.targetSize, orig_size)
+
+        # successful set of target size should also be reflected in size attr
+        new_target = Size("900 MiB")
+        lv.targetSize = new_target
+        self.assertEqual(lv.targetSize, new_target)
+        self.assertEqual(lv.size, new_target)
+
+        # reset target size to original size
+        lv.targetSize = orig_size
+        self.assertEqual(lv.targetSize, orig_size)
+        self.assertEqual(lv.size, orig_size)
+
+class PartitionDeviceTestCase(unittest.TestCase):
+
+    def testTargetSize(self):
+        with sparsetmpfile("targetsizetest", Size("10 MiB")) as disk_file:
+            disk = DiskFile(disk_file)
+            disk.format = getFormat("disklabel", device=disk.path)
+            grain_size = disk.format.alignment.grainSize
+            sector_size = disk.format.partedDevice.sectorSize

!!! Could you turn the two above into Sizes(), to make their units explicit?

+            start = grain_size
+            orig_size = Size("6 MiB")
+            end = start + (orig_size / sector_size) - 1

!!! This line is confusing to me, because of the mix of units.
It seems that start should be a Size, but orig_size / sector_size has no units,
and I'm not sure about 1.

+            disk.format.addPartition(start, end)
+            partition = disk.format.partedDisk.getPartitionBySector(start)
+            self.assertNotEqual(partition, None)
+            self.assertEqual(orig_size, Size(partition.getLength(unit='B')))
+
+            device = PartitionDevice(os.path.basename(partition.path),
+                                     size=orig_size)
+            device.disk = disk
+            device.exists = True
+            device.partedPartition = partition
+
+            device.format = getFormat("ext4", device=device.path)
+            device.format.exists = True
+            # grain size should be 1 MiB
+            device.format._minInstanceSize = Size("2 MiB") + (grain_size / 2)
+
+            # Make sure things are as expected to begin with.
+            self.assertEqual(device.size, orig_size)
+            self.assertEqual(device.minSize, Size("3 MiB"))
+            # start sector's at 1 MiB
+            self.assertEqual(device.maxSize, Size("9 MiB"))
+
+            # ValueError if not Size
+            with self.assertRaisesRegexp(ValueError,
+                                         "new size must.*type Size"):
+                device.targetSize = 22
+
+            self.assertEqual(device.targetSize, orig_size)
+
+            # ValueError if size smaller than minSize
+            with self.assertRaisesRegexp(ValueError,
+                                         "size.*smaller than the minimum"):
+                device.targetSize = Size("1 MiB")
+
+            self.assertEqual(device.targetSize, orig_size)
+
+            # ValueError if size larger than maxSize
+            with self.assertRaisesRegexp(ValueError,
+                                         "size.*larger than the maximum"):
+                device.targetSize = Size("11 MiB")
+
+            self.assertEqual(device.targetSize, orig_size)
+
+            # ValueError if unaligned
+            with self.assertRaisesRegexp(ValueError, "new size.*not.*aligned"):
+                device.targetSize = Size("3.1 MiB")
+
+            self.assertEqual(device.targetSize, orig_size)
+
+            # successfully set a new target size
+            new_target = device.maxSize
+            device.targetSize = new_target
+            self.assertEqual(device.targetSize, new_target)
+            self.assertEqual(device.size, new_target)
+            parted_size = Size(device.partedPartition.getLength(unit='B'))
+            self.assertEqual(parted_size, device.targetSize)
+
+            # reset target size to original size
+            device.targetSize = orig_size
+            self.assertEqual(device.targetSize, orig_size)
+            self.assertEqual(device.size, orig_size)
+            parted_size = Size(device.partedPartition.getLength(unit='B'))
+            self.assertEqual(parted_size, device.targetSize)
+
+    def testMinMaxSizeAlignment(self):
+        with sparsetmpfile("minsizetest", Size("10 MiB")) as disk_file:
+            disk = DiskFile(disk_file)
+            disk.format = getFormat("disklabel", device=disk.path)
+            grain_size = disk.format.alignment.grainSize
+            sector_size = disk.format.partedDevice.sectorSize

!!! Also be explicit about sizes here.

+            start = grain_size
+            end = start + (Size("6 MiB") / sector_size)

!!! Here, too is a somewhat confusing mix of units.

+            disk.format.addPartition(start, end)
+            partition = disk.format.partedDisk.getPartitionBySector(start)
+            self.assertNotEqual(partition, None)
+
+            device = PartitionDevice(os.path.basename(partition.path))
+            device.disk = disk
+            device.exists = True
+            device.partedPartition = partition
+
+            # Typical sector size is 512 B.
+            # Default optimum alignment grain size is 2048 sectors, or 1 MiB.
+            device.format = getFormat("ext4", device=device.path)
+            device.format.exists = True
+            device.format._minInstanceSize = Size("2 MiB") + (grain_size / 2)
+
+            ##
+            ## minSize
+            ##
+
+            # The end sector based only on format min size should be unaligned.
+            min_sectors = int(device.format.minSize / sector_size)
+            min_end_sector = partition.geometry.start + min_sectors - 1

!!! Again there is a mix of units here or units are unspecified.

+            self.assertEqual(
+                disk.format.endAlignment.isAligned(partition.geometry,
+                                                   min_end_sector),
+                False)
+
+            # The end sector based on device min size should be aligned.
+            min_sectors = int(device.minSize / sector_size)
+            min_end_sector = partition.geometry.start + min_sectors - 1

!!! Uncertain of units here as well.

+            self.assertEqual(
+                disk.format.endAlignment.isAligned(partition.geometry,
+                                                   min_end_sector),
+                True)
+
+            ##
+            ## maxSize
+            ##
+
+            # Add a partition starting three sectors past an aligned sector and
+            # extending to the end of the disk so that there's a free region
+            # immediately following the first partition with an unaligned end
+            # sector.
+            free = disk.format.partedDisk.getFreeSpaceRegions()[-1]
+            raw_start = int(Size("9 MiB") / sector_size)
+            start = disk.format.alignment.alignUp(free, raw_start) + 3

!!! Units?

+            disk.format.addPartition(start, disk.partedDevice.length - 1)
+

!!! Units?

+            # Verify the end of the free region immediately following the first
+            # partition is unaligned.
+            free = disk.format.partedDisk.getFreeSpaceRegions()[1]
+            self.assertEqual(disk.format.endAlignment.isAligned(free, free.end),
+                             False)
+
+            # The end sector based on device min size should be aligned.
+            max_sectors = int(device.maxSize / sector_size)
+            max_end_sector = partition.geometry.start + max_sectors - 1

!!! Units?

+            self.assertEqual(
+                disk.format.endAlignment.isAligned(free, max_end_sector),
+                True)
+
 class DeviceNameTestCase(unittest.TestCase):
     """Test device name validation"""
 
diff --git a/tests/storagetestcase.py b/tests/storagetestcase.py
index 11de630..693d119 100644
--- a/tests/storagetestcase.py
+++ b/tests/storagetestcase.py
@@ -36,7 +36,9 @@ class StorageTestCase(unittest.TestCase):
         # prevent PartitionDevice from trying to dig around in the partition's
         # geometry
         blivet.devices.PartitionDevice._setTargetSize = StorageDevice._setTargetSize
+        blivet.devices.PartitionDevice.alignTargetSize = StorageDevice.alignTargetSize
         blivet.devices.PartitionDevice.maxSize = StorageDevice.maxSize
+        blivet.devices.PartitionDevice.minSize = StorageDevice.minSize
 
         def partition_probe(device):
             if isinstance(device._partedPartition, Mock):
-- 
1.9.3

_______________________________________________
anaconda-patches mailing list
anaconda-patches at lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches

A bunch of inline comments, all preceded by !!! for easy location.

- mulhern


More information about the anaconda-patches mailing list