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

David Lehman dlehman at redhat.com
Fri Sep 11 20:46:54 UTC 2015


On Fri, 2015-09-11 at 16:35 -0400, Samantha N. Bueno wrote:
> 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.

/me makes taunting chicken sounds...

You could at least keep this one:

diff --git a/blivet/partitioning.py b/blivet/partitioning.py
index 6505825..a0ae641 100644
--- a/blivet/partitioning.py
+++ b/blivet/partitioning.py
@@ -539,7 +539,7 @@ def doPartitioning(storage):
             storage.devicetree._removeDevice(partition,
modparent=False, force=True)
 
     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

David

> 
> 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
> _______________________________________________
> 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