[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