[PATCH 3/3] Minimize number of times storage.partitions is accessed. (#1155984)

Samantha N. Bueno sbueno+anaconda at redhat.com
Fri Sep 11 20:35:43 UTC 2015


On Fri, Aug 21, 2015 at 12:56:41PM -0500, David Lehman wrote:
> On 08/21/2015 12:25 PM, Samantha N. Bueno wrote:
> >This is also a non-trivial culprit in causing disk partitioning to seem
> >slow. Every time storage.partitions is accessed, getDevicesByInstance()
> >is called, which, collectively, had significant overhead with high
> >numbers of disks.
> >
> >Resolves: rhbz#1155984
> >---
> >  blivet/partitioning.py | 17 +++++++++--------
> >  1 file changed, 9 insertions(+), 8 deletions(-)
> >
> >diff --git a/blivet/partitioning.py b/blivet/partitioning.py
> >index f0a9a7a..c6c0ca5 100644
> >--- a/blivet/partitioning.py
> >+++ b/blivet/partitioning.py
> >@@ -860,11 +860,12 @@ def updateExtendedPartitions(storage, disks):
> >      """
> >      # XXX hack -- if we created any extended partitions we need to add
> >      #             them to the tree now
> >+    partitions = storage.partitions
> >      for disk in disks:
> >          extended = disk.format.extendedPartition
> >          if not extended:
> >              # remove any obsolete extended partitions
> >-            for part in storage.partitions:
> >+            for part in partitions:
> >                  if part.disk == disk and part.isExtended:
> >                      if part.exists:
> >                          storage.destroyDevice(part)
> >@@ -880,7 +881,7 @@ def updateExtendedPartitions(storage, disks):
> >                  device.partedPartition = extended
> >
> >          # remove any obsolete extended partitions
> >-        for part in storage.partitions:
> >+        for part in partitions:
> >              if part.disk == disk and part.isExtended and \
> >                 part.partedPartition not in disk.format.partitions:
> >                  if part.exists:
> >@@ -929,13 +930,13 @@ def doPartitioning(storage):
> >      # XXX This does not remove the extended from the parted.Disk, but it should
> >      #     cause removeNewPartitions to remove it since there will no longer be
> >      #     a PartitionDevice for it.
> >-    for partition in storage.partitions:
> >+    partitions = storage.partitions
> >+    for partition in partitions:
> >          if not partition.exists and partition.isExtended and \
> >             not storage.devicetree.findActions(device=partition, action_type="create"):
> >              storage.devicetree._removeDevice(partition, modparent=False, force=True)
> 
> Since you might be removing a partition from the list here you should
> also remove it from 'partitions' at the same time, which will mean you
> should iterate over a slice/copy of partitions in the above loop.
> 
> In the same spirit, please make sure nothing is done in between the
> rest of these that might change storage.partitions.

Now that you've mentioned this, I'm a bit wary to push this patch. It
doesn't seem to have as big an impact to the time lag as I thought, so I
think I am going to leave this out of the set for now if that's all
right with you.

Samantha
 
> David
> 
> >
> >-    partitions = storage.partitions[:]
> >-    for part in storage.partitions:
> >+    for part in partitions:
> >          part.req_bootable = False
> >          if not part.exists:
> >              # start over with flexible-size requests
> >@@ -956,7 +957,7 @@ def doPartitioning(storage):
> >          raise
> >      else:
> >          # Mark all growable requests as no longer growable.
> >-        for partition in storage.partitions:
> >+        for partition in partitions:
> >              log.debug("fixing size of %s", partition)
> >              partition.req_grow = False
> >              partition.req_base_size = partition.size
> >@@ -968,7 +969,7 @@ def doPartitioning(storage):
> >          # The number and thus the name of partitions may have changed now,
> >          # allocatePartitions() takes care of this for new partitions, but not
> >          # for pre-existing ones, so we update the name of all partitions here
> >-        for part in storage.partitions:
> >+        for part in partitions:
> >              # leave extended partitions as-is -- we'll handle them separately
> >              if part.isExtended:
> >                  continue
> >@@ -976,7 +977,7 @@ def doPartitioning(storage):
> >
> >          updateExtendedPartitions(storage, disks)
> >
> >-        for part in [p for p in storage.partitions if not p.exists]:
> >+        for part in [p for p in partitions if not p.exists]:
> >              problem = part.checkSize()
> >              if problem < 0:
> >                  raise PartitioningError(_("partition is too small for %(format)s formatting "
> >
> 
> _______________________________________________
> anaconda-patches mailing list
> anaconda-patches at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches


More information about the anaconda-patches mailing list