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(a)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