Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

growpart: Add support for overprovisioning #35

Merged
merged 2 commits into from
Apr 27, 2022

Conversation

dermotbradley
Copy link
Contributor

Add option to 'growpart' to specify percentage of device that should be
left unallocated when growing partition. This is intended for consumer
SSDs and SD cards where the performance and/or lifetime of these devices
can be improved if some disk space (in addition to any the device "hides"
from users) is left unallocated.

Copy link

@blackboxsw blackboxsw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

quick pass. will actually exercise this in practice on a couple VMs to ensure I understand the implications.

bin/growpart Outdated Show resolved Hide resolved
bin/growpart Outdated Show resolved Hide resolved
bin/growpart Show resolved Hide resolved
bin/growpart Outdated Show resolved Hide resolved
Copy link
Collaborator

@paride paride left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a nice addition, thanks! Before merging I think it would be good to have a simple test for it. Using https://github.com/canonical/cloud-utils/blob/main/test/test-growpart as a base this shouldn't be too much work: we grow the partition with --free-percent and then we check the number of blocks in /proc/partitions, somehow turning this:

echo "==== after ===="
grep "${lodev##*/}" /proc/partitions

into a check. No need to resize2fs IMO.

@dermotbradley would you be willing to add it to your PR?

@dermotbradley
Copy link
Contributor Author

@dermotbradley would you be willing to add it to your PR?

Yes, working on that currently.

Add option to 'growpart' to specify percentage of device that should be
left unallocated when growing partition. This is intended for consumer
SSDs and SD cards where the performance and/or lifetime of these devices
can be improved if some disk space (in addition to any the device "hides"
from users) is left unallocated.

Overprovisioning code caters for several distinct scenarios:

(1) MSDOS/MBR partitioned disk where the disk is >2TB and so MBR
    partitions cannot extend beyond 2TB - if disk is larger than
    (2TB + overprovisioning requirement) then nothing needs to be
    done.

(2) MSDOS/MBR partitioned disk where the disk is >2TB and so MBR
    partitions cannot extend beyond 2TB - if disk is not larger
    than (2TB + overprovisioning requirement) then *some*
    overprovisioning space still needs to be reserved.

(3) MSDOS/MBR partitioned disk <=2TB where overprovisioning space
    needs to be reserved.

(4) GPT partitioned disk where overprovisioning space needs to be
    reserved.

Also added a testcase script, test-growpart-overprovision.
@dermotbradley
Copy link
Contributor Author

I have overhauled the overprovisioning code and added a testcase.

Copy link
Collaborator

@paride paride left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great, thanks for adding the test! O just have one question (see inline comment).

bin/growpart Show resolved Hide resolved
Correct some of the overprovisioning logic.

Change overprovisioning testcase to not require root.

Also correct some off-by-one errors in the existing growpart code and
correct some existing testcases affected by this.
Copy link
Collaborator

@paride paride left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM, including the off-by-one fixes, thanks!

@dermotbradley
Copy link
Contributor Author

Any chance to get this finally merged?

@paride paride merged commit c00ff0c into canonical:main Apr 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants