[PATCH 3/3] Make sure autopart requests fit in somewhere (#978266)

Vratislav Podzimek vpodzime at redhat.com
Thu Sep 18 08:17:10 UTC 2014


On Wed, 2014-09-17 at 09:23 -0500, David Lehman wrote:
> On 09/17/2014 02:14 AM, Vratislav Podzimek wrote:
> > When doing LVM/BTRFS autopart we implicitly schedule one partition on each disk
> > as a device for LVM/BTRFS. Those implicitly scheduled partitions are requested
> > to be at least 500 MiB big and grow if possible. That's fine and works well
> > until we have many small disks (e.g. DASDs) and need to fit another partition
> > (for e.g. swap) that is in combination with the 500MiB implicit partition bigger
> > than any disk we have.
> >
> > Thus we need to make sure all our partition requests fit somewhere with the
> > implicitly scheduled partitions by making the latter ones smaller or by removing
> > them if needed.
> 
> I still think this is a nonsense use-case and we should not be spending 
> time on it. Six DASDs of 2GiB each is just silly. Since I realize people 
> actually care about this, I will grudgingly move past the silliness.
Same here.

> 
> What about changing the swap request to one that is growable with a 
> small base size so it can adapt using existing code?
That would result in a pile of bugs about 'swap --recommended' being
different on different machines, blah, blah, blah. And we still need
something like that, because the same could happen with /boot or
whatever else in the future.

> 
> >
> > Signed-off-by: Vratislav Podzimek <vpodzime at redhat.com>
> > ---
> >   blivet/partitioning.py | 32 ++++++++++++++++++++++++++++----
> >   1 file changed, 28 insertions(+), 4 deletions(-)
> >
> > diff --git a/blivet/partitioning.py b/blivet/partitioning.py
> > index 9e45c1a..cddeb14 100644
> > --- a/blivet/partitioning.py
> > +++ b/blivet/partitioning.py
> > @@ -119,7 +119,7 @@ def _scheduleImplicitPartitions(storage, disks):
> >
> >       return devs
> >
> > -def _schedulePartitions(storage, disks):
> > +def _schedulePartitions(storage, disks, implicit_devices):
> >       """ Schedule creation of autopart partitions.
> >
> >           This only schedules the requests for actual partitions.
> > @@ -236,8 +236,32 @@ def _schedulePartitions(storage, disks):
> >                                     parents=dev)
> >               storage.createDevice(luks_dev)
> >
> > -    # make sure preexisting broken lvm/raid configs get out of the way
> > -    return
> > +        if storage.autoPartType in (AUTOPART_TYPE_LVM, AUTOPART_TYPE_LVM_THINP,
> > +                                    AUTOPART_TYPE_BTRFS):
> > +            # doing LVM/BTRFS -- make sure the newly created partition fits in some
> > +            # free space together with one of the implicitly requested partitions
> > +            biggest_free = all_free[0]
> > +            smallest_implicit = sorted(implicit_devices, key=lambda d: d.size)[0]
> > +            if (request.size + smallest_implicit.size) > biggest_free:
> > +                # not enough space to allocate the smallest implicit partition
> > +                # and the request, make the implicit partition smaller with
> > +                # fixed size in order to make space for the request
> > +                new_size = biggest_free - request.size
> > +
> > +                # subtract the size from the biggest free region and reorder the
> > +                # list
> > +                biggest_free -= request.size
> > +                all_free.sort(reverse=True)
> > +
> 
> This seems like it might have limitations due to the fact that all_free 
> is not updated as you account for each partition.
Oh, this might be a bug. If I take all_free[0] it (biggest_free) is a
copy not the Size instance itself, right? That would mean that
subtracting something from it doesn't change the value in all_free. I
need to check that. But by fixing this, it should work because Sizes of
free regions are updated in the loop.

> 
> > +                # cannot set size of a non-existing partition, just destroy it
> > +                # and create new (if it makes sense)
> > +                implicit_devices.remove(smallest_implicit)
> > +                storage.destroyDevice(smallest_implicit)
> > +                if new_size > Size(0):
> > +                    new_dev = _createImplicitPartition(storage, size=new_size, grow=False)
> > +                    implicit_devices.append(new_dev)
> 
> You could replace the above block (starting with the comment) with this:
> 
> if new_size > Size(0):
>      smallest_implicit._size = new_size
>      smallest_implicit.req_size = new_size
>      smallest_implicit.req_base_size =  new_size
>      smallest_implicit.req_grow = False
> 
> Or you could implement _setSize for non-existing partitions. Replacing 
> the device just seems a bit wasteful.
I think implementing _setSize for non-existing partitions (not having a
partedPartition associated) with the above block is the best way to go.
I don't see a reason why we shouldn't allow that.

Thanks, testing, fixing and sending v2 PATCH.

-- 
Vratislav Podzimek

Anaconda Rider | Red Hat, Inc. | Brno - Czech Republic



More information about the anaconda-patches mailing list